[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