Fix WebSocket{Read,Write}Queue.

Fix WebSocket{Read,Write}Queue.

This CL fixes:
- WebSocketReadQueue may discard data unexpectedly.
- WebSocket{Read,Write}Queue: may stop processing queued operations.

BUG=490196, 490200
TEST=None

Committed: https://crrev.com/b34518faaa7fa63d3b99b91ef28bd98bf7e725f1
Cr-Commit-Position: refs/heads/master@{#330846}

[yzshen1, 2015-05-20 17:54:52]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org

[yzshen1, 2015-05-20 17:54:52]

Hi, John.

Would you please take a look? Thanks!

[jam, 2015-05-20 18:23:13]

https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):

https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
mojo/services/network/public/cpp/web_socket_read_queue.cc:68: destructed_ =
&destructed;
checkout this pattern:
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...

I had used booleans like you did in the past, but the weak factory approach is a
bit cleaner.

[yzshen1, 2015-05-20 19:23:33]

https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):

https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
mojo/services/network/public/cpp/web_socket_read_queue.cc:68: destructed_ =
&destructed;
On 2015/05/20 18:23:13, jam wrote:
> checkout this pattern:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
> 
> I had used booleans like you did in the past, but the weak factory approach is
a
> bit cleaner.

I agree that the pattern is cleaner.

One issue is that this folder is supposed to be consumed by WebSocket Mojo API
clients which may not depend on the chromium repo (base/ in particular). There
were discussions before about getting rid of existing base/ dependencies from
WebSocket{Read,Write}Queue.

There is presubmit check preventing me from adding new base/ includes.

If you think it is okay, I would like to leave it as it is. What do you think?
Thanks!

[jam, 2015-05-20 20:25:29]

On 2015/05/20 19:23:33, yzshen1 wrote:
>
https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
> File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):
> 
>
https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
> mojo/services/network/public/cpp/web_socket_read_queue.cc:68: destructed_ =
> &destructed;
> On 2015/05/20 18:23:13, jam wrote:
> > checkout this pattern:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
> > 
> > I had used booleans like you did in the past, but the weak factory approach
is
> a
> > bit cleaner.
> 
> I agree that the pattern is cleaner.
> 
> One issue is that this folder is supposed to be consumed by WebSocket Mojo API
> clients which may not depend on the chromium repo (base/ in particular). There
> were discussions before about getting rid of existing base/ dependencies from
> WebSocket{Read,Write}Queue.

This isn't going to be the case anymore now that we forked application/shell
interfaces. So network_services in chromium will only be used by mandoline.

> 
> There is presubmit check preventing me from adding new base/ includes.
> 
> If you think it is okay, I would like to leave it as it is. What do you think?
> Thanks!

[yzshen1, 2015-05-20 20:38:30]

On 2015/05/20 20:25:29, jam wrote:
> On 2015/05/20 19:23:33, yzshen1 wrote:
> >
>
https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
> > File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):
> > 
> >
>
https://codereview.chromium.org/1148913002/diff/1/mojo/services/network/publi...
> > mojo/services/network/public/cpp/web_socket_read_queue.cc:68: destructed_ =
> > &destructed;
> > On 2015/05/20 18:23:13, jam wrote:
> > > checkout this pattern:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...
> > > 
> > > I had used booleans like you did in the past, but the weak factory
approach
> is
> > a
> > > bit cleaner.
> > 
> > I agree that the pattern is cleaner.
> > 
> > One issue is that this folder is supposed to be consumed by WebSocket Mojo
API
> > clients which may not depend on the chromium repo (base/ in particular).
There
> > were discussions before about getting rid of existing base/ dependencies
from
> > WebSocket{Read,Write}Queue.
> 
> This isn't going to be the case anymore now that we forked application/shell
> interfaces. So network_services in chromium will only be used by mandoline.
> 
> > 
> > There is presubmit check preventing me from adding new base/ includes.
> > 
> > If you think it is okay, I would like to leave it as it is. What do you
think?
> > Thanks!

Thanks for the explanation. PTAL.

[jam, 2015-05-20 22:02:09]

lgtm

https://codereview.chromium.org/1148913002/diff/20001/mojo/services/network/p...
File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):

https://codereview.chromium.org/1148913002/diff/20001/mojo/services/network/p...
mojo/services/network/public/cpp/web_socket_read_queue.cc:39:
MOJO_DCHECK(is_busy_);
why MOJO_DCHECK instead of just DCHECK? I see many instances of the latter in
client libs, i.e.
https://code.google.com/p/chromium/codesearch#search/&q=file:public/cpp%20DCH...

[yzshen1, 2015-05-20 22:52:56]

https://codereview.chromium.org/1148913002/diff/20001/mojo/services/network/p...
File mojo/services/network/public/cpp/web_socket_read_queue.cc (right):

https://codereview.chromium.org/1148913002/diff/20001/mojo/services/network/p...
mojo/services/network/public/cpp/web_socket_read_queue.cc:39:
MOJO_DCHECK(is_busy_);
On 2015/05/20 22:02:09, jam wrote:
> why MOJO_DCHECK instead of just DCHECK? I see many instances of the latter in
> client libs, i.e.
>
https://code.google.com/p/chromium/codesearch#search/&q=file:public/cpp%20DCH...

Yeah, DCHECK is from base/ and if the client lib is intended to be consumed by
others then we shouldn't use it.

I guess that is not a concern for the code I am changing anymore. I have changed
all MOJO_DCHECK to DCHECK. Thanks!

[yzshen1, 2015-05-20 22:53:06]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-05-20 22:53:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1148913002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-20 22:54:46]

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

[commit-bot: I haz the power, 2015-05-20 23:43:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-20 23:45:39]

Patchset 3 (id:??) landed as
https://crrev.com/b34518faaa7fa63d3b99b91ef28bd98bf7e725f1
Cr-Commit-Position: refs/heads/master@{#330846}