Bug 67329

Summary: WebSocket should have EventTarget on the prototype chain
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: DOMAssignee: Dominic Cooney <dominicc>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, ap, arv, ashvayka, bfulgham, dglazkov, eric, ggaren, gustavo.noronha, gustavo, haraken, jamesr, laszlo.gombos, oliver, sam, webkit.review.bot, wilander, xan.lopez, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 69599    
Bug Blocks: 67312    
Attachments:
Description Flags
Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP patch
none
Patch abarth: review-

Description Dominic Cooney 2011-08-31 15:31:49 PDT
The rationale is described in meta bug 67312.
Comment 1 Dominic Cooney 2011-08-31 15:47:03 PDT
Created attachment 105843 [details]
Patch
Comment 2 Gyuyoung Kim 2011-08-31 16:05:38 PDT
Comment on attachment 105843 [details]
Patch

Attachment 105843 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9579004
Comment 3 Early Warning System Bot 2011-08-31 16:14:21 PDT
Comment on attachment 105843 [details]
Patch

Attachment 105843 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9578092
Comment 4 Yuta Kitamura 2011-08-31 20:29:14 PDT
Comment on attachment 105843 [details]
Patch

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

It seems you also need to fix project files for Qt, Win and EFL (and probably GTK too).

> Source/WebCore/ChangeLog:8
> +        Test=Existing http/tests/websockets tests.

It may be possible to write a test that checks whether a WebSocket object has EventTarget in its prototype chain by digging up __proto__ attributes.

> Source/WebCore/bindings/js/JSEventTargetCustom.cpp:200
> +    ASSERT(target->toWebSocket());

