Use enum BinaryType for WebSocket.binaryType, instead of DOMString

This addresses a FIXME in WebSocket.idl

Also removing logging for wrong value as now we are logging in binding code:
https://codereview.chromium.org/955413002/.

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

[Paritosh Kumar, 2015-01-28 13:00:55]

paritosh.in@samsung.com changed reviewers:
+ habib.virji@samsung.com

[Paritosh Kumar, 2015-01-28 13:00:55]

Please have a look.

[Habib Virji, 2015-01-28 13:11:50]

On 2015/01/28 13:00:55, Paritosh Kumar wrote:
> Please have a look.

Since DOMWebSocket.h defines returns as DOMString for binaryType, you have to
change that.

Also DOMWebSocket.cpp, also has few conversion from String to enum value. That
needs to be changed. 

What's the result of DOMWebSocketTest.cpp? It is setting binaryType as string,
perhaps updating setBinaryType is not required.

[Paritosh Kumar, 2015-01-28 13:37:11]

On 2015/01/28 13:11:50, Habib Virji wrote:
> On 2015/01/28 13:00:55, Paritosh Kumar wrote:
> > Please have a look.
> 
> Since DOMWebSocket.h defines returns as DOMString for binaryType, you have to
> change that.
> 
> Also DOMWebSocket.cpp, also has few conversion from String to enum value. That
> needs to be changed. 
> 
> What's the result of DOMWebSocketTest.cpp? It is setting binaryType as string,
> perhaps updating setBinaryType is not required.

Thanks. But, I think, similar to responseType attribute of XMLHttpRequest we
do'nt
need to change the data type of parameter for getter and setter, as in

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Habib Virji, 2015-01-28 14:06:27]

On 2015/01/28 13:37:11, Paritosh Kumar wrote:
> On 2015/01/28 13:11:50, Habib Virji wrote:
> > On 2015/01/28 13:00:55, Paritosh Kumar wrote:
> > > Please have a look.
> > 
> > Since DOMWebSocket.h defines returns as DOMString for binaryType, you have
to
> > change that.
> > 
> > Also DOMWebSocket.cpp, also has few conversion from String to enum value.
That
> > needs to be changed. 
> > 
> > What's the result of DOMWebSocketTest.cpp? It is setting binaryType as
string,
> > perhaps updating setBinaryType is not required.
> 
> Thanks. But, I think, similar to responseType attribute of XMLHttpRequest we
> do'nt
> need to change the data type of parameter for getter and setter, as in
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Though i do not have counter argument. It seems bit strange keeping String value
when enum values are defined. 

Anyways, IMO something similar has to be there "ResponseTypeCode
responseTypeCode() const { return m_responseTypeCode; }" as in XMLHttpRequest. 

I would suggest checking with nbarth and yutak might be good idea.

[Paritosh Kumar, 2015-01-29 06:41:05]

paritosh.in@samsung.com changed reviewers:
+ nbarth@chromium.org, yutak@chromium.org

[Paritosh Kumar, 2015-01-29 06:41:05]

Thanks Habib

Adding nbarth and yutak for more clarification.

@nbarth, yutak: PTAL.

[Yuta Kitamura, 2015-01-29 09:17:04]

yutak@chromium.org changed reviewers:
+ tyoshino@chromium.org

[Yuta Kitamura, 2015-01-29 09:17:04]

I'm not aware of the context behind this FIXME; tyoshino
may know something.

[tyoshino (SeeGerritForStatus), 2015-01-29 12:18:12]

On 2015/01/29 09:17:04, Yuta Kitamura wrote:
> I'm not aware of the context behind this FIXME; tyoshino
> may know something.

I'm not, too. This comment was added on
https://codereview.chromium.org/282873004/
It looks not directly related to the enum. Maybe nbarth@ noticed that the IDL is
old and doesn't use enum unlike one in the latest spec and added the TODO as a
bonus?

For XHR, we have XMLHttpRequestResponseType which is an enum of strings. The
generated code for it is just a pair of String accessors but the setter includes
validation code which rejects strings not listed in the enum. responseTypeCode()
is only for use in V8XMLHttpRequestCustom to choose the accessor to call. This
is useful but DOMWebSocket::setBinaryType() has a code to print console message
that is useful for developers to notice mistakes. So, I'm not so motivated to
use the IDL functionality while killing this error message emitting code.

