Log in Binding code for invalid enum type.

Log in Binding code for invalid enum type.

As per discussion on https://codereview.chromium.org/871013007/
logging the message for invalid enum type
in binding code.

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

[Paritosh Kumar, 2015-02-26 11:33:44]

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

[Paritosh Kumar, 2015-02-26 11:33:45]

Please have a look.

[haraken, 2015-02-26 11:39:26]

haraken@chromium.org changed reviewers:
+ aandrey@chromium.org, pfeldman@chromium.org, yurys@chromium.org

[haraken, 2015-02-26 11:39:27]

LGTM, but devtool people should confirm this.

[yurys, 2015-02-26 14:07:43]

https://codereview.chromium.org/955413002/diff/1/LayoutTests/http/tests/inspe...
File LayoutTests/http/tests/inspector/console-xhr-logging-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/http/tests/inspe...
LayoutTests/http/tests/inspector/console-xhr-logging-expected.txt:6: CONSOLE
ERROR: The provided value 'undefined' is not a valid value of type
'XMLHttpRequestResponseType'.
Please update InspectorTest.makeXHR in network-test.js so that we don't set
'undefined' as responseType. It should fix most of the inspector tests.
Otherwise this change adds a lot of noise with no good reason.

[Nils Barth (inactive), 2015-02-26 17:21:54]

Thanks Paritosh!
I checked the non-inspector changes.
All but one of them are expected (intentional bogus enum value checks).

There is one test error, however, in TextTrackMode, which uses a numeric value,
not an enum value.
(Yay! This change catches a real mistake!)

Could you:
1. Fix the incorrect test.
2. Add a test showing that numeric values don't work any more.

I give the exact code to use in comments below; could you commit this separately
*BEFORE* this CL?
Thanks!

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-add-remove-cue-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-add-remove-cue-expected.txt:1: CONSOLE ERROR: The
provided value '2' is not a valid value of type 'TextTrackMode'.
Test bug:
line 52 sets the mode to 2
newTrack.mode = 2;
It should have the enum value:
run("newTrack.mode = 'showing'");

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

Could you fix this *FIRST* in a separate CL to eliminate this noise?
Thanks!

