DOMWebSocket::bufferedAmount() should not return an "unsigned long"

DOMWebSocket::bufferedAmount() should not return an "unsigned long"

The WebIDL defines it as unsigned long, for which equivalent c++ should be
uint32. So changed the type of m_bufferedAmount to unsigned, and return type
of bufferedAmount() to unsigned.

BUG=432733
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185976

[Paritosh Kumar, 2014-11-17 13:36:57]

paritosh.in@samsung.com changed reviewers:
+ eroman@chromium.org, tyoshino@chromium.org

[Paritosh Kumar, 2014-11-17 13:36:58]

Please have a look on this cl.

Thanks & Regards

[tyoshino (SeeGerritForStatus), 2014-11-17 14:15:32]

Thanks for the fix. It looks we should use saturateAdd() at more places to
correctly update bufferedAmount, and to do so, saturateAdd()'s second argument
should be templatized. Feel free to land this CL once you address the comments.
We can address that later.

lgtm

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
File Source/modules/websockets/DOMWebSocket.cpp (left):

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:208: static unsigned long
saturateAdd(unsigned long a, unsigned long b)
please leave the second argument type "unsigned long".

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:352: void
DOMWebSocket::updateBufferedAmountAfterClose(unsigned long payloadSize)
please leave this unchanged

[tyoshino (SeeGerritForStatus), 2014-11-17 14:23:07]

On 2014/11/17 14:15:32, tyoshino wrote:
> Thanks for the fix. It looks we should use saturateAdd() at more places to
> correctly update bufferedAmount, and to do so, saturateAdd()'s second argument

If you have time to do this, please feel free to take it.

> should be templatized. Feel free to land this CL once you address the
comments.
> We can address that later.

[eroman, 2014-11-18 03:02:11]

Paritosh, please make sure you have signed the CL for Chromium project (as I do
not see you in the AUTHORS file yet).

NOT LGTM until that is updated.

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:363: WTF_LOG(Network, "WebSocket %p
reflectBufferedAmountConsumption() %lu => %lu", this, m_bufferedAmount,
m_bufferedAmount - m_consumedBufferedAmount);
This should be %u now

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:385:
updateBufferedAmountAfterClose(message.utf8().length());
This (and other callsites) are potentially an integer truncation (going from
size_t to unsigned). toyoshino is this a concern?

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:644: void
DOMWebSocket::didConsumeBufferedAmount(unsigned long consumed)
Should this be changed to an unsigned too?

[eroman, 2014-11-18 03:04:54]

Seems I was mistaken about no being in AUTHORS!

I will sign off once comments are addressed.

[Paritosh Kumar, 2014-11-18 03:10:37]

Thanks Eric

I'll update soon..

-Paritosh

[tyoshino (SeeGerritForStatus), 2014-11-18 03:38:03]

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:385:
updateBufferedAmountAfterClose(message.utf8().length());
On 2014/11/18 03:02:10, eroman wrote:
> This (and other callsites) are potentially an integer truncation (going from
> size_t to unsigned). toyoshino is this a concern?

Yes. They should be fixed. It's ok to either take care of it in this CL or leave
it to us.

[Paritosh Kumar, 2014-11-18 09:38:45]

Thanks Takeshi and Eric

As per your suggestion modified some function parameter.

Please have a look on this.

-Paritosh

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
File Source/modules/websockets/DOMWebSocket.cpp (left):

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:208: static unsigned long
saturateAdd(unsigned long a, unsigned long b)
On 2014/11/17 14:15:32, tyoshino wrote:
> please leave the second argument type "unsigned long".

Thanks

Done.

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:352: void
DOMWebSocket::updateBufferedAmountAfterClose(unsigned long payloadSize)
On 2014/11/17 14:15:32, tyoshino wrote:
> please leave this unchanged

Done.

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:363: WTF_LOG(Network, "WebSocket %p
reflectBufferedAmountConsumption() %lu => %lu", this, m_bufferedAmount,
m_bufferedAmount - m_consumedBufferedAmount);
On 2014/11/18 03:02:10, eroman wrote:
> This should be %u now

Done.

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:385:
updateBufferedAmountAfterClose(message.utf8().length());
Thanks, I'm leaving this one now.

