Bug 65012 - [CMAKE] Wrap files of websocket and worker in each macro
Summary: [CMAKE] Wrap files of websocket and worker in each macro
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-21 23:59 PDT by Gyuyoung Kim
Modified: 2011-08-15 22:18 PDT (History)
6 users (show)

See Also:


Attachments
Proposed Patch (8.52 KB, patch)
2011-07-22 00:01 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (9.89 KB, patch)
2011-07-22 01:04 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (11.74 KB, patch)
2011-08-10 23:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2011-08-10 23:33 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2011-08-12 00:19 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-07-21 23:59:55 PDT
In CMakeLists.txt, web sockets and workers files aren't wrapped by macro. So, this patch move files of web sockets and workers into each macro block.
Comment 1 Gyuyoung Kim 2011-07-22 00:01:18 PDT
Created attachment 101698 [details]
Proposed Patch
Comment 2 Gyuyoung Kim 2011-07-22 01:04:04 PDT
Created attachment 101704 [details]
Modified Patch

Move shared workers's idl files into it's macro in CMakeList.txt
Comment 3 Gyuyoung Kim 2011-07-22 01:05:23 PDT
CC'ing Patrick, Ryuan, Lucas.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-07-22 04:50:19 PDT
Is there any reason for this besides fixing the build error you mentioned in the ChangeLog entry?
Comment 5 Patrick R. Gansterer 2011-07-22 08:14:52 PDT
Comment on attachment 101704 [details]
Modified Patch

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

> Source/WebCore/CMakeLists.txt:1963
> +    IF (ENABLE_WORKERS)

I'd prefere an additional "IF (ENABLE_WEB_SOCKETS AND ENABLE_WORKERS)" section.
Comment 6 Gyuyoung Kim 2011-08-10 23:31:32 PDT
Created attachment 103581 [details]
Patch
Comment 7 Gyuyoung Kim 2011-08-10 23:33:47 PDT
Created attachment 103582 [details]
Patch
Comment 8 Gyuyoung Kim 2011-08-10 23:34:40 PDT
(In reply to comment #4)
> Is there any reason for this besides fixing the build error you mentioned in the ChangeLog entry?

Ok, I think the description is unneed. So, I remove it in ChangeLog.
Comment 9 Gyuyoung Kim 2011-08-10 23:35:23 PDT
(In reply to comment #5)
> (From update of attachment 101704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101704&action=review
> 
> > Source/WebCore/CMakeLists.txt:1963
> > +    IF (ENABLE_WORKERS)
> 
> I'd prefere an additional "IF (ENABLE_WEB_SOCKETS AND ENABLE_WORKERS)" section.

I use "IF (ENABLE_WEB_SOCKETS AND ENABLE_WORKERS)". Please review again.
Comment 10 Patrick R. Gansterer 2011-08-11 08:34:33 PDT
Comment on attachment 103582 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1978
> +        loader/WorkerThreadableLoader.cpp
> +        page/WorkerNavigator.cpp

we usually have an empty line after "change directory" (an empty line between different folders helps when reading the list of files.)

> Source/WebCore/ChangeLog:6
> +        Move files of web socket and workers into each macro block.

You don't mention any change in the CPP files. An additional option is to split this change into CMake and CPP changes.
Comment 11 Gyuyoung Kim 2011-08-12 00:19:39 PDT
Created attachment 103746 [details]
Patch
Comment 12 Gyuyoung Kim 2011-08-12 00:23:54 PDT
(In reply to comment #10)
> (From update of attachment 103582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103582&action=review
> 
> > Source/WebCore/CMakeLists.txt:1978
> > +        loader/WorkerThreadableLoader.cpp
> > +        page/WorkerNavigator.cpp
> 
> we usually have an empty line after "change directory" (an empty line between different folders helps when reading the list of files.)

Ok, I add an empty line after "change directory". However, there are many codes which don't add empty line after "change directory" in CMakeList files. If you don't have plan to fix them, I'd like to fix them.

> > Source/WebCore/ChangeLog:6
> > +        Move files of web socket and workers into each macro block.
> 
> You don't mention any change in the CPP files. An additional option is to split this change into CMake and CPP changes.

Ok, I add description related to cpp files change.
Comment 13 Patrick R. Gansterer 2011-08-12 02:21:26 PDT
Comment on attachment 103746 [details]
Patch

LGTM

(In reply to comment #12)
> Ok, I add an empty line after "change directory". However, there are many codes which don't add empty line after "change directory" in CMakeList files. If you don't have plan to fix them, I'd like to fix them.

Feel free to upload this change.
Comment 14 Leandro Pereira 2011-08-12 14:06:11 PDT
Comment on attachment 103746 [details]
Patch

LGTM also.
Comment 15 Daniel Bates 2011-08-15 21:06:09 PDT
Comment on attachment 103746 [details]
Patch

OK.
Comment 16 WebKit Review Bot 2011-08-15 22:18:53 PDT
Comment on attachment 103746 [details]
Patch

Clearing flags on attachment: 103746

Committed r93086: <http://trac.webkit.org/changeset/93086>
Comment 17 WebKit Review Bot 2011-08-15 22:18:58 PDT
All reviewed patches have been landed.  Closing bug.