Bug 72784 - Clean up Web Inspector target in Source/WebKit/blackberry/CMakeListsBlackBerry.txt
Summary: Clean up Web Inspector target in Source/WebKit/blackberry/CMakeListsBlackBerr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-11-18 18:34 PST by Daniel Bates
Modified: 2011-11-26 02:48 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.83 KB, patch)
2011-11-26 00:07 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (9.84 KB, patch)
2011-11-26 01:12 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2011-11-18 18:34:02 PST
Following up on Adam Barth's comment in bug #72768 (https://bugs.webkit.org/show_bug.cgi?id=72768#c8), we should clean up the Web Inspector target in Source/WebKit/blackberry/CMakeListsBlackBerry.txt so that it's more straight-forward to read and modify.
Comment 1 Daniel Bates 2011-11-18 18:49:53 PST
PR 123483
Comment 2 Jonathan Dong 2011-11-26 00:07:07 PST
Created attachment 116651 [details]
Patch
Comment 3 Nikolas Zimmermann 2011-11-26 00:15:24 PST
Comment on attachment 116651 [details]
Patch

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

I'll r- this, as I have some open questions, and it needs a fixup in the ChangeLog.

> Source/WebKit/ChangeLog:9
> +		Abstracted the JavaScript file names from inspector/front-end/inspector.html
> +		to keep them in-sync with the changes of inspector.html.
> +
> +        Reviewed by NOBODY (OOPS!).

The order is wrong here, It needs to say Reviewed by... then the description should follow.
Also the indentation is off.

> Source/WebKit/blackberry/CMakeListsBlackBerry.txt:186
> +SET (JS_FILES ${JS_FILES} ${WEBKIT_DIR}/blackberry/WebCoreSupport/inspectorBB.js)

Hm, this seems useful for any CMake based build, no?
How does eg. the Efl port manage this?
Comment 4 Jonathan Dong 2011-11-26 01:12:40 PST
Created attachment 116652 [details]
Patch
Comment 5 Nikolas Zimmermann 2011-11-26 01:22:24 PST
(In reply to comment #4)
> Created an attachment (id=116652) [details]
> Patch
Can you answer my question regarding sharing this with other CMake builds systems?
Comment 6 Jonathan Dong 2011-11-26 01:32:52 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=116652) [details] [details]
> > Patch
> Can you answer my question regarding sharing this with other CMake builds systems?

Perhaps we are the only porting who implements the inspector feature for now and uses cmake as the build system.
Although EFL porting set ENABLE_INSPECTOR on by default, but it haven't implemented the inspector feature in WebKit/WebCoreSupport/InspectorClientEfl.cpp.
WinCE set ENABLE_INSPECTOR off by default and haven't implemented the inspector feature either.

So I'm not sure whether we need to share this with other cmake porting for now, is it appropriate to add an unnecessary target into the common cmake file that other build won't use? but if it's ok, we could move the custom target into WebKit/CMakeLists.txt. Would you give me some suggestion? thanks! :)
Comment 7 Nikolas Zimmermann 2011-11-26 01:38:12 PST
Comment on attachment 116652 [details]
Patch

This is completely sufficient, if no-one else uses inspector yet with CMake based builds. If this ever happens, it can be refactored. r=me.
Comment 8 WebKit Review Bot 2011-11-26 02:48:03 PST
Comment on attachment 116652 [details]
Patch

Clearing flags on attachment: 116652

Committed r101191: <http://trac.webkit.org/changeset/101191>
Comment 9 WebKit Review Bot 2011-11-26 02:48:08 PST
All reviewed patches have been landed.  Closing bug.