Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444363002:
binding: Replaces DictionaryHelper::get(ScriptValue) with get(Nullable<T>). (Closed)
Descriptionbinding: Replaces DictionaryHelper::get(ScriptValue) with get(Nullable<T>).
Replaces DictionaryHelper::get(dict, key, ScriptValue&) with
get(dict, key, Nullable<T>&).
The only client of get(ScriptValue&) is EffectInput.cpp and it
expects "double?" in IDL. So, Nullable<T> should be better.
The motivation of this CL is to remove an use of
ScriptState::current() in order to make Blink use the correct
context. Also we'd like to reduce usage of ScriptValue that
saves a ScriptState inside.
BUG=618672
Committed: https://crrev.com/05d7c9d9594f75024b20a53cfbb1d6a8f5deb938
Cr-Commit-Position: refs/heads/master@{#431876}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : Synced. #Patch Set 4 : Synced. #Patch Set 5 : Applied git-cl-format. #Patch Set 6 : Applied git-cl-format. #Patch Set 7 : Applied git-cl-format. #
Messages
Total messages: 47 (36 generated)
The CQ bit was checked by yukishiino@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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...
yukishiino@chromium.org changed reviewers: + bashi@chromium.org, haraken@chromium.org
Could you guys review this CL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM
peria@chromium.org changed reviewers: + peria@chromium.org
https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h (right): https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h:61: if (DictionaryHelper::get(dictionary, key, innerValue)) { Shouldn't |innerValue| in the argument be a pointer?
lgtm https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h (right): https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h:61: if (DictionaryHelper::get(dictionary, key, innerValue)) { On 2016/10/26 10:01:35, peria wrote: > Shouldn't |innerValue| in the argument be a pointer? I think we pass |innerValue| by reference?
https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h (right): https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h:61: if (DictionaryHelper::get(dictionary, key, innerValue)) { On 2016/10/26 23:12:51, bashi1 wrote: > On 2016/10/26 10:01:35, peria wrote: > > Shouldn't |innerValue| in the argument be a pointer? > > I think we pass |innerValue| by reference? Yes, we're passing it by reference, unlike Chromium's style.
lgtm https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h (right): https://codereview.chromium.org/2444363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryHelperForBindings.h:61: if (DictionaryHelper::get(dictionary, key, innerValue)) { On 2016/10/27 13:04:25, Yuki wrote: > On 2016/10/26 23:12:51, bashi1 wrote: > > On 2016/10/26 10:01:35, peria wrote: > > > Shouldn't |innerValue| in the argument be a pointer? > > > > I think we pass |innerValue| by reference? > > Yes, we're passing it by reference, unlike Chromium's style. Ah, I wasn't aware of many get() overloads.
The CQ bit was checked by yukishiino@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yukishiino@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 yukishiino@chromium.org
The CQ bit was checked by yukishiino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, peria@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2444363002/#ps120001 (title: "Applied git-cl-format.")
The CQ bit was unchecked by yukishiino@chromium.org
The CQ bit was checked by yukishiino@chromium.org
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== binding: Replaces DictionaryHelper::get(ScriptValue) with get(Nullable<T>). Replaces DictionaryHelper::get(dict, key, ScriptValue&) with get(dict, key, Nullable<T>&). The only client of get(ScriptValue&) is EffectInput.cpp and it expects "double?" in IDL. So, Nullable<T> should be better. The motivation of this CL is to remove an use of ScriptState::current() in order to make Blink use the correct context. Also we'd like to reduce usage of ScriptValue that saves a ScriptState inside. BUG=618672 ========== to ========== binding: Replaces DictionaryHelper::get(ScriptValue) with get(Nullable<T>). Replaces DictionaryHelper::get(dict, key, ScriptValue&) with get(dict, key, Nullable<T>&). The only client of get(ScriptValue&) is EffectInput.cpp and it expects "double?" in IDL. So, Nullable<T> should be better. The motivation of this CL is to remove an use of ScriptState::current() in order to make Blink use the correct context. Also we'd like to reduce usage of ScriptValue that saves a ScriptState inside. BUG=618672 Committed: https://crrev.com/05d7c9d9594f75024b20a53cfbb1d6a8f5deb938 Cr-Commit-Position: refs/heads/master@{#431876} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/05d7c9d9594f75024b20a53cfbb1d6a8f5deb938 Cr-Commit-Position: refs/heads/master@{#431876} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
