|
|
Created:
4 years ago by bradnelson Modified:
4 years ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, Benedikt Meurer, chromium-reviews, danno Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wasm][asm.js] Routing asm.js warnings to the dev console.
Route asm.js warnings from V8 to the developer console.
Depends on: https://codereview.chromium.org/2526703002/
BUG=v8:4203, chromium:660016
R=dgozman@chromium.org,jochen@chromium.org
CC=danno@chromium.org,bmeurer@chromium.org
Committed: https://crrev.com/07d5887d7471da0cf7bacb87d9742927f83f0c6d
Cr-Commit-Position: refs/heads/master@{#438511}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 2
Patch Set 3 : add tests #Patch Set 4 : worker #Patch Set 5 : merge #Patch Set 6 : merge #Patch Set 7 : deps #
Total comments: 4
Patch Set 8 : fix #Patch Set 9 : merge #
Messages
Total messages: 49 (27 generated)
bradnelson@google.com changed reviewers: + bradnelson@google.com
Ping?
was it possible to use the regular message handler?
On 2016/11/29 15:45:11, jochen wrote: > was it possible to use the regular message handler? Creating a separate but similar message handler was my suggestion, as I'm worried that existing clients could break after receiving an unexpected message. For example, blink treats all messages as errors, while this one is intended to be a warning. Any suggestions maybe?
Dimitry and I talked. I'm going to change these two round so that jsmessage is shared (adds an error level field). And add a parameter to AddMessageListener to select which error levels to receive. It will default to the current behavior (ie just errors). How does this sound? On Tue, Nov 29, 2016, 10:53 AM <dgozman@chromium.org> wrote: > On 2016/11/29 15:45:11, jochen wrote: > > was it possible to use the regular message handler? > > Creating a separate but similar message handler was my suggestion, as I'm > worried that existing clients could break after receiving an unexpected > message. > For example, blink treats all messages as errors, while this one is > intended to > be a warning. Any suggestions maybe? > > https://codereview.chromium.org/2530503002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Dimitry and I talked. I'm going to change these two round so that jsmessage is shared (adds an error level field). And add a parameter to AddMessageListener to select which error levels to receive. It will default to the current behavior (ie just errors). How does this sound? On Tue, Nov 29, 2016, 10:53 AM <dgozman@chromium.org> wrote: > On 2016/11/29 15:45:11, jochen wrote: > > was it possible to use the regular message handler? > > Creating a separate but similar message handler was my suggestion, as I'm > worried that existing clients could break after receiving an unexpected > message. > For example, blink treats all messages as errors, while this one is > intended to > be a warning. Any suggestions maybe? > > https://codereview.chromium.org/2530503002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
that sounds already better. I wonder, however, have we considered adding the message directly from V8 to the console, now that the console object is part of v8?
(sorry for my delayed responses, somehow this CL never shows up as pending feedback from me in the codereview dashboard..)
On 2016/11/30 19:02:49, jochen wrote: > that sounds already better. > > I wonder, however, have we considered adding the message directly from V8 to the > console, now that the console object is part of v8? We don't have such a mechanism yet, except for console.xxx APIs, but that's user-land.
PTAL at this and the V8 side change.
Is it possible to produce wasm warning in a test? https://codereview.chromium.org/2530503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2530503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:167: frame->document()->addConsoleMessage(ConsoleMessage::create( ExecutionContext has addConsoleMessage method, let's just call it. There is no need to artificially constrain messages to documents only.
also, where will warnings from workers go?
The CQ bit was checked by bradnelson@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
The CQ bit was checked by bradnelson@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...
PTAL Added tests, added worker support.
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bradnelson@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bradnelson@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2530503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2530503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:167: frame->document()->addConsoleMessage(ConsoleMessage::create( On 2016/12/01 18:38:33, dgozman wrote: > ExecutionContext has addConsoleMessage method, let's just call it. There is no > need to artificially constrain messages to documents only. Done.
Ping?
lgtm https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:142: DCHECK(false); NOTREACHED(); https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:146: }; no ; at the end of namespace. Please also add a comment like // namespace
https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:142: DCHECK(false); On 2016/12/14 11:42:43, jochen wrote: > NOTREACHED(); Done. https://codereview.chromium.org/2530503002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:146: }; On 2016/12/14 11:42:43, jochen wrote: > no ; at the end of namespace. Please also add a comment like // namespace Done.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2530503002/#ps140001 (title: "fix")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2530503002/#ps160001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481718383867150, "parent_rev": "48ae264eaf2f4c1e1a506f1b80e848e62cc21baa", "commit_rev": "fd462705097457eee26eae732a7cedb96b1e3b6d"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Routing asm.js warnings to the dev console. Route asm.js warnings from V8 to the developer console. Depends on: https://codereview.chromium.org/2526703002/ BUG=v8:4203, chromium:660016 R=dgozman@chromium.org,jochen@chromium.org CC=danno@chromium.org,bmeurer@chromium.org ========== to ========== [wasm][asm.js] Routing asm.js warnings to the dev console. Route asm.js warnings from V8 to the developer console. Depends on: https://codereview.chromium.org/2526703002/ BUG=v8:4203, chromium:660016 R=dgozman@chromium.org,jochen@chromium.org CC=danno@chromium.org,bmeurer@chromium.org Review-Url: https://codereview.chromium.org/2530503002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Routing asm.js warnings to the dev console. Route asm.js warnings from V8 to the developer console. Depends on: https://codereview.chromium.org/2526703002/ BUG=v8:4203, chromium:660016 R=dgozman@chromium.org,jochen@chromium.org CC=danno@chromium.org,bmeurer@chromium.org Review-Url: https://codereview.chromium.org/2530503002 ========== to ========== [wasm][asm.js] Routing asm.js warnings to the dev console. Route asm.js warnings from V8 to the developer console. Depends on: https://codereview.chromium.org/2526703002/ BUG=v8:4203, chromium:660016 R=dgozman@chromium.org,jochen@chromium.org CC=danno@chromium.org,bmeurer@chromium.org Committed: https://crrev.com/07d5887d7471da0cf7bacb87d9742927f83f0c6d Cr-Commit-Position: refs/heads/master@{#438511} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/07d5887d7471da0cf7bacb87d9742927f83f0c6d Cr-Commit-Position: refs/heads/master@{#438511} |