Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236133002:
Ignore filtered event if an event matcher cannot be added. (Closed)
|
Created:
4 years, 4 months ago by meacer Modified:
4 years, 4 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIgnore filtered event if an event matcher cannot be added.
BUG=625404
Committed: https://crrev.com/ba011d9f8322c62633a069a59c2c5525e3ff46cc
Cr-Commit-Position: refs/heads/master@{#411472}
Patch Set 1 #
Total comments: 8
Patch Set 2 : devlin comments #
Total comments: 2
Patch Set 3 : DCHECK #Messages
Total messages: 14 (6 generated)
meacer@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin: PTAL, thanks.
https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:277: base::DictionaryValue* filter_weak = filter.get(); This makes me sad, because there's an awful lot of redirection going on here where it's easy for this to get lost. I think it would be a little better (though still not great) if we kept around a reference to the EventMatcher and accessed filter through that, rather than a reference to the filter itself - at least that's one less layer of redirection to keep track of. https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:281: return; It looks like other places this fails, we return -1 (e.g. line 269).
The CQ bit was checked by meacer@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...
https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:277: base::DictionaryValue* filter_weak = filter.get(); On 2016/08/11 19:08:12, Devlin wrote: > This makes me sad, because there's an awful lot of redirection going on here > where it's easy for this to get lost. I think it would be a little better > (though still not great) if we kept around a reference to the EventMatcher and > accessed filter through that, rather than a reference to the filter itself - at > least that's one less layer of redirection to keep track of. Done. It seems safe to do this, but I wonder if I should add a CHECK to see if the previous filter value is equal to the one we get after adding the event matcher and getting it back. Since this is single threaded JS code, I guess it's not needed? https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:281: return; On 2016/08/11 19:08:12, Devlin wrote: > It looks like other places this fails, we return -1 (e.g. line 269). Line 259 doesn't set it to -1. Should I add it there too?
lgtm https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:277: base::DictionaryValue* filter_weak = filter.get(); On 2016/08/11 21:53:00, Mustafa Emre Acer wrote: > On 2016/08/11 19:08:12, Devlin wrote: > > This makes me sad, because there's an awful lot of redirection going on here > > where it's easy for this to get lost. I think it would be a little better > > (though still not great) if we kept around a reference to the EventMatcher and > > accessed filter through that, rather than a reference to the filter itself - > at > > least that's one less layer of redirection to keep track of. > > Done. It seems safe to do this, but I wonder if I should add a CHECK to see if > the previous filter value is equal to the one we get after adding the event > matcher and getting it back. Since this is single threaded JS code, I guess it's > not needed? I wouldn't be opposed to a DCHECK, if you want to add one. https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:281: return; On 2016/08/11 21:53:00, Mustafa Emre Acer wrote: > On 2016/08/11 19:08:12, Devlin wrote: > > It looks like other places this fails, we return -1 (e.g. line 269). > > Line 259 doesn't set it to -1. Should I add it there too? I think line 259 throws an error if it fails, so we should be okay there. https://codereview.chromium.org/2236133002/diff/20001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/20001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:285: g_event_filter.Get().GetEventMatcher(id)->value(); I wonder if we should also DCHECK that we get an EventMatcher* value. I'll leave it up to you.
https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:277: base::DictionaryValue* filter_weak = filter.get(); On 2016/08/11 21:57:39, Devlin wrote: > On 2016/08/11 21:53:00, Mustafa Emre Acer wrote: > > On 2016/08/11 19:08:12, Devlin wrote: > > > This makes me sad, because there's an awful lot of redirection going on here > > > where it's easy for this to get lost. I think it would be a little better > > > (though still not great) if we kept around a reference to the EventMatcher > and > > > accessed filter through that, rather than a reference to the filter itself - > > at > > > least that's one less layer of redirection to keep track of. > > > > Done. It seems safe to do this, but I wonder if I should add a CHECK to see if > > the previous filter value is equal to the one we get after adding the event > > matcher and getting it back. Since this is single threaded JS code, I guess > it's > > not needed? > > I wouldn't be opposed to a DCHECK, if you want to add one. On second thought this seemed overkill so decided not to do it :) https://codereview.chromium.org/2236133002/diff/1/extensions/renderer/event_b... extensions/renderer/event_bindings.cc:281: return; On 2016/08/11 21:57:39, Devlin wrote: > On 2016/08/11 21:53:00, Mustafa Emre Acer wrote: > > On 2016/08/11 19:08:12, Devlin wrote: > > > It looks like other places this fails, we return -1 (e.g. line 269). > > > > Line 259 doesn't set it to -1. Should I add it there too? > > I think line 259 throws an error if it fails, so we should be okay there. Ah, okay didn't read it well. https://codereview.chromium.org/2236133002/diff/20001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/2236133002/diff/20001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:285: g_event_filter.Get().GetEventMatcher(id)->value(); On 2016/08/11 21:57:39, Devlin wrote: > I wonder if we should also DCHECK that we get an EventMatcher* value. I'll > leave it up to you. Done.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2236133002/#ps40001 (title: "DCHECK")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ignore filtered event if an event matcher cannot be added. BUG=625404 ========== to ========== Ignore filtered event if an event matcher cannot be added. BUG=625404 Committed: https://crrev.com/ba011d9f8322c62633a069a59c2c5525e3ff46cc Cr-Commit-Position: refs/heads/master@{#411472} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ba011d9f8322c62633a069a59c2c5525e3ff46cc Cr-Commit-Position: refs/heads/master@{#411472} |