[Nils Barth (inactive), 2015-02-02 23:02:18]

nbarth@chromium.org changed reviewers:
+ haraken@chromium.org

[Nils Barth (inactive), 2015-02-02 23:02:28]

- haraken@: What's our feeling on logging invalid enumeration values?
  (Current status is silently ignore them.)
- Paritosh: Could you update DOMWebSocket::setBinaryType as well?
- Yuta: Does logging discussion below make sense?

Thanks for addressing this Paritosh!
Yes, it's just saying that we should use an enum, as that's what the spec says
and it avoids having to have manual code to handle it.
(I didn't do it earlier because that changed behavior, admittedly in a small
way.)

I believe you should also update:
DOMWebSocket::setBinaryType
...to throw an exception (or have an assert?) instead of logging an error, as
the bindings will no longer call DOMWebSocket::setBinaryType with an invalid
type, so this method should NEVER be called with an invalid value.

Regarding logging:
Per Web IDL spec, invalid enum values are ignored:
http://www.w3.org/TR/WebIDL/#es-type-mapping
"In the ECMAScript binding, assignment of an invalid string value to an
attribute is ignored, while passing such a value as an operation argument
results in an exception being thrown."
https://heycam.github.io/webidl/#es-attributes

We currently do not log these.
If we want to log setting invalid enumerations, this should be done
consistently, by the *bindings* code, and shouldn't need to be done in the Blink
code (by every single attribute that has an enumeration).

The relevant code is:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Does this help?

[haraken, 2015-02-02 23:36:54]

Thanks Nils!

> - haraken@: What's our feeling on logging invalid enumeration values?
>   (Current status is silently ignore them.)

This is an edge case, so I'm OK with logging if it's really useful.

What's the behavior of other browsers?

[Paritosh Kumar, 2015-02-11 18:29:25]

Thanks to all

Apologize for delay in update.

@Nils:
>I believe you should also update:
>DOMWebSocket::setBinaryType
>...to throw an exception (or have an assert?) instead of logging an error

As label on http://crbug.com/408045 says setBinaryType wont throw exception.
Using Assert in setBinaryType().

And logging the error message in binding code  instead of blink code.


@haraken:
> What's the behavior of other browsers?

Some findings about behavior of other browsers are,

FF does not throw any exception and does not log any error message also, FF just
ignores
the wrong value.

IE throws  Exception SyntaxError when we try to set binaryType to any wrong
value.

Please have a look.

[Nils Barth (inactive), 2015-02-11 18:58:34]

Thanks for update Paritosh!
I think CL is fine, so long as you move the logging code to another CL and
commit that one *first*.

On 2015/02/11 18:29:25, Paritosh Kumar wrote:
> @Nils:
> >I believe you should also update:
> >DOMWebSocket::setBinaryType
> >...to throw an exception (or have an assert?) instead of logging an error
> 
> As label on http://crbug.com/408045 says setBinaryType wont throw exception.
> Using Assert in setBinaryType().

Sounds good!

> And logging the error message in binding code  instead of blink code.

haraken@: do we want to log to console?
If we do, this should be done in a separate CL, because it affects *ALL* enums.

> @haraken:
> > What's the behavior of other browsers?
> 
> Some findings about behavior of other browsers are,
> 
> FF does not throw any exception and does not log any error message also, FF
just
> ignores
> the wrong value.
> 
> IE throws  Exception SyntaxError when we try to set binaryType to any wrong
> value.

Thanks for looking into this!

Could you see what IE does when you try to set some other enum value?
For this question, we're interested in *enum* behavior, not binaryType behavior.
(Looks like IE is just using an old version of the spec, where this wasn't an
enum,
 per http://crbug.com/408045 )

haraken@:
I think it's fine to log or not log for invalid enum:
logging is probably helpful for developers, so I'd favor it (as per Yoshino's
comment).
...but anyway should be in a separate CL!

[Nils Barth (inactive), 2015-02-11 18:58:45]

https://codereview.chromium.org/871013007/diff/20001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/871013007/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:266:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
ErrorMessageLevel, "The provided value '"+string+"' is not a valid value of type
'{{attribute.idl_type}}'."));
Could you do this in a separate CL, since it affects *ALL* enums?

https://codereview.chromium.org/871013007/diff/20001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/871013007/diff/20001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocket.cpp:532: ASSERT_NOT_REACHED();
ASSERT sounds fine, thanks!

