Bug 79663 - Move WebSocket APIs from WorkerContext.idl to WorkerContextWebSocket.idl
Summary: Move WebSocket APIs from WorkerContext.idl to WorkerContextWebSocket.idl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 79327
  Show dependency treegraph
 
Reported: 2012-02-27 05:11 PST by Kentaro Hara
Modified: 2012-03-23 17:56 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.66 KB, patch)
2012-02-27 05:21 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-03-02 05:26 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-02-27 05:11:40 PST
For WebKit modularization, we can move WebSocket APIs from WorkerContext.idl to WorkerContextWebSocket.idl.
Comment 1 Kentaro Hara 2012-02-27 05:21:58 PST
Created attachment 129018 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-27 06:18:00 PST
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
Comment 3 Alexey Proskuryakov 2012-02-27 09:25:38 PST
I still don't understand the benefit of such refactoring. Is anyone seriously not considering WebSocket part if the Web platform at this point?
Comment 4 Adam Barth 2012-02-27 11:10:14 PST
(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.
Comment 5 Adam Barth 2012-02-27 11:10:55 PST
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.
Comment 6 Alexey Proskuryakov 2012-02-27 11:35:54 PST
Comment on attachment 129018 [details]
Patch

So let's move the discussion further.
Comment 7 Kentaro Hara 2012-02-27 16:11:05 PST
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?
Comment 8 Alexey Proskuryakov 2012-02-27 16:34:47 PST
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.
Comment 9 Adam Barth 2012-02-27 16:37:04 PST
webkit-dev is a more appropriate forum for this discussion.
Comment 10 Kentaro Hara 2012-02-27 16:40:01 PST
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.
Comment 11 Kentaro Hara 2012-02-27 16:53:08 PST
(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.
Comment 12 Kentaro Hara 2012-03-02 05:26:39 PST
Created attachment 129886 [details]
Patch
Comment 13 Kentaro Hara 2012-03-05 17:20:47 PST
ap: review?
Comment 14 Alexey Proskuryakov 2012-03-05 19:47:47 PST
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.
Comment 15 Kentaro Hara 2012-03-05 20:02:22 PST
(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.
Comment 16 Adam Barth 2012-03-23 17:24:44 PDT
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.
Comment 17 WebKit Review Bot 2012-03-23 17:56:14 PDT
Comment on attachment 129886 [details]
Patch

Clearing flags on attachment: 129886

Committed r111949: <http://trac.webkit.org/changeset/111949>
Comment 18 WebKit Review Bot 2012-03-23 17:56:20 PDT
All reviewed patches have been landed.  Closing bug.