WebSocket Pepper API: GetBufferedAmount() implementation

WebSocket Pepper API: GetBufferedAmount() implementation

BUG=87310
TEST=n/a because of waiting for landing WebKit side API change


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

[Takashi Toyoshima, 2011-11-30 07:19:15]

This change is basically only on WebSocket API and its protocol.
So, I think Yuta is the best person to look into this change first.
After that, I'd like get owners' review for LGTM.

[Takashi Toyoshima, 2011-11-30 10:32:43]

Originally, bufferedAmount implementation in WebKit has a bug.
https://bugs.webkit.org/show_bug.cgi?id=73404

This is a WebKit side bug.
Patch Set 3 is based on Kent-san's WebKit side review.

[Yuta Kitamura, 2011-11-30 11:40:48]

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

http://codereview.chromium.org/8748002/diff/4001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:261: payload_size +=
message_string->value().length();
Is this byte length in UTF-8?

http://codereview.chromium.org/8748002/diff/4001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:411: uint64_t
PPB_WebSocket_Impl::GetFrameSize(uint64_t payload_size) {
See comments below.

http://codereview.chromium.org/8748002/diff/4001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:423: uint64_t
PPB_WebSocket_Impl::SaturateAdd(uint64_t a, uint64_t b) {
I prefer this function to belong unnamed namespace. Maybe GetFrameSize() should,
too, because these two functions do not depend on any member functions or
variables.

As for function name, I slightly prefer "AddSaturated". This is optional,
though, as it's a matter of taste.

[Takashi Toyoshima, 2011-11-30 12:29:12]

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

http://codereview.chromium.org/8748002/diff/4001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:261: payload_size +=
message_string->value().length();
Yes.
StringVar::value() returns std::string& directly.
(See, ppapi/shared_impl/var.h)
So, I think length() return its size in bytes.

http://codereview.chromium.org/8748002/diff/4001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:423: uint64_t
PPB_WebSocket_Impl::SaturateAdd(uint64_t a, uint64_t b) {
OK.
I move them to top of this file as static functions.

[Yuta Kitamura, 2011-11-30 13:46:20]

Only subtle issues, lgtm. Please get approval from owners.

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

http://codereview.chromium.org/8748002/diff/6001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:42: static const size_t
kHybiBaseFramingOverhead = 2;
I guess |static| is unnecessary; in C++, namespace-level const variables have
internal linkage by default.

http://codereview.chromium.org/8748002/diff/6001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:48: static uint64_t
SaturateAdd(uint64_t a, uint64_t b) {
We prefer functions in unnamed namespace over static functions (see
http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces).

[Takashi Toyoshima, 2011-11-30 14:47:22]

use unnamed namespace for them.

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

http://codereview.chromium.org/8748002/diff/6001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:42: static const size_t
kHybiBaseFramingOverhead = 2;
On 2011/11/30 13:46:20, Yuta Kitamura wrote:
> I guess |static| is unnecessary; in C++, namespace-level const variables have
> internal linkage by default.

Done.

http://codereview.chromium.org/8748002/diff/6001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:48: static uint64_t
SaturateAdd(uint64_t a, uint64_t b) {
On 2011/11/30 13:46:20, Yuta Kitamura wrote:
> We prefer functions in unnamed namespace over static functions (see
> http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces).

Done.

[Takashi Toyoshima, 2011-12-01 04:38:44]

I added piman@ and viettrungluu@ as a OWNERS' reviewer
because I guess dmichael@ is very busy now.

For now, in viewpoint of WebSocket protocol and API, Yuta reviewed this change.
Then, I'd like to take OWNERS' review in viewpoint of PPAPI to land.

Thanks

[piman, 2011-12-01 04:56:52]

lgtm

[commit-bot: I haz the power, 2011-12-01 05:49:19]

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

[Takashi Toyoshima, 2011-12-01 05:52:13]

piman,
Thank you for reviews!

[commit-bot: I haz the power, 2011-12-01 06:55:39]

Change committed as 112419