EventTarget::toWebSocket() is only defined if WEB_SOCKETS feature is enabled (#if ENABLE(WEB_SOCKETS)).
Comment 5 Collabora GTK+ EWS bot 2011-08-31 21:07:49 PDT
Comment on attachment 105843 [details]
Patch

Attachment 105843 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9570874
Comment 6 Alexey Proskuryakov 2011-08-31 22:41:10 PDT
> It may be possible to write a test that checks whether a WebSocket object has EventTarget in its prototype chain by digging up __proto__ attributes.

I think that a more direct way to test could be to define a property on window.EventTarget, and see if it appears on a WebSocket instance.
Comment 7 Dominic Cooney 2011-09-01 17:33:27 PDT
Created attachment 106069 [details]
WIP Patch
Comment 8 Dominic Cooney 2011-09-01 17:35:40 PDT
(In reply to comment #6)
> I think that a more direct way to test could be to define a property on window.EventTarget, and see if it appears on a WebSocket instance.

That means I will have to expose window.EventTarget, but since I plan to do that anyway, I may as well do it now and write a better test. Thanks for the feedback.

yutak: Thanks for the point about #if. Will do.

Just bouncing this patch off the Qt and Win bots.
Comment 9 Gyuyoung Kim 2011-09-01 18:36:14 PDT
Comment on attachment 106069 [details]
WIP Patch

Attachment 106069 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9586239
Comment 10 Dominic Cooney 2011-09-01 20:44:28 PDT
Created attachment 106089 [details]
Patch
Comment 11 Gyuyoung Kim 2011-09-01 20:56:32 PDT
Comment on attachment 106089 [details]
Patch

Attachment 106089 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9584312
Comment 12 WebKit Review Bot 2011-09-01 21:43:17 PDT
Comment on attachment 106089 [details]
Patch

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

New failing tests:
fast/dom/dom-constructors.html
fast/dom/prototype-chain.html
Comment 13 Dominic Cooney 2011-09-01 21:46:21 PDT
Created attachment 106091 [details]
Patch
Comment 14 WebKit Review Bot 2011-09-01 22:24:36 PDT
Comment on attachment 106091 [details]
Patch

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

New failing tests:
fast/dom/prototype-chain.html
Comment 15 Dominic Cooney 2011-09-02 06:49:57 PDT
Created attachment 106127 [details]
Patch
Comment 16 WebKit Review Bot 2011-09-02 07:40:52 PDT
Comment on attachment 106127 [details]
Patch

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

New failing tests:
fast/dom/prototype-chain.html
Comment 17 Dominic Cooney 2011-09-04 22:32:43 PDT
Created attachment 106306 [details]
Patch
Comment 18 Dominic Cooney 2011-09-05 11:50:59 PDT
Created attachment 106349 [details]
Patch
Comment 19 Dominic Cooney 2011-09-05 12:35:04 PDT
Comment on attachment 106349 [details]
Patch

This is ready for review now.
Comment 20 James Robinson 2011-09-12 21:40:10 PDT
This patch looks great to me.  Would someone more familiar with the JavaScriptCore bindings like to review the CodeGeneratorJS.pm changes?
Comment 21 Alexey Proskuryakov 2011-09-12 22:56:24 PDT
For some context: this idea has gained lukewarm acceptance (at best) on webkit-dev, see <https://lists.webkit.org/pipermail/webkit-dev/2011-June/017041.html>.

Since making this change doesn't seem to benefit anyone, but comes with all early adopter costs and risks, it's not clear to me why WebKit should be the first engine to make and ship it.
Comment 22 James Robinson 2011-09-13 10:48:35 PDT
(In reply to comment #21)
> For some context: this idea has gained lukewarm acceptance (at best) on webkit-dev, see <https://lists.webkit.org/pipermail/webkit-dev/2011-June/017041.html>.
> 
> Since making this change doesn't seem to benefit anyone, but comes with all early adopter costs and risks, it's not clear to me why WebKit should be the first engine to make and ship it.

I think you are a bit behind on your email, Alexey!  Maciej's point in the email discussion was that we should be aligning with standards - which seems like a good idea.  Since that thread happened there was further discussion on public-webapps and in #whatwg (which you did not participate in) and an agreement was reached with strong support from Mozilla to put EventTarget on the prototype chain.  This behavior is not standardized in the HTML spec and other related specs.

See http://www.w3.org/Bugs/Public/show_bug.cgi?id=12574 for discussion and http://html5.org/tools/web-apps-tracker?from=6377&to=6378, which is the actual change to standardize this for WebSocket.

In the future, could you please make an effort to be informed on issues before objecting to patches?
Comment 23 Alexey Proskuryakov 2011-09-13 11:00:49 PDT
James, I'm not sure how to interpret your comment. Do you actually disagree with any of the points I made?
Comment 24 James Robinson 2011-09-13 11:09:25 PDT
(In reply to comment #23)
> James, I'm not sure how to interpret your comment. Do you actually disagree with any of the points I made?

Yes.  The specific message you cite was stating that we should be trying to move towards the standard, which this patch is doing - the spec says that EventTarget should be on the prototype chain for WebSocket.

If you disagree with that change, the proper way to address it is to engage with the appropriate standards bodies.  There was discussion of this change already and clear agreement that putting EventTarget in the prototype chain is the right thing to do, and Mozilla has stated that they are moving in this direction across the board.
Comment 25 Alexey Proskuryakov 2011-09-13 11:31:52 PDT
So, what exactly did you disagree with? You must have strongly disagreed with something in my comment to choose such an impolite and unprofessional tone.

What I said was that it's not clear to me why WebKit should pay the early adopter costs for something that other browser manufacturers want, while we only reluctantly agree to. I also provided webkit-dev discussion context, which honestly should have been in the bug from the start. 

Anyway, this discussion is not productive. All I'm trying to achieve is that this patch gets reviewed by someone with a high degree of authority over JS bindings. That's not me.
Comment 26 Dominic Cooney 2011-09-29 23:04:16 PDT
Created attachment 109256 [details]
WIP patch

Updating this to HEAD.
Comment 27 Dominic Cooney 2011-10-07 23:06:32 PDT
Created attachment 110260 [details]
Patch
Comment 28 WebKit Review Bot 2011-10-07 23:09:32 PDT
Attachment 110260 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/bindings/js/JSEventTargetCustom.cpp:54:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/JSEventTargetCustom.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/JSEventTargetCustom.cpp:62:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/JSEventTargetCustom.cpp:91:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Dominic Cooney 2011-10-08 02:01:05 PDT
Comment on attachment 110260 [details]
Patch

This is ready for review now.

This patch is the first conversion from mixing in EventTarget to putting it on the prototype chain. So it is a bigger than subsequent patches will be.

The style warnings are not new; they are from moving most of JSEventTarget.cpp to JSEventTargetCustom.cpp since JSEventTarget is generated now. I am happy to sort the includes, but it will result in a lot more #ifs and I judge it not to be worth it.

I would really appreciate it if someone familiar with CodeGeneratorJSC.pm could look at that part.

There was some friction over this bug earlier, with the suggestion that WebKit should consider when to make this change. I think we should make it now and start with this patch.
Comment 30 Adam Barth 2012-05-21 15:27:46 PDT
Comment on attachment 110260 [details]
Patch

Without commenting one way or another as to whether this is something we'd like to do, this patch, as written, combines too many changes.  For example, renaming JSEventTarget.cpp to JSEventTargetCustom.cpp can be done in a separate patch and would reduce the noise in this patch.
Comment 31 Dominic Cooney 2012-06-01 12:15:29 PDT
(In reply to comment #30)
> Without commenting one way or another as to whether this is something we'd like to do, this patch, as written, combines too many changes.  For example, renaming JSEventTarget.cpp to JSEventTargetCustom.cpp can be done in a separate patch and would reduce the noise in this patch.

I have filed bug 88120 for renaming JSEventTarget.cpp to JSEventTargetCustom.cpp and turning on generation of EventTarget.idl.
Comment 32 Alexey Shvayka 2021-03-17 04:46:18 PDT
r196466 fixed the prototype chain of WebSocket (and other interfaces extending EventTarget too).

*** This bug has been marked as a duplicate of bug 154121 ***