Bug 146466 - Web Inspector: no need to allocate protocolErrors array for every dispatched backend command
Summary: Web Inspector: no need to allocate protocolErrors array for every dispatched ...
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: BJ Burg
URL:
Keywords: InRadar
Depends on: 147097
Blocks: InspectorProtocol 148480
  Show dependency tree / graph
 
Reported: 2015-06-30 12:29 PDT by Brian Burg
Modified: 2015-08-26 11:16 PDT (History)
13 users (show)

See Also:

Attachments
Patch (no tests) (88.75 KB, patch)
2015-07-06 12:12 PDT, Brian Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix (104.85 KB, patch)
2015-08-25 11:50 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix (fix linker) (105.05 KB, patch)
2015-08-25 15:19 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix (EWS) (134.93 KB, patch)
2015-08-25 16:52 PDT, BJ Burg 		joepeck: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-06-30 12:29:14 PDT 
This can be reused per domain dispatcher, removing one InspectorArray allocation per dispatched command in the normal case.

While this is done, can also extract the code for getting the parameters object out of the payload into the dispatcher's main dispatch method. This removes more duplicated code from every generated command binding.
Comment 1 Radar WebKit Bug Importer 2015-06-30 12:30:00 PDT 
<rdar://problem/21616637>
Comment 2 Brian Burg 2015-07-06 12:12:58 PDT 
Created attachment 256229 [details]
Patch (no tests)
Comment 3 Brian Burg 2015-07-06 12:13:44 PDT 
This patch makes some subtle changes to how errors are reported. I'd like to write a test to cover the existing error cases before landing this, so we have a clear idea of the behavior change.
Comment 4 BJ Burg 2015-08-25 11:50:29 PDT 
Created attachment 259866 [details]
Proposed Fix
Comment 5 WebKit Commit Bot 2015-08-25 11:52:59 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-08-25 12:18:50 PDT 
Comment on attachment 259866 [details]
Proposed Fix

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

Nice! Much cleaner

> Source/JavaScriptCore/ChangeLog:67
> +        * inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:

Duplicated in the changelog?

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:75
> +    unsigned protocolErrorCount() const { return m_protocolErrors.size(); }

This is only ever used alongside "> 0". Maybe it should just be `bool hasProtocolErrors()`?
Comment 7 BJ Burg 2015-08-25 15:19:35 PDT 
Created attachment 259889 [details]
Proposed Fix (fix linker)
Comment 8 BJ Burg 2015-08-25 16:52:20 PDT 
Created attachment 259903 [details]
Proposed Fix (EWS)

Now includes fixes for ObjC generated code.
Comment 9 Joseph Pecoraro 2015-08-26 00:55:36 PDT 
Comment on attachment 259903 [details]
Proposed Fix (EWS)

r=me
Comment 10 BJ Burg 2015-08-26 07:34:54 PDT 
Committed r188965: <http://trac.webkit.org/changeset/188965>
Comment 11 Csaba Osztrogonác 2015-08-26 08:57:56 PDT 
(In reply to comment #10)
> Committed r188965: <http://trac.webkit.org/changeset/188965>

It broke the WinCairo build: https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/48696
Comment 12 Brent Fulgham 2015-08-26 09:02:56 PDT 
Interesting. Looks like a symbol isn't getting exported. Alex: I wonder if it's our old friend "the overly zealous symbol stripper".
Comment 13 Alex Christensen 2015-08-26 11:16:30 PDT 
I deleted the WebKitBuild directory on my bot.  I'm just going to close my eyes and hope this magically fixes it.