Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545683003:
Add a test for KeywordExtensionsDelegateImpl::IsEnabledExtension(). (Closed)
|
Created:
6 years, 3 months ago by Lei Zhang Modified:
6 years, 3 months ago Reviewers:
Yoyo Zhou CC:
chromium-reviews, chromium-apps-reviews_chromium.org, James Su, extensions-reviews_chromium.org, Peter Kasting, Mark P Base URL:
https://chromium.googlesource.com/chromium/src.git@fix409705 Project:
chromium Visibility:
Public. |
DescriptionAdd a test for KeywordExtensionsDelegateImpl::IsEnabledExtension().
BUG=409705
Committed: https://crrev.com/8a2d7f6350bd05f29e38c57dd77e9192a220162d
Cr-Commit-Position: refs/heads/master@{#293658}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : rebase to head where the fix for bug 409705 has gone in, address comments #
Total comments: 1
Patch Set 3 : rebase #
Messages
Total messages: 27 (11 generated)
Patchset #1 (id:1) has been deleted
thestig@chromium.org changed reviewers: + yoz@chromium.org
Test case that will fail as is and pass with https://codereview.chromium.org/537283002/ applied. Wrangling the TestExtensionFoo classes to work in incognito mode too a while to figure out. @_@
On 2014/09/05 05:28:13, Lei Zhang wrote: > Test case that will fail as is and pass with > https://codereview.chromium.org/537283002/ applied. > > Wrangling the TestExtensionFoo classes to work in incognito mode too a while to > figure out. @_@ s/too/took/
https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:26: class ExtensionLoadObserver : public ExtensionRegistryObserver { Consider making this a ScopedExtensionLoadObserver. In the constructor, AddObserver, and destructor, RemoveObserver. https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:73: extension_system->CreateProcessManager(); This is all just InitializeProcessManager() https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:120: KeywordExtensionsDelegateImpl delegate_impl(profile_to_use, Here's a suggestion. We could use dependency injection for KeywordExtensionDelegateImpl so that its constructor takes (Profile*, ExtensionService*, KeywordProvider*) and just pass in service() here. This would allow us to skip the association with the incognito profile here (and the changes to TestExtensionSystem). This does make the implementation a little bit more constrained, but it is a test for an Impl class anyways. https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.cc (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.cc:49: if (extension_service_owned_) && extension_service_? https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.h (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.h:64: // Takes ownership of |service| if |owned| if true. typo: is true
(Replying from your desk) https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:26: class ExtensionLoadObserver : public ExtensionRegistryObserver { On 2014/09/05 16:21:33, Yoyo Zhou wrote: > Consider making this a ScopedExtensionLoadObserver. > In the constructor, AddObserver, and destructor, RemoveObserver. Done. https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:73: extension_system->CreateProcessManager(); On 2014/09/05 16:21:33, Yoyo Zhou wrote: > This is all just InitializeProcessManager() Done. https://codereview.chromium.org/545683003/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc:120: KeywordExtensionsDelegateImpl delegate_impl(profile_to_use, On 2014/09/05 16:21:33, Yoyo Zhou wrote: > Here's a suggestion. We could use dependency injection for > KeywordExtensionDelegateImpl so that its constructor takes (Profile*, > ExtensionService*, KeywordProvider*) and just pass in service() here. This would > allow us to skip the association with the incognito profile here (and the > changes to TestExtensionSystem). This does make the implementation a little bit > more constrained, but it is a test for an Impl class anyways. Better yet, I'm just going to take this portion of my other cleanup CL and use it in this CL instead. THen we don't have to inject an ExtensionService*. https://codereview.chromium.org/543693003/diff/40001/chrome/browser/autocompl... https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.cc (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.cc:49: if (extension_service_owned_) On 2014/09/05 16:21:33, Yoyo Zhou wrote: > && extension_service_? Nope. http://www.parashift.com/c++-faq/delete-handles-null.html https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.h (right): https://codereview.chromium.org/545683003/diff/20001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.h:64: // Takes ownership of |service| if |owned| if true. On 2014/09/05 16:21:33, Yoyo Zhou wrote: > typo: is true Reverted TestExtensionSystem changes.
LGTM (from US-CAM) https://codereview.chromium.org/545683003/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/keyword_extensions_delegate_impl.cc (right): https://codereview.chromium.org/545683003/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/keyword_extensions_delegate_impl.cc:55: extensions::ExtensionRegistry::Get( right, I should have noticed this.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/545683003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/52369)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/545683003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/545683003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/545683003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/545683003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as f8237201400c663da86332f3f596657861cb500a
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8a2d7f6350bd05f29e38c57dd77e9192a220162d Cr-Commit-Position: refs/heads/master@{#293658} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
