Bug 163835 - Web Inspector: Include RuntimeAgent in Workers - evaluate in Worker context
Summary: Web Inspector: Include RuntimeAgent in Workers - evaluate in Worker context
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on: 163817
Blocks: 127634 163844
  Show dependency treegraph
 
Reported: 2016-10-21 19:28 PDT by Joseph Pecoraro
Modified: 2016-10-27 15:23 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (128.06 KB, patch)
2016-10-21 19:54 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Parts (215.28 KB, patch)
2016-10-21 19:54 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (975.96 KB, application/zip)
2016-10-21 20:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.57 MB, application/zip)
2016-10-21 21:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.81 MB, application/zip)
2016-10-21 21:09 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (346.50 KB, patch)
2016-10-22 03:13 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Parts (434.46 KB, patch)
2016-10-22 03:14 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix - Rebaselined (368.93 KB, patch)
2016-10-25 16:52 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-10-21 19:28:01 PDT
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
Comment 1 Radar WebKit Bug Importer 2016-10-21 19:28:23 PDT
<rdar://problem/28901465>
Comment 2 Joseph Pecoraro 2016-10-21 19:54:09 PDT
Created attachment 292448 [details]
[PATCH] Proposed Fix

Won't build without other patches in review.
Comment 3 Joseph Pecoraro 2016-10-21 19:54:55 PDT
Created attachment 292449 [details]
[PATCH] For Bots - All Parts
Comment 4 WebKit Commit Bot 2016-10-21 19:56:48 PDT
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 5 Build Bot 2016-10-21 20:59:38 PDT
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
Comment 6 Build Bot 2016-10-21 20:59:41 PDT
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 7 Build Bot 2016-10-21 21:01:42 PDT
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
Comment 8 Build Bot 2016-10-21 21:01:45 PDT
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 9 Build Bot 2016-10-21 21:09:36 PDT
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
Comment 10 Build Bot 2016-10-21 21:09:39 PDT
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
Comment 11 Joseph Pecoraro 2016-10-22 03:10:18 PDT
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!
Comment 12 Joseph Pecoraro 2016-10-22 03:13:33 PDT
Created attachment 292489 [details]
[PATCH] Proposed Fix
Comment 13 Joseph Pecoraro 2016-10-22 03:14:24 PDT
Created attachment 292490 [details]
[PATCH] For Bots - All Parts
Comment 14 WebKit Commit Bot 2016-10-22 03:44:32 PDT
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.
Comment 15 Joseph Pecoraro 2016-10-25 16:52:14 PDT
Created attachment 292848 [details]
[PATCH] Proposed Fix - Rebaselined
Comment 16 Brian Burg 2016-10-25 21:59:20 PDT
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.
Comment 17 Joseph Pecoraro 2016-10-26 01:34:07 PDT
Such a patch is already attached. A test without adjusting these tests would fail EWS.
Comment 18 Brian Burg 2016-10-26 15:42:39 PDT
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'.
Comment 19 Joseph Pecoraro 2016-10-27 15:23:31 PDT
<https://trac.webkit.org/changeset/208009>