Bug 102889 - WebSocket's MessageEvent.origin attribute is an empty string
Summary: WebSocket's MessageEvent.origin attribute is an empty string
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: Chris Dumez
URL: http://www.whatwg.org/specs/web-apps/...
Keywords:
Depends on: 102887
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 00:01 PST by Chris Dumez
Modified: 2012-11-23 03:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (14.22 KB, patch)
2012-11-23 01:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.43 KB, patch)
2012-11-23 02:49 PST, Chris Dumez
haraken: review+
haraken: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (12.43 KB, patch)
2012-11-23 02:58 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-11-21 00:01:28 PST
According to specification (http://www.w3.org/TR/2012/CR-websockets-20120920/#feedback-from-the-protocol), we should "initialize [message] event's origin attribute to the Unicode serialization of the origin of the URL that was passed to the WebSocket object's constructor.". However, the current implementation returns an empty string for the event's origin.

Firefox seems to populate MessageEvent.origin according to the specification.
Comment 1 Ian 'Hixie' Hickson 2012-11-21 09:18:37 PST
Note that that URL is out of date. The current spec for Web Sockets is at:
   http://www.whatwg.org/specs/web-apps/current-work/multipage/network.html#feedback-from-the-protocol

Basically you never want to be referring to /TR/ versions of specs because they're always going to be out of date, because they're just snapshots of the current work.

In this particular case the requirement in question hasn't changed since the W3C made that snapshot, though.
Comment 2 Chris Dumez 2012-11-23 01:41:32 PST
Created attachment 175753 [details]
Patch
Comment 3 Kentaro Hara 2012-11-23 01:55:13 PST
Comment on attachment 175753 [details]
Patch

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

> LayoutTests/ChangeLog:15
> +        * http/tests/websocket/tests/hybi/send-once_wsh.py: Added.

What's this? Why can't you write a layout test using the current layout test framework?

> LayoutTests/http/tests/websocket/tests/hybi/send-once_wsh.py:2
> +# Copyright (C) 2009 Google Inc. All rights reserved.
> +# Copyright (C) 2012 Intel Corporation. All rights reserved.

Is this copyright correct? If this is a new file, the copyright would be intel's one.

> LayoutTests/http/tests/websocket/tests/hybi/send-onmessage-origin.html:20
> +var MESSAGE_TO_SEND = "This is the message to send to the server.";

Nit: messageToSend. WebKit uses camelCase variables even for constants.

> LayoutTests/http/tests/websocket/tests/hybi/send-onmessage-origin.html:38
> +    shouldBe("origin", "'ws://localhost:8880'");

Nit: You can use ShouldBeEqualToString("origin", "ws://localhost:8880")
Comment 4 Chris Dumez 2012-11-23 02:35:09 PST
Comment on attachment 175753 [details]
Patch

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

>> LayoutTests/http/tests/websocket/tests/hybi/send-once_wsh.py:2
>> +# Copyright (C) 2012 Intel Corporation. All rights reserved.
> 
> Is this copyright correct? If this is a new file, the copyright would be intel's one.

Well, this is basically a simplified version of LayoutTests/http/tests/websocket/tests/hybi/send_wsh.py, which expects 1 message instead of 2. Since the original file was with Google copyright and the code is mostly the same, I kept the copyright.
Anyway, it does not really matter since you prefer I use existing infrastructure. I guess I'll use LayoutTests/http/tests/websocket/tests/hybi/send_wsh.py and send 2 messages.
Comment 5 Kentaro Hara 2012-11-23 02:43:28 PST
(In reply to comment #4)
> Anyway, it does not really matter since you prefer I use existing infrastructure. I guess I'll use LayoutTests/http/tests/websocket/tests/hybi/send_wsh.py and send 2 messages.

Sounds nicer.
Comment 6 Chris Dumez 2012-11-23 02:49:48 PST
Created attachment 175758 [details]
Patch

Take Haraken's feedback into consideration.
Comment 7 Kentaro Hara 2012-11-23 02:53:18 PST
Comment on attachment 175758 [details]
Patch

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

Looks OK.

> LayoutTests/http/tests/websocket/tests/hybi/send-onmessage-origin.html:21
> +var FirstMessageToSend = "This is the first message to send to the server.";
> +var SecondMessageToSend = "This is the second.";

Sorry for nit-picking. WebKit uses camelCase even for constants. So this could be firstMessageToSend and secondMessageToSend.
Comment 8 Chris Dumez 2012-11-23 02:55:10 PST
(In reply to comment #7)
> (From update of attachment 175758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175758&action=review
> 
> Looks OK.
> 
> > LayoutTests/http/tests/websocket/tests/hybi/send-onmessage-origin.html:21
> > +var FirstMessageToSend = "This is the first message to send to the server.";
> > +var SecondMessageToSend = "This is the second.";
> 
> Sorry for nit-picking. WebKit uses camelCase even for constants. So this could be firstMessageToSend and secondMessageToSend.

My bad. I'll fix it.
Comment 9 Chris Dumez 2012-11-23 02:58:35 PST
Created attachment 175762 [details]
Patch for landing

Fixed camel case nit. Could someone please cq+ ?
Comment 10 Kentaro Hara 2012-11-23 03:03:43 PST
Comment on attachment 175762 [details]
Patch for landing

thanks
Comment 11 WebKit Review Bot 2012-11-23 03:52:54 PST
Comment on attachment 175762 [details]
Patch for landing

Clearing flags on attachment: 175762

Committed r135587: <http://trac.webkit.org/changeset/135587>
Comment 12 WebKit Review Bot 2012-11-23 03:52:59 PST
All reviewed patches have been landed.  Closing bug.