[webkit-reviews] review denied: [Bug 199555] Web Inspector: Layers: Uncaught Exception: Request with id = 2 failed. {"code":-32601, "message":"'Page' domain was not found", "data":[{"code":-32601, "message":"'Page' domain was not found"}]} : [Attachment 373579] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 199555: Web Inspector: Layers: Uncaught Exception: Request with id = 2
failed. {"code":-32601,"message":"'Page' domain was not
found","data":[{"code":-32601,"message":"'Page' domain was not found"}]}
https://bugs.webkit.org/show_bug.cgi?id=199555

Attachment 373579: Patch

https://bugs.webkit.org/attachment.cgi?id=373579&action=review




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 373579
  --> https://bugs.webkit.org/attachment.cgi?id=373579
Patch

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

r- to see an updated patch. The changes in general seem the right direction.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:41
> +	   if (InspectorBackend.domains.Page &&
InspectorBackend.domains.Page.showPaintRects) {

There is a peculiar difference between these two features after the change:

  • Compositing Borders button shows even if the backend doesn't support it,
but shows disabled
  • Paint Rects button shows only if the backend supports it

I'd rather they be consistent. Seem they should both only check Page domain or
they should both check Page domain + feature. Any reason to differ?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:46
> +	       this._paintFlashingButtonNavigationItem.enabled =
!!InspectorBackend.domains.Page.setShowPaintRects;
> +	       this._paintFlashingButtonNavigationItem.activated =
InspectorBackend.domains.Page.setShowPaintRects &&
WI.settings.showPaintRects.value;

If we decide to keep the change in the condition above, then this code can be
improved.

  • We wouldn't need the `enabled` assignment since it would always be true.
  • Likewise this doesn't need to check
`InspectorBackend.domains.Page.setShowPaintRects` since it is guaranteed to be
true.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:599
> +		  target.PageAgent.setCompositingBordersVisible(activated);

Style: Leading whitespace is off (for all of these target.PageAgent calls after
a target.PageAgent check).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:743
> +		  target.PageAgent.setCompositingBordersVisible(activated);

Wrong API call! This should be PageAgent.ssetShowRulers