[Webkit-unassigned] [Bug 53409] InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-08 15:47:00 PST ---
(From update of attachment 84819)
View in context: https://bugs.webkit.org/attachment.cgi?id=84819&action=review

> Source/WebCore/editing/InsertListCommand.cpp:175
> +                VisiblePosition nextStart(startOfNextParagraph(endingSelection().visibleStart()));

I think we normally use operator= instead of copy constructor.

> Source/WebCore/editing/InsertListCommand.cpp:393
> -    moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
> +    bool preserveSelection = true;
> +    moveParagraph(start, end, positionBeforeNode(placeholder.get()), preserveSelection);

I don't like this change since we're not modifying this function call in any other way.  I'd rather replace the boolean argument by an enum in a separate patch.

> Source/WebCore/editing/htmlediting.h:176
> +bool shouldSkipParagraphForIteration(const VisiblePosition& paragraphStart);

Is there a plan to use this function in other places?  If not, I'd prefer to put in InsertListCommand.cpp to avoid polluting the namespace.

> Source/WebCore/editing/visible_units.cpp:883
>  VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
>  {
> -    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition));
> +    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition, CanSkipOverEditingBoundary));

Did you really check that all call sites expect this function to cross the editing boundary?

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