[jsbell, 2015-02-25 18:56:45]

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

[jsbell, 2015-02-25 18:56:45]

Awesome to see this. I'm tackling another FIXME in WebSocket.idl over in:

https://codereview.chromium.org/954933003/

These hopefully shouldn't collide other than whitespace changes.

...

Paritosh - are you still able to make progress here? I didn't see a new CL for
the bindings/logging change but may just have missed it.

IE throwing SyntaxError is based on an older iteration of the spec, which was
changed c/o https://www.w3.org/Bugs/Public/show_bug.cgi?id=18416 - and FYI,
there's a great comment in https://www.w3.org/Bugs/Public/show_bug.cgi?id=18749
about this from Hixie: "Good browsers can give a warning in their consoles, btw"

So great work making Chrome a "good browser" :)

[Paritosh Kumar, 2015-02-26 11:32:27]

Many Apologize for too much delay in update.

I uploaded a new patch for logging in binding code.

https://codereview.chromium.org/955413002/

Please have a look.

[Paritosh Kumar, 2015-03-30 08:02:32]

As now we are logging in binding code for invalid enum type, as
https://codereview.chromium.org/955413002/ does this.

Updated CL after https://codereview.chromium.org/955413002/.

PTAL.

[tyoshino (SeeGerritForStatus), 2015-03-30 08:12:02]

lgtm

Thanks!

[tyoshino (SeeGerritForStatus), 2015-03-30 08:13:01]

Please update the description to explain what you did than saying "some FIXME is
addressed" before committing

[Paritosh Kumar, 2015-03-30 08:33:12]

On 2015/03/30 08:13:01, tyoshino wrote:
> Please update the description to explain what you did than saying "some FIXME
is
> addressed" before committing

Thanks. Updated the description.

[Paritosh Kumar, 2015-03-30 08:46:02]

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

[commit-bot: I haz the power, 2015-03-30 08:46:14]

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

[commit-bot: I haz the power, 2015-03-30 09:29:28]

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

