Bug 163297 - Web Inspector: move Mac-specific automation commands to a separate implementation file
Summary: Web Inspector: move Mac-specific automation commands to a separate implementa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on: 163401
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-11 14:09 PDT by BJ Burg
Modified: 2017-01-19 16:57 PST (History)
7 users (show)

See Also:


Attachments
Proposed Fix (148.22 KB, patch)
2016-10-11 17:05 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Fix CMake (150.78 KB, patch)
2016-10-13 15:01 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Fix CMake/EFL (151.04 KB, patch)
2016-10-13 15:31 PDT, BJ Burg
joepeck: review+
Details | Formatted Diff | Diff
Proposed Fix v2 (86.65 KB, patch)
2017-01-17 17:05 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix v2.1 (90.19 KB, patch)
2017-01-17 18:16 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix v2.2 (90.31 KB, patch)
2017-01-17 21:06 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-10-11 14:09:18 PDT
>
Comment 1 BJ Burg 2016-10-11 14:09:24 PDT
<rdar://problem/28718990>
Comment 2 BJ Burg 2016-10-11 17:05:38 PDT
Created attachment 291320 [details]
Proposed Fix
Comment 3 BJ Burg 2016-10-13 15:01:53 PDT
Created attachment 291522 [details]
Fix CMake
Comment 4 BJ Burg 2016-10-13 15:31:55 PDT
Created attachment 291528 [details]
Fix CMake/EFL
Comment 5 Joseph Pecoraro 2016-10-14 14:29:49 PDT
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.
Comment 6 BJ Burg 2017-01-17 16:02:53 PST
Reworking this to just move implementations. The generator side changes have landed already.
Comment 7 BJ Burg 2017-01-17 17:05:33 PST
Created attachment 299089 [details]
Proposed Fix v2
Comment 8 BJ Burg 2017-01-17 18:16:38 PST
Created attachment 299100 [details]
Proposed Fix v2.1
Comment 9 BJ Burg 2017-01-17 21:06:51 PST
Created attachment 299117 [details]
Proposed Fix v2.2
Comment 10 WebKit Commit Bot 2017-01-19 09:53:48 PST
Comment on attachment 299117 [details]
Proposed Fix v2.2

Clearing flags on attachment: 299117

Committed r210927: <http://trac.webkit.org/changeset/210927>
Comment 11 WebKit Commit Bot 2017-01-19 09:53:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Csaba Osztrogonác 2017-01-19 12:06:06 PST
(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
Comment 13 BJ Burg 2017-01-19 16:57:14 PST
(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.