Could you also add a check to track-mode.html that setting a numeric value '2'
doesn't work?
Something like this:

                consoleWrite("<b>*** Set to numeric value (no longer supported),
should return default</b>");
                var value = "2";
                run("textTrack.mode = " + value);
                testExpected("textTrack.mode", "showing");
                testExpected("video.textTracks[0].mode", "showing");
                consoleWrite("");
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-cue-mutable-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-cue-mutable-expected.txt:2: CONSOLE ERROR: The
provided value 'End' is not a valid value of type 'AlignSetting'.
These two are ok: lines 51/52 and 93/94 explicitly give invalid values.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
File LayoutTests/media/track/track-mode-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/media/track/trac...
LayoutTests/media/track/track-mode-expected.txt:1: CONSOLE ERROR: The provided
value 'bogus' is not a valid value of type 'TextTrackMode'.
Obviously intentional (good test!):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/biquadf...
File LayoutTests/webaudio/biquadfilternode-basic-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/biquadf...
LayoutTests/webaudio/biquadfilternode-basic-expected.txt:1: CONSOLE ERROR: The
provided value '99' is not a valid value of type 'BiquadFilterType'.
Expected; see line 20 below:
// Check that numerical values are no longer supported
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/dom-exc...
File LayoutTests/webaudio/dom-exceptions-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/dom-exc...
LayoutTests/webaudio/dom-exceptions-expected.txt:5: CONSOLE ERROR: The provided
value 'junk' is not a valid value of type 'ChannelCountMode'.
All expected; these are testing that invalid values shouldNotThrow:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/oscilla...
File LayoutTests/webaudio/oscillator-basic-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/oscilla...
LayoutTests/webaudio/oscillator-basic-expected.txt:1: CONSOLE ERROR: The
provided value '0' is not a valid value of type 'OscillatorType'.
Expected (see line 12 below):
// Check that numerical values are no longer supported
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/pannern...
File LayoutTests/webaudio/pannernode-basic-expected.txt (right):

https://codereview.chromium.org/955413002/diff/1/LayoutTests/webaudio/pannern...
LayoutTests/webaudio/pannernode-basic-expected.txt:2: CONSOLE ERROR: The
provided value '1' is not a valid value of type 'DistanceModelType'.
Expected; see lines 25 and 29 below.
// Check that numerical values are no longer supported.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Paritosh Kumar, 2015-02-27 07:00:26]

Thanks Nils, haraken and yurys for reviewing this.

@Nils: Uploaded a new cl for fix in TextTracMode tests
https://codereview.chromium.org/965683002/
Please have a look.

[Paritosh Kumar, 2015-03-02 12:12:14]

On 2015/02/27 07:00:26, Paritosh Kumar wrote:
> Thanks Nils, haraken and yurys for reviewing this.
> 
> @Nils: Uploaded a new cl for fix in TextTracMode tests
> https://codereview.chromium.org/965683002/
> Please have a look.

Updated the "network-test.js" with a check of undefined type
in makeXHR like,

if(type == undefined)
    xhr.responseType = "";
else
    xhr.responseType = type;

This removes noise, but it removes previous messages in
LayoutTests/http/tests/inspector/console-xhr-logging-expected.txt and
LayoutTests/http/tests/inspector/console-xhr-logging-async-expected.txt.


Updated the trac-mode-expected.txt and removed console message from
track-add-remove-cue-expected.txt.

PTAL.

[yurys, 2015-03-02 13:09:46]

inspector changes - lgtm

[pfeldman, 2015-03-02 13:33:25]

We are currently in the opposite process of removing the console spam. How much
spam is it likely to generate on the top 1M sites? Are those warnings or
unrecoverable errors?

[pfeldman, 2015-03-02 13:34:34]

I'm overriding yurys's comment with not lgtm while we are settling on this.

[pfeldman, 2015-03-02 13:34:47]

pfeldman@chromium.org changed reviewers:
+ paulirish@chromium.org

[Nils Barth (inactive), 2015-03-02 13:49:56]

On 2015/03/02 13:33:25, pfeldman wrote:
> We are currently in the opposite process of removing the console spam. How
much
> spam is it likely to generate on the top 1M sites? Are those warnings or
> unrecoverable errors?

Hi Pavel,
These are warnings: the invalid value is simply silently ignored.
(No exception is thrown, no recovery is necessary: no behavior change.)

Point taken about reducing spam.
In bindings we consider these valuable warnings, since they almost certainly
indicate a developer mistake, typically using a typo or deprecated name.
For example, notice how it caught an existing error in our tests here (setting
to an integer value, which is no longer valid, results in the integer being
coerced to a string value which is then silently ignored).

See for example Takeshi (toyoshino@):
http://crrev.com/871013007/#msg10
Joshua (jsbell@):
http://crrev.com/871013007/#msg18
...and Hixie:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=18749#c6

Does this seem reasonable?

[pfeldman, 2015-03-02 16:07:38]

> In bindings we consider these valuable warnings, since they almost certainly
> indicate a developer mistake, typically using a typo or deprecated name.
> For example, notice how it caught an existing error in our tests here (setting
> to an integer value, which is no longer valid, results in the integer being
> coerced to a string value which is then silently ignored).

Ok, thanks for the explanation, lgtm.

https://codereview.chromium.org/955413002/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/955413002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:243:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
ErrorMessageLevel, "The provided value '" + string + "' is not a valid value of
type 'TestEnum'."));
WarningMessageLevel

https://codereview.chromium.org/955413002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:832:
currentExecutionContext(info.GetIsolate())->addConsoleMessage(ConsoleMessage::create(JSMessageSource,
ErrorMessageLevel, "The provided value '" + string + "' is not a valid value of
type 'PartialEnumType'."));
WarningMessageLevel

[Nils Barth (inactive), 2015-03-02 16:23:48]

Thanks Pavel!
(Good catch about Warning, not Error.)

Paritosh, could you:
1. Submit http://crrev.com/965683002 "Fix TextTrackMode test"
2. Sync this CL so one of the TextTrackMode errors goes away:
CONSOLE ERROR: The provided value '2' is not a valid value of type
'TextTrackMode'.
3. Change from Error to Warning (as Pavel notes), updating the results.

...and then this should be good to go!

[Nils Barth (inactive), 2015-03-02 16:24:00]

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

https://codereview.chromium.org/955413002/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}}'."));
ErrorMessageLevel → WarningMessageLevel

[Nils Barth (inactive), 2015-03-02 16:24:20]

nbarth@chromium.org changed required reviewers:
+ haraken@chromium.org, pfeldman@chromium.org

[Paritosh Kumar, 2015-03-24 12:11:47]

Patchset #3 (id:40001) has been deleted

[Paritosh Kumar, 2015-03-24 12:20:11]

Thanks all

Apologize, for delay in update, actually I was OOO.

Updated the cl after syncing with the CL of the TextTrackMode

and changed the ErrorMessageLevel to WarningMessageLevel as suggested.

Please have a look.

[jsbell, 2015-03-24 16:49:17]

lgtm

[Paritosh Kumar, 2015-03-25 04:28:55]

Modified code in "Source/bindings/scripts/v8_attributes.py" also. 

PTAL.

https://codereview.chromium.org/955413002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/955413002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:54:
includes.add('core/inspector/ConsoleMessage.h')
Adding "core/inspector/ConsoleMessage.h" to includes, as error was coming on
using ConsoleMessage::create() in attributes.cpp.

Eroor was:
"error: incomplete type 'blink::ConsoleMessage' named in nested name specifier"

[haraken, 2015-03-25 04:31:31]

LGTM

[Paritosh Kumar, 2015-03-26 10:56:19]

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

[Paritosh Kumar, 2015-03-26 10:56:20]

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

[commit-bot: I haz the power, 2015-03-26 10:56:37]

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

[commit-bot: I haz the power, 2015-03-26 13:40:19]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192603