Bug 148484 - Web Inspector: Resources tab shouldn't show HTML resource types when inspecting JSContext
Summary: Web Inspector: Resources tab shouldn't show HTML resource types when inspecti...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: EasyFix, InRadar
Depends on:
Blocks:
 
Reported: 2015-08-26 11:12 PDT by Brian Burg
Modified: 2016-12-13 15:36 PST (History)
3 users (show)

See Also:


Attachments
Screenshot of the useless UI (158.23 KB, image/png)
2015-08-26 11:12 PDT, Brian Burg
no flags Details
Proposed Fix (2.88 KB, patch)
2015-08-26 11:51 PDT, Brian Burg
joepeck: review-
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-08-26 11:12:02 PDT
I think we can simply hide the Resource Type selector from the sidebar if inspecting a JSContext.
Comment 1 Brian Burg 2015-08-26 11:12:32 PDT
Created attachment 259963 [details]
Screenshot of the useless UI
Comment 2 Radar WebKit Bug Importer 2015-08-26 11:12:46 PDT
<rdar://problem/22440394>
Comment 3 Timothy Hatcher 2015-08-26 11:34:55 PDT
It could be repurposed to filter by script type (anonymous, named, etc.)
Comment 4 Brian Burg 2015-08-26 11:51:25 PDT
Created attachment 259967 [details]
Proposed Fix
Comment 5 Brian Burg 2015-08-26 11:58:14 PDT
(In reply to comment #3)
> It could be repurposed to filter by script type (anonymous, named, etc.)

Filed: https://bugs.webkit.org/show_bug.cgi?id=148487
Comment 6 Joseph Pecoraro 2015-08-26 12:20:55 PDT
Comment on attachment 259967 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:44
> +        let shouldDisplayResourceTypes = WebInspector.debuggableType === WebInspector.DebuggableType.Web;

An augmented JavaScript debuggable could show non-Script resources if there is a NetworkAgent/PageAgent. So we may want to tweak this.
Comment 7 Brian Burg 2015-08-26 14:02:40 PDT
(In reply to comment #6)
> Comment on attachment 259967 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259967&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:44
> > +        let shouldDisplayResourceTypes = WebInspector.debuggableType === WebInspector.DebuggableType.Web;
> 
> An augmented JavaScript debuggable could show non-Script resources if there
> is a NetworkAgent/PageAgent. So we may want to tweak this.

I wish there was a 1:1 between (debuggable type+protocol version) and its capabilities. Maybe we should have something more fine-grained? I want to use feature tests like ".supportsResource(type)".

There might be similar things going on in other parts of the inspector where we show irrelevant UI, such as legacy versions.
Comment 8 Joseph Pecoraro 2015-08-26 16:08:59 PDT
Comment on attachment 259967 [details]
Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:44
>>> +        let shouldDisplayResourceTypes = WebInspector.debuggableType === WebInspector.DebuggableType.Web;
>> 
>> An augmented JavaScript debuggable could show non-Script resources if there is a NetworkAgent/PageAgent. So we may want to tweak this.
> 
> I wish there was a 1:1 between (debuggable type+protocol version) and its capabilities. Maybe we should have something more fine-grained? I want to use feature tests like ".supportsResource(type)".
> 
> There might be similar things going on in other parts of the inspector where we show irrelevant UI, such as legacy versions.

r- at the moment because of this case, let me know if you want help testing this edge case.

Augmented JSContext can have any set of capabilities and we need to feature-check the Agents to then see their capabilities. A "supportsResource(type)" might be overkill because currently each backend supports the same set of resource types (see the Page.ResourceType enum). Basically here we can check, if window.PageAgent, then all resource types are possible from the backend.

There likely are some irrelevant pieces, but I've tested it a bunch in the past, we should be pretty solid right now.
Comment 9 Devin Rousso 2015-09-08 16:00:00 PDT
Comment on attachment 259967 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:54
> +        let shouldGroupNonExclusiveItems = shouldDisplayResourceTypes;

Is this variable only for the sake of clarity, or is there some other reason you are creating another variable that duplicates the value of shouldDisplayResourceTypes?
Comment 10 Brian Burg 2015-09-08 16:08:47 PDT
Comment on attachment 259967 [details]
Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:54
>> +        let shouldGroupNonExclusiveItems = shouldDisplayResourceTypes;
> 
> Is this variable only for the sake of clarity, or is there some other reason you are creating another variable that duplicates the value of shouldDisplayResourceTypes?

It is part of my crusade against bool positional parameters. I am having more luck in our C++ code, since we can do enum classes there.