Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2350813005:
bindings: Support sequence in callback function arguments (Closed)
Descriptionbindings: Support sequence in callback function arguments
We need to support sequence as IntersectionObserverCallback uses
a sequence.
BUG=648486
Committed: https://crrev.com/a14a3854c3e69df4c7d1cb8c460816b54d6199a9
Cr-Commit-Position: refs/heads/master@{#421154}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Pass ExceptionState #
Total comments: 5
Patch Set 3 : SetVerbose(true) #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Messages
Total messages: 64 (31 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bashi@chromium.org changed reviewers: + haraken@chromium.org, lkawai@google.com, peria@chromium.org, yukishiino@chromium.org
PTAL https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: ExceptionState exceptionState(scriptState->isolate(), ExceptionState::ExecutionContext, "{{cpp_class}}"); Always declare exceptionState for simplicity. Another option would be passing exceptionState to call(). Maybe this approach is more reasonable but number of arguments increases. WDYT? I don't have strong opinion here. https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp (left): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/tests/results/core/V8LongExperimentalCallbackFunction.cpp:45: int cppValue = toInt32(info.GetIsolate(), v8ReturnValue, NormalConversion, exceptionState); oh, this was wrong...
https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: ExceptionState exceptionState(scriptState->isolate(), ExceptionState::ExecutionContext, "{{cpp_class}}"); On 2016/09/21 02:00:21, bashi1 (slow til Sep 26) wrote: > Always declare exceptionState for simplicity. > > Another option would be passing exceptionState to call(). Maybe this approach is > more reasonable but number of arguments increases. WDYT? I don't have strong > opinion here. +1 to declare it here. https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback VoidCallbackFunctionSequenceStringArg = void (sequence<DOMString> arg); I feel it confusing to have two types of callbacks for one objective; "void (seq<String>)" and "seq<String> (seq<long>)" Is it intended? Latter one is defined in callbackFunctionTest.idl.
https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: ExceptionState exceptionState(scriptState->isolate(), ExceptionState::ExecutionContext, "{{cpp_class}}"); On 2016/09/21 02:00:21, bashi1 (slow til Sep 26) wrote: > Always declare exceptionState for simplicity. > > Another option would be passing exceptionState to call(). Maybe this approach is > more reasonable but number of arguments increases. WDYT? I don't have strong > opinion here. What is an expected behavior when an exception is thrown to the ExceptionState? IIUC an expected behavior would be just to suppress the exception (c.f., we have the v8::TryCatch block at line 47). If that is the case, we'll need to put the ExceptionState inside the v8::TryCatch block.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: ExceptionState exceptionState(scriptState->isolate(), ExceptionState::ExecutionContext, "{{cpp_class}}"); On 2016/09/21 02:25:12, haraken wrote: > On 2016/09/21 02:00:21, bashi1 (slow til Sep 26) wrote: > > Always declare exceptionState for simplicity. > > > > Another option would be passing exceptionState to call(). Maybe this approach > is > > more reasonable but number of arguments increases. WDYT? I don't have strong > > opinion here. > > What is an expected behavior when an exception is thrown to the ExceptionState? > > IIUC an expected behavior would be just to suppress the exception (c.f., we have > the v8::TryCatch block at line 47). If that is the case, we'll need to put the > ExceptionState inside the v8::TryCatch block. > Hmm, glanced at existing hand-written callbacks and it seems there are three cases: 1. suppress exceptions (like this template) 2. rethrow exceptions (CustomElementCallback) 3. Don't use TryCatch (IDBObserverCallback) Maybe the right behavior is: - Pass ExceptionState - Use TryCatch inside call() - ExceptionState::rethrowV8Exception() when V8ScriptRunner::callFunction() fails - Callers may call ExceptionState::clearException() ? https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback VoidCallbackFunctionSequenceStringArg = void (sequence<DOMString> arg); On 2016/09/21 02:21:36, peria wrote: > I feel it confusing to have two types of callbacks for one objective; > "void (seq<String>)" and "seq<String> (seq<long>)" > Is it intended? Latter one is defined in callbackFunctionTest.idl. Will change this to align with the layout test.
On 2016/09/21 05:01:46, bashi1 (slow til Sep 26) wrote: > https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): > > https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: > ExceptionState exceptionState(scriptState->isolate(), > ExceptionState::ExecutionContext, "{{cpp_class}}"); > On 2016/09/21 02:25:12, haraken wrote: > > On 2016/09/21 02:00:21, bashi1 (slow til Sep 26) wrote: > > > Always declare exceptionState for simplicity. > > > > > > Another option would be passing exceptionState to call(). Maybe this > approach > > is > > > more reasonable but number of arguments increases. WDYT? I don't have strong > > > opinion here. > > > > What is an expected behavior when an exception is thrown to the > ExceptionState? > > > > IIUC an expected behavior would be just to suppress the exception (c.f., we > have > > the v8::TryCatch block at line 47). If that is the case, we'll need to put the > > ExceptionState inside the v8::TryCatch block. > > > > Hmm, glanced at existing hand-written callbacks and it seems there are three > cases: > 1. suppress exceptions (like this template) > 2. rethrow exceptions (CustomElementCallback) > 3. Don't use TryCatch (IDBObserverCallback) > > Maybe the right behavior is: > - Pass ExceptionState > - Use TryCatch inside call() > - ExceptionState::rethrowV8Exception() when V8ScriptRunner::callFunction() fails > - Callers may call ExceptionState::clearException() > > ? Do you know what the spec is saying and how other browsers behave? FWIW, CustomElementCallback is very special so we won't need to worry about it very much.
https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (right): https://codereview.chromium.org/2350813005/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/templates/callback_function.cpp:34: ExceptionState exceptionState(scriptState->isolate(), ExceptionState::ExecutionContext, "{{cpp_class}}"); On 2016/09/21 05:01:45, bashi1 (slow til Sep 26) wrote: > On 2016/09/21 02:25:12, haraken wrote: > > On 2016/09/21 02:00:21, bashi1 (slow til Sep 26) wrote: > > > Always declare exceptionState for simplicity. > > > > > > Another option would be passing exceptionState to call(). Maybe this > approach > > is > > > more reasonable but number of arguments increases. WDYT? I don't have strong > > > opinion here. > > > > What is an expected behavior when an exception is thrown to the > ExceptionState? > > > > IIUC an expected behavior would be just to suppress the exception (c.f., we > have > > the v8::TryCatch block at line 47). If that is the case, we'll need to put the > > ExceptionState inside the v8::TryCatch block. > > > > Hmm, glanced at existing hand-written callbacks and it seems there are three > cases: > 1. suppress exceptions (like this template) > 2. rethrow exceptions (CustomElementCallback) > 3. Don't use TryCatch (IDBObserverCallback) > > Maybe the right behavior is: > - Pass ExceptionState > - Use TryCatch inside call() > - ExceptionState::rethrowV8Exception() when V8ScriptRunner::callFunction() fails > - Callers may call ExceptionState::clearException() > > ? Yes, I think that's the expected design. Always killing an exception is wrong. (But l-g-t-m'ed because I thought it's okay as the first step.) In general, I think an owner of this callback function object should be responsible for everything (e.g. lifetime, exception handling) except for invoking a callback function.
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Followd shiino-san's comment. PTAL. https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (left): https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: exceptionCatcher.SetVerbose(true); Removed SetVerbose(true) call because the testharness (layout test) fails with a mysterious error message (It said "PASS" but run-webkit-tests reported it failed).
https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (left): https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: exceptionCatcher.SetVerbose(true); On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > Removed SetVerbose(true) call because the testharness (layout test) fails with a > mysterious error message (It said "PASS" but run-webkit-tests reported it > failed). Hmm, this might be problematic. SetVerbose(true) is needed to synchronously report the exception on the devtool console. If we don't have SetVerbose(true), the exception won't be reported to the devtool console until the next V8 function is fired.
https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/templates/callback_function.cpp (left): https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: exceptionCatcher.SetVerbose(true); On 2016/09/23 01:50:34, haraken wrote: > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > Removed SetVerbose(true) call because the testharness (layout test) fails with > a > > mysterious error message (It said "PASS" but run-webkit-tests reported it > > failed). > > Hmm, this might be problematic. > > SetVerbose(true) is needed to synchronously report the exception on the devtool > console. If we don't have SetVerbose(true), the exception won't be reported to > the devtool console until the next V8 function is fired. > What's a solution then? Also, why don't we always call SetVerbose(true)? It seems there are a lot of TryCatch which we don't call SetVerbose().
On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/templates/callback_function.cpp (left): > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > exceptionCatcher.SetVerbose(true); > On 2016/09/23 01:50:34, haraken wrote: > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > Removed SetVerbose(true) call because the testharness (layout test) fails > with > > a > > > mysterious error message (It said "PASS" but run-webkit-tests reported it > > > failed). > > > > Hmm, this might be problematic. > > > > SetVerbose(true) is needed to synchronously report the exception on the > devtool > > console. If we don't have SetVerbose(true), the exception won't be reported to > > the devtool console until the next V8 function is fired. > > > > What's a solution then? Also, why don't we always call SetVerbose(true)? It > seems there are a lot of TryCatch which we don't call SetVerbose(). That totally depends on the spec. We add SetVerbose(true) only when the spec requires to output the exception synchronously to the devtool console.
On 2016/09/23 02:04:13, haraken wrote: > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/templates/callback_function.cpp > (left): > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > exceptionCatcher.SetVerbose(true); > > On 2016/09/23 01:50:34, haraken wrote: > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > Removed SetVerbose(true) call because the testharness (layout test) fails > > with > > > a > > > > mysterious error message (It said "PASS" but run-webkit-tests reported it > > > > failed). > > > > > > Hmm, this might be problematic. > > > > > > SetVerbose(true) is needed to synchronously report the exception on the > > devtool > > > console. If we don't have SetVerbose(true), the exception won't be reported > to > > > the devtool console until the next V8 function is fired. > > > > > > > What's a solution then? Also, why don't we always call SetVerbose(true)? It > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > That totally depends on the spec. We add SetVerbose(true) only when the spec > requires to output the exception synchronously to the devtool console. I haven't checked the spec, but basically script invocation should have SetVerbase(true). Hence I think callback functions should have SetVerbose(true). So the solution would be to make SetVerbose(true) work with layout tests, I guess.
On 2016/09/23 02:07:14, haraken wrote: > On 2016/09/23 02:04:13, haraken wrote: > > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/bindings/templates/callback_function.cpp > > (left): > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > > exceptionCatcher.SetVerbose(true); > > > On 2016/09/23 01:50:34, haraken wrote: > > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > > Removed SetVerbose(true) call because the testharness (layout test) > fails > > > with > > > > a > > > > > mysterious error message (It said "PASS" but run-webkit-tests reported > it > > > > > failed). > > > > > > > > Hmm, this might be problematic. > > > > > > > > SetVerbose(true) is needed to synchronously report the exception on the > > > devtool > > > > console. If we don't have SetVerbose(true), the exception won't be > reported > > to > > > > the devtool console until the next V8 function is fired. > > > > > > > > > > What's a solution then? Also, why don't we always call SetVerbose(true)? It > > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > > > That totally depends on the spec. We add SetVerbose(true) only when the spec > > requires to output the exception synchronously to the devtool console. > > I haven't checked the spec, but basically script invocation should have > SetVerbase(true). Hence I think callback functions should have SetVerbose(true). > Hmm, could you elaborate how specs mention the devtool? My understanding is that DevTools isn't a part of DOM spec nor other Web platform APIs (maybe no?). Another question: If invoking another function is required to fire exception (asynchronously?), why the current test passes? I don't invoke another functions in the test. > So the solution would be to make SetVerbose(true) work with layout tests, I > guess. Is your suggestion to make `content_shell --run-layout-test` work with SetVerbose(true) ? It always fails even when I don't use assert_throws(). Anyway I'll take closer look at the failure.
On 2016/09/23 02:29:19, bashi1 (slow til Sep 26) wrote: > On 2016/09/23 02:07:14, haraken wrote: > > On 2016/09/23 02:04:13, haraken wrote: > > > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/bindings/templates/callback_function.cpp > > > (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > > > exceptionCatcher.SetVerbose(true); > > > > On 2016/09/23 01:50:34, haraken wrote: > > > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > > > Removed SetVerbose(true) call because the testharness (layout test) > > fails > > > > with > > > > > a > > > > > > mysterious error message (It said "PASS" but run-webkit-tests reported > > it > > > > > > failed). > > > > > > > > > > Hmm, this might be problematic. > > > > > > > > > > SetVerbose(true) is needed to synchronously report the exception on the > > > > devtool > > > > > console. If we don't have SetVerbose(true), the exception won't be > > reported > > > to > > > > > the devtool console until the next V8 function is fired. > > > > > > > > > > > > > What's a solution then? Also, why don't we always call SetVerbose(true)? > It > > > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > > > > > That totally depends on the spec. We add SetVerbose(true) only when the spec > > > requires to output the exception synchronously to the devtool console. > > > > I haven't checked the spec, but basically script invocation should have > > SetVerbase(true). Hence I think callback functions should have > SetVerbose(true). > > > Hmm, could you elaborate how specs mention the devtool? My understanding is that > DevTools isn't a part of DOM spec nor other Web platform APIs (maybe no?). For example, https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-r... (see "If this throws any exception, then report the exception") and https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception (see how "report the exception" is defined). > > Another question: If invoking another function is required to fire exception > (asynchronously?), why the current test passes? I don't invoke another functions > in the test. If we don't set SetVerbose(true), the exception is thrown at the next time V8 does "something". It's pretty random and at the best effort -- for example, the exception will be thrown when v8::Function::New is called, when V8 runs a microtask etc. > > > So the solution would be to make SetVerbose(true) work with layout tests, I > > guess. > > Is your suggestion to make `content_shell --run-layout-test` work with > SetVerbose(true) ? It always fails even when I don't use assert_throws(). > > Anyway I'll take closer look at the failure. Yeah, it will be worth looking.
https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback StringSequenceCallbackFunctionSequenceLongArg = sequence<DOMString> (sequence<long> arg); nit: inconsistent name; StrSeq in return type and SeqLong in the argument.
On 2016/09/23 02:40:31, haraken wrote: > On 2016/09/23 02:29:19, bashi1 (slow til Sep 26) wrote: > > On 2016/09/23 02:07:14, haraken wrote: > > > On 2016/09/23 02:04:13, haraken wrote: > > > > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/bindings/templates/callback_function.cpp > > > > (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > > > > exceptionCatcher.SetVerbose(true); > > > > > On 2016/09/23 01:50:34, haraken wrote: > > > > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > > > > Removed SetVerbose(true) call because the testharness (layout test) > > > fails > > > > > with > > > > > > a > > > > > > > mysterious error message (It said "PASS" but run-webkit-tests > reported > > > it > > > > > > > failed). > > > > > > > > > > > > Hmm, this might be problematic. > > > > > > > > > > > > SetVerbose(true) is needed to synchronously report the exception on > the > > > > > devtool > > > > > > console. If we don't have SetVerbose(true), the exception won't be > > > reported > > > > to > > > > > > the devtool console until the next V8 function is fired. > > > > > > > > > > > > > > > > What's a solution then? Also, why don't we always call SetVerbose(true)? > > It > > > > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > > > > > > > That totally depends on the spec. We add SetVerbose(true) only when the > spec > > > > requires to output the exception synchronously to the devtool console. > > > > > > I haven't checked the spec, but basically script invocation should have > > > SetVerbase(true). Hence I think callback functions should have > > SetVerbose(true). > > > > > Hmm, could you elaborate how specs mention the devtool? My understanding is > that > > DevTools isn't a part of DOM spec nor other Web platform APIs (maybe no?). > > For example, > https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-r... > (see "If this throws any exception, then report the exception") and > https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception (see > how "report the exception" is defined). > > > > > Another question: If invoking another function is required to fire exception > > (asynchronously?), why the current test passes? I don't invoke another > functions > > in the test. > > If we don't set SetVerbose(true), the exception is thrown at the next time V8 > does "something". It's pretty random and at the best effort -- for example, the > exception will be thrown when v8::Function::New is called, when V8 runs a > microtask etc. Thanks for the explanation. That sounds not calling SetVerbose almost always works if throwing asynchronously is allowed. Do we really need it for callback functions? Here is a spec description but it's very hard to understand correctly, but I couldn't find it should throw exception synchronously. http://heycam.github.io/webidl/#es-invoking-callback-functions > > > > > > So the solution would be to make SetVerbose(true) work with layout tests, I > > > guess. > > > > Is your suggestion to make `content_shell --run-layout-test` work with > > SetVerbose(true) ? It always fails even when I don't use assert_throws(). > > > > Anyway I'll take closer look at the failure. > > Yeah, it will be worth looking. I checked assert_throws() and it just wraps a test function with a try-catch. If it doesn't work, the implementation would be wrong (i.e. immediately throws an exception may be wrong for callback functions). In practice, I think there are two options: 1. Don't call SetVerbose(true) 2. Don't use v8::TryCatch Downside of 2. is that we can't set an exception thrown by the callback to ExceptionState.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/23 03:13:38, bashi1 (slow til Sep 26) wrote: > On 2016/09/23 02:40:31, haraken wrote: > > On 2016/09/23 02:29:19, bashi1 (slow til Sep 26) wrote: > > > On 2016/09/23 02:07:14, haraken wrote: > > > > On 2016/09/23 02:04:13, haraken wrote: > > > > > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > > File > third_party/WebKit/Source/bindings/templates/callback_function.cpp > > > > > (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > > > > > exceptionCatcher.SetVerbose(true); > > > > > > On 2016/09/23 01:50:34, haraken wrote: > > > > > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > > > > > Removed SetVerbose(true) call because the testharness (layout > test) > > > > fails > > > > > > with > > > > > > > a > > > > > > > > mysterious error message (It said "PASS" but run-webkit-tests > > reported > > > > it > > > > > > > > failed). > > > > > > > > > > > > > > Hmm, this might be problematic. > > > > > > > > > > > > > > SetVerbose(true) is needed to synchronously report the exception on > > the > > > > > > devtool > > > > > > > console. If we don't have SetVerbose(true), the exception won't be > > > > reported > > > > > to > > > > > > > the devtool console until the next V8 function is fired. > > > > > > > > > > > > > > > > > > > What's a solution then? Also, why don't we always call > SetVerbose(true)? > > > It > > > > > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > > > > > > > > > That totally depends on the spec. We add SetVerbose(true) only when the > > spec > > > > > requires to output the exception synchronously to the devtool console. > > > > > > > > I haven't checked the spec, but basically script invocation should have > > > > SetVerbase(true). Hence I think callback functions should have > > > SetVerbose(true). > > > > > > > Hmm, could you elaborate how specs mention the devtool? My understanding is > > that > > > DevTools isn't a part of DOM spec nor other Web platform APIs (maybe no?). > > > > For example, > > > https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-r... > > (see "If this throws any exception, then report the exception") and > > https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception > (see > > how "report the exception" is defined). > > > > > > > > Another question: If invoking another function is required to fire exception > > > (asynchronously?), why the current test passes? I don't invoke another > > functions > > > in the test. > > > > If we don't set SetVerbose(true), the exception is thrown at the next time V8 > > does "something". It's pretty random and at the best effort -- for example, > the > > exception will be thrown when v8::Function::New is called, when V8 runs a > > microtask etc. > Thanks for the explanation. That sounds not calling SetVerbose almost always > works if throwing asynchronously is allowed. Do we really need it for callback > functions? Here is a spec description but it's very hard to understand > correctly, but I couldn't find it should throw exception synchronously. > http://heycam.github.io/webidl/#es-invoking-callback-functions Yeah, maybe you're correct. However, if you remove SetVerbose(true), the call site is not allowed to suppress the exception thrown by call(), because if the call site suppresses the exception, it won't be reported anywhere. I don't think it's a good idea to ask all the call sites to rethrow the exception, which means that we should not remove SetVerbose(true). > > > > > > > > > > So the solution would be to make SetVerbose(true) work with layout tests, > I > > > > guess. > > > > > > Is your suggestion to make `content_shell --run-layout-test` work with > > > SetVerbose(true) ? It always fails even when I don't use assert_throws(). > > > > > > Anyway I'll take closer look at the failure. > > > > Yeah, it will be worth looking. > > I checked assert_throws() and it just wraps a test function with a try-catch. If > it doesn't work, the implementation would be wrong (i.e. immediately throws an > exception may be wrong for callback functions). In practice, I think there are > two options: > 1. Don't call SetVerbose(true) > 2. Don't use v8::TryCatch > > Downside of 2. is that we can't set an exception thrown by the callback to > ExceptionState. Why isn't the problem happening in the current hand-written callback functions?
On 2016/09/23 04:31:35, haraken wrote: > On 2016/09/23 03:13:38, bashi1 (slow til Sep 26) wrote: > > On 2016/09/23 02:40:31, haraken wrote: > > > On 2016/09/23 02:29:19, bashi1 (slow til Sep 26) wrote: > > > > On 2016/09/23 02:07:14, haraken wrote: > > > > > On 2016/09/23 02:04:13, haraken wrote: > > > > > > On 2016/09/23 01:58:32, bashi1 (slow til Sep 26) wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > > > File > > third_party/WebKit/Source/bindings/templates/callback_function.cpp > > > > > > (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... > > > > > > > > third_party/WebKit/Source/bindings/templates/callback_function.cpp:46: > > > > > > > exceptionCatcher.SetVerbose(true); > > > > > > > On 2016/09/23 01:50:34, haraken wrote: > > > > > > > > On 2016/09/23 01:47:10, bashi1 (slow til Sep 26) wrote: > > > > > > > > > Removed SetVerbose(true) call because the testharness (layout > > test) > > > > > fails > > > > > > > with > > > > > > > > a > > > > > > > > > mysterious error message (It said "PASS" but run-webkit-tests > > > reported > > > > > it > > > > > > > > > failed). > > > > > > > > > > > > > > > > Hmm, this might be problematic. > > > > > > > > > > > > > > > > SetVerbose(true) is needed to synchronously report the exception > on > > > the > > > > > > > devtool > > > > > > > > console. If we don't have SetVerbose(true), the exception won't be > > > > > reported > > > > > > to > > > > > > > > the devtool console until the next V8 function is fired. > > > > > > > > > > > > > > > > > > > > > > What's a solution then? Also, why don't we always call > > SetVerbose(true)? > > > > It > > > > > > > seems there are a lot of TryCatch which we don't call SetVerbose(). > > > > > > > > > > > > That totally depends on the spec. We add SetVerbose(true) only when > the > > > spec > > > > > > requires to output the exception synchronously to the devtool console. > > > > > > > > > > I haven't checked the spec, but basically script invocation should have > > > > > SetVerbase(true). Hence I think callback functions should have > > > > SetVerbose(true). > > > > > > > > > Hmm, could you elaborate how specs mention the devtool? My understanding > is > > > that > > > > DevTools isn't a part of DOM spec nor other Web platform APIs (maybe no?). > > > > > > For example, > > > > > > https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-r... > > > (see "If this throws any exception, then report the exception") and > > > https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception > > (see > > > how "report the exception" is defined). > > > > > > > > > > > Another question: If invoking another function is required to fire > exception > > > > (asynchronously?), why the current test passes? I don't invoke another > > > functions > > > > in the test. > > > > > > If we don't set SetVerbose(true), the exception is thrown at the next time > V8 > > > does "something". It's pretty random and at the best effort -- for example, > > the > > > exception will be thrown when v8::Function::New is called, when V8 runs a > > > microtask etc. > > Thanks for the explanation. That sounds not calling SetVerbose almost always > > works if throwing asynchronously is allowed. Do we really need it for callback > > functions? Here is a spec description but it's very hard to understand > > correctly, but I couldn't find it should throw exception synchronously. > > http://heycam.github.io/webidl/#es-invoking-callback-functions > > Yeah, maybe you're correct. > > However, if you remove SetVerbose(true), the call site is not allowed to > suppress the exception thrown by call(), because if the call site suppresses the > exception, it won't be reported anywhere. I don't think it's a good idea to ask > all the call sites to rethrow the exception, which means that we should not > remove SetVerbose(true). Hmm, I'm confused. - If a caller wants to suppress exceptions -> call exceptionState.clearException() - If a caller wants to throw exceptions -> do nothing. Exceptions are thrown Isn't above enough? > > > > > > > > > > > > > > > So the solution would be to make SetVerbose(true) work with layout > tests, > > I > > > > > guess. > > > > > > > > Is your suggestion to make `content_shell --run-layout-test` work with > > > > SetVerbose(true) ? It always fails even when I don't use assert_throws(). > > > > > > > > Anyway I'll take closer look at the failure. > > > > > > Yeah, it will be worth looking. > > > > I checked assert_throws() and it just wraps a test function with a try-catch. > If > > it doesn't work, the implementation would be wrong (i.e. immediately throws an > > exception may be wrong for callback functions). In practice, I think there are > > two options: > > 1. Don't call SetVerbose(true) > > 2. Don't use v8::TryCatch > > > > Downside of 2. is that we can't set an exception thrown by the callback to > > ExceptionState. > > Why isn't the problem happening in the current hand-written callback functions? I guess that there is no tests for exception.
> > However, if you remove SetVerbose(true), the call site is not allowed to > > suppress the exception thrown by call(), because if the call site suppresses > the > > exception, it won't be reported anywhere. I don't think it's a good idea to > ask > > all the call sites to rethrow the exception, which means that we should not > > remove SetVerbose(true). > > Hmm, I'm confused. > - If a caller wants to suppress exceptions -> call > exceptionState.clearException() > - If a caller wants to throw exceptions -> do nothing. Exceptions are thrown > > Isn't above enough? My point is that if you stop calling SetVerbose(true), "do nothing" is not allowed. If the caller site does nothing, the JS exception thrown in the callback function is not reported anywhere. I don't think it's allowed. If you have SetVerbose(true), "do nothing" is allowed because SetVerbose(true) has already reported the error to the console. It's up to the caller site if he/she wants to handle the exception more or not.
On 2016/09/23 05:30:06, haraken wrote: > > > However, if you remove SetVerbose(true), the call site is not allowed to > > > suppress the exception thrown by call(), because if the call site suppresses > > the > > > exception, it won't be reported anywhere. I don't think it's a good idea to > > ask > > > all the call sites to rethrow the exception, which means that we should not > > > remove SetVerbose(true). > > > > Hmm, I'm confused. > > - If a caller wants to suppress exceptions -> call > > exceptionState.clearException() > > - If a caller wants to throw exceptions -> do nothing. Exceptions are thrown > > > > Isn't above enough? > > My point is that if you stop calling SetVerbose(true), "do nothing" is not > allowed. If the caller site does nothing, the JS exception thrown in the > callback function is not reported anywhere. I don't think it's allowed. > > If you have SetVerbose(true), "do nothing" is allowed because SetVerbose(true) > has already reported the error to the console. It's up to the caller site if > he/she wants to handle the exception more or not. As this CL does, "do nothing" on call sites actually reports exceptions.
On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote: > On 2016/09/23 05:30:06, haraken wrote: > > > > However, if you remove SetVerbose(true), the call site is not allowed to > > > > suppress the exception thrown by call(), because if the call site > suppresses > > > the > > > > exception, it won't be reported anywhere. I don't think it's a good idea > to > > > ask > > > > all the call sites to rethrow the exception, which means that we should > not > > > > remove SetVerbose(true). > > > > > > Hmm, I'm confused. > > > - If a caller wants to suppress exceptions -> call > > > exceptionState.clearException() > > > - If a caller wants to throw exceptions -> do nothing. Exceptions are thrown > > > > > > Isn't above enough? > > > > My point is that if you stop calling SetVerbose(true), "do nothing" is not > > allowed. If the caller site does nothing, the JS exception thrown in the > > callback function is not reported anywhere. I don't think it's allowed. > > > > If you have SetVerbose(true), "do nothing" is allowed because SetVerbose(true) > > has already reported the error to the console. It's up to the caller site if > > he/she wants to handle the exception more or not. > > As this CL does, "do nothing" on call sites actually reports exceptions. Sorry, I wanted to mean that "clear exception" is not allowed. Let's chat offline.
lgtm
On 2016/09/23 05:34:46, haraken wrote: > On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote: > > On 2016/09/23 05:30:06, haraken wrote: > > > > > However, if you remove SetVerbose(true), the call site is not allowed to > > > > > suppress the exception thrown by call(), because if the call site > > suppresses > > > > the > > > > > exception, it won't be reported anywhere. I don't think it's a good idea > > to > > > > ask > > > > > all the call sites to rethrow the exception, which means that we should > > not > > > > > remove SetVerbose(true). > > > > > > > > Hmm, I'm confused. > > > > - If a caller wants to suppress exceptions -> call > > > > exceptionState.clearException() > > > > - If a caller wants to throw exceptions -> do nothing. Exceptions are > thrown > > > > > > > > Isn't above enough? > > > > > > My point is that if you stop calling SetVerbose(true), "do nothing" is not > > > allowed. If the caller site does nothing, the JS exception thrown in the > > > callback function is not reported anywhere. I don't think it's allowed. > > > > > > If you have SetVerbose(true), "do nothing" is allowed because > SetVerbose(true) > > > has already reported the error to the console. It's up to the caller site if > > > he/she wants to handle the exception more or not. > > > > As this CL does, "do nothing" on call sites actually reports exceptions. > > Sorry, I wanted to mean that "clear exception" is not allowed. Let's chat > offline. Chatted offline and the point is whether try-catch in user-space should be able to catch exceptions which occurs in callbacks. I tested behavior of addEventListener() on Firefox and I can't catch exceptions. Though EventListener is not a callback function (it is callback interface), we might want to keep the current behavior. I'll remove the test (because we can't test actually) and revert SetVerbose(true) removal.
On 2016/09/23 07:05:58, bashi1 (slow til Sep 26) wrote: > On 2016/09/23 05:34:46, haraken wrote: > > On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote: > > > On 2016/09/23 05:30:06, haraken wrote: > > > > > > However, if you remove SetVerbose(true), the call site is not allowed > to > > > > > > suppress the exception thrown by call(), because if the call site > > > suppresses > > > > > the > > > > > > exception, it won't be reported anywhere. I don't think it's a good > idea > > > to > > > > > ask > > > > > > all the call sites to rethrow the exception, which means that we > should > > > not > > > > > > remove SetVerbose(true). > > > > > > > > > > Hmm, I'm confused. > > > > > - If a caller wants to suppress exceptions -> call > > > > > exceptionState.clearException() > > > > > - If a caller wants to throw exceptions -> do nothing. Exceptions are > > thrown > > > > > > > > > > Isn't above enough? > > > > > > > > My point is that if you stop calling SetVerbose(true), "do nothing" is not > > > > allowed. If the caller site does nothing, the JS exception thrown in the > > > > callback function is not reported anywhere. I don't think it's allowed. > > > > > > > > If you have SetVerbose(true), "do nothing" is allowed because > > SetVerbose(true) > > > > has already reported the error to the console. It's up to the caller site > if > > > > he/she wants to handle the exception more or not. > > > > > > As this CL does, "do nothing" on call sites actually reports exceptions. > > > > Sorry, I wanted to mean that "clear exception" is not allowed. Let's chat > > offline. > > Chatted offline and the point is whether try-catch in user-space should be able > to catch exceptions which occurs in callbacks. > > I tested behavior of addEventListener() on Firefox and I can't catch exceptions. > Though EventListener is not a callback function (it is callback interface), we > might want to keep the current behavior. I'll remove the test (because we can't > test actually) and revert SetVerbose(true) removal. Sounds good to me. On the other hand, your proposed behavior makes sense and it would be worth discussing in platform-architecture-dev@.
On 2016/09/23 07:09:39, haraken wrote:
> On 2016/09/23 07:05:58, bashi1 (slow til Sep 26) wrote:
> > On 2016/09/23 05:34:46, haraken wrote:
> > > On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote:
> > > > On 2016/09/23 05:30:06, haraken wrote:
> > > > > > > However, if you remove SetVerbose(true), the call site is not
> allowed
> > to
> > > > > > > suppress the exception thrown by call(), because if the call site
> > > > suppresses
> > > > > > the
> > > > > > > exception, it won't be reported anywhere. I don't think it's a
good
> > idea
> > > > to
> > > > > > ask
> > > > > > > all the call sites to rethrow the exception, which means that we
> > should
> > > > not
> > > > > > > remove SetVerbose(true).
> > > > > >
> > > > > > Hmm, I'm confused.
> > > > > > - If a caller wants to suppress exceptions -> call
> > > > > > exceptionState.clearException()
> > > > > > - If a caller wants to throw exceptions -> do nothing. Exceptions
are
> > > thrown
> > > > > >
> > > > > > Isn't above enough?
> > > > >
> > > > > My point is that if you stop calling SetVerbose(true), "do nothing" is
> not
> > > > > allowed. If the caller site does nothing, the JS exception thrown in
the
> > > > > callback function is not reported anywhere. I don't think it's
allowed.
> > > > >
> > > > > If you have SetVerbose(true), "do nothing" is allowed because
> > > SetVerbose(true)
> > > > > has already reported the error to the console. It's up to the caller
> site
> > if
> > > > > he/she wants to handle the exception more or not.
> > > >
> > > > As this CL does, "do nothing" on call sites actually reports exceptions.
> > >
> > > Sorry, I wanted to mean that "clear exception" is not allowed. Let's chat
> > > offline.
> >
> > Chatted offline and the point is whether try-catch in user-space should be
> able
> > to catch exceptions which occurs in callbacks.
> >
> > I tested behavior of addEventListener() on Firefox and I can't catch
> exceptions.
> > Though EventListener is not a callback function (it is callback interface),
we
> > might want to keep the current behavior. I'll remove the test (because we
> can't
> > test actually) and revert SetVerbose(true) removal.
>
> Sounds good to me.
>
> On the other hand, your proposed behavior makes sense and it would be worth
> discussing in platform-architecture-dev@.
Event listeners registered by addEventListener are called asynchronously, so
there should be no way to catch an exception. However, callback functions are
not always used asynchronously, like Lisa's test.
function callback() {}
try {
domObj.invokeCallback(callback);
} catch (e) {
}
In this case, I think the user script can catch an exception.
In the case of addEventListener, we don't have a try-catch block like above when
invoking.
On 2016/09/23 07:19:56, Yuki wrote:
> On 2016/09/23 07:09:39, haraken wrote:
> > On 2016/09/23 07:05:58, bashi1 (slow til Sep 26) wrote:
> > > On 2016/09/23 05:34:46, haraken wrote:
> > > > On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote:
> > > > > On 2016/09/23 05:30:06, haraken wrote:
> > > > > > > > However, if you remove SetVerbose(true), the call site is not
> > allowed
> > > to
> > > > > > > > suppress the exception thrown by call(), because if the call
site
> > > > > suppresses
> > > > > > > the
> > > > > > > > exception, it won't be reported anywhere. I don't think it's a
> good
> > > idea
> > > > > to
> > > > > > > ask
> > > > > > > > all the call sites to rethrow the exception, which means that we
> > > should
> > > > > not
> > > > > > > > remove SetVerbose(true).
> > > > > > >
> > > > > > > Hmm, I'm confused.
> > > > > > > - If a caller wants to suppress exceptions -> call
> > > > > > > exceptionState.clearException()
> > > > > > > - If a caller wants to throw exceptions -> do nothing. Exceptions
> are
> > > > thrown
> > > > > > >
> > > > > > > Isn't above enough?
> > > > > >
> > > > > > My point is that if you stop calling SetVerbose(true), "do nothing"
is
> > not
> > > > > > allowed. If the caller site does nothing, the JS exception thrown in
> the
> > > > > > callback function is not reported anywhere. I don't think it's
> allowed.
> > > > > >
> > > > > > If you have SetVerbose(true), "do nothing" is allowed because
> > > > SetVerbose(true)
> > > > > > has already reported the error to the console. It's up to the caller
> > site
> > > if
> > > > > > he/she wants to handle the exception more or not.
> > > > >
> > > > > As this CL does, "do nothing" on call sites actually reports
exceptions.
> > > >
> > > > Sorry, I wanted to mean that "clear exception" is not allowed. Let's
chat
> > > > offline.
> > >
> > > Chatted offline and the point is whether try-catch in user-space should be
> > able
> > > to catch exceptions which occurs in callbacks.
> > >
> > > I tested behavior of addEventListener() on Firefox and I can't catch
> > exceptions.
> > > Though EventListener is not a callback function (it is callback
interface),
> we
> > > might want to keep the current behavior. I'll remove the test (because we
> > can't
> > > test actually) and revert SetVerbose(true) removal.
> >
> > Sounds good to me.
> >
> > On the other hand, your proposed behavior makes sense and it would be worth
> > discussing in platform-architecture-dev@.
>
> Event listeners registered by addEventListener are called asynchronously, so
> there should be no way to catch an exception. However, callback functions are
> not always used asynchronously, like Lisa's test.
>
> function callback() {}
> try {
> domObj.invokeCallback(callback);
> } catch (e) {
> }
>
> In this case, I think the user script can catch an exception.
>
> In the case of addEventListener, we don't have a try-catch block like above
when
> invoking.
I couldn't catch exceptions even when it's synchronous. Here is a snippet I used
to check the behavior.
var b = document.getElementById('button');
b.addEventListener('myevent', function(e) {
throw new Error('foo');
});
var event = new Event('myevent');
try {
b.dispatchEvent(event);
console.log('Exception suppressed');
} catch(e) {
console.log('Exception caught');
}
So I think it's good to keep the current behavior (at least until it becomes a
real problem).
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/23 07:52:41, bashi1 (slow til Sep 26) wrote:
> On 2016/09/23 07:19:56, Yuki wrote:
> > On 2016/09/23 07:09:39, haraken wrote:
> > > On 2016/09/23 07:05:58, bashi1 (slow til Sep 26) wrote:
> > > > On 2016/09/23 05:34:46, haraken wrote:
> > > > > On 2016/09/23 05:32:17, bashi1 (slow til Sep 26) wrote:
> > > > > > On 2016/09/23 05:30:06, haraken wrote:
> > > > > > > > > However, if you remove SetVerbose(true), the call site is not
> > > allowed
> > > > to
> > > > > > > > > suppress the exception thrown by call(), because if the call
> site
> > > > > > suppresses
> > > > > > > > the
> > > > > > > > > exception, it won't be reported anywhere. I don't think it's a
> > good
> > > > idea
> > > > > > to
> > > > > > > > ask
> > > > > > > > > all the call sites to rethrow the exception, which means that
we
> > > > should
> > > > > > not
> > > > > > > > > remove SetVerbose(true).
> > > > > > > >
> > > > > > > > Hmm, I'm confused.
> > > > > > > > - If a caller wants to suppress exceptions -> call
> > > > > > > > exceptionState.clearException()
> > > > > > > > - If a caller wants to throw exceptions -> do nothing.
Exceptions
> > are
> > > > > thrown
> > > > > > > >
> > > > > > > > Isn't above enough?
> > > > > > >
> > > > > > > My point is that if you stop calling SetVerbose(true), "do
nothing"
> is
> > > not
> > > > > > > allowed. If the caller site does nothing, the JS exception thrown
in
> > the
> > > > > > > callback function is not reported anywhere. I don't think it's
> > allowed.
> > > > > > >
> > > > > > > If you have SetVerbose(true), "do nothing" is allowed because
> > > > > SetVerbose(true)
> > > > > > > has already reported the error to the console. It's up to the
caller
> > > site
> > > > if
> > > > > > > he/she wants to handle the exception more or not.
> > > > > >
> > > > > > As this CL does, "do nothing" on call sites actually reports
> exceptions.
> > > > >
> > > > > Sorry, I wanted to mean that "clear exception" is not allowed. Let's
> chat
> > > > > offline.
> > > >
> > > > Chatted offline and the point is whether try-catch in user-space should
be
> > > able
> > > > to catch exceptions which occurs in callbacks.
> > > >
> > > > I tested behavior of addEventListener() on Firefox and I can't catch
> > > exceptions.
> > > > Though EventListener is not a callback function (it is callback
> interface),
> > we
> > > > might want to keep the current behavior. I'll remove the test (because
we
> > > can't
> > > > test actually) and revert SetVerbose(true) removal.
> > >
> > > Sounds good to me.
> > >
> > > On the other hand, your proposed behavior makes sense and it would be
worth
> > > discussing in platform-architecture-dev@.
> >
> > Event listeners registered by addEventListener are called asynchronously, so
> > there should be no way to catch an exception. However, callback functions
are
> > not always used asynchronously, like Lisa's test.
> >
> > function callback() {}
> > try {
> > domObj.invokeCallback(callback);
> > } catch (e) {
> > }
> >
> > In this case, I think the user script can catch an exception.
> >
> > In the case of addEventListener, we don't have a try-catch block like above
> when
> > invoking.
>
> I couldn't catch exceptions even when it's synchronous. Here is a snippet I
used
> to check the behavior.
>
> var b = document.getElementById('button');
> b.addEventListener('myevent', function(e) {
> throw new Error('foo');
> });
> var event = new Event('myevent');
> try {
> b.dispatchEvent(event);
> console.log('Exception suppressed');
> } catch(e) {
> console.log('Exception caught');
> }
>
> So I think it's good to keep the current behavior (at least until it becomes a
> real problem).
I'm fine to go with this CL as is for now, thus LGTM, but I'd say it may be
wrong.
https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke
See step 2- 7, when dispatching an event, an exception will be swallowed.
"If this throws an exception, report the exception."
Thus, I'd say that the behavior observed (= no exception observed) is the
behavior of dispatching events, not a behavior of callback functions in general.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PS3 LGTM
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right): https://codereview.chromium.org/2350813005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:37: [ExperimentalCallbackFunction] callback StringSequenceCallbackFunctionSequenceLongArg = sequence<DOMString> (sequence<long> arg); On 2016/09/23 03:04:45, peria wrote: > nit: inconsistent name; StrSeq in return type and SeqLong in the argument. Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2350813005/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2350813005/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== bindings: Support sequence in callback function arguments We need to support sequence as IntersectionObserverCallback uses a sequence. BUG=648486 ========== to ========== bindings: Support sequence in callback function arguments We need to support sequence as IntersectionObserverCallback uses a sequence. BUG=648486 Committed: https://crrev.com/a14a3854c3e69df4c7d1cb8c460816b54d6199a9 Cr-Commit-Position: refs/heads/master@{#421154} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a14a3854c3e69df4c7d1cb8c460816b54d6199a9 Cr-Commit-Position: refs/heads/master@{#421154} |
