Bug 67056 – Logic from HTMLElement::deprecatedCreateContextualFragment moved into Range::createContextualFragment function

• Component: DOM
• Status: RESOLVED FIXED

[Kaustubh Atrawalkar]

Logic from HTMLElement::deprecatedCreateContextualFragment moved into Range::createContextualFragment function.
Also corresponding calls need to be reformatted. Range::createContextualFragment semantics do not make sense for the rest of the DOM implementation to use.

[Kaustubh Atrawalkar]

Created attachment 105394 [details]
Patch

[WebKit Review Bot]

Attachment 105394 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 105394 [details]
Patch

Attachment 105394 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9550086

New failing tests:
editing/deleting/delete-block-merge-contents-004.html
editing/deleting/delete-block-merge-contents-003.html
editing/deleting/5115601.html
editing/deleting/delete-at-paragraph-boundaries-003.html
editing/deleting/5369009.html
editing/deleting/delete-block-merge-contents-001.html
editing/deleting/4922367.html
editing/deleting/delete-3857753-fix.html
editing/deleting/25322-3.html
editing/deleting/4916235-1.html
editing/deleting/5206311-2.html
editing/deleting/delete-at-paragraph-boundaries-009.html
editing/deleting/backspace-avoid-preceding-style.html
editing/deleting/delete-at-paragraph-boundaries-007.html
editing/deleting/delete-across-editable-content-boundaries-2.html
editing/deleting/delete-across-editable-content-boundaries-3.html
editing/deleting/delete-block-merge-contents-002.html
http/tests/misc/copy-resolves-urls.html
editing/deleting/5890684.html
editing/deleting/5032066.html

[Adam Barth]

This is generally a good thing to do, but it would be nice if this patch didn't make Range::createContextualFragment huge.  Maybe keep this code in a separate function in Range.cpp?  Also it looks like you're having trouble with tests failing.

[Kaustubh Atrawalkar]

Created attachment 105438 [details]
Update patch with trimmed down createContextFragment function

Broken down createContextFragment function into two functionality keeping Element:createDocumentFragmentFromElement function separate.

[WebKit Review Bot]

Comment on attachment 105438 [details]
Update patch with trimmed down createContextFragment function

Attachment 105438 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9541529

New failing tests:
editing/deleting/delete-block-merge-contents-004.html
editing/deleting/delete-block-merge-contents-003.html
editing/deleting/5115601.html
editing/deleting/delete-at-paragraph-boundaries-003.html
editing/deleting/5369009.html
editing/deleting/delete-block-merge-contents-001.html
editing/deleting/4922367.html
editing/deleting/delete-3857753-fix.html
editing/deleting/25322-3.html
editing/deleting/4916235-1.html
editing/deleting/5206311-2.html
editing/deleting/delete-at-paragraph-boundaries-009.html
editing/deleting/backspace-avoid-preceding-style.html
editing/deleting/delete-at-paragraph-boundaries-007.html
editing/deleting/delete-across-editable-content-boundaries-2.html
editing/deleting/delete-across-editable-content-boundaries-3.html
editing/deleting/delete-block-merge-contents-002.html
http/tests/misc/copy-resolves-urls.html
editing/deleting/5890684.html
editing/deleting/5032066.html

[Kaustubh Atrawalkar]

Created attachment 105477 [details]
Respecting  FragmentScriptingPermission for failing chrome layout tests.

[WebKit Review Bot]

Comment on attachment 105477 [details]
Respecting  FragmentScriptingPermission for failing chrome layout tests.

Attachment 105477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9567033

New failing tests:
editing/deleting/delete-block-merge-contents-004.html
editing/deleting/delete-block-merge-contents-003.html
editing/deleting/5115601.html
editing/deleting/delete-at-paragraph-boundaries-003.html
editing/deleting/5369009.html
editing/deleting/delete-block-merge-contents-001.html
editing/deleting/4922367.html
editing/deleting/delete-3857753-fix.html
editing/deleting/25322-3.html
editing/deleting/4916235-1.html
editing/deleting/5206311-2.html
editing/deleting/delete-at-paragraph-boundaries-009.html
editing/deleting/backspace-avoid-preceding-style.html
editing/deleting/delete-at-paragraph-boundaries-007.html
editing/deleting/delete-across-editable-content-boundaries-2.html
editing/deleting/delete-across-editable-content-boundaries-3.html
editing/deleting/delete-block-merge-contents-002.html
http/tests/misc/copy-resolves-urls.html
editing/deleting/5890684.html
editing/deleting/5032066.html

[Eric Seidel (no email)]

Comment on attachment 105477 [details]
Respecting  FragmentScriptingPermission for failing chrome layout tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=105477&action=review

