Bug 42841 - [cleanup] logic to extract adjacent lists and list children in listifyParagraph and unlistifyParagraph should be isolated
Summary: [cleanup] logic to extract adjacent lists and list children in listifyParagra...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Blocker
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 33668
  Show dependency treegraph
 
Reported: 2010-07-22 12:04 PDT by Ryosuke Niwa
Modified: 2010-07-22 19:27 PDT (History)
6 users (show)

See Also:


Attachments
cleanup patch (6.54 KB, patch)
2010-07-22 12:28 PDT, Ryosuke Niwa
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-07-22 12:04:35 PDT
To simplify the patch for the bug 33668, we have to extract the code to find adjacent lists in listifyParagraph and list children in unlistifyParagraph.
Comment 1 Ryosuke Niwa 2010-07-22 12:28:49 PDT
Created attachment 62326 [details]
cleanup patch
Comment 2 Kent Tamura 2010-07-22 18:34:32 PDT
Comment on attachment 62326 [details]
cleanup patch

> @@ -268,8 +281,6 @@ PassRefPtr<HTMLElement> InsertListComman
>          // Update the start of content, so we don't try to move the list into itself.  bug 19066
>          if (insertionPos == start.deepEquivalent())
>              start = startOfParagraph(originalStart);
> -        previousList = outermostEnclosingList(previousPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> -        nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
>      }
>  
>      moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> @@ -281,6 +292,8 @@ PassRefPtr<HTMLElement> InsertListComman
>          listElement = m_listElement;
>  
>      if (listElement) {
> +        previousList = listElement->previousElementSibling();
> +        nextList = listElement->nextElementSibling();
>          if (canMergeLists(previousList, listElement.get()))
>              mergeIdenticalElements(previousList, listElement.get());
>          if (canMergeLists(listElement.get(), nextList))

I don't understand why these changes for previousList/nextList are correct.  Would you explain it please?
Comment 3 Ryosuke Niwa 2010-07-22 18:52:06 PDT
(In reply to comment #2)
> (From update of attachment 62326 [details])
> > @@ -268,8 +281,6 @@ PassRefPtr<HTMLElement> InsertListComman
> >          // Update the start of content, so we don't try to move the list into itself.  bug 19066
> >          if (insertionPos == start.deepEquivalent())
> >              start = startOfParagraph(originalStart);
> > -        previousList = outermostEnclosingList(previousPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> > -        nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> >      }
> >  
> >      moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> > @@ -281,6 +292,8 @@ PassRefPtr<HTMLElement> InsertListComman
> >          listElement = m_listElement;
> >  
> >      if (listElement) {
> > +        previousList = listElement->previousElementSibling();
> > +        nextList = listElement->nextElementSibling();
> >          if (canMergeLists(previousList, listElement.get()))
> >              mergeIdenticalElements(previousList, listElement.get());
> >          if (canMergeLists(listElement.get(), nextList))
> 
> I don't understand why these changes for previousList/nextList are correct.  Would you explain it please?

If we're inserting a list element, then previousList must be the previous element sibling of the list element we've just inserted because that's the only case we can merge the two lists.  Notice that the only reason we're obtaining previous and next lists are to merge them with the list we've inserted.  But we can't merge the lists unless they're siblings of each other since if had some other elements in between, we shouldn't be deleting that element. The argument for nextList is similar.

While the original code may obtain elements that are not siblings of listElement, conditions in canMergeLists prevented such elements to be merged with listElement.
Comment 4 Kent Tamura 2010-07-22 18:58:00 PDT
Comment on attachment 62326 [details]
cleanup patch

Ok.
Comment 5 Ryosuke Niwa 2010-07-22 19:27:52 PDT
Landed as http://trac.webkit.org/changeset/63941.