Bug 98442 - [BlackBerry] Sync up CMake files
Summary: [BlackBerry] Sync up CMake files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-04 13:04 PDT by Rob Buis
Modified: 2013-01-04 09:05 PST (History)
4 users (show)

See Also:

Attachments
Patch (14.42 KB, patch)
2012-10-04 13:11 PDT, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (14.31 KB, patch)
2012-10-04 14:32 PDT, Rob Buis 		gyuyoung.kim: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2012-10-04 13:04:17 PDT 
SSIA
Comment 1 Rob Buis 2012-10-04 13:11:35 PDT 
Created attachment 167161 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-10-04 13:45:57 PDT 
Comment on attachment 167161 [details]
Patch

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

Random comments from a bystander below.

> Source/WebKit/PlatformBlackBerry.cmake:7
> +    "${JAVASCRIPTCORE_DIR}/.."

Are you sure you need this? This is the same directory you are removing below at the end of this LIST(APPEND ...) call, ${CMAKE_SOURCE_DIR}/Source. Isn't ${JAVASCRIPTCORE_DIR}/ForwardingHeaders (or some generated JSC directory) enough for the JSC API?

> Source/WebKit/PlatformBlackBerry.cmake:28
> -    "${CMAKE_SOURCE_DIR}/Source" # For JavaScriptCore API headers
> +    "${CMAKE_SOURCE_DIR}" # For JavaScriptCore API headers

The comment here does not make much sense after the change.

> Source/WebKit/PlatformBlackBerry.cmake:34
> +  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> +    "${WEBCORE_DIR}/Modules/notifications"
> +  )

The indentation looks wrong.

> Source/WebKit/PlatformBlackBerry.cmake:245
> +ENDIF (NOT PUBLIC_BUILD)

ENDIF () should be fine.

> Source/WebKit/PlatformBlackBerry.cmake:251
> +ENDIF (ENABLE_VIDEO_TRACK)

Ditto.
Comment 3 Rob Buis 2012-10-04 13:51:52 PDT 
(In reply to comment #2)
> (From update of attachment 167161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167161&action=review
> 
> Random comments from a bystander below.
> 
> > Source/WebKit/PlatformBlackBerry.cmake:7
> > +    "${JAVASCRIPTCORE_DIR}/.."
> 
> Are you sure you need this? This is the same directory you are removing below at the end of this LIST(APPEND ...) call, ${CMAKE_SOURCE_DIR}/Source. Isn't ${JAVASCRIPTCORE_DIR}/ForwardingHeaders (or some generated JSC directory) enough for the JSC API?
> 
> > Source/WebKit/PlatformBlackBerry.cmake:28
> > -    "${CMAKE_SOURCE_DIR}/Source" # For JavaScriptCore API headers
> > +    "${CMAKE_SOURCE_DIR}" # For JavaScriptCore API headers
> 
> The comment here does not make much sense after the change.
> 
> > Source/WebKit/PlatformBlackBerry.cmake:34
> > +  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> > +    "${WEBCORE_DIR}/Modules/notifications"
> > +  )
> 
> The indentation looks wrong.
> 
> > Source/WebKit/PlatformBlackBerry.cmake:245
> > +ENDIF (NOT PUBLIC_BUILD)
> 
> ENDIF () should be fine.
> 
> > Source/WebKit/PlatformBlackBerry.cmake:251
> > +ENDIF (ENABLE_VIDEO_TRACK)
> 
> Ditto.

Seems to make sense, I'll make a new patch, thanks!
Comment 4 Rob Buis 2012-10-04 14:32:41 PDT 
Created attachment 167176 [details]
Patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-10-04 14:39:54 PDT 
Comment on attachment 167176 [details]
Patch

Looks fine from a build system PoV.
Comment 6 Gyuyoung Kim 2012-10-04 18:06:16 PDT 
Comment on attachment 167176 [details]
Patch

Looks fine.
Comment 7 Rob Buis 2013-01-04 09:05:49 PST 
This was landed in r130503, closing.