WebSocket Pepper API: allow to close connection without code and reason

WebSocket Pepper API: allow to close connection without code and reason

 - define PP_WEBSOCKETSTATUSCODE_NOT_SPECIFIED to close without status code
 - omit reason for WebKit API explicitly when code is not specified

BUG=none
TEST=browser_tests --gtest_filter'PPAPI*WebSocket_*Close'


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139764

[Takashi Toyoshima, 2012-05-14 12:05:03]

Hi David,

Could you look into this small change to support connection closure without code
and reason?

After this change, supporting blob and out of process proxy are the only things
to be done if I had no bug. So, I'll suspend pepper related works until blob
appears in ppapi.

FYI; XMLHttpRequest and WebSocket APIs are going to be changed as it accept not
ArrayBuffer but ArrayBufferView as a sending data type. When these changes are
applied to WebKit officially, we might have to discuss on the PPAPI, too.

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
File ppapi/tests/test_websocket.cc (left):

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
ppapi/tests/test_websocket.cc:552: CHECK_CALLBACK_BEHAVIOR(callback);
I'm afraid that these checks will not work in the future.

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_websocket_impl.cc:228: // |reason| must be ignored if
it is PP_VARTYPE_UNDEFINED.
I moved reason handling code inside of 'code != not specified' check.
To tell the truth, this change is not mandatory because WebKit will ignore the
reason when code is not specified. But this new version will be better because
it doesn't depend on WebKit side implementation and it can remove redundant
reason string handling.

[dmichael (off chromium), 2012-05-14 16:10:49]

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/api/ppb_websocke...
File ppapi/api/ppb_websocket.idl (right):

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/api/ppb_websocke...
ppapi/api/ppb_websocket.idl:65: PP_WEBSOCKETSTATUSCODE_NOT_SPECIFIED = 65535,
Maybe you should use -1? Since code is unsigned in the spec, that should never
conflict with a real code. What do you think?

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
File ppapi/tests/test_websocket.cc (left):

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
ppapi/tests/test_websocket.cc:552: CHECK_CALLBACK_BEHAVIOR(callback);
On 2012/05/14 12:05:04, toyoshim wrote:
> I'm afraid that these checks will not work in the future.
Can you elaborate on your concerns?

The check simply ensures that the callback followed expected PPAPI callback
behavior. I.e.:
- If the callback type is REQUIRED, and the PPAPI function happens
synchronously, that's an error.
- If the callback type is blocking, and the PPAPI function happens
asynchronously, that's an error.

