Bug 67056 - Logic from HTMLElement::deprecatedCreateContextualFragment moved into Range::createContextualFragment function
Summary: Logic from HTMLElement::deprecatedCreateContextualFragment moved into Range::...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-26 13:07 PDT by Kaustubh Atrawalkar
Modified: 2011-09-05 00:46 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.46 KB, patch)
2011-08-26 13:08 PDT, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Update patch with trimmed down createContextFragment function (17.01 KB, patch)
2011-08-27 08:34 PDT, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Respecting FragmentScriptingPermission for failing chrome layout tests. (18.16 KB, patch)
2011-08-29 02:12 PDT, Kaustubh Atrawalkar
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (16.46 KB, patch)
2011-09-02 08:47 PDT, Kaustubh Atrawalkar
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2011-09-02 09:28 PDT, Kaustubh Atrawalkar
eric: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Re-factoring Patch (17.04 KB, patch)
2011-09-03 12:02 PDT, Kaustubh Atrawalkar
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (17.10 KB, patch)
2011-09-04 10:15 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch after getting in Ryosuke's comments (16.55 KB, patch)
2011-09-04 11:51 PDT, Kaustubh Atrawalkar
rniwa: review+
Details | Formatted Diff | Diff
Final Updated patch (16.56 KB, patch)
2011-09-04 23:15 PDT, Kaustubh Atrawalkar
rniwa: review+
Details | Formatted Diff | Diff
Patch (16.55 KB, patch)
2011-09-04 23:29 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaustubh Atrawalkar 2011-08-26 13:07:39 PDT
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.
Comment 1 Kaustubh Atrawalkar 2011-08-26 13:08:22 PDT
Created attachment 105394 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-26 13:10:59 PDT
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.
Comment 3 WebKit Review Bot 2011-08-26 15:10:56 PDT
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
Comment 4 Adam Barth 2011-08-26 17:15:29 PDT
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.
Comment 5 Kaustubh Atrawalkar 2011-08-27 08:34:12 PDT
Created attachment 105438 [details]
Update patch with trimmed down createContextFragment function

Broken down createContextFragment function into two functionality keeping Element:createDocumentFragmentFromElement function separate.
Comment 6 WebKit Review Bot 2011-08-27 09:12:18 PDT
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
Comment 7 Kaustubh Atrawalkar 2011-08-29 02:12:37 PDT
Created attachment 105477 [details]
Respecting  FragmentScriptingPermission for failing chrome layout tests.
Comment 8 WebKit Review Bot 2011-08-29 02:49:57 PDT
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
Comment 9 Eric Seidel (no email) 2011-08-29 12:02:56 PDT
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.
Comment 10 Eric Seidel (no email) 2011-08-29 12:03:08 PDT
It's great that you're working on this!  Thanks!
Comment 11 Kaustubh Atrawalkar 2011-09-02 08:47:23 PDT
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 :)
Comment 12 WebKit Review Bot 2011-09-02 08:49:00 PDT
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.
Comment 13 Early Warning System Bot 2011-09-02 09:00:39 PDT
Comment on attachment 106139 [details]
Updated Patch

Attachment 106139 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9582699
Comment 14 Kaustubh Atrawalkar 2011-09-02 09:28:00 PDT
Created attachment 106141 [details]
Patch
Comment 15 WebKit Review Bot 2011-09-02 11:49:57 PDT
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
Comment 16 Eric Seidel (no email) 2011-09-02 12:13:44 PDT
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?
Comment 17 Ryosuke Niwa 2011-09-02 12:28:57 PDT
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.
Comment 18 Kaustubh Atrawalkar 2011-09-03 05:33:20 PDT
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));
Comment 19 Kaustubh Atrawalkar 2011-09-03 05:51:18 PDT
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.
Comment 20 Kaustubh Atrawalkar 2011-09-03 12:02:22 PDT
Created attachment 106266 [details]
Re-factoring Patch
Comment 21 WebKit Review Bot 2011-09-03 12:03:59 PDT
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.
Comment 22 Early Warning System Bot 2011-09-03 12:19:12 PDT
Comment on attachment 106266 [details]
Re-factoring Patch

Attachment 106266 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9587349
Comment 23 Kaustubh Atrawalkar 2011-09-04 10:15:52 PDT
Created attachment 106288 [details]
Updated Patch
Comment 24 Ryosuke Niwa 2011-09-04 11:04:40 PDT
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?
Comment 25 Kaustubh Atrawalkar 2011-09-04 11:51:51 PDT
Created attachment 106290 [details]
Patch after getting in Ryosuke's comments

Ryosuke can u check once this patch? Thanks.
Comment 26 Kaustubh Atrawalkar 2011-09-04 11:58:28 PDT
 
> 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.
Comment 27 Ryosuke Niwa 2011-09-04 18:02:45 PDT
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.
Comment 28 Kaustubh Atrawalkar 2011-09-04 23:15:14 PDT
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.
Comment 29 Ryosuke Niwa 2011-09-04 23:26:47 PDT
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.
Comment 30 Kaustubh Atrawalkar 2011-09-04 23:29:17 PDT
Created attachment 106308 [details]
Patch

Aah..Even i missed that too.
Comment 31 Ryosuke Niwa 2011-09-04 23:34:09 PDT
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.
Comment 32 Kaustubh Atrawalkar 2011-09-04 23:37:46 PDT
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.
Comment 33 Ryosuke Niwa 2011-09-04 23:39:27 PDT
(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 :)
Comment 34 WebKit Review Bot 2011-09-05 00:46:01 PDT
Comment on attachment 106308 [details]
Patch

Clearing flags on attachment: 106308

Committed r94516: <http://trac.webkit.org/changeset/94516>
Comment 35 WebKit Review Bot 2011-09-05 00:46:07 PDT
All reviewed patches have been landed.  Closing bug.