Modify MainThreadWebSocketChannel to use enumeration for denotating its current state

Modify MainThreadWebSocketChannel to use enumeration for denotating its current state

R=tyoshino

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

[tyoshino (SeeGerritForStatus), 2013-06-07 02:26:25]

Are you sure it's fine to forget original state when suspending?

[tyoshino (SeeGerritForStatus), 2013-06-07 02:27:58]

On 2013/06/07 02:26:25, tyoshino wrote:
> Are you sure it's fine to forget original state when suspending?

Merging m_closed and m_closing sounds good. Anyway, we need to assert that the
enum provides state machine equivalent to the old one.

[vivekg_samsung, 2013-06-07 03:48:30]

On 2013/06/07 02:27:58, tyoshino wrote:
> On 2013/06/07 02:26:25, tyoshino wrote:
> > Are you sure it's fine to forget original state when suspending?
> 

I am not sure whether to forget the old state. Do you have any pointers in that
direction if any?


> Merging m_closed and m_closing sounds good. Anyway, we need to assert that the
> enum provides state machine equivalent to the old one.

Yes we need to have the state machine as the current implementation.

[tyoshino (SeeGerritForStatus), 2013-06-07 05:38:16]

On 2013/06/07 03:48:30, vivekg wrote:
> On 2013/06/07 02:27:58, tyoshino wrote:
> > On 2013/06/07 02:26:25, tyoshino wrote:
> > > Are you sure it's fine to forget original state when suspending?
> > 
> 
> I am not sure whether to forget the old state. Do you have any pointers in
that
> direction if any?
> 
> 
> > Merging m_closed and m_closing sounds good. Anyway, we need to assert that
the
> > enum provides state machine equivalent to the old one.
> 
> Yes we need to have the state machine as the current implementation.

Could you please keep m_suspended and merge only m_closed and m_closing into one
state variable?

[tyoshino (SeeGerritForStatus), 2013-06-07 08:18:27]

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (left):

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:480: LOG(Network,
"MainThreadWebSocketChannel %p startClosingHandshake() code=%d
m_receivedClosingHandshake=%d", this, m_closing, m_receivedClosingHandshake);
wow this log is a wrong. i'll fix it later. thanks.

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:199: if ((m_state ==
ChannelClosing) && !m_closingTimer.isActive())
(m_state == ChannelClosing || m_state == ChannelClosed)

maybe i'll remove this later...

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:354: if (m_client &&
(m_state != ChannelClosing) && !m_didFailOfClientAlreadyRun) {
(m_state != ChannelClosing && m_state != ChannelClosed)

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:480: if (m_state ==
ChannelClosing)
m_state == ChannelClosing || m_state == ChannelClosed

https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:668: if (m_state ==
ChannelClosing) {
m_state == ChannelClosing || m_state == ChannelClosed

anyway I'll check and remove this if later. It seems useless.

[vivekg__, 2013-06-07 08:46:07]

On 2013/06/07 08:18:27, tyoshino wrote:
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> File Source/modules/websockets/MainThreadWebSocketChannel.cpp (left):
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> Source/modules/websockets/MainThreadWebSocketChannel.cpp:480: LOG(Network,
> "MainThreadWebSocketChannel %p startClosingHandshake() code=%d
> m_receivedClosingHandshake=%d", this, m_closing, m_receivedClosingHandshake);
> wow this log is a wrong. i'll fix it later. thanks.
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> Source/modules/websockets/MainThreadWebSocketChannel.cpp:199: if ((m_state ==
> ChannelClosing) && !m_closingTimer.isActive())
> (m_state == ChannelClosing || m_state == ChannelClosed)
> 
> maybe i'll remove this later...
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> Source/modules/websockets/MainThreadWebSocketChannel.cpp:354: if (m_client &&
> (m_state != ChannelClosing) && !m_didFailOfClientAlreadyRun) {
> (m_state != ChannelClosing && m_state != ChannelClosed)
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> Source/modules/websockets/MainThreadWebSocketChannel.cpp:480: if (m_state ==
> ChannelClosing)
> m_state == ChannelClosing || m_state == ChannelClosed
> 
>
https://codereview.chromium.org/16049014/diff/7001/Source/modules/websockets/...
> Source/modules/websockets/MainThreadWebSocketChannel.cpp:668: if (m_state ==
> ChannelClosing) {
> m_state == ChannelClosing || m_state == ChannelClosed
> 
> anyway I'll check and remove this if later. It seems useless.

Thank you for the detailed review, really appreciated. And sorry about the
missing conditional bits :)

[tyoshino (SeeGerritForStatus), 2013-06-07 10:04:37]

Thank you. LGTM

[commit-bot: I haz the power, 2013-06-07 10:04:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/16049014/13001

[commit-bot: I haz the power, 2013-06-07 10:23:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/16049014/13001

[commit-bot: I haz the power, 2013-06-07 12:16:54]

Change committed as 152017