[Webkit-unassigned] [Bug 30946] Extra layout on keypress after a space (problem with rebalanceWhitespaceAt in InsertTextCommand)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30946


Justin Garcia <justin.garcia at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|webkit-unassigned at lists.web |justin.garcia at apple.com
                   |kit.org                     |
  Attachment #43452|0                           |1
        is obsolete|                            |
  Attachment #43622|                            |review?
               Flag|                            |




--- Comment #4 from Justin Garcia <justin.garcia at apple.com>  2009-11-20 16:08:40 PST ---
Created an attachment (id=43622)
 --> (https://bugs.webkit.org/attachment.cgi?id=43622)
patch

> I am concerned about copying the function named isWhitespace in two different
> source files. 

Fixed, it didn't need to be in InsertTextCommand at all.

> Can we put this into PlatformString.h or StringImpl.h instead?
> What's the right name for it to distinguish it from other functions of similar
> name?

It's a strange function, I'd rather leave it in htmlediting.cpp for now.  I'm
not sure that it needs to check for tabs and newlines, since those will be
removed as insignificant whitespace before we get to whitespace rebalancing. 
But this is part of the old code that I'd like to address after.

> > +void InsertTextCommand::insertTextIntoNodeAndRebalanceWhitespace(const String& textToInsert, PassRefPtr<Text> node, unsigned offset)
> 
> This function does not take ownership of the text node, so the argument type
> should be Text*, not PassRefPtr<Text>.

OK.  But when does a function take ownership of the node? 
insertTextIntoNode(...) takes in a PassRefPtr<Node>, even though it seems like
it only calls a function that takes ownership of the node (the create
function).

> There should not be a blank line after the if statement.

FIxed.

> > +    ASSERT(whitespaceEnd >= whitespaceStart);
> > +    unsigned length = whitespaceEnd - whitespaceStart + 1;
> 
> Why the +1 here? It seems like the "end" value may be inclusive, which seems
> like a mistake.

whitespaceEnd was strangely the offset *of* the last whitespace character, not
the offset after the last whitespace character, thus the plus 1.  Fixed this.

> > +// Finds the extent of whitespace around a particular offset, as long as it is in a node with collapsable whitespace.
> 
> The word "collapsible" is misspelled here.
> 
> The comment could be clearer on what "as long as" means.
> 
> There are multiple modes of collapsible whitespace. For example, pre-wrap
> collapses other whitespace, but not "\n". To handle those correctly, I think
> RenderStyle::isCollapsibleWhiteSpace needs to be used instead of a local
> isWhiteSpace function.

The comment now reads: "Find the extent of whitespace around a particular
offset.  If whitespace characters (' ') are preserved, then we don't do
whitespace rebalancing and this function returns false." and the function name
is "extentOfWhitespaceForRebalancingAt".

> Do we really need a special case for empty text nodes? Is this a common case
> that needs performance optimization? Does the code below fail for empty text
> nodes?

We don't need it, I'll remove it.

> I'd like to see an assertion about offset being <= node->length() and also get
> a sense of whether callers really guarantee that or not.

They can't.  They get their offsets from VisiblePositions, which can
technically have invalid offsets.

> > +    if (offset == text.length() || !isWhitespace(text[offset])) {
> 
> Because of how String indexing works, I don't think you need the "offset ==
> text.length" check here. Getting a character past the end returns 0, and that
> will return false from isWhitespace.

Seems strange to rely on this detail, and no index checking would look odd
since it looks like straight array indexing at first glance.

> > +    while (start > 0 && isWhitespace(text[start - 1]))
> > +        start--;
> > +        
> > +    for (unsigned i = end + 1; i < text.length(); i++) {
> > +        if (!isWhitespace(text[i]))
> > +            break;
> > +        end = i;
> > +    }
> 
> I don't understand why these loops are structured differently, since they both
> do the same thing. It would be slightly nicer to make them match.

Fixed.

> I'd like to see more test cases -- I worry that we are making performance
> faster but correctness suffer in various edge cases.

Added a few test cases that insert content in and around multiple spaces.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.