Bug 163835 – Web Inspector: Include RuntimeAgent in Workers - evaluate in Worker context

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/28901465>

[Joseph Pecoraro]

Created attachment 292448 [details]
[PATCH] Proposed Fix

Won't build without other patches in review.

[Joseph Pecoraro]

Created attachment 292449 [details]
[PATCH] For Bots - All Parts

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

Created attachment 292489 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 292490 [details]
[PATCH] For Bots - All Parts

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

Created attachment 292848 [details]
[PATCH] Proposed Fix - Rebaselined

[Blaze Burg]

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.

[Joseph Pecoraro]

Such a patch is already attached. A test without adjusting these tests would fail EWS.

[Blaze Burg]

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'.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/208009>