Bug 146091 - Web Inspector: improve generated types for objects passed to backend commands
Summary: Web Inspector: improve generated types for objects passed to backend commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-17 17:44 PDT by Brian Burg
Modified: 2015-06-18 15:08 PDT (History)
10 users (show)

See Also:


Attachments
WIP, need to get WebCore compiling (22.86 KB, patch)
2015-06-17 22:51 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (92.34 KB, patch)
2015-06-18 12:56 PDT, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff
[PATCH] ObjC Diff (5.79 KB, patch)
2015-06-18 14:01 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (95.83 KB, patch)
2015-06-18 14:17 PDT, Joseph Pecoraro
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-06-17 17:44:57 PDT
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&.
Comment 1 Darin Adler 2015-06-17 18:30:58 PDT
If there was a good reason to use PassRefPtr before, the newfangled equivalent of PassRefPtr is RefPtr&&, not const RefPtr&&.
Comment 2 Brian Burg 2015-06-17 22:51:11 PDT
Created attachment 255086 [details]
WIP, need to get WebCore compiling
Comment 3 Brian Burg 2015-06-17 22:55:03 PDT
(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.
Comment 4 Brian Burg 2015-06-18 12:56:10 PDT
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.
Comment 5 WebKit Commit Bot 2015-06-18 12:58:50 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`)
Comment 6 Joseph Pecoraro 2015-06-18 14:01:23 PDT
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.
Comment 7 Joseph Pecoraro 2015-06-18 14:16:41 PDT
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
Comment 8 Joseph Pecoraro 2015-06-18 14:17:40 PDT
Created attachment 255132 [details]
[PATCH] For Landing
Comment 9 Joseph Pecoraro 2015-06-18 15:08:31 PDT
http://trac.webkit.org/changeset/185722