WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
138236
Web Inspector: ALTERNATE_DISPATCHERS Let the frontend know about extra agents
https://bugs.webkit.org/show_bug.cgi?id=138236
Summary
Web Inspector: ALTERNATE_DISPATCHERS Let the frontend know about extra agents
Joseph Pecoraro
Reported
2014-10-30 18:53:59 PDT
* 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.
Attachments
[PATCH] Proposed Fix
(26.43 KB, patch)
2014-10-30 19:04 PDT
,
Joseph Pecoraro
burg
: review+
burg
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Proposed Fix
(29.71 KB, patch)
2014-10-31 12:55 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
(521.52 KB, application/zip)
2014-10-31 14:01 PDT
,
Build Bot
no flags
Details
[PATCH] Proposed Fix
(31.20 KB, patch)
2014-10-31 14:50 PDT
,
Joseph Pecoraro
burg
: review+
burg
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2014-10-30 18:54:15 PDT
<
rdar://problem/18833087
>
Joseph Pecoraro
Comment 2
2014-10-30 19:04:08 PDT
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 3
2014-10-30 19:06:04 PDT
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
Comment 4
2014-10-30 19:06:17 PDT
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 5
2014-10-30 19:07:37 PDT
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 6
2014-10-30 19:21:36 PDT
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
Comment 7
2014-10-31 11:16:55 PDT
(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
Comment 8
2014-10-31 12:55:59 PDT
Created
attachment 240750
[details]
[PATCH] Proposed Fix Addressed review comments.
Build Bot
Comment 9
2014-10-31 14:01:30 PDT
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
Comment 10
2014-10-31 14:50:05 PDT
Created
attachment 240758
[details]
[PATCH] Proposed Fix
Brian Burg
Comment 11
2014-11-01 18:49:49 PDT
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
Comment 12
2014-11-03 11:36:35 PST
http://trac.webkit.org/changeset/175478
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug