Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523913003:
Fix bugs in filtered event removal code (Closed)
DescriptionFix bugs in filtered event removal code
1. There was a typo bug in EventRouter::RemoveFilterFromEvent()
2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo
Add unit tests to cover these changes.
BUG=670852
Test=None
Committed: https://crrev.com/348e5ca732110d639d2aea576535c12383836ffb
Cr-Commit-Position: refs/heads/master@{#436419}
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #Patch Set 3 : . #
Total comments: 18
Patch Set 4 : address comments #Patch Set 5 : sync @tott #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Fix a typo in EventRouter::RemoveFilterFromEvent() BUG=None Test=None ========== to ========== Fix a typo bug in EventRouter::RemoveFilterFromEvent() BUG=None Test=None ==========
Description was changed from ========== Fix a typo in EventRouter::RemoveFilterFromEvent() BUG=None Test=None ========== to ========== Fix a typo bug in EventRouter::RemoveFilterFromEvent() BUG=None Test=None ==========
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
The CQ bit was checked by lazyboy@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, but the fact that no tests caught this makes me worried. https://codereview.chromium.org/2523913003/diff/1/extensions/browser/event_ro... File extensions/browser/event_router.cc (right): https://codereview.chromium.org/2523913003/diff/1/extensions/browser/event_ro... extensions/browser/event_router.cc:424: if (filter_value->Equals(filter)) { ... so this meant we were always removing the first filter whenever this was called?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Fix a typo bug in EventRouter::RemoveFilterFromEvent() BUG=None Test=None ========== to ========== Fix bugs in filtered event removal code 1. There was a typo bug in EventRouter::RemoveFilterFromEvent() 2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo Add unit tests to cover these changes. BUG=670852 Test=None ==========
CL updated with fixing both bugs instead and I wrote unit tests to cover them, PTAL. https://codereview.chromium.org/2523913003/diff/1/extensions/browser/event_ro... File extensions/browser/event_router.cc (right): https://codereview.chromium.org/2523913003/diff/1/extensions/browser/event_ro... extensions/browser/event_router.cc:424: if (filter_value->Equals(filter)) { On 2016/11/24 01:21:32, Devlin wrote: > ... > > so this meant we were always removing the first filter whenever this was called? *Almost*. The fact that we had bug in writing those filtered events in prefs made this Equals bug obscure.
https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... File extensions/browser/event_router_unittest.cc (right): https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:12: #include "base/compiler_specific.h" #include "base/macros.h" typo https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:126: filter_dict->Set("hostSuffix", new StringValue(suffix)); ditto with line 128 https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:128: filter->Set("url", filter_list.release()); nit: DictionaryValue::Set() can take a unique_ptr - prefer that one. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:195: // extensions::ExtensionServiceTestBase::SetUp(); dd https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:266: if (iter.key() != event_name) This confuses me. Why do we only check one entry? Why not use DictionaryValue::GetList? https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:271: return nullptr; assuming we keep this impl, we could just ignore the result of the GetAsList() and return filter_list, which will be null if it failed. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:399: const std::string event_name = "chrome.onMyEvent"; Just for test clarity, it might be better to use a real event name (or one closer to it). We don't include the "chrome" normally, so this might confuse readers as to what it's actually testing. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:399: const std::string event_name = "chrome.onMyEvent"; nit: prefer kEventName, kExtensionId, etc for constants. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:409: extension_id, *filter1.get(), true); *foo.get() is redundant; -> *foo.
https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... File extensions/browser/event_router_unittest.cc (right): https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:12: #include "base/compiler_specific.h" #include "base/macros.h" On 2016/12/02 21:41:24, Devlin wrote: > typo Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:126: filter_dict->Set("hostSuffix", new StringValue(suffix)); On 2016/12/02 21:41:25, Devlin wrote: > ditto with line 128 Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:128: filter->Set("url", filter_list.release()); On 2016/12/02 21:41:25, Devlin wrote: > nit: DictionaryValue::Set() can take a unique_ptr - prefer that one. Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:195: // extensions::ExtensionServiceTestBase::SetUp(); On 2016/12/02 21:41:24, Devlin wrote: > dd Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:266: if (iter.key() != event_name) On 2016/12/02 21:41:25, Devlin wrote: > This confuses me. Why do we only check one entry? Why not use > DictionaryValue::GetList? The pref dictionary value looks like this: { "onEventFoo" : [ filter1, filter2, ..], "onEventBar" : [ filterA, filterB, ...] } Since we only wrote only one event, there's only one entry. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:271: return nullptr; On 2016/12/02 21:41:24, Devlin wrote: > assuming we keep this impl, we could just ignore the result of the GetAsList() > and return filter_list, which will be null if it failed. Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:399: const std::string event_name = "chrome.onMyEvent"; On 2016/12/02 21:41:24, Devlin wrote: > nit: prefer kEventName, kExtensionId, etc for constants. Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:399: const std::string event_name = "chrome.onMyEvent"; On 2016/12/02 21:41:24, Devlin wrote: > Just for test clarity, it might be better to use a real event name (or one > closer to it). We don't include the "chrome" normally, so this might confuse > readers as to what it's actually testing. Done. https://codereview.chromium.org/2523913003/diff/40001/extensions/browser/even... extensions/browser/event_router_unittest.cc:409: extension_id, *filter1.get(), true); On 2016/12/02 21:41:25, Devlin wrote: > *foo.get() is redundant; -> *foo. Done.
lgtm
The CQ bit was checked by lazyboy@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/2523913003/#ps80001 (title: "sync @tott")
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": 80001, "attempt_start_ts": 1480971370531230,
"parent_rev": "90a5b80c15d5840fcbc6b283fa5968d720e9fe58", "commit_rev":
"a55b66213152ae3bbaa2ffd0c319746d7a5603b9"}
Message was sent while issue was closed.
Description was changed from ========== Fix bugs in filtered event removal code 1. There was a typo bug in EventRouter::RemoveFilterFromEvent() 2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo Add unit tests to cover these changes. BUG=670852 Test=None ========== to ========== Fix bugs in filtered event removal code 1. There was a typo bug in EventRouter::RemoveFilterFromEvent() 2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo Add unit tests to cover these changes. BUG=670852 Test=None ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix bugs in filtered event removal code 1. There was a typo bug in EventRouter::RemoveFilterFromEvent() 2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo Add unit tests to cover these changes. BUG=670852 Test=None ========== to ========== Fix bugs in filtered event removal code 1. There was a typo bug in EventRouter::RemoveFilterFromEvent() 2. The way we are writing and reading filtered events in ExtensionPrefs was inconsistent: using GetList vs GetListWithoutPathExpansion. This matters because event names, which are used as dictionary keys, generally contain dots in them. e.g. chrome.onFoo Add unit tests to cover these changes. BUG=670852 Test=None Committed: https://crrev.com/348e5ca732110d639d2aea576535c12383836ffb Cr-Commit-Position: refs/heads/master@{#436419} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/348e5ca732110d639d2aea576535c12383836ffb Cr-Commit-Position: refs/heads/master@{#436419} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
