Bug 163297 – Web Inspector: move Mac-specific automation commands to a separate implementation file

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

[Blaze Burg]

>

[Blaze Burg]

<rdar://problem/28718990>

[Blaze Burg]

Created attachment 291320 [details]
Proposed Fix

[Blaze Burg]

Created attachment 291522 [details]
Fix CMake

[Blaze Burg]

Created attachment 291528 [details]
Fix CMake/EFL

[Joseph Pecoraro]

Comment on attachment 291528 [details]
Fix CMake/EFL

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

r=me, but I do think it would be better to just update the JSON and the Generator instead of using the preprocessor and two levels of generation. More comments below

> Source/WebKit2/CMakeLists.txt:836
> +# Right now we don't need to build with any defines, but it might be necessary later.
> +set(WebKit2_AUTOMATION_PROTOCOL_PREPROCESSOR_DEFINES
> +)

Is this worth keeping? We could just add it when its needed.

> Source/WebKit2/UIProcess/Automation/Automation.json.in:281
> +#if !defined(PLATFORM_IOS) || !PLATFORM_IOS

Having thought about this for a little while, this seems overly complex. The json file is already an input for generation. Its like having an Automation.in.in to generate Automation.in to generate code.

Given its limited to just this it seems fine for now but longer term it just seems it would be better to move back to a single Automation.json that can handle customization per-command. It might be easier than expected.

The generator already has support for a whole domain:

    "featureGuard": "ENABLE(INDEXED_DATABASE)"

If that, or something similar, was carried to individual commands it seems things would be much simpler.

Likewise, having 1 json file that can be opened and edited with JSON tools would be nice. Having #if/else in json files probably breaks syntax highlighting in some editors.

> Source/WebKit2/UIProcess/Automation/cocoa/WebAutomationSessionCocoa.mm:40
> +#pragma mark Platform-dependent Implementations

This is not needed, it is in a FooCocoa.mm file.

[Blaze Burg]

Reworking this to just move implementations. The generator side changes have landed already.

[Blaze Burg]

Created attachment 299089 [details]
Proposed Fix v2

[Blaze Burg]

Created attachment 299100 [details]
Proposed Fix v2.1

[Blaze Burg]

Created attachment 299117 [details]
Proposed Fix v2.2

[WebKit Commit Bot]

Comment on attachment 299117 [details]
Proposed Fix v2.2

Clearing flags on attachment: 299117

Committed r210927: <http://trac.webkit.org/changeset/210927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #10)
> Comment on attachment 299117 [details]
> Proposed Fix v2.2
> 
> Clearing flags on attachment: 299117
> 
> Committed r210927: <http://trac.webkit.org/changeset/210927>

It broke the Apple Mac cmake build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/11365

[Blaze Burg]

(In reply to comment #12)
> (In reply to comment #10)
> > Comment on attachment 299117 [details]
> > Proposed Fix v2.2
> > 
> > Clearing flags on attachment: 299117
> > 
> > Committed r210927: <http://trac.webkit.org/changeset/210927>
> 
> It broke the Apple Mac cmake build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/11365

Oops, we need to pass --platform=macOS when building for Mac with CMake.