Bug 146091 – Web Inspector: improve generated types for objects passed to backend commands

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

I believe this was introduced by https://bugs.webkit.org/show_bug.cgi?id=140053 which got rid of PassRefPtr in the protocol. It was a known issue, but I had to draw the line for that bug somewhere.

If we really have a non-null reference that is being 'borrowed' then the generated type signature here should be const T&.

[Darin Adler]

If there was a good reason to use PassRefPtr before, the newfangled equivalent of PassRefPtr is RefPtr&&, not const RefPtr&&.

[Brian Burg]

Created attachment 255086 [details]
WIP, need to get WebCore compiling

[Brian Burg]

(In reply to comment #1)
> If there was a good reason to use PassRefPtr before, the newfangled
> equivalent of PassRefPtr is RefPtr&&, not const RefPtr&&.

I don't think there was ever a good reason to do refcounted objects here, since the backend agents should copy data out of these marshalled, untyped objects and not take ownership.

In the JS frontend we are a lot more careful to "firewall" the inspector app's data from the protocol layer by copying out data.

In the attached patch, the signatures change to const T& (required param) and const T* (optional param). The objects are owned by the dispatcher implementations in the generated InspectorBackendDispatchers.cpp file.

[Brian Burg]

Created attachment 255126 [details]
Proposed Fix

JoePeck should take a look at this, and see what ObjC bindings stuff it broke. Should only require little fixes.

[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]

Created attachment 255130 [details]
[PATCH] ObjC Diff

These are the pieces needed to update the ObjC generator. There will be an Internal piece needed to coordinate with these changes. I'll post a combined patch.

[Joseph Pecoraro]

Comment on attachment 255126 [details]
Proposed Fix

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

r+, but I'm going to post a combined patch / handle landing to coordinate with the internal side.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:304
> +    

Nit: trailing whitespace.

> Source/WebCore/inspector/InspectorResourceAgent.h:142
> -    RefPtr<Inspector::InspectorObject> m_extraRequestHeaders;
> +    HashMap<String, String> m_extraRequestHeaders;

Nice

[Joseph Pecoraro]

Created attachment 255132 [details]
[PATCH] For Landing

[Joseph Pecoraro]

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