[commit-bot: I haz the power, 2015-03-30 09:29:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[Nils Barth (inactive), 2015-03-30 12:36:03]

On 2015/03/30 08:33:12, Paritosh Kumar wrote:
> Thanks. Updated the description.

In future, could you also make sure the *Title* is descriptive?
I've fixed it here, so it's
"Use enum BinaryType for WebSocket.binaryType, instead of DOMString"
instead of
"Addressing FIXME in WebSocket.idl"

Thanks for seeing this through!

[Paritosh Kumar, 2015-03-30 13:10:10]

Thanks Nils

> In future, could you also make sure the *Title* is descriptive?

Sure.
 
> I've fixed it here, so it's
> "Use enum BinaryType for WebSocket.binaryType, instead of DOMString"
> instead of
> "Addressing FIXME in WebSocket.idl"

Thanks..
 

As we are using ASSERT_NOT_REACHED() in DOMWebSocket::binaryType()
This results in failure of DOMWebSocketTest.binaryType Test  as 
we are seeing it in 
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...

Actually, This test doesn't hit the ASSERT in Release mode, but Hits in Debug
mode of webkit_unit_tests, so didn't identified.

[Nils Barth (inactive), 2015-03-30 14:12:27]

On 2015/03/30 13:10:10, Paritosh Kumar wrote:
> As we are using ASSERT_NOT_REACHED() in DOMWebSocket::binaryType()
> This results in failure of DOMWebSocketTest.binaryType Test  as 
> we are seeing it in 
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...
> 
> Actually, This test doesn't hit the ASSERT in Release mode, but Hits in Debug
> mode of webkit_unit_tests, so didn't identified.

Could you fix these failures by updating the tests?

Specifically, could you fix DOMWebSocketTest.binaryType in:
modules/websockets/DOMWebSocketTest.cpp
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The lines:
m_websocket->setBinaryType("hoge");
// ...
m_websocket->setBinaryType("fuga");
...are causing the crash, and such calls should be prevented by the bindings
code;
this is a precondition violation.

Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
// ...
EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");

You'll need:
#include <gtest/gtest-spi.h>

See:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Catching_Failures

[Nils Barth (inactive), 2015-03-30 14:13:19]

On 2015/03/30 13:10:10, Paritosh Kumar wrote:
> Actually, This test doesn't hit the ASSERT in Release mode, but Hits in Debug
> mode of webkit_unit_tests, so didn't identified.

To be precise:
It *did* hit the ASSERT line, but ASSERT_* statements are compiled *out* of
Release builds.

[Paritosh Kumar, 2015-03-31 06:51:57]

Thanks Nils


> Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
> EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
> // ...
> EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");

But, we cannot use non-static member m_websocket in this macro.
according to:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...


> To be precise:
> It *did* hit the ASSERT line, but ASSERT_* statements are compiled *out* of
> Release builds.

Ohhkk.. Thanks.

[Nils Barth (inactive), 2015-03-31 13:54:26]

On 2015/03/31 06:51:57, Paritosh Kumar wrote:
> > Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
> > EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
> > // ...
> > EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");
> 
> But, we cannot use non-static member m_websocket in this macro.
> according to:
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

Oops, sorry, wrong assert/expect.
Could you try using EXPECT_DEATH instead?
https://code.google.com/p/googletest/wiki/AdvancedGuide
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests

[Paritosh Kumar, 2015-04-01 07:06:01]

Thanks Nils

> Could you try using EXPECT_DEATH instead?
> https://code.google.com/p/googletest/wiki/AdvancedGuide
> https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests

As ASSERT_* statements are compiled *out* of Release builds. I think, We can't
expect
CRASH on setting invalid binaryType in DOMWebSocketTest, for Release build also.
and use
of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType Test, so
using
EXPECT_DEBUG_DEATH instead.

Please have a look.

Thanks & Regards

[Nils Barth (inactive), 2015-04-01 14:23:11]

On 2015/04/01 07:06:01, Paritosh Kumar wrote:
> Thanks Nils
> 
> > Could you try using EXPECT_DEATH instead?
> > https://code.google.com/p/googletest/wiki/AdvancedGuide
> > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
> 
> As ASSERT_* statements are compiled *out* of Release builds. I think, We can't
> expect
> CRASH on setting invalid binaryType in DOMWebSocketTest, for Release build
also.
> and use
> of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType Test,
so
> using
> EXPECT_DEBUG_DEATH instead.
> 
> Please have a look.
> 
> Thanks & Regards

Thanks Paritosh, that's correct.
LGTM!

[Paritosh Kumar, 2015-04-01 14:25:33]

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

[Paritosh Kumar, 2015-04-01 14:25:33]

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

[commit-bot: I haz the power, 2015-04-01 14:25:57]

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

[Paritosh Kumar, 2015-04-01 14:26:11]

On 2015/04/01 14:23:11, Nils Barth (inactive) wrote:
> On 2015/04/01 07:06:01, Paritosh Kumar wrote:
> > Thanks Nils
> > 
> > > Could you try using EXPECT_DEATH instead?
> > > https://code.google.com/p/googletest/wiki/AdvancedGuide
> > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
> > 
> > As ASSERT_* statements are compiled *out* of Release builds. I think, We
can't
> > expect
> > CRASH on setting invalid binaryType in DOMWebSocketTest, for Release build
> also.
> > and use
> > of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType Test,
> so
> > using
> > EXPECT_DEBUG_DEATH instead.
> > 
> > Please have a look.
> > 
> > Thanks & Regards
> 
> Thanks Paritosh, that's correct.
> LGTM!

Thanks.

[Paritosh Kumar, 2015-04-01 14:59:40]

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

[Paritosh Kumar, 2015-04-01 15:02:33]

On 2015/04/01 14:26:11, Paritosh Kumar wrote:
> On 2015/04/01 14:23:11, Nils Barth (inactive) wrote:
> > On 2015/04/01 07:06:01, Paritosh Kumar wrote:
> > > Thanks Nils
> > > 
> > > > Could you try using EXPECT_DEATH instead?
> > > > https://code.google.com/p/googletest/wiki/AdvancedGuide
> > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
> > > 
> > > As ASSERT_* statements are compiled *out* of Release builds. I think, We
> can't
> > > expect
> > > CRASH on setting invalid binaryType in DOMWebSocketTest, for Release build
> > also.
> > > and use
> > > of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType
Test,
> > so
> > > using
> > > EXPECT_DEBUG_DEATH instead.
> > > 
> > > Please have a look.
> > > 
> > > Thanks & Regards
> > 
> > Thanks Paritosh, that's correct.
> > LGTM!
> 
> Thanks.

Still, CRASH occurs, as we can see, It fails for Release build only according to
build command
"/b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests".

So, I think we should use EXPECT_DEATH instead of EXPEXT_DEBUG_DEATH. Am I
right?

[Nils Barth (inactive), 2015-04-01 15:34:50]

On 2015/04/01 15:02:33, Paritosh Kumar wrote:
> On 2015/04/01 14:26:11, Paritosh Kumar wrote:
> > On 2015/04/01 14:23:11, Nils Barth (inactive) wrote:
> > > On 2015/04/01 07:06:01, Paritosh Kumar wrote:
> > > > Thanks Nils
> > > > 
> > > > > Could you try using EXPECT_DEATH instead?
> > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide
> > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
> > > > 
> > > > As ASSERT_* statements are compiled *out* of Release builds. I think, We
> > can't
> > > > expect
> > > > CRASH on setting invalid binaryType in DOMWebSocketTest, for Release
build
> > > also.
> > > > and use
> > > > of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType
> Test,
> > > so
> > > > using
> > > > EXPECT_DEBUG_DEATH instead.
> > > > 
> > > > Please have a look.
> > > > 
> > > > Thanks & Regards
> > > 
> > > Thanks Paritosh, that's correct.
> > > LGTM!
> > 
> > Thanks.
> 
> Still, CRASH occurs, as we can see, It fails for Release build only according
to
> build command
> "/b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests".
> 
> So, I think we should use EXPECT_DEATH instead of EXPEXT_DEBUG_DEATH. Am I
> right?

That sounds right, thanks for checking!
Also, could you move the EXPECT_DEATH statements to a separate test case called
DOMWebSocketDeathTest (to avoid threading issues) as:

TEST_F(DOMWebSocketDeathTest, binaryType)
...per:
"Important: We strongly recommend you to follow the convention of naming your
test case (not test) *DeathTest when it contains a death test, as demonstrated
in the above example."
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests_And_Threads

[Paritosh Kumar, 2015-04-02 13:09:40]

On 2015/04/01 15:34:50, Nils Barth (inactive) wrote:
> On 2015/04/01 15:02:33, Paritosh Kumar wrote:
> > On 2015/04/01 14:26:11, Paritosh Kumar wrote:
> > > On 2015/04/01 14:23:11, Nils Barth (inactive) wrote:
> > > > On 2015/04/01 07:06:01, Paritosh Kumar wrote:
> > > > > Thanks Nils
> > > > > 
> > > > > > Could you try using EXPECT_DEATH instead?
> > > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide
> > > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests
> > > > > 
> > > > > As ASSERT_* statements are compiled *out* of Release builds. I think,
We
> > > can't
> > > > > expect
> > > > > CRASH on setting invalid binaryType in DOMWebSocketTest, for Release
> build
> > > > also.
> > > > > and use
> > > > > of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType
> > Test,
> > > > so
> > > > > using
> > > > > EXPECT_DEBUG_DEATH instead.
> > > > > 
> > > > > Please have a look.
> > > > > 
> > > > > Thanks & Regards
> > > > 
> > > > Thanks Paritosh, that's correct.
> > > > LGTM!
> > > 
> > > Thanks.
> > 
> > Still, CRASH occurs, as we can see, It fails for Release build only
according
> to
> > build command
> > "/b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests".
> > 
> > So, I think we should use EXPECT_DEATH instead of EXPEXT_DEBUG_DEATH. Am I
> > right?
> 
> That sounds right, thanks for checking!
> Also, could you move the EXPECT_DEATH statements to a separate test case
called
> DOMWebSocketDeathTest (to avoid threading issues) as:
> 
> TEST_F(DOMWebSocketDeathTest, binaryType)
> ...per:
> "Important: We strongly recommend you to follow the convention of naming your
> test case (not test) *DeathTest when it contains a death test, as demonstrated
> in the above example."
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests_And_Threads


Thanks. Moved the *DeathTest to separate Test case.
Using EXPECT_DEATH for CRASH.

PTAL.

[Paritosh Kumar, 2015-04-02 13:20:37]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[Paritosh Kumar, 2015-04-02 13:20:37]

The patchset sent to the CQ was uploaded after l-g-t-m from nbarth@chromium.org,
tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/871013007/#ps80001 (title:
" ")