> Source/WebCore/dom/Range.cpp:1138
> +    RefPtr<Node> nextNode;
> +    for (RefPtr<Node> node = fragment->firstChild(); node; node = nextNode) {
> +        nextNode = node->nextSibling();
> +        if (node->hasTagName(htmlTag) || node->hasTagName(headTag) || node->hasTagName(bodyTag)) {
> +            HTMLElement* element = toHTMLElement(node.get());
> +            Node* firstChild = element->firstChild();
> +            if (firstChild)
> +                nextNode = firstChild;
> +            RefPtr<Node> nextChild;
> +            for (RefPtr<Node> child = firstChild; child; child = nextChild) {
> +                nextChild = child->nextSibling();
> +                element->removeChild(child.get(), ignoredExceptionCode);
> +                ASSERT(!ignoredExceptionCode);
> +                fragment->insertBefore(child, element, ignoredExceptionCode);
> +                ASSERT(!ignoredExceptionCode);
> +            }
> +            fragment->removeChild(element, ignoredExceptionCode);
> +            ASSERT(!ignoredExceptionCode);
> +        }
> +    }

Seems this could be split of into a separate helper function.  Even static inline.

> Source/WebCore/editing/markup.cpp:687
> +    RefPtr<DocumentFragment> fragment = (Range::create(document))->createContextualFragment(markup, ec, scriptingPermission);

Again here, no parens around Range::create() are needed.

> Source/WebKit/qt/Api/qwebelement.cpp:1421
> +    RefPtr<DocumentFragment> fragment = (Range::create(doc))->createContextualFragment(markup, exception);

These extra () around Range::create() are not needed.

[Eric Seidel (no email)]

It's great that you're working on this!  Thanks!

[Kaustubh Atrawalkar]

Created attachment 106139 [details]
Updated Patch

Hi Eric, 
Thanks for reviewing my patch. I have incorporated your suggested changes. Can u please review this one?
Thanks :)

[WebKit Review Bot]

Attachment 106139 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Range.h:93:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Range.h:93:  The parameter name "scriptingPermission" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Range.cpp:1100:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 106139 [details]
Updated Patch

Attachment 106139 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9582699

[Kaustubh Atrawalkar]

Created attachment 106141 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106141 [details]
Patch

Attachment 106141 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9583705

New failing tests:
editing/execCommand/insert-list-xml.xhtml

[Eric Seidel (no email)]

Comment on attachment 106141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106141&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

You should replace this with an explanation of why no tests are needed (just refactoring) and remove the OOPS.

> Source/WebCore/dom/Range.cpp:7
> + * Portions  (C) 2011 Motorola Mobility. All rights reserved.

I think folks normally just say "Copyright"

> Source/WebCore/dom/Range.cpp:1102
> +    // Exceptions are ignored because none ought to happen here.

That's understood from the ASSERTs below.  You can just remove thsi comment.

> Source/WebCore/dom/Range.cpp:1170
> +    RefPtr<DocumentFragment> fragment = createDocumentFragmentForElement(markup, static_cast<Element*>(element), scriptingPermission);

We don't have a toElement cast?  the to* functions will check ->isElement first, which is useful.

> Source/WebCore/editing/markup.cpp:686
> +    // Still using a fake body element here to trick the HTML parser to using the

The "still" won't mean anything to anyone reading this comment later.  Should just be removed.  "Use a fake body element to make the HTMLParser start in InBody mode."

> Source/WebCore/editing/markup.cpp:688
> +    RefPtr<DocumentFragment> fragment =  Range::create(document)->createDocumentFragmentForElement(markup, document->body(), scriptingPermission);

This seems bad.  We're now doing a another malloc.  Why does this need to be on Range?

> Source/WebKit/qt/Api/qwebelement.cpp:1027
> +    WebCore::Element* e = m_element;
> +    Document* doc = e ? e->document() : 0;
> +    if (!doc)

Why the local variable e?

[Ryosuke Niwa]

Comment on attachment 106141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106141&action=review

> Source/WebCore/ChangeLog:4
> +        Logic from HTMLElement::deprecatedCreateContextualFragment moved into
> +        Range::createContextualFragment function.

This line should match the bug title You should either rename the bug or replace this line by the actual bug title.

> Source/WebCore/ChangeLog:8
> +

You should add description here (keep the blank line after Reviewed by ~~).

> Source/WebCore/dom/Range.cpp:1127
> +    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
> +        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
> +        return 0;
> +

Where did this code come from?  We need a test if we're making any behavioral changes.

> Source/WebKit/qt/Api/qwebelement.cpp:1077
> +    WebCore::Element* e = m_element;
> +    Document* doc = e ? e->document() : 0;

I don't think m_element can be null here. See the the line above. We already call isHTMLElement on m_element. If m_element was null, then we would have blown up before reaching this line.

[Kaustubh Atrawalkar]

Thanks Eric for the review. Please see the reply below.

