|
|
Created:
3 years, 10 months ago by jochen (gone - plz use gerrit) Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCHECK correct context usage in V8ScriptRunner::callFunction
Not tests, since it shouldn't be possible to reach that.
BUG=693695
R=dcheng@chromium.org,haraken@chromium.org
Review-Url: https://codereview.chromium.org/2706813002
Cr-Commit-Position: refs/heads/master@{#451622}
Committed: https://chromium.googlesource.com/chromium/src/+/5cde67543e5c89f1f9da84e1ae860c30283dee3c
Patch Set 1 #
Total comments: 2
Patch Set 2 : updates #Messages
Total messages: 20 (12 generated)
The CQ bit was checked by jochen@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...
LGTM https://codereview.chromium.org/2706813002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right): https://codereview.chromium.org/2706813002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:114: frame, BindingSecurity::ErrorReportOption::DoNotReport)); Another idea would be to move the CHECK into V8ScriptRunner::callFunction?
LGTM https://codereview.chromium.org/2706813002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ErrorHandler.cpp (right): https://codereview.chromium.org/2706813002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ErrorHandler.cpp:92: toDOMWindow(callFunction->CreationContext())->toLocalDOMWindow(), Hmm... I guess I should fix this to return a LocalDOMWindow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
new approach (will update the CL description), ptal
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
Description was changed from ========== Sprinkle some CHECKs about correct context usage over event listeners Not tests, since it shouldn't be possible to reach them. BUG=693695 R=dcheng@chromium.org,haraken@chromium.org ========== to ========== CHECK correct context usage in V8ScriptRunner::callFunction Not tests, since it shouldn't be possible to reach that. BUG=693695 R=dcheng@chromium.org,haraken@chromium.org ==========
The CQ bit was unchecked by jochen@chromium.org
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2706813002/#ps20001 (title: "updates")
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": 20001, "attempt_start_ts": 1487598223405950, "parent_rev": "abf0ba6a507ee0eb418f49829a23caeb29778fd0", "commit_rev": "5cde67543e5c89f1f9da84e1ae860c30283dee3c"}
Message was sent while issue was closed.
Description was changed from ========== CHECK correct context usage in V8ScriptRunner::callFunction Not tests, since it shouldn't be possible to reach that. BUG=693695 R=dcheng@chromium.org,haraken@chromium.org ========== to ========== CHECK correct context usage in V8ScriptRunner::callFunction Not tests, since it shouldn't be possible to reach that. BUG=693695 R=dcheng@chromium.org,haraken@chromium.org Review-Url: https://codereview.chromium.org/2706813002 Cr-Commit-Position: refs/heads/master@{#451622} Committed: https://chromium.googlesource.com/chromium/src/+/5cde67543e5c89f1f9da84e1ae86... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5cde67543e5c89f1f9da84e1ae86...
Message was sent while issue was closed.
This is adding even more overhead to custom element callbacks and other callback APIs, can we make this a DCHECK? This alone is adding a dozen branches and many nested function calls to every call into JS from blink. |