[commit-bot: I haz the power, 2015-04-02 13:21:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871013007/80001

[commit-bot: I haz the power, 2015-04-02 13:30:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871013007/80001

[Paritosh Kumar, 2015-04-02 13:34:38]

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

[Nils Barth (inactive), 2015-04-02 14:56:36]

Thanks, still LGTM (with nits).

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:668: 
BTW, could you remove these blank lines?
They're useless and nowhere else in this test uses them.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:685: 
Same for these lines.

[Paritosh Kumar, 2015-04-02 15:41:32]

Thanks.
As we can see, the result of cq-dry-run is failing test
http/tests/websocket/binary-type.html

I didn't get any reason of this result, as this should be prevented by binding
code.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:668: 
On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> BTW, could you remove these blank lines?
> They're useless and nowhere else in this test uses them.

I think, this space is between one operation call and EXPECT_* macro.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:689: 
We can remove this space.

[Nils Barth (inactive), 2015-04-02 15:54:56]

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:668: 
On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> > BTW, could you remove these blank lines?
> > They're useless and nowhere else in this test uses them.
> 
> I think, this space is between one operation call and EXPECT_* macro.

Ok, point taken.
(It's a bit loose spacing for my taste, but ok to leave as is.)

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:689: 
On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> We can remove this space.

Good point; could you? Thanks!

[Paritosh Kumar, 2015-04-02 16:14:04]

Thanks Nils

Addressed your comments.

Please have a look.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:668: 
On 2015/04/02 15:54:55, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> > > BTW, could you remove these blank lines?
> > > They're useless and nowhere else in this test uses them.
> > 
> > I think, this space is between one operation call and EXPECT_* macro.
> 
> Ok, point taken.
> (It's a bit loose spacing for my taste, but ok to leave as is.)

Thanks. Leaving this space.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websocket...
Source/modules/websockets/DOMWebSocketTest.cpp:689: 
On 2015/04/02 15:54:55, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > We can remove this space.
> 
> Good point; could you? Thanks!

Thanks Removed.

[Nils Barth (inactive), 2015-04-02 16:20:39]

On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> Thanks.
> As we can see, the result of cq-dry-run is failing test
> http/tests/websocket/binary-type.html
> 
> I didn't get any reason of this result, as this should be prevented by binding
> code.

Thanks Paritosh!
Could you debug this failures?
(Try running the tests locally?)

[Paritosh Kumar, 2015-04-02 17:04:18]

On 2015/04/02 16:20:39, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > Thanks.
> > As we can see, the result of cq-dry-run is failing test
> > http/tests/websocket/binary-type.html
> > 
> > I didn't get any reason of this result, as this should be prevented by
binding
> > code.
> 
> Thanks Paritosh!
> Could you debug this failures?
> (Try running the tests locally?)

Yes, I ran it locally, its working fine and no failure found.

BTW, binding code will prevent any call to setBinaryType() with wrong
value of binaryType. and It won't hit ASSERT line in setBinaryType().

I think, CRASH occurs due to hitting that ASSERT.(may be other cause?)

[Nils Barth (inactive), 2015-04-02 17:32:09]

On 2015/04/02 17:04:18, Paritosh Kumar wrote:
> On 2015/04/02 16:20:39, Nils Barth (inactive) wrote:
> > On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > > Thanks.
> > > As we can see, the result of cq-dry-run is failing test
> > > http/tests/websocket/binary-type.html
> > > 
> > > I didn't get any reason of this result, as this should be prevented by
> binding
> > > code.
> > 
> > Thanks Paritosh!
> > Could you debug this failures?
> > (Try running the tests locally?)
> 
> Yes, I ran it locally, its working fine and no failure found.

Could you run the test in Debug mode?
(If still nothing, could you ask active developers for help?)

I think the command is:
Tools/Scripts/run-webkit-tests --debug http/tests/websocket/binary-type.html

[jsbell, 2015-04-02 17:36:21]

jsbell@chromium.org changed reviewers:
+ bashi@chromium.org

[jsbell, 2015-04-02 17:38:36]

I believe this is now failing due to https://codereview.chromium.org/1047993002

Paritosh: if you sync to the latest code and re-run you should see the failure.

That CL - which just landed a few hours ago - added support for enum validation
in sequences/arrays. But it seems to have undone some of the work by Paritosh.
For example:

https://codereview.chromium.org/1047993002/diff/100001/LayoutTests/webaudio/p...

it replaces this nice error:

CONSOLE WARNING: The provided value '1' is not a valid value of type
'PanningModelType'.

with this generic one:

CONSOLE WARNING: The provided value '1' is not a valid enum value.

bashi@ - can we get the nice errors back?

[jsbell, 2015-04-02 17:46:09]

FWIW, I suggest that Paritosh updated the -expected.txt file in this CL to match
the generic output ("not a valid enum value"), then land this CL. As a follow-up
we should work with bashi to explore restoring the more detailed enum logging
that Paritosh worked so hard to get in.

The review comments in https://codereview.chromium.org/1047993002 explain why
the change was made as a simplifying change, but it's a shame to lose it given
the context here and also https://codereview.chromium.org/955413002/

[Nils Barth (inactive), 2015-04-02 17:58:56]

Thanks for details Joshua!

Paritosh, per suggestion, could you sync, update, and land this CL, then follow
up?

[bashi, 2015-04-03 01:35:10]

On 2015/04/02 17:38:36, jsbell wrote:
> I believe this is now failing due to
https://codereview.chromium.org/1047993002
> 
> Paritosh: if you sync to the latest code and re-run you should see the
failure.
> 
> That CL - which just landed a few hours ago - added support for enum
validation
> in sequences/arrays. But it seems to have undone some of the work by Paritosh.
> For example:
> 
>
https://codereview.chromium.org/1047993002/diff/100001/LayoutTests/webaudio/p...
> 
> it replaces this nice error:
> 
> CONSOLE WARNING: The provided value '1' is not a valid value of type
> 'PanningModelType'.
> 
> with this generic one:
> 
> CONSOLE WARNING: The provided value '1' is not a valid enum value.
> 
> bashi@ - can we get the nice errors back?

Sure. I'll create a CL.

[Nils Barth (inactive), 2015-04-03 14:41:46]

On 2015/04/02 17:38:36, jsbell wrote:
> > bashi@ - can we get the nice errors back?

On 2015/04/03 01:35:10, bashi1 wrote:
> Sure. I'll create a CL.

Thanks Bashi!

Paritosh, feel free to sync, update, and land this CL;
Bashi can handle the updates ;)

[Paritosh Kumar, 2015-04-03 16:45:22]

On 2015/04/03 14:41:46, Nils Barth (inactive) wrote:
> On 2015/04/02 17:38:36, jsbell wrote:
> > > bashi@ - can we get the nice errors back?
> 
> On 2015/04/03 01:35:10, bashi1 wrote:
> > Sure. I'll create a CL.
> 
> Thanks Bashi!
> 
> Paritosh, feel free to sync, update, and land this CL;
> Bashi can handle the updates ;)

Okay, Thanks. I'll update it.

[Paritosh Kumar, 2015-04-04 11:27:47]

On 2015/04/03 16:45:22, Paritosh Kumar wrote:
> On 2015/04/03 14:41:46, Nils Barth (inactive) wrote:
> > On 2015/04/02 17:38:36, jsbell wrote:
> > > > bashi@ - can we get the nice errors back?
> > 
> > On 2015/04/03 01:35:10, bashi1 wrote:
> > > Sure. I'll create a CL.
> > 
> > Thanks Bashi!
> > 
> > Paritosh, feel free to sync, update, and land this CL;
> > Bashi can handle the updates ;)
> 
> Okay, Thanks. I'll update it.

Running CQ dry run..

[Paritosh Kumar, 2015-04-04 11:28:08]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[Paritosh Kumar, 2015-04-04 11:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from nbarth@chromium.org,
tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/871013007/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-04-04 11:28:19]

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

[Paritosh Kumar, 2015-04-04 12:32:35]

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

[jsbell, 2015-04-06 17:11:54]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-04-06 17:12:06]

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

[commit-bot: I haz the power, 2015-04-06 18:25:45]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193201

[jsbell, 2015-04-06 19:10:36]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1053013006/ by jsbell@chromium.org.

The reason for reverting is: Failing in gtests:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds...

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:825:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test detected 5 threads.
../../third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp:687:
Failure
Death test: m_websocket->setBinaryType("hoge")
    Result: failed to die.
 Error msg:
[  DEATH   ]
[  FAILED  ] DOMWebSocketDeathTest.binaryType (5 ms)
.

[Nils Barth (inactive), 2015-04-06 19:16:00]

Thanks Joshua!
Looks like we can't use death tests.
Paritosh, could you remove the death test and resubmit?
Thanks!