[webkit-reviews] review granted: [Bug 182069] Web Inspector: Replace isAncestor and isDescendant with native DOM contains method : [Attachment 332224] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 30 09:47:33 PST 2018
Brian Burg <bburg at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 182069: Web Inspector: Replace isAncestor and isDescendant with native DOM
contains method
https://bugs.webkit.org/show_bug.cgi?id=182069
Attachment 332224: Patch
https://bugs.webkit.org/attachment.cgi?id=332224&action=review
--- Comment #3 from Brian Burg <bburg at apple.com> ---
Comment on attachment 332224
--> https://bugs.webkit.org/attachment.cgi?id=332224
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=332224&action=review
r=me
Please test manually before landing. in particular, the descendant checks
previously null-checked the node argument, but now we are unconditionally
calling .contains on the node receiver which could throw an exception if it was
null.
> Source/WebInspectorUI/UserInterface/Views/BoxModelDetailsSectionRow.js:308
> + if (!element.contains(selectionRange.commonAncestorContainer))
This seems unlikely to be nil. Can you add a console assertion?
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:710
> + if (event.relatedTarget &&
this.element.contains(event.relatedTarget)) {
We should always have this.element.
> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:331
> + if (nodeUnderMouse && this.element.contains(nodeUnderMouse))
We should always have this.element.
> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:284
> + if (!element.contains(range.commonAncestorContainer))
We already unconditionally call a method on element.
> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:451
> + if (this._selectedMessages.length &&
!this._selectedMessages.some(element => element.contains(event.target))) {
Ugh, this is not good style in existing code. Sometime, _selectedMessages
should either be renamed to _selectedMessageElements, or store a non-Node type
to be less ambiguous. This change seems fine, though.
> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:45
> + if (this._statusElement.contains(event.target))
this._statusElement should be generated by setting this.status in the
constructor.
>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:55
> + if (focusedElement && this.element.contains(focusedElement))
Should always exist.
More information about the webkit-reviews
mailing list