(In reply to comment #16)
> (From update of attachment 106141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106141&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests. (OOPS!)
> 
> You should replace this with an explanation of why no tests are needed (just refactoring) and remove the OOPS.
> 

Added "No New Tests. Code Re-factoring" as per the bug.

> > Source/WebCore/dom/Range.cpp:7
> > + * Portions  (C) 2011 Motorola Mobility. All rights reserved.
> 
> I think folks normally just say "Copyright"
> 

OK. I thought if you change very small portion of the whole file u need to add "Portions". But anyway changed it to "Copyright".

> > Source/WebCore/dom/Range.cpp:1102
> > +    // Exceptions are ignored because none ought to happen here.
> 
> That's understood from the ASSERTs below.  You can just remove thsi comment.
> 

Removed unnecessary comment.

> > Source/WebCore/dom/Range.cpp:1170
> > +    RefPtr<DocumentFragment> fragment = createDocumentFragmentForElement(markup, static_cast<Element*>(element), scriptingPermission);
> 
> We don't have a toElement cast?  the to* functions will check ->isElement first, which is useful.
> 

Incorporated this change.

> > Source/WebCore/editing/markup.cpp:686
> > +    // Still using a fake body element here to trick the HTML parser to using the
> 
> The "still" won't mean anything to anyone reading this comment later.  Should just be removed.  "Use a fake body element to make the HTMLParser start in InBody mode."
> 

Agree with ambiguous "still" word. I have removed that.

> > Source/WebCore/editing/markup.cpp:688
> > +    RefPtr<DocumentFragment> fragment =  Range::create(document)->createDocumentFragmentForElement(markup, document->body(), scriptingPermission);
> 
> This seems bad.  We're now doing a another malloc.  Why does this need to be on Range?
> 

The API createDocumentFragmentForElement is non-static API. We can not call it directly without creating object of Range. Previously, it was called on the fakebody element. ( the implementation was in HTMLElement.cpp which is now removed).

> > Source/WebKit/qt/Api/qwebelement.cpp:1027
> > +    WebCore::Element* e = m_element;
> > +    Document* doc = e ? e->document() : 0;
> > +    if (!doc)
> 
> Why the local variable e?

Just added one more check. But realized after Ryosuke's comment its not useful. Changed the line to - 

    if(!m_element->document())
        return;

    RefPtr<DocumentFragment> fragment =  Range::create(m_element->document)->createDocumentFragmentForElement(markup, toHTMLElement(m_element));

[Kaustubh Atrawalkar]

Thanks for review Ryosuke. Please check my reply below.

(In reply to comment #17)
> (From update of attachment 106141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106141&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Logic from HTMLElement::deprecatedCreateContextualFragment moved into
> > +        Range::createContextualFragment function.
> 
> This line should match the bug title You should either rename the bug or replace this line by the actual bug title.
> 
Changed the bug title. That's more relevant.
> > Source/WebCore/ChangeLog:8
> > +
> 
> You should add description here (keep the blank line after Reviewed by ~~).
> 
Added description. 
> > Source/WebCore/dom/Range.cpp:1127
> > +    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
> > +        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
> > +        return 0;
> > +
> 
> Where did this code come from?  We need a test if we're making any behavioral changes.
> 
This is directly picked up from HTMLElement::deprecatedCreateContextualFragment. No new behavioral changes here.
> > Source/WebKit/qt/Api/qwebelement.cpp:1077
> > +    WebCore::Element* e = m_element;
> > +    Document* doc = e ? e->document() : 0;
> 
> I don't think m_element can be null here. See the the line above. We already call isHTMLElement on m_element. If m_element was null, then we would have blown up before reaching this line.

Yes agree with you. I have changed that part as per my previous comment. Please review that once. I will update the patch soon.

[Kaustubh Atrawalkar]

Created attachment 106266 [details]
Re-factoring Patch

[WebKit Review Bot]

Attachment 106266 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/Api/qwebelement.cpp:1025:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit/qt/Api/qwebelement.cpp:1073:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit/qt/Api/qwebelement.cpp:1127:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit/qt/Api/qwebelement.cpp:1179:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit/qt/Api/qwebelement.cpp:1328:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit/qt/Api/qwebelement.cpp:1405:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 6 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 106266 [details]
Re-factoring Patch

Attachment 106266 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9587349

[Kaustubh Atrawalkar]

Created attachment 106288 [details]
Updated Patch

[Ryosuke Niwa]

Comment on attachment 106288 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106288&action=review

> Source/WebCore/dom/Range.cpp:1100
> +static inline void insertIntoFragment(PassRefPtr<DocumentFragment> fragment, HTMLElement* element)

I don't think this function name is appropriate because element and its children are already in the fragment at this point. What this function is doing is to remove element preserving children.  I'd say removeElementPreservingChildren will be a better name (I'm kind of sad about the fact we can't share code with RemoveNodePreservingChildrenCommand).

> Source/WebCore/dom/Range.cpp:1104
> +    Node* firstChild = element->firstChild();

I don't see any benefit in declaring in this variable.  You should just do:
for (RefPtr<Node> child = element->firstChild(); child; child = nextChild)

> Source/WebCore/dom/Range.cpp:1133
> +    else {
> +        if (!fragment->parseXML(markup, element, scriptingPermission))

This should be a single else-if

> Source/WebCore/dom/Range.cpp:1135
> +            // FIXME: We should propagate a syntax error exception out here.
> +            return 0;

This comment should either be on the same line as return 0; or be wrapped by curly brackets (because the comment & return 0 occupy 2 lines).

> Source/WebCore/dom/Range.h:93
> +    PassRefPtr<DocumentFragment> createDocumentFragmentForElement(const String& markup, Element*,  FragmentScriptingPermission = FragmentScriptingAllowed);

This function can be static, no?

[Kaustubh Atrawalkar]

Created attachment 106290 [details]
Patch after getting in Ryosuke's comments

Ryosuke can u check once this patch? Thanks.

[Kaustubh Atrawalkar]

 
> I don't think this function name is appropriate because element and its children are already in the fragment at this point. What this function is doing is to remove element preserving children.  I'd say removeElementPreservingChildren will be a better name (I'm kind of sad about the fact we can't share code with RemoveNodePreservingChildrenCommand).
> 
Hmm..seems to be relevant name. Ok I changed it.
> I don't see any benefit in declaring in this variable.  You should just do:
> for (RefPtr<Node> child = element->firstChild(); child; child = nextChild)
> 
Just habit of getting pointers into variable before using it. Need to break it :)
> This should be a single else-if
> 
Actually it was copy pasted from previous code. So didn't changed it much. But will make it.
> This comment should either be on the same line as return 0; or be wrapped by curly brackets (because the comment & return 0 occupy 2 lines).
> 
Yep. Done this.

> This function can be static, no?

Yes this can be static. And then subsequent usage can also be without creating object of Range. So much more beneficial declaring it static. I missed that during splitting of the function createContextualFragment. Thanks you got that.

[Ryosuke Niwa]

Comment on attachment 106290 [details]
Patch after getting in Ryosuke's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=106290&action=review

> Source/WebCore/dom/Range.cpp:1145
> +            Node* firstChild = element->firstChild();
> +            if (firstChild)
> +                nextNode = firstChild;

firstChild can be declared inside if to limit the scope as in:
if (Node* firstChild = element->firstChild())
    nextNode = firstChild;

> Source/WebCore/editing/markup.cpp:687
>      // We use a fake body element here to trick the HTML parser to using the
> -    // InBody insertion mode.  Really, all this code is wrong and need to be
> -    // changed not to use deprecatedCreateContextualFragment.
> +    // InBody insertion mode.

The entire comment seems to fit in one line.

[Kaustubh Atrawalkar]

Created attachment 106307 [details]
Final Updated patch

Hi Ryosuke, I have added ur suggested changes in my final patch. 
Can u please review it once? Thanks.

[Ryosuke Niwa]

Comment on attachment 106307 [details]
Final Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106307&action=review

sorry, I didn't catch one nit.

> Source/WebCore/dom/Range.cpp:1139
> +            return 0; // FIXME: We should propagate a syntax error exception out here.

Nit: wrong indentation.

[Kaustubh Atrawalkar]

Created attachment 106308 [details]
Patch

Aah..Even i missed that too.

[Ryosuke Niwa]

Comment on attachment 106308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106308&action=review

> Source/WebCore/dom/Range.cpp:1132
> +    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
> +        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
> +        return 0;

Maybe we should consider using HashMap in the future.

[Kaustubh Atrawalkar]

Yes I was thinking about the same. But for the first step i thought just move the logic/code as it is from HTMLElement to Range.cpp. Without changing much. I can take up the next step to make it hashMap soon. Thanks Ryosuke. 
(In reply to comment #31)
> (From update of attachment 106308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106308&action=review
> 
> > Source/WebCore/dom/Range.cpp:1132
> > +    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
> > +        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
> > +        return 0;
> 
> Maybe we should consider using HashMap in the future.

[Ryosuke Niwa]

(In reply to comment #32)
> Yes I was thinking about the same. But for the first step i thought just move the logic/code as it is from HTMLElement to Range.cpp. Without changing much. I can take up the next step to make it hashMap soon. 

Right. I think it'll be a nice followup patch :)

[WebKit Review Bot]

Comment on attachment 106308 [details]
Patch

Clearing flags on attachment: 106308

Committed r94516: <http://trac.webkit.org/changeset/94516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.