Summary: | Web Inspector: move Mac-specific automation commands to a separate implementation file | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||||||
Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, bburg, commit-queue, inspector-bugzilla-changes, ossy, timothy, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 163401 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
BJ Burg
2016-10-11 14:09:18 PDT
Created attachment 291320 [details]
Proposed Fix
Created attachment 291522 [details]
Fix CMake
Created attachment 291528 [details]
Fix CMake/EFL
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. Reworking this to just move implementations. The generator side changes have landed already. Created attachment 299089 [details]
Proposed Fix v2
Created attachment 299100 [details]
Proposed Fix v2.1
Created attachment 299117 [details]
Proposed Fix v2.2
Comment on attachment 299117 [details] Proposed Fix v2.2 Clearing flags on attachment: 299117 Committed r210927: <http://trac.webkit.org/changeset/210927> All reviewed patches have been landed. Closing bug. (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 (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. |