Bug 138236 – Web Inspector: ALTERNATE_DISPATCHERS Let the frontend know about extra agents

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Now that the backend can have extra agents, inform the frontend about them so the frontend can lazily activate the domains and do any associated work that may be required.

[Radar WebKit Bug Importer]

<rdar://problem/18833087>

[Joseph Pecoraro]

Created attachment 240720 [details]
[PATCH] Proposed Fix

This is the final functionality patch.

After this should just be some cleanup and maybe a few bug fixes for the extra agents case as needed.

[Joseph Pecoraro]

Comment on attachment 240720 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/protocol/Inspector.json:41
> +            "description": "Fired when the backend has alternate domain that need to be activated.",

Typo: "domain" => "domains"

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Joseph Pecoraro]

Comment on attachment 240720 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:89
> +            case "DOMStorage":
> +                storageHandling = true;
> +                break;

FWIW I was just including DOMStorage for testing. I have a feeling I'll be able to remove all the cases for DOMStorage (objc generator and here).

[Brian Burg]

Comment on attachment 240720 [details]
[PATCH] Proposed Fix

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

Aside from handleExtraDomainActivation methods (which are not a big deal), it looks good to me. Any comments from Tim?

> Source/JavaScriptCore/inspector/agents/InspectorAgent.cpp:130
> +    for (auto domainName : extraDomains)

auto&

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:70
> +            PageAgent.enable();

Why do the managers decide what agents to enable, instead of WebInspector in Main.js?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:76
> +        var resourceHandling = false;

can/shouldHandleResources?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:98
> +            WebInspector.resourceSidebarPanel.handleExtraDomainActivation();

Is this a layering violation? It seems to me like the manager should fire an event. 

In fact, why can't we enable agents here, and have the managers/UI listen for an event if they need to do anything else? At a glance it's two steps removed to see what agents actually get enabled per-domain.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:105
> +            WebInspector.CSSCompletions.requestCSSNameCompletions();

same comment as above.

[Joseph Pecoraro]

(In reply to comment #6)
> Comment on attachment 240720 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240720&action=review
> 
> Aside from handleExtraDomainActivation methods (which are not a big deal),
> it looks good to me. Any comments from Tim?
> 
> > Source/JavaScriptCore/inspector/agents/InspectorAgent.cpp:130
> > +    for (auto domainName : extraDomains)
> 
> auto&
> 
> > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:70
> > +            PageAgent.enable();
> 
> Why do the managers decide what agents to enable, instead of WebInspector in
> Main.js?

I think that is just a holdover from the old frontend. That said, I want to eliminate enable/disable in the future. The backend should just auto-enable each agent.


> > Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:76
> > +        var resourceHandling = false;
> 
> can/shouldHandleResources?
> 
> > Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:98
> > +            WebInspector.resourceSidebarPanel.handleExtraDomainActivation();
> 
> Is this a layering violation? It seems to me like the manager should fire an
> event.

Doh! You're totally right.


> In fact, why can't we enable agents here, and have the managers/UI listen
> for an event if they need to do anything else? At a glance it's two steps
> removed to see what agents actually get enabled per-domain.

I think that sounds fine.


I'll clean this up and send out a new patch. Thanks

[Joseph Pecoraro]

Created attachment 240750 [details]
[PATCH] Proposed Fix

Addressed review comments.

[Build Bot]

Created attachment 240752 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Joseph Pecoraro]

Created attachment 240758 [details]
[PATCH] Proposed Fix

[Brian Burg]

Comment on attachment 240758 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:17
> +        are activated, re-perform agent specific one time initialization,

"When extra domains are activated, some agents need to be re-initialized. Dispatch a model event so views also know to re-initialize."

> Source/WebInspectorUI/ChangeLog:18
> +        and dispatch and event so others may perform re-initialization.

typo: an event

[Joseph Pecoraro]

http://trac.webkit.org/changeset/175478