Bug 65012 – [CMAKE] Wrap files of websocket and worker in each macro

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 101698 [details]
Proposed Patch

[Gyuyoung Kim]

Created attachment 101704 [details]
Modified Patch

Move shared workers's idl files into it's macro in CMakeList.txt

[Gyuyoung Kim]

CC'ing Patrick, Ryuan, Lucas.

[Raphael Kubo da Costa (:rakuco)]

Is there any reason for this besides fixing the build error you mentioned in the ChangeLog entry?

[Patrick R. Gansterer]

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.

[Gyuyoung Kim]

Created attachment 103581 [details]
Patch

[Gyuyoung Kim]

Created attachment 103582 [details]
Patch

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

(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.

[Patrick R. Gansterer]

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.

[Gyuyoung Kim]

Created attachment 103746 [details]
Patch

[Gyuyoung Kim]

(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.

[Patrick R. Gansterer]

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.

[Leandro Pereira]

Comment on attachment 103746 [details]
Patch

LGTM also.

[Daniel Bates]

Comment on attachment 103746 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 103746 [details]
Patch

Clearing flags on attachment: 103746

Committed r93086: <http://trac.webkit.org/changeset/93086>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.