Bug 178222 - Web Inspector: Layers tab mistakenly throws out the root element's layer.
Summary: Web Inspector: Layers tab mistakenly throws out the root element's layer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 174176
  Show dependency treegraph
 
Reported: 2017-10-12 12:03 PDT by Ross Kirsling
Modified: 2017-10-16 13:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.29 KB, patch)
2017-10-12 12:07 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2017-10-12 20:52 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2017-10-12 12:03:05 PDT
Whoops, the Layers tab is currently always throwing out the layer for the root element.
(This means any worries about "no layers available" UX are moot...)

This should be fixed in LayerTreeManager#layersForNode, as the API is currently overtailored to the legacy sidebar.
Comment 1 Ross Kirsling 2017-10-12 12:07:32 PDT
Created attachment 323542 [details]
Patch
Comment 2 BJ Burg 2017-10-12 13:28:46 PDT
Comment on attachment 323542 [details]
Patch

Can you write a test for the manager level change?
Comment 3 Ross Kirsling 2017-10-12 14:54:09 PDT
It looks like there are protocol tests for LayerTree (which are not affected by this change) but no inspector tests for the manager. I'm really just trying to make the manager method a simple passthrough for the agent method -- do you know of an existing example where both get tested?
Comment 4 BJ Burg 2017-10-12 15:15:21 PDT
(In reply to Ross Kirsling from comment #3)
> It looks like there are protocol tests for LayerTree (which are not affected
> by this change) but no inspector tests for the manager. I'm really just
> trying to make the manager method a simple passthrough for the agent method
> -- do you know of an existing example where both get tested?

Tests in LayoutTests/inspector/layers/ are protocol tests, so they don't use the manager. That said, all you need to do is borrow the same test content from those tests and dump out the layers for root node.

Sorry to force you into writing tests, but we have to start somewhere. Fortunately there is test content you can already use.
Comment 5 Ross Kirsling 2017-10-12 20:52:41 PDT
Created attachment 323627 [details]
Patch
Comment 6 BJ Burg 2017-10-16 12:57:11 PDT
Comment on attachment 323627 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/LayerTreeDetailsSidebarPanel.js:83
> +            let layerForNode = layers[0] && layers[0].nodeId === this.domNode.id && !layers[0].isGeneratedContent ? layers[0] : null;

This is kind of complicated for one line, but that's okay I guess.
Comment 7 WebKit Commit Bot 2017-10-16 13:25:33 PDT
Comment on attachment 323627 [details]
Patch

Clearing flags on attachment: 323627

Committed r223428: <https://trac.webkit.org/changeset/223428>
Comment 8 WebKit Commit Bot 2017-10-16 13:25:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-10-16 13:28:17 PDT
<rdar://problem/35013279>