| Summary: | ShadowRoot's nodeType should be DOCUMENT_FRAGMENT_NODE. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Hayato Ito <hayato> |
| Component: | DOM | Assignee: | Hayato Ito <hayato> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | abarth, darin, dglazkov, dominicc, enrica, ggaren, gustavo, haraken, japhet, morrita, ossy, rniwa, rolandsteiner, shinyak, webkit.review.bot, xan.lopez, zimmermann |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 77511, 78591 | ||
| Bug Blocks: | 63601 | ||
| Attachments: | |||
|
Description
Hayato Ito
2012-01-31 23:24:18 PST
I filed the bug for spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15852 I think shadowRoot's nodeName should be '#shadow-root' unless there is a strong reason for ShadowRoot to have '#document-fragment' as nodeName. I'ver renamed title of bug, focusing on only ShadowRoot.nodeType. Created attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE
Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE
Let's land this once cr-linux gets gtreen.
Comment on attachment 125257 [details] get rid of NodeType.SHADOW_ROOT_NODE Attachment 125257 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11421127 It seems we need to update JSNodeCustom too. Let me fix it. Comment on attachment 125257 [details] get rid of NodeType.SHADOW_ROOT_NODE Attachment 125257 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11421143 New failing tests: editing/deleting/delete-ligature-001.html editing/pasteboard/copy-in-password-field.html editing/deleting/delete-all-text-in-text-field-assertion.html Comment on attachment 125257 [details] get rid of NodeType.SHADOW_ROOT_NODE Attachment 125257 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11418185 Comment on attachment 125257 [details] get rid of NodeType.SHADOW_ROOT_NODE Attachment 125257 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11421140 Created attachment 125264 [details]
fix JSC.
Comment on attachment 125264 [details] fix JSC. View in context: https://bugs.webkit.org/attachment.cgi?id=125264&action=review > Source/WebCore/bindings/js/JSNodeCustom.cpp:-259 > - case Node::SHADOW_ROOT_NODE: I don't fully understand what we should do here. I need help from IDL/binding experts. > Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:169 > default: break; // XPATH_NAMESPACE_NODE The same here. Created attachment 125276 [details]
Add ASSERT
I've decided to add ASSERT there after a discussion with haraken, morrita. See the comment for the reason. Let me watch the result of ews. If all bots get green, I'll land this patch. (In reply to comment #11) > (From update of attachment 125264 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125264&action=review > > > Source/WebCore/bindings/js/JSNodeCustom.cpp:-259 > > - case Node::SHADOW_ROOT_NODE: > > I don't fully understand what we should do here. > I need help from IDL/binding experts. > > > Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:169 > > default: break; // XPATH_NAMESPACE_NODE > > The same here. It seems I wrongly cleared the r+ flag on the previous patch. So let me set r? in the latest patch. Created attachment 125282 [details]
Fix mac build hopefully
Comment on attachment 125282 [details] Fix mac build hopefully Attachment 125282 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11416275 New failing tests: editing/deleting/delete-ligature-001.html editing/pasteboard/copy-in-password-field.html editing/deleting/delete-all-text-in-text-field-assertion.html Created attachment 125304 [details]
rebase some layout tests
Comment on attachment 125304 [details] rebase some layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=125304&action=review > LayoutTests/ChangeLog:12 > + * fast/dom/shadow/nodetype-expected.txt: I rebased the following 3 layout tests. 10 * editing/deleting/delete-ligature-001-expected.txt: 11 * editing/pasteboard/copy-in-password-field-expected.txt: 12 * fast/dom/shadow/nodetype-expected.txt: I am not sure that rebasing is correct way to fix. Its difficult to judge for me, but I'd like to avoid inserting, 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && node->isShadowRoot()' to every places where 'node->nodeType() == DOCUMENT_FRAGMENT_NODE' appears in WebKit's codebase. I expect its be time to rebase. I am adding rniwa and ggaren as reviewers since I expect they know much better about this rebasing make sense or not. [Wrong in the previous post] > 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && node->isShadowRoot()' [Correct] > 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && !node->isShadowRoot()' Created attachment 125307 [details]
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.
Comment on attachment 125307 [details] cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen. View in context: https://bugs.webkit.org/attachment.cgi?id=125307&action=review Ok. But why is the mac EWS sad? Can you fix before landing? > Source/WebCore/bindings/js/JSNodeCustom.cpp:259 > + // Once we have such APIs, we should call toV8(static_cast<ShadowRoot*>..). not sure why you're mentioning V8 in JSC bindings. Typo or mystery? :) Comment on attachment 125307 [details] cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen. You need to update http://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L428 In general, you have to examine every place that compares the node type to Node::DOCUMENT_FRAGMENT_NODE. Created attachment 125570 [details]
regression has gone.
Comment on attachment 125570 [details] regression has gone. View in context: https://bugs.webkit.org/attachment.cgi?id=125570&action=review > Source/WebCore/editing/FrameSelection.cpp:428 > +static inline bool isHighestAncestorDocumentFragment(Node* node) I would have named this function nodeIsDetachedFromDocument or negated the condition inside and called it nodeIsInDocument. Created attachment 125575 [details]
ready for review. let me watch whether mac-build error is reproducible or not
Thank you for the reviews. (In reply to comment #21) > (From update of attachment 125307 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125307&action=review > > Ok. But why is the mac EWS sad? Can you fix before landing? > > > Source/WebCore/bindings/js/JSNodeCustom.cpp:259 > > + // Once we have such APIs, we should call toV8(static_cast<ShadowRoot*>..). > > not sure why you're mentioning V8 in JSC bindings. Typo or mystery? :) Done. It seems I forgot to update the comment after coping it from V8's. (In reply to comment #22) > (From update of attachment 125307 [details]) > You need to update http://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L428 You are my hero. All regression of LayoutTest has gone. Thank you! (In reply to comment #25) > (From update of attachment 125570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125570&action=review > > > Source/WebCore/editing/FrameSelection.cpp:428 > > +static inline bool isHighestAncestorDocumentFragment(Node* node) > > I would have named this function nodeIsDetachedFromDocument or negated the condition inside and called it nodeIsInDocument. Sounds good name. Done. Ready for review. Could you take another look? Comment on attachment 125575 [details]
ready for review. let me watch whether mac-build error is reproducible or not
How about line 1000 in Range.cpp:
if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) {
line 1054:
lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode;
Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294:
bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE;
in ContainerNode.cpp?
There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison.
Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.) 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break. 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches. In general, I chose A over B as long as layout test do not break. A. 'node->nodeType() == DOCUMENT_TYPE_NODE B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()' My approach is totally assuming existing layout tests will catch a regression, hopefully. If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches. Yours is that I tried at first. I tried to replace all B with A at first. But I thought most of them are unnecessary and I got to feel I should rely on existing layout tests for now. So either is okay for me. Changing all B with A is an easy task and safest, but I am afraid that will leave unnecessary cluttering of code. Any advices are welcome. (In reply to comment #29) > (From update of attachment 125575 [details]) > How about line 1000 in Range.cpp: > if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) { > > line 1054: > lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode; > > Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294: > bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE; > in ContainerNode.cpp? > > There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison. <correction> A and B should be swapped in the my explanation. (In reply to comment #30) > Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.) > > 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break. > 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches. > > In general, I chose A over B as long as layout test do not break. > A. 'node->nodeType() == DOCUMENT_TYPE_NODE > B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()' > > My approach is totally assuming existing layout tests will catch a regression, hopefully. > If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches. > > Yours is that I tried at first. I tried to replace all B with A at first. But I thought most of them are unnecessary and I got to feel I should rely on existing layout tests for now. > > So either is okay for me. Changing all B with A is an easy task and safest, but I am afraid that will leave unnecessary cluttering of code. > > Any advices are welcome. > > > > > > > (In reply to comment #29) > > (From update of attachment 125575 [details] [details]) > > How about line 1000 in Range.cpp: > > if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) { > > > > line 1054: > > lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode; > > > > Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294: > > bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE; > > in ContainerNode.cpp? > > > > There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison. (In reply to comment #30) > Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.) > > 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break. Unfortunately, having passed layout test isn't a good indication of the change being correct especially in editing/DOM code. I can make hundreds of incorrect changes without breaking existing layout tests. > 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches. > > In general, I chose A over B as long as layout test do not break. > A. 'node->nodeType() == DOCUMENT_TYPE_NODE > B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()' > > My approach is totally assuming existing layout tests will catch a regression, hopefully. I don't think we can make this assumption. We need to examine every and each occurrence of DOCUMENT_TYPE_NODE and carefully decide whether we need to do A or B. > If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches. I tend to agree with you. But we should do our best to prevent regressions by inspecting every and each occurrence of DOCUMENT_TYPE_NODE in codebase like we did for parentNode when we switched text form controls to new shadow dom model. I'm with the answer that all such conditions are tested by existing layout tests but when they're not, we need to make educated decisions. For example, the code in FrameSelection I asked you to modify may have introduced a crash if it wasn't properly updated. I'm more than happy to help you on such code inspection since I've helped other contributors creating similar patches in the past. However, I don't think we should land the patch as is without it. Luckily, this process won't be as bad as parentNode one since there are only 49 occurrences of DOCUMENT_FRAGMENT_NODE in WebCore. Thank you for the comment. I tend to agree with you. I am thinking that we should have most safest way to prevent any regressions. To avoid having a lot of responsibility in one patch, I'd like to choose replacing all A with B in this patch. Then, in follow-up patches, we should try to remove unnecessary isShadowRootCheck one-by-one. Let me replace all A with B in the next patch. Thank you. (In reply to comment #32) > (In reply to comment #30) > > Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.) > > > > 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break. > > Unfortunately, having passed layout test isn't a good indication of the change being correct especially in editing/DOM code. I can make hundreds of incorrect changes without breaking existing layout tests. > > > 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches. > > > > In general, I chose A over B as long as layout test do not break. > > A. 'node->nodeType() == DOCUMENT_TYPE_NODE > > B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()' > > > > My approach is totally assuming existing layout tests will catch a regression, hopefully. > > I don't think we can make this assumption. We need to examine every and each occurrence of DOCUMENT_TYPE_NODE and carefully decide whether we need to do A or B. > > > If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches. > > I tend to agree with you. But we should do our best to prevent regressions by inspecting every and each occurrence of DOCUMENT_TYPE_NODE in codebase like we did for parentNode when we switched text form controls to new shadow dom model. I'm with the answer that all such conditions are tested by existing layout tests but when they're not, we need to make educated decisions. For example, the code in FrameSelection I asked you to modify may have introduced a crash if it wasn't properly updated. > > I'm more than happy to help you on such code inspection since I've helped other contributors creating similar patches in the past. However, I don't think we should land the patch as is without it. This is great, guys. One thing to consider is to replace these checks with their intent-based equivalents (or create these equivalents). For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed. The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root. (In reply to comment #35) > For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed. > > The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root. Yup, that'll be an excellent way to approach patches like this. Created attachment 125787 [details]
no change in behavior
I've updated the patch so that it does not cause any change in behavior where Node.DOCUMENT_FRAGMENT_NODE is used, considering also the context around the point. I expect I covered every places. (In reply to comment #36) > (In reply to comment #35) > > For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed. > > > > The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root. > > Yup, that'll be an excellent way to approach patches like this. That's good. But it might be difficult for me to think a good name of member_function for each points. I think 'node->nodeType() == Node.DOCUMENT_FRAGMENT_NODE && !node->isShadowRoot()' is very intuitive and is not so bad for future engineers in most cases. I expect that such a long condition reminds us of 'Do we need this isShadowRoot() check? What should we do if it is ShadowRoot?' in the future. The one good news is that we don't have so many places where we must use 'isShadowRoot()' check as I thought at first. Completely ignorant question of mine: Why do we have to test nodeType() everywhere in the first place? That looks like a JavaScript-ism to me. Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()? (In reply to comment #39) > Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()? We should. However it’s a separate question whether to optimize that function rather than using a virtual function. That’s something we do for isElementNode, for example. However I don’t think that having a name isDocumentFragment would have any effect on this patch. It seemed that I missed your comment. My bad. I think we can do, assuming that there is no big penalty of performance. But I'd like to confirm whether using virtual function here is encouraged or not. If it is okay, I am willing to introduce and use isDocumentFragment() in a follow-up patch. (In reply to comment #39) > Completely ignorant question of mine: Why do we have to test nodeType() everywhere in the first place? That looks like a JavaScript-ism to me. Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()? Comment on attachment 125787 [details] no change in behavior View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review > Source/WebCore/dom/ContainerNode.cpp:72 > + if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) { Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner. I feel bad to keep pushing your patch back but we've had a lot of crashes and security vulnerabilities when we switched text form controls to use new shadow DOM model, and I want to make sure we don't repeat the same here. Thank you for the review. (In reply to comment #43) > (From update of attachment 125787 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review > > > Source/WebCore/dom/ContainerNode.cpp:72 > > + if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) { > > Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner. I am afraid it's not good name since ShadowRoot can be also *detached* from Document: var shadowHost = document.createElement('div'); var shadowRoot = new WebKitShadowRoot(shadowHost); In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document. I am willing to replace all these conditions once we have a good name. (In reply to comment #44) > I feel bad to keep pushing your patch back but we've had a lot of crashes and security vulnerabilities when we switched text form controls to use new shadow DOM model, and I want to make sure we don't repeat the same here. No problem. You don't have to feel bad since that's a signal of good reviewers. (In reply to comment #45) > Thank you for the review. > > (In reply to comment #43) > > (From update of attachment 125787 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review > > > > > Source/WebCore/dom/ContainerNode.cpp:72 > > > + if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) { > > > > Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner. > > I am afraid it's not good name since ShadowRoot can be also *detached* from Document: > var shadowHost = document.createElement('div'); > var shadowRoot = new WebKitShadowRoot(shadowHost); > In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document. > I am willing to replace all these conditions once we have a good name. That's a good point. In that case, our change to FrameSelection is incorrect. It breaks the optimization for the case where the detached fragment was a shadow root. So resolving this patch is even tricker than I initially thought. In fact, in the case of the change in FrameSelection, isDetachedDocumentFragment is exactly what it's asking. (In reply to comment #46) > (In reply to comment #45) > > Thank you for the review. > > > > (In reply to comment #43) > > > (From update of attachment 125787 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review > > > > > > > Source/WebCore/dom/ContainerNode.cpp:72 > > > > + if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) { > > > > > > Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner. > > > > I am afraid it's not good name since ShadowRoot can be also *detached* from Document: > > var shadowHost = document.createElement('div'); > > var shadowRoot = new WebKitShadowRoot(shadowHost); > > In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document. > > I am willing to replace all these conditions once we have a good name. > > That's a good point. In that case, our change to FrameSelection is incorrect. It breaks the optimization for the case where the detached fragment was a shadow root. So resolving this patch is even tricker than I initially thought. > > In fact, in the case of the change in FrameSelection, isDetachedDocumentFragment is exactly what it's asking. I see. But that should be fixed in another patch, right? It is not good to include any bug fix in this patch. Our naming of 'nodeIsDetachedFromDocument(Node* node)' in FrameSelection.cpp is not correct, but it is not the change in behavior. Hi reviewers, could you take a look? I think this patch is not harmful in theory. No change in behavior. No blocking issue. Follow-up patches could fix some minor points which were found in the review. Comment on attachment 125787 [details] no change in behavior View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review > Source/WebCore/inspector/InspectorDOMAgent.cpp:1143 > + if (!node->isShadowRoot()) > + break; > + nodeName = node->nodeName(); > + localName = node->localName(); Where did this code come from? Comment on attachment 125787 [details] no change in behavior View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review >> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143 >> + localName = node->localName(); > > Where did this code come from? Take a look at default case of this switch/case statements. Comment on attachment 125787 [details] no change in behavior View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review >>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143 >>> + localName = node->localName(); >> >> Where did this code come from? > > Take a look at default case of this switch/case statements. I think it's better to move this case below Node::ELEMENT_NODE and let it fall through when the node is a shadow root rather than duplicating code here. Comment on attachment 125787 [details] no change in behavior View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review >>>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143 >>>> + localName = node->localName(); >>> >>> Where did this code come from? >> >> Take a look at default case of this switch/case statements. > > I think it's better to move this case below Node::ELEMENT_NODE and let it fall through when the node is a shadow root rather than duplicating code here. Moving this case below Node::ELEMENT_NODE causes unnecessary isShadowRoot() calls for Node::DOCUMENT_NODE and NODE::ELEMENT_NODE, doesn't that? case Node::DOCUMENT_NODE: case Node::ELEMENT_NODE: case NODE::DOCUMENT_FRAGMENT_NODE: if (!node->isShadowRoot()) break; // Fall through default: nodename = node->nodeName(); localName = node->localName(); break; I guess you meant this? case NODE::DOCUMENT_FRAGMENT_NODE: if (!node->isShadowRoot()) break; // Fall through case Node::DOCUMENT_NODE: case Node::ELEMENT_NODE: default: nodename = node->nodeName(); localName = node->localName(); break; Created attachment 126721 [details]
fall through
After I get r+, I'll rebase and check all code path again for ToT. Comment on attachment 126721 [details]
fall through
Looks okay to me. Maybe Dimitri should take another look as well?
Comment on attachment 126721 [details]
fall through
Looks good to me.
Comment on attachment 126721 [details] fall through Clearing flags on attachment: 126721 Committed r107661: <http://trac.webkit.org/changeset/107661> All reviewed patches have been landed. Closing bug. This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt (In reply to comment #59) > This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt and Qt too - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r107661%20%2821103%29/results.html Thank you for letting me know it. Let me roll out. (In reply to comment #60) > (In reply to comment #59) > > This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt > > and Qt too - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r107661%20%2821103%29/results.html r107661 was rolled out in http://trac.webkit.org/changeset/107691. I am sure that the root cause is ASSERT in JSNodeCustom. View in context: https://bugs.webkit.org/attachment.cgi?id=126721&action=review > Source/WebCore/bindings/js/JSNodeCustom.cpp:257 > + // In case of ShadowRoot, a cached ShadowRoot binding is always used and this FYI. morrita-san, our assumption was apparently wrong. Let me update the patch so that it does not include this assertion. Created attachment 126945 [details]
Patch for landing
I've updated the patch, removing ASSERT which caused the crashes. I've tested it on Mac debug build, and confirmed that crash has gone. Let me land the patch using land-safely. Comment on attachment 126945 [details] Patch for landing Clearing flags on attachment: 126945 Committed r107700: <http://trac.webkit.org/changeset/107700> All reviewed patches have been landed. Closing bug. |