| Summary: | Web Inspector: Include RuntimeAgent in Workers - evaluate in Worker context | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | bburg, buildbot, commit-queue, joepeck, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | 163817 | ||||||||||||||||||||
| Bug Blocks: | 127634, 163844 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Created attachment 292448 [details]
[PATCH] Proposed Fix
Won't build without other patches in review.
Created attachment 292449 [details]
[PATCH] For Bots - All Parts
Attachment 292449 [details] did not pass style-queue:
ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 66 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 292449 [details] [PATCH] For Bots - All Parts Attachment 292449 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2342262 New failing tests: inspector/debugger/tail-deleted-frames.html inspector/debugger/tail-deleted-frames-from-vm-entry.html inspector/console/console-table.html inspector/debugger/tail-recursion.html Created attachment 292455 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292449 [details] [PATCH] For Bots - All Parts Attachment 292449 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2342256 New failing tests: inspector/worker/runtime-basic.html inspector/debugger/tail-deleted-frames-from-vm-entry.html inspector/debugger/tail-deleted-frames.html inspector/console/console-table.html inspector/debugger/tail-recursion.html Created attachment 292456 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 292449 [details] [PATCH] For Bots - All Parts Attachment 292449 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2342272 New failing tests: inspector/worker/runtime-basic.html inspector/debugger/tail-deleted-frames-from-vm-entry.html inspector/debugger/tail-deleted-frames.html inspector/console/console-table.html inspector/debugger/tail-recursion.html Created attachment 292460 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Test failures are because introducing WebInspector.Target on WebInspector.RemoteObject made it not JSON.stringify. Easy fix but its going to make the patch explode in size to clean up other tests. Sorry! Created attachment 292489 [details]
[PATCH] Proposed Fix
Created attachment 292490 [details]
[PATCH] For Bots - All Parts
Attachment 292490 [details] did not pass style-queue:
ERROR: Source/WebCore/inspector/WorkerInspectorController.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 73 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 292848 [details]
[PATCH] Proposed Fix - Rebaselined
Comment on attachment 292848 [details] [PATCH] Proposed Fix - Rebaselined View in context: https://bugs.webkit.org/attachment.cgi?id=292848&action=review > LayoutTests/inspector/model/remote-object.html:200 > + return "<filtered>"; Please split this reworking and rebaselining of remote object filters into a separate patch. No need to make this patch contain all that. It's really hard to read in bugzilla like this. Such a patch is already attached. A test without adjusting these tests would fail EWS. Comment on attachment 292848 [details] [PATCH] Proposed Fix - Rebaselined View in context: https://bugs.webkit.org/attachment.cgi?id=292848&action=review > LayoutTests/inspector/worker/runtime-basic.html:12 > + return url.replace(/^.*?LayoutTests\//, ""); Put this in inspector-test.js or something since we use it many places. > Source/WebCore/inspector/WorkerRuntimeAgent.cpp:3 > + * Copyright (C) 2015 Apple Inc. All rights reserved. Update to 2016 > Source/WebInspectorUI/UserInterface/Models/CollectionEntry.js:42 > + static fromPayload(payload, target) Should this have a default for target? > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:74 > + runAfterPendingDispatches(script) I would have expected InspectorBackend to keep this functionality. It could check each of its target connections and bail if any has pending responses. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:281 > + this._agents = InspectorBackend._agents; Similar to below comment, you can use InspectorBackend.agentsForMainConnection or something as the API. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:298 > + // FIXME: Get this list from generated InspectorBackendCommands / InspectorBackend. I think this cloning of agents should move to InspectorBackend. InspectorBackend.agentsForWorkerConnection or something could take `this` and reset the connection and dispatcher. This cloning may change if we can find a way to not add an argument to invoke(), so i'd rather it not hide in a connection object. There is no other good place to request the clones, though, since we need a connection to create a target. Please file a followup bug where we can hash this out. I don't want to dither on this massive patch but I don't like the cloning approach either (and it breaks .invoke()). > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:156 > + // FIXME: Should this respect pending dispatches in all connections? See comment in the connection method. I think the answer should be yes, but would require checking for pending responses here instead of in a connection. > Source/WebInspectorUI/UserInterface/Protocol/InspectorObserver.js:42 > + var remoteObject = WebInspector.RemoteObject.fromPayload(payload, WebInspector.mainTarget); Please make followup bug if you still want to switch argument order on this function. I think either is fine, TBH, since it's not defaulted right now anyway. > Source/WebInspectorUI/UserInterface/Protocol/Target.js:39 > + if (this._type === WebInspector.Target.Type.Main) Please make subclass? This whole class is littered with switches on the type.. > Source/WebInspectorUI/UserInterface/Protocol/Target.js:47 > + get RuntimeAgent() { return this._connection._agents.Runtime; } .. and things like this shouldn't be called on mainTarget (unless we want to make it explicit everywhere) > Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:38 > + this._targetToPathComponent = new Map; Not sure this needs to be two maps, as we don't clear them independently. > Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:263 > + _removeOtherExecutionContextPathComponent(executionContextPathComponent, skipRebuild) I would call this Extra (or something), but not 'Other'. |
Summary: Include RuntimeAgent in Workers - evaluate in Worker context. This introduces the ability to evaluate in a Worker context by letting the frontend choose between the Page's RuntimeAgent or a Worker's RuntimeAgent. This adds WebInspector.Target objects: - WebInspector.Target - Has its own list of Agents - Has a InspectorBackend.Connection to communicate with the backend - WebInspector.mainTarget - Always exists and represents the thing we are debugging (Page or JSContext) - WebInspector.targets / WebInspector.targetManager - Management of all targets This slowly introduces the concept that Model objects may be tied to a specific Target: - WebInspector.RemoteObject - in order to evaluate JS and interact with this object we must know the target (Page or Worker) - fetching PropertyDescriptor and other RemoteObjects we must continue to pass on the target Usage can access the Agent on the Target: RuntimeAgent.evaluate - Same as WebInspector.mainTarget.RuntimeAgent target.RuntimeAgent.evaluate - Ensures the Agent for a specific target