Bug 146466

Summary:

Web Inspector: no need to allocate protocolErrors array for every dispatched backend command

Product:

WebKit

Reporter:

Brian Burg <burg>

Component:

Web Inspector

Assignee:

BJ Burg <bburg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, bburg, bfulgham, commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, ossy, peavo, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

147097

Bug Blocks:

147067, 148480

Attachments:

Description	Flags
Patch (no tests)	none
Proposed Fix	none
Proposed Fix (fix linker)	none
Proposed Fix (EWS)	joepeck: review+

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.