The CHECK_CALLBACK_BEHAVIOR is safe. If it ever fails, it means you (or I) have
a bug. That said, we don't *have* to use it everywhere.

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_websocket_impl.cc:207: int32_t
PPB_WebSocket_Impl::Close(uint16_t code,
I think this should have changed to enum once the enum got added in the C API. I
think it should be okay to change it; it's a source incompatibility, but not an
ABI compatibility problem. What do you think?

[Takashi Toyoshima, 2012-05-15 10:55:28]

Thank you for looking.
I change type of |code| from uint16_t to enum.

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/api/ppb_websocke...
File ppapi/api/ppb_websocket.idl (right):

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/api/ppb_websocke...
ppapi/api/ppb_websocket.idl:65: PP_WEBSOCKETSTATUSCODE_NOT_SPECIFIED = 65535,
Yes, in the same reason, I defined the equivalent code as -1 in the WebKit API.
As you said in the following comments, -1 must be better if I can change
argument type from unsigned short.

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
File ppapi/tests/test_websocket.cc (left):

https://chromiumcodereview.appspot.com/10332138/diff/1/ppapi/tests/test_webso...
ppapi/tests/test_websocket.cc:552: CHECK_CALLBACK_BEHAVIOR(callback);
Sorry, I misunderstood on CHECK_CALLBACK_BEHAVIOR macro as it always checked
callback being invoked asynchronously.
OK, I keep these macro here and around.

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

https://chromiumcodereview.appspot.com/10332138/diff/1/webkit/plugins/ppapi/p...
webkit/plugins/ppapi/ppb_websocket_impl.cc:207: int32_t
PPB_WebSocket_Impl::Close(uint16_t code,
I see. SRC handles the code as int32_t, so we can safely change its type from
uint16_t to enum in the same ABI. That sound great. I'll use enum for this.

[dmichael (off chromium), 2012-05-15 15:18:33]

http://codereview.chromium.org/10332138/diff/8001/ppapi/api/ppb_websocket.idl
File ppapi/api/ppb_websocket.idl (left):

http://codereview.chromium.org/10332138/diff/8001/ppapi/api/ppb_websocket.idl...
ppapi/api/ppb_websocket.idl:247: * @param[in] code The WebSocket close code.
This is ignored if it is 0.
So, what does ignored mean in this case? When the close event fires, what is the
close code that JavaScript gets? I'm trying to find it in the spec...  I think ,
and I don't see it.
The WhatWG WebSocket spec says:
"Create an event that uses the CloseEvent interface, with the event type close
[...] whose code attribute is initialized to the WebSocket connection close
code,"
And RFC6455 says:
"   If this Close control frame contains no status code, _The WebSocket
   Connection Close Code_ is considered to be 1005."

So can we just say that the caller should provide 1005
(PP_WEBSOCKETSTATUSCODE_NO_STATUS_RECEIVED)? Or possibly provide another name
for it that maps to the same value, if you think that makes sense.

I see that the RFC says endpoints "must not" pass 1005, but...  I think that's
intended for JS, and I can't see the harm in using it this way. What do you
think?

http://codereview.chromium.org/10332138/diff/8001/ppapi/api/ppb_websocket.idl...
ppapi/api/ppb_websocket.idl:276: [in] uint16_t code,
Ack...  I'm sorry, I didn't realize you were using uint16_t in the C API, or I
wouldn't have suggested this change. If we change it here, we have to provide
backwards compatibility and support both interfaces. It may not be worth the
effort to do that.

We should probably have used an enum here from the beginning :-(.

[Takashi Toyoshima, 2012-05-16 09:20:32]

Hi David,

Thank you for looking my change in details.
As you said, if enum approach needs new API version, it's bad.
I also think reusing 1005 code will be better.

I made new CL which use 1005.
Could you review it?

https://chromiumcodereview.appspot.com/10332138/diff/8001/ppapi/api/ppb_webso...
File ppapi/api/ppb_websocket.idl (left):

https://chromiumcodereview.appspot.com/10332138/diff/8001/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:247: * @param[in] code The WebSocket close code.
This is ignored if it is 0.
When I defined this API, I think I can use '0' to specify no status code and
reason because spec want to use only codes >= 1000. I think using '0' as
_NOT_SPECIFIED instead of '-1' is still acceptable another plan.

As you said, 1005, 1006, and 1015 are defined as pseudo code to represent
network framing layer's exceptions and spec explicitly forbid to use them from
endpoints. But, I think it will be acceptable to use them as an internal
intermediate code from the viewpoint of spec. On the other hand, these values
will be defined in ppapi header files and users (developers) can check the
actual values. I guess users who see 1005 will be confused at specifying a
forbidden value. I must write some comments on this. Our WebSocket team's tech
lead also agreed reusing 1005. So 1005 looks best solution.

https://chromiumcodereview.appspot.com/10332138/diff/8001/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:276: [in] uint16_t code,
I guess uint16_t argument is compatible to one of int32_t and enum in i686 and
x86_64 ABIs because compiler generates the same caller codes for both of old and
new API definitions.
But I'm not super familiar to NaCl's ABI. As you said, reusing 1005 will be
safe.

[dmichael (off chromium), 2012-05-17 20:03:16]

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
File ppapi/api/ppb_websocket.idl (right):

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:59: enum PP_WebSocketStatusCode {
Sorry I didn't notice this before...  you're breaking source compatibility here.
I.e., existing websocket code may break and need to be modified when they update
to a new SDK that includes this change.

We can do this, but... I try to avoid it unless there's a pretty good reason. Is
it worth it in this case? Is this to bring it closer to the updated spec? I
still see "close code" phrasing in the latest spec.

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:61: * Indicates to request closing connection
without status code and reasom.
nit: reasom->reason

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:64: * the actual close frames.
nit: I think this is information most developers won't care about. I would maybe
put it in a separate paragraph and in parentheses to de-emphasize it. It's kind
of an implementation detail. I.e.,
"(Note that the code 1005..."

[Takashi Toyoshima, 2012-05-30 10:15:45]

Sorry for late update.

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
File ppapi/api/ppb_websocket.idl (right):

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:59: enum PP_WebSocketStatusCode {
The reason why I changed enum name here is that the enum members have different
prefix 'PP_WEBSOCKETSTATUSCODE_' from the enum name. Originally this is my old
mistake and looks inconsistent to other PPAPI enums.

Actually, I'm not sure that we should keep this enum name or not. I'll agree
your opinion.

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:61: * Indicates to request closing connection
without status code and reasom.
On 2012/05/17 20:03:16, dmichael wrote:
> nit: reasom->reason

Done.

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:64: * the actual close frames.
Agreed.
Done.

[dmichael (off chromium), 2012-05-30 15:20:11]

I think it's not worth it to change the enum name. Otherwise LGTM.

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
File ppapi/api/ppb_websocket.idl (right):

https://chromiumcodereview.appspot.com/10332138/diff/8015/ppapi/api/ppb_webso...
ppapi/api/ppb_websocket.idl:59: enum PP_WebSocketStatusCode {
On 2012/05/30 10:15:46, toyoshim wrote:
> The reason why I changed enum name here is that the enum members have
different
> prefix 'PP_WEBSOCKETSTATUSCODE_' from the enum name. Originally this is my old
> mistake and looks inconsistent to other PPAPI enums.
I see.

> 
> Actually, I'm not sure that we should keep this enum name or not. I'll agree
> your opinion.
I can see how the new name would be somewhat better, but I don't think it's
better enough to be worth breaking source compatibility.

[commit-bot: I haz the power, 2012-05-31 06:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10332138/21002

[commit-bot: I haz the power, 2012-05-31 09:25:47]

Change committed as 139764