Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787713002:
Remove deprecated extension notification from shortcuts_extensions_manager.h/cc (Closed)
DescriptionRemove deprecated extension notification from shortcuts_extensions_manager.h/cc
Use ExtensionRegistryObserver instead of deprecated extension notifications.
Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension.
TEST=unit_tests --gtest_filter=ShortcutsProviderExtensionTest.Extension
BUG=411568
Review-Url: https://codereview.chromium.org/2787713002
Cr-Commit-Position: refs/heads/master@{#461078}
Committed: https://chromium.googlesource.com/chromium/src/+/01409c3315646cb0acca04308f67421410791800
Patch Set 1 #Patch Set 2 : move to enable_extensions #
Total comments: 10
Patch Set 3 : review #
Messages
Total messages: 33 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by limasdf@gmail.com 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #2 (id:60001) has been deleted
Description was changed from ========== WIP BUG= ========== to ========== WIP BUG= ==========
Description was changed from ========== WIP BUG= ========== to ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc BUG=411568 ==========
Description was changed from ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc BUG=411568 ========== to ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. BUG=411568 ==========
limasdf@gmail.com changed reviewers: + sky@chromium.org
Hi Sky, Could you take a look when you have time?
Description was changed from ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. BUG=411568 ========== to ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. TEST=unittest --gtest_filter=ShortcutsProviderExtensionTest.Extension BUG=411568 ==========
Description was changed from ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. TEST=unittest --gtest_filter=ShortcutsProviderExtensionTest.Extension BUG=411568 ========== to ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. TEST=unit_tests --gtest_filter=ShortcutsProviderExtensionTest.Extension BUG=411568 ==========
sky@chromium.org changed reviewers: + mpearson@chromium.org - sky@chromium.org
sky->mpearson
I'm not familiar with this code at all. Handing off to pkasting@. --mark
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
mpearson->pkasting --mark
LGTM https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_backend_factory.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_backend_factory.cc:21: #if BUILDFLAG(ENABLE_EXTENSIONS) Nit: Have one #if cover the #include here and the namespace just below. https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.cc:20: ShortcutsExtensionsManager::~ShortcutsExtensionsManager() {} Nit: You're also welcome to =default this in the header if the bots like it https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.h (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.h:13: #if !BUILDFLAG(ENABLE_EXTENSIONS) Nit: I'd omit this as I'm not sure it buys much? https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.h:43: Nit: Blank line optional
Thank you very much for the review! https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_backend_factory.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_backend_factory.cc:21: #if BUILDFLAG(ENABLE_EXTENSIONS) On 2017/03/30 23:35:31, Peter Kasting wrote: > Nit: Have one #if cover the #include here and the namespace just below. Done. https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.cc:20: ShortcutsExtensionsManager::~ShortcutsExtensionsManager() {} On 2017/03/30 23:35:31, Peter Kasting wrote: > Nit: You're also welcome to =default this in the header if the bots like it Linking error for `= default` in the header :( https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.h (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.h:13: #if !BUILDFLAG(ENABLE_EXTENSIONS) On 2017/03/30 23:35:31, Peter Kasting wrote: > Nit: I'd omit this as I'm not sure it buys much? Done. https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.h:43: On 2017/03/30 23:35:31, Peter Kasting wrote: > Nit: Blank line optional Done.
The CQ bit was checked by limasdf@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2787713002/#ps100001 (title: "review")
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...)
https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.cc:20: ShortcutsExtensionsManager::~ShortcutsExtensionsManager() {} On 2017/03/31 03:26:27, limasdf wrote: > On 2017/03/30 23:35:31, Peter Kasting wrote: > > Nit: You're also welcome to =default this in the header if the bots like it > > Linking error for `= default` in the header :( Oh? I would have expected the warning, if any, to be a presubmit complaint about not having complicated inline functions.
btw, ios-simulator failure doesn't seem to related to this change. I'll merge when flakiness disappeared. https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... File chrome/browser/autocomplete/shortcuts_extensions_manager.cc (right): https://codereview.chromium.org/2787713002/diff/80001/chrome/browser/autocomp... chrome/browser/autocomplete/shortcuts_extensions_manager.cc:20: ShortcutsExtensionsManager::~ShortcutsExtensionsManager() {} On 2017/03/31 06:09:59, Peter Kasting wrote: > On 2017/03/31 03:26:27, limasdf wrote: > > On 2017/03/30 23:35:31, Peter Kasting wrote: > > > Nit: You're also welcome to =default this in the header if the bots like it > > > > Linking error for `= default` in the header :( > > Oh? I would have expected the warning, if any, to be a presubmit complaint > about not having complicated inline functions. Sorry for the confusion. First I just commented out from .cc file. so that presubmit didn't blame. After removing, presubmit complaint about not having complicated inline functions.
The CQ bit was checked by limasdf@gmail.com
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": 100001, "attempt_start_ts": 1490947433112790,
"parent_rev": "1a96e39ce2f4cd1956cba47c4e6413de95d11039", "commit_rev":
"01409c3315646cb0acca04308f67421410791800"}
Message was sent while issue was closed.
Description was changed from ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. TEST=unit_tests --gtest_filter=ShortcutsProviderExtensionTest.Extension BUG=411568 ========== to ========== Remove deprecated extension notification from shortcuts_extensions_manager.h/cc Use ExtensionRegistryObserver instead of deprecated extension notifications. Also, Make shortcuts_extensions_manager.h/cc buildable only for enable_extension. TEST=unit_tests --gtest_filter=ShortcutsProviderExtensionTest.Extension BUG=411568 Review-Url: https://codereview.chromium.org/2787713002 Cr-Commit-Position: refs/heads/master@{#461078} Committed: https://chromium.googlesource.com/chromium/src/+/01409c3315646cb0acca04308f67... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/01409c3315646cb0acca04308f67... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
