Bug 67329

Created attachment 105843 [details] Patch

Comment on attachment 105843 [details] Patch Attachment 105843 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9579004

Comment on attachment 105843 [details] Patch Attachment 105843 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9578092

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 on attachment 105843 [details] Patch Attachment 105843 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9570874

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

Created attachment 106069 [details] WIP Patch

(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 on attachment 106069 [details] WIP Patch Attachment 106069 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9586239

Created attachment 106089 [details] Patch

Comment on attachment 106089 [details] Patch Attachment 106089 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9584312

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

Created attachment 106091 [details] Patch

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

Created attachment 106127 [details] Patch

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

Created attachment 106306 [details] Patch

Created attachment 106349 [details] Patch

Comment on attachment 106349 [details] Patch This is ready for review now.

This patch looks great to me. Would someone more familiar with the JavaScriptCore bindings like to review the CodeGeneratorJS.pm changes?

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.

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

James, I'm not sure how to interpret your comment. Do you actually disagree with any of the points I made?

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

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.

Created attachment 109256 [details] WIP patch Updating this to HEAD.

Created attachment 110260 [details] Patch

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

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

r196466 fixed the prototype chain of WebSocket (and other interfaces extending EventTarget too). *** This bug has been marked as a duplicate of bug 154121 ***