Bug 79663 – Move WebSocket APIs from WorkerContext.idl to WorkerContextWebSocket.idl

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kentaro Hara]

For WebKit modularization, we can move WebSocket APIs from WorkerContext.idl to WorkerContextWebSocket.idl.

[Kentaro Hara]

Created attachment 129018 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129018 [details]
Patch

Attachment 129018 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11615039

New failing tests:
inspector/protocol/console-agent.html

[Alexey Proskuryakov]

I still don't understand the benefit of such refactoring. Is anyone seriously not considering WebSocket part if the Web platform at this point?

[Adam Barth]

(In reply to comment #3)
> I still don't understand the benefit of such refactoring. Is anyone seriously not considering WebSocket part if the Web platform at this point?

It's not about whether something is part of the web platform.  It's about the organization of the code.  The goal is to separate the concerns of features like WebSockets from the other concerns of code in WebCore.

As a result of this process, folks hacking on WebCore proper won't encounter WebSocket-specific code anymore.  All the WebSocket-specific code has been moved to the Modules/websockets directory.  If we repeat this refactoring for the many features where it's appropriate, that makes it easier to hack on WebCore proper.

There are several threads on this topic on webkit-dev if you'd like to discussion further.

[Adam Barth]

Comment on attachment 129018 [details]
Patch

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

Thanks.  Sorry I missed this one.  I was greping for ENABLE(WEB_SOCKET).  I should also grep for ENABLE_WEB_SOCKET to catch these cases.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17222
> +				A886CDC214FBBAA300D279F4 /* WorkerContextWebSocket.idl */,

When you modify the xcodeproj file, please run ./Tools/Scripts/sort-xcodeproj to keep the file sorted.

[Alexey Proskuryakov]

Comment on attachment 129018 [details]
Patch

So let's move the discussion further.

[Kentaro Hara]

Comment on attachment 129018 [details]
Patch

(In reply to comment #6)
> (From update of attachment 129018 [details])
> So let's move the discussion further.

ap: As we have discussed in the webkit-dev@ thread, we are trying to do the following two things:

[1] Move "self-contained" features from WebCore to WebCore/Modules/.
[2] Make DOMWindow.idl less epic (like moving HTML-related APIs to DOMWindowHTML.idl)

As far as I understand the discussion, while [2] is controversial, we are reaching a consensus on [1] (although we should have taken more care of licences, as you pointed out).

This patch is categorized to [1]. WebSocket is a self-contained feature and can be split out of WebCore beautifully. The benefit is as follows, as Adam pointed out:

> As a result of this process, folks hacking on WebCore proper won't encounter WebSocket-specific code anymore.  All the WebSocket-specific code has been moved to the Modules/websockets directory.  If we repeat this refactoring for the many features where it's appropriate, that makes it easier to hack on WebCore proper.

Wouldn't it make sense?

ap: review?

[Alexey Proskuryakov]

There is just one line that's being moved out of DOMWindow.idl. It was also discussed on webkit-dev that there are many downsides to such refactoring.

Speaking about WebSocket for a concrete example, has having the files and the attribute where they are caused difficulties to anyone? Do you expect that it might in any practical circumstances?

Moving properties from DOMWindow doesn't really create separation of concerns - they are still on the same object, just more difficult to work on due to arbitrary separation into multiple files. We could as well split it into 26 files based on first letter of the property.

[Adam Barth]

webkit-dev is a more appropriate forum for this discussion.

[Kentaro Hara]

Comment on attachment 129018 [details]
Patch

ap: thanks for the comments. OK, let's move the discussion to webkit-dev@. I'll post some comments in an hour.

[Kentaro Hara]

(In reply to comment #10)
> ap: thanks for the comments. OK, let's move the discussion to webkit-dev@. I'll post some comments in an hour.

ap: s/in an hour/tomorrow/ :-) We are making a webkit wiki to explain the big picture of our idea.

[Kentaro Hara]

Created attachment 129886 [details]
Patch

[Kentaro Hara]

ap: review?

[Alexey Proskuryakov]

Although there was some discussion on webkit-dev, I don't think that we achieved much agreement on what modularization should achieve. Replacing one ifdef with a separate file and tons of to project files to maintain is most definitely not an improvement in my eyes.

[Kentaro Hara]

(In reply to comment #14)
> Although there was some discussion on webkit-dev, I don't think that we achieved much agreement on what modularization should achieve.

As far as I see the discussion, it seems that we have agreement on modularizing websockets, since websockets is a self-contained feature under an enable/disable flag. Given that all other websocket APIs are already modularized, I guess that moving the last API (i.e. this patch) would make sense.

[Adam Barth]

Comment on attachment 129886 [details]
Patch

I think landing this patch fits in with the overall module pattern.  While this patch in isolation might not have the greatest benefit, following the pattern is desirable because it makes WebCore as a whole easier to hack on.

[WebKit Review Bot]

Comment on attachment 129886 [details]
Patch

Clearing flags on attachment: 129886

Committed r111949: <http://trac.webkit.org/changeset/111949>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.