WebSocket Pepper API: Add unit test to call Close()

Pepper WebSocket API: Add unit test to call Close()
  - Add unit tests to validate Close() operation in various situations
  - Bug fix to invoke ongoing receive completion callback on Close()

BUG=87310
TEST=ui_tests --gtest_filter='PPAPITest.WebSocket_*Close'

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

[Takashi Toyoshima, 2011-12-06 08:26:48]

On Close(), I found a bug which appears when a receive operation is in progress.
I added tests and fix the bug.

Could you review this change?

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (left):

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc#o...
ppapi/tests/test_websocket.cc:20: static const char kEchoServerURL[] =
Constant variables are not exported out of file by default in C++.
We can remove the static safely.

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:33: const uint16_t kCloseCodeNormalClosure = 1000;
This value is defined in WebSocket protocol spec.

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:275: result = callback.WaitForResult();
This |WaitForResult| found the bug.

[dmichael (off chromium), 2011-12-06 17:07:34]

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:26: const char* kInvalidURLs[] = {
'const char*' -> 'const char* const'
(indicating that the pointer can't be changed. As written, the pointed-to
characters are const, but the pointers in the array are not)

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:226: result = callback.WaitForResult();
Should you check the value of result is PP_OK?

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:280: 
I think you need to release 'reason' and 'url'

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:215: return PP_ERROR_NOACCESS;
This should probably be PP_ERROR_BADARGUMENT.

PP_ERROR_NOACCESS is for insufficient privileges, such as trying to fetch
something from a different origin using URLLoader.

[Takashi Toyoshima, 2011-12-06 17:47:18]

Update.

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:26: const char* kInvalidURLs[] = {
On 2011/12/06 17:07:34, dmichael wrote:
> 'const char*' -> 'const char* const'
> (indicating that the pointer can't be changed. As written, the pointed-to
> characters are const, but the pointers in the array are not)

Done.

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:226: result = callback.WaitForResult();
On 2011/12/06 17:07:34, dmichael wrote:
> Should you check the value of result is PP_OK?

Done.

http://codereview.chromium.org/8821008/diff/3001/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:280: 
On 2011/12/06 17:07:34, dmichael wrote:
> I think you need to release 'reason' and 'url'

Done.

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:215: return PP_ERROR_NOACCESS;
WebSocket protocol spec forbid applications to use these region of error code
because of they are reserved for lower layers. In other words, application layer
lacks privilege level to access these codes. That's why Yuzo-san and I decided
to return PP_ERROR_NOACCESS here.

[Takashi Toyoshima, 2011-12-06 17:54:51]

> webkit/plugins/ppapi/ppb_websocket_impl.cc:215: return PP_ERROR_NOACCESS;
> WebSocket protocol spec forbid applications to use these region of error code
> because of they are reserved for lower layers. In other words, application
layer
> lacks privilege level to access these codes. That's why Yuzo-san and I decided
> to return PP_ERROR_NOACCESS here.

FYI; The same error in JavaScript is InvalidAccessError exception.
See also,
http://dev.w3.org/html5/websockets/
"The close() method must run the following steps:"... (please search by
'InvalidAccessError')

[dmichael (off chromium), 2011-12-06 17:59:01]

Please add comments in the code explaining the things that look 'weird' at a
glance but are correct (e.g., magic number 1000, PP_ERROR_NOACCESS)

o/w LGTM

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:33: const uint16_t kCloseCodeNormalClosure = 1000;
On 2011/12/06 08:26:48, toyoshim wrote:
> This value is defined in WebSocket protocol spec.
Can you add a comment with a reference to it?

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:215: return PP_ERROR_NOACCESS;
On 2011/12/06 17:47:18, toyoshim wrote:
> WebSocket protocol spec forbid applications to use these region of error code
> because of they are reserved for lower layers. In other words, application
layer
> lacks privilege level to access these codes. That's why Yuzo-san and I decided
> to return PP_ERROR_NOACCESS here.
Can you add a comment referencing that?

[Takashi Toyoshima, 2011-12-06 18:27:26]

Thanks Dave.

I added comments and reference to the API spec and protocol RFC which will be
published in a few days.

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821008/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:33: const uint16_t kCloseCodeNormalClosure = 1000;
On 2011/12/06 17:59:01, dmichael wrote:
> On 2011/12/06 08:26:48, toyoshim wrote:
> > This value is defined in WebSocket protocol spec.
> Can you add a comment with a reference to it?

Done.

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8821008/diff/3001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:215: return PP_ERROR_NOACCESS;
On 2011/12/06 17:59:01, dmichael wrote:
> On 2011/12/06 17:47:18, toyoshim wrote:
> > WebSocket protocol spec forbid applications to use these region of error
code
> > because of they are reserved for lower layers. In other words, application
> layer
> > lacks privilege level to access these codes. That's why Yuzo-san and I
decided
> > to return PP_ERROR_NOACCESS here.
> Can you add a comment referencing that?

Done.

[commit-bot: I haz the power, 2011-12-06 18:29:31]

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

[commit-bot: I haz the power, 2011-12-06 18:29:35]

Presubmit check for 8821008-1011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: webkit/plugins/ppapi/ppb_websocket_impl.cc

Presubmit checks took 1.3s to calculate.