https://codereview.chromium.org/735433002/diff/1/Source/modules/websockets/DO...
Source/modules/websockets/DOMWebSocket.cpp:644: void
DOMWebSocket::didConsumeBufferedAmount(unsigned long consumed)
Thanks
I think this should be changed to unsigned, since m_consumedBufferedAmount is
unsigned only.

[Paritosh Kumar, 2014-11-18 11:28:25]

paritosh.in@samsung.com changed reviewers:
+ mkwst@chromium.org

[Paritosh Kumar, 2014-11-18 11:28:26]

mkwst@chromium.org: Please review changes in source/web

[Mike West, 2014-11-18 12:36:18]

source/web LGTM.

[Mike West, 2014-11-18 12:36:34]

(please drop the "[WIP]" from the CL description)

[Paritosh Kumar, 2014-11-18 12:37:52]

On 2014/11/18 12:36:34, Mike West wrote:
> (please drop the "[WIP]" from the CL description)

Thanks Mike.
Done.

[eroman, 2014-11-18 23:42:51]

Removing myself as reviewer since I am not an OWNER for this code. However
looksgood to me.

[Paritosh Kumar, 2014-11-19 02:52:25]

@tyoshino - Please have a look.

[tyoshino (SeeGerritForStatus), 2014-11-25 07:40:15]

lgtm

[Paritosh Kumar, 2014-11-25 07:50:48]

The CQ bit was checked by paritosh.in@samsung.com

[commit-bot: I haz the power, 2014-11-25 07:51:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735433002/20001

[commit-bot: I haz the power, 2014-11-25 07:51:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 07:51:15]

A disapproval has been posted by following reviewers: eroman@chromium.org.
Please make sure to get positive review before another CQ attempt.

[tyoshino (SeeGerritForStatus), 2014-11-25 07:52:19]

On 2014/11/25 07:51:15, I haz the power (commit-bot) wrote:
> A disapproval has been posted by following reviewers:
mailto:eroman@chromium.org.
> Please make sure to get positive review before another CQ attempt.

Feel free to remove eroman from the reviewer list as he said he's fine with the
patch.

[Paritosh Kumar, 2014-11-25 07:52:55]

paritosh.in@samsung.com changed reviewers:
- eroman@chromium.org

[Paritosh Kumar, 2014-11-25 07:53:07]

The CQ bit was checked by paritosh.in@samsung.com

[Paritosh Kumar, 2014-11-25 07:54:06]

On 2014/11/25 07:52:19, tyoshino wrote:
> On 2014/11/25 07:51:15, I haz the power (commit-bot) wrote:
> > A disapproval has been posted by following reviewers:
> mailto:eroman@chromium.org.
> > Please make sure to get positive review before another CQ attempt.
> 
> Feel free to remove eroman from the reviewer list as he said he's fine with
the
> patch.

Thanks. Done.

[commit-bot: I haz the power, 2014-11-25 07:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735433002/20001

[commit-bot: I haz the power, 2014-11-25 07:54:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 07:54:17]

A disapproval has been posted by following reviewers: eroman@chromium.org.
Please make sure to get positive review before another CQ attempt.

[tyoshino (SeeGerritForStatus), 2014-11-25 07:55:56]

On 2014/11/25 07:54:17, I haz the power (commit-bot) wrote:
> A disapproval has been posted by following reviewers:
mailto:eroman@chromium.org.
> Please make sure to get positive review before another CQ attempt.

Wow.. It doesn't work. Please mail eroman@ to ask him to override the n o t l g
t m

[tyoshino (SeeGerritForStatus), 2014-11-25 07:59:58]

On 2014/11/25 07:55:56, tyoshino wrote:
> On 2014/11/25 07:54:17, I haz the power (commit-bot) wrote:
> > A disapproval has been posted by following reviewers:
> mailto:eroman@chromium.org.
> > Please make sure to get positive review before another CQ attempt.
> 
> Wow.. It doesn't work. Please mail eroman@ to ask him to override the n o t l
g
> t m

I asked one of my co-worker who knows cq's behavior more than me. Confirmed that
eroman needs to l-g-t-m this CL to put this into cq.

[eroman, 2014-11-25 20:06:10]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2014-11-25 20:06:11]

lgtm

[eroman, 2014-11-25 20:06:34]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-11-25 20:07:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735433002/20001

[commit-bot: I haz the power, 2014-11-25 21:42:05]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185976