Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334083002:
Add a test helper class for ExtensionRegistry. (Closed)
|
Created:
6 years, 6 months ago by limasdf Modified:
6 years, 5 months ago Reviewers:
Lei Zhang, Ken Rockot(use gerrit already), Yoyo Zhou, sky, not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a test helper class for ExtensionRegistry.
And use it from c/e/b/extensions/.
BUG=354046
R=yoz@chromium.org, thestig@chromium.org, kalman@chromium.org
TEST=browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280415
Patch Set 1 #
Total comments: 10
Patch Set 2 : Ben's comments #
Total comments: 8
Patch Set 3 : Y and B comments. 2014 brazil world cup morning at 6AM korea vs russia #
Total comments: 8
Patch Set 4 : pass extension id #
Total comments: 4
Patch Set 5 : yoyo #
Total comments: 6
Patch Set 6 : lei #Patch Set 7 : remove failed test to fix later #
Messages
Total messages: 41 (0 generated)
https://codereview.chromium.org/334083002/diff/1/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/334083002/diff/1/extensions/DEPS#newcode4 extensions/DEPS:4: ################################# I'm not sure if this is allowed. Could you help in right direction?
Great idea. https://codereview.chromium.org/334083002/diff/1/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/334083002/diff/1/extensions/DEPS#newcode4 extensions/DEPS:4: ################################# On 2014/06/15 13:36:54, limasdf wrote: > I'm not sure if this is allowed. > Could you help in right direction? +rockot, yoz https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... File extensions/browser/extension_test_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.cc:31: runner_uninstalled_->Run(); it would be nice to factor all of this shared state into a single class, that is, the tuple of (waiting, received, runner). For example, === extension_test_registry_observer.h class Waiter; scoped_ptr<Waiter> installed_waiter_; scoped_ptr<Waiter> uninstalled_waiter_; === extension_test_registry_observer.cc class ExtensionTestRegistryObserver::Waiter { public: Waiter(...) : ... {} void Wait(...) { } void OnObserved(...) { } private: ... }; ExtensionTestRegistryObserver::ExtensionTestRegistryObserver(...) : ..., installed_waiter_(new Waiter()), uninstalled_watier_(new Waiter()) { } void ExtensionTestRegistryObserver::WaitForExtensionInstalled() { installed_waiter_->Wait(); } void ExtensionTestRegistryObserver::OnExtensionWillBeInstalled(...) { installed_waiter_->OnObserved(); } https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... File extensions/browser/extension_test_registry_observer.h (right): https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:6: #define EXTENSIONS_BROWSER_EXTENSION_TEST_REGISTRY_OBSERVER_H_ +yoz, rockot Can this file go in extensions/test? It would be nice to prepare for a time when this class can integrate with gtest, and return testing::AssertionResults for example. However, it depends on extensions/browser. not sure if that matters. https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:10: #include "extensions/browser/extension_registry.h" forward declare? https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:21: void WaitForExtensionInstalled(); This should probably be "WaitForExtensionWillBeInstalled" I suppose. https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:22: void WaitForExtensionUninstalled(); I expect that eventually we'll want to be checking for specific extensions being installed, uninstalled, loaded, unloaded, etc. In the meantime can you name these WaitForAnyExtensionUninstalled and WaitForAnyExtensionWillBeInstalled? Maybe one day we'll have a version of WaitForExtensionUninstalled(const std::string& id, UninstallReason reason) etc.
Addressed ben's comment. Still waiting for +rockot, yoz 's review (doesn't mean pushing you :) https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... File extensions/browser/extension_test_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.cc:31: runner_uninstalled_->Run(); On 2014/06/15 21:20:16, kalman wrote: > it would be nice to factor all of this shared state into a single class, that > is, the tuple of (waiting, received, runner). For example, > > === extension_test_registry_observer.h > class Waiter; > scoped_ptr<Waiter> installed_waiter_; > scoped_ptr<Waiter> uninstalled_waiter_; > > === extension_test_registry_observer.cc > class ExtensionTestRegistryObserver::Waiter { > public: > Waiter(...) : ... {} > > void Wait(...) { > } > > void OnObserved(...) { > } > > private: > ... > }; > > ExtensionTestRegistryObserver::ExtensionTestRegistryObserver(...) > : ..., installed_waiter_(new Waiter()), uninstalled_watier_(new Waiter()) { > } > > void ExtensionTestRegistryObserver::WaitForExtensionInstalled() { > installed_waiter_->Wait(); > } > > void ExtensionTestRegistryObserver::OnExtensionWillBeInstalled(...) { > installed_waiter_->OnObserved(); > } Done. https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... File extensions/browser/extension_test_registry_observer.h (right): https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:10: #include "extensions/browser/extension_registry.h" On 2014/06/15 21:20:16, kalman wrote: > forward declare? Done. https://codereview.chromium.org/334083002/diff/1/extensions/browser/extension... extensions/browser/extension_test_registry_observer.h:21: void WaitForExtensionInstalled(); On 2014/06/15 21:20:17, kalman wrote: > This should probably be "WaitForExtensionWillBeInstalled" I suppose. WaitForAnyExtensionWillBeInstalled.
https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... File extensions/browser/extension_test_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... extensions/browser/extension_test_registry_observer.cc:35: scoped_refptr<content::MessageLoopRunner> runner_; Doesn't look like this is ever initialized? And when it is, you could use its existence rather than |waiting_|.
https://chromiumcodereview.appspot.com/334083002/diff/20001/extensions/browse... File extensions/browser/extension_test_registry_observer.h (right): https://chromiumcodereview.appspot.com/334083002/diff/20001/extensions/browse... extensions/browser/extension_test_registry_observer.h:16: class ExtensionTestRegistryObserver : public ExtensionRegistryObserver { nit: TestExtensionRegistryObserver? https://chromiumcodereview.appspot.com/334083002/diff/20001/extensions/browse... extensions/browser/extension_test_registry_observer.h:21: void WaitForAnyExtensionWillBeInstalled(); These should be documented. What do you think of changing this interface to wait for a specific extension ID? It seems like in all of our tests that use this right now, there is an extension ID we can get at. This would make the test more reliable. https://chromiumcodereview.appspot.com/334083002/diff/20001/extensions/browse... extensions/browser/extension_test_registry_observer.h:37: scoped_ptr<Waiter> installed_waiter_; should be will_be_uninstalled_ for consistency.
Could you? https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... File extensions/browser/extension_test_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... extensions/browser/extension_test_registry_observer.cc:35: scoped_refptr<content::MessageLoopRunner> runner_; On 2014/06/16 21:42:08, kalman wrote: > Doesn't look like this is ever initialized? And when it is, you could use its > existence rather than |waiting_|. Exactly. Happy to remove |waiting_|. https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... File extensions/browser/extension_test_registry_observer.h (right): https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... extensions/browser/extension_test_registry_observer.h:16: class ExtensionTestRegistryObserver : public ExtensionRegistryObserver { On 2014/06/17 02:29:54, Yoyo Zhou wrote: > nit: TestExtensionRegistryObserver? Done. https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... extensions/browser/extension_test_registry_observer.h:21: void WaitForAnyExtensionWillBeInstalled(); On 2014/06/17 02:29:54, Yoyo Zhou wrote: > These should be documented. > What do you think of changing this interface to wait for a specific extension > ID? It seems like in all of our tests that use this right now, there is an > extension ID we can get at. This would make the test more reliable. Done. https://codereview.chromium.org/334083002/diff/20001/extensions/browser/exten... extensions/browser/extension_test_registry_observer.h:37: scoped_ptr<Waiter> installed_waiter_; On 2014/06/17 02:29:54, Yoyo Zhou wrote: > should be will_be_uninstalled_ for consistency. Done.
https://chromiumcodereview.appspot.com/334083002/diff/60001/chrome/browser/ex... File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc (right): https://chromiumcodereview.appspot.com/334083002/diff/60001/chrome/browser/ex... chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:203: LOG(INFO) << "uninstallid:" + last_loaded_extension_id(); Please remove these log lines. https://chromiumcodereview.appspot.com/334083002/diff/60001/chrome/browser/ex... File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/334083002/diff/60001/chrome/browser/ex... chrome/browser/extensions/extension_storage_monitor_browsertest.cc:340: observer.WaitForExtensionUninstalled(extension->id()); You should copy extension->id() into a temporary string because if the extension is uninstalled, extension can become invalid. https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/DEPS File extensions/DEPS (right): https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/DEPS#n... extensions/DEPS:6: "+content/public/test", Yes, this is fine. https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/browse... File extensions/browser/test_extension_registry_observer.cc (right): https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/browse... extensions/browser/test_extension_registry_observer.cc:46: : will_be_installed_waiter_(new Waiter()), Do we need to instantiate all these if they're not being used? We can do e.g. in WaitForExtensionUninstalled, uninstalled_waiter_.reset(new Waiter); uninstalled_waiter_->Wait... As it is, we can't wait for multiple events in parallel anyway (at least with this design).
Some of the comments here represent behavioural changes. The Waiter is initialized at the start so that the following code works: TestExtensionRegistryObserver observer; DoSomethingWhichCausesExtensionInstallation(); observer.WaitForInstallation(); maybe I'm misunderstanding your comment there actually. But the thing with the extension ID: I think this is good, typically you're only ever waiting for a single extension and it would save needing to return the Extension or something, but then you need to decide what to happen when an event is for a different extension. I think just ignore it, but record the event, then have a method like HasUnhandledActivity() or something like that which tests can EXPECT_FALSE() of.
https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/browse... File extensions/browser/test_extension_registry_observer.cc (right): https://chromiumcodereview.appspot.com/334083002/diff/60001/extensions/browse... extensions/browser/test_extension_registry_observer.cc:46: : will_be_installed_waiter_(new Waiter()), On 2014/06/17 18:15:37, Yoyo Zhou wrote: > Do we need to instantiate all these if they're not being used? > We can do e.g. in WaitForExtensionUninstalled, > uninstalled_waiter_.reset(new Waiter); > uninstalled_waiter_->Wait... > > As it is, we can't wait for multiple events in parallel anyway (at least with > this design). Ah, I see what you mean. Right now there is a flaw that if 2 extensions get installed, for example, we only keep the 2nd extension's ID (even if we wanted to wait for the first one). Since this is for the purpose of observing a single extension, can we pass the ID in the constructor for TestExtensionRegistryObserver and Waiter?
yeah passing into the constructor would be easier.
Sorry for being late. PTAL! https://codereview.chromium.org/334083002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc (right): https://codereview.chromium.org/334083002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:203: LOG(INFO) << "uninstallid:" + last_loaded_extension_id(); On 2014/06/17 18:15:37, Yoyo Zhou wrote: > Please remove these log lines. Done. https://codereview.chromium.org/334083002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right): https://codereview.chromium.org/334083002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_storage_monitor_browsertest.cc:340: observer.WaitForExtensionUninstalled(extension->id()); On 2014/06/17 18:15:37, Yoyo Zhou wrote: > You should copy extension->id() into a temporary string because if the extension > is uninstalled, extension can become invalid. Pass to the contructor. https://codereview.chromium.org/334083002/diff/60001/extensions/browser/test_... File extensions/browser/test_extension_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/60001/extensions/browser/test_... extensions/browser/test_extension_registry_observer.cc:46: : will_be_installed_waiter_(new Waiter()), On 2014/06/17 21:48:06, Yoyo Zhou wrote: > On 2014/06/17 18:15:37, Yoyo Zhou wrote: > > Do we need to instantiate all these if they're not being used? > > We can do e.g. in WaitForExtensionUninstalled, > > uninstalled_waiter_.reset(new Waiter); > > uninstalled_waiter_->Wait... > > > > As it is, we can't wait for multiple events in parallel anyway (at least with > > this design). > > Ah, I see what you mean. > > Right now there is a flaw that if 2 extensions get installed, for example, we > only keep the 2nd extension's ID (even if we wanted to wait for the first one). > > Since this is for the purpose of observing a single extension, can we pass the > ID in the constructor for TestExtensionRegistryObserver and Waiter? Done.
https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... File extensions/browser/test_extension_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... extensions/browser/test_extension_registry_observer.cc:15: : observed_(false), runner_(NULL), extension_id_(extension_id) {} Actually, I think the waiter doesn't need the extension id. See below. https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... extensions/browser/test_extension_registry_observer.cc:81: will_be_installed_waiter_->OnObserved(extension->id()); I think I would prefer if the extension_id_ was a member of TestExtensionRegistryObserver: if (extension->id() == extension_id_) will_be_installed_waiter_->OnObserved();
https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... File extensions/browser/test_extension_registry_observer.cc (right): https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... extensions/browser/test_extension_registry_observer.cc:15: : observed_(false), runner_(NULL), extension_id_(extension_id) {} On 2014/06/19 18:23:35, Yoyo Zhou wrote: > Actually, I think the waiter doesn't need the extension id. See below. Done. https://codereview.chromium.org/334083002/diff/80001/extensions/browser/test_... extensions/browser/test_extension_registry_observer.cc:81: will_be_installed_waiter_->OnObserved(extension->id()); On 2014/06/19 18:23:35, Yoyo Zhou wrote: > I think I would prefer if the extension_id_ was a member of > TestExtensionRegistryObserver: > if (extension->id() == extension_id_) > will_be_installed_waiter_->OnObserved(); Done.
LGTM
Added Lei, for the gypi file. Lei could you take a look?
lgtm with some nits https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc (right): https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:201: extensions::TestExtensionRegistryObserver observer( you can omit extensions:: because you are already in the extensions namespace. https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right): https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_storage_monitor_browsertest.cc:337: extensions::TestExtensionRegistryObserver observer( ditto, already in the extensions namespace https://codereview.chromium.org/334083002/diff/100001/extensions/browser/test... File extensions/browser/test_extension_registry_observer.h (right): https://codereview.chromium.org/334083002/diff/100001/extensions/browser/test... extensions/browser/test_extension_registry_observer.h:9: #include "content/public/test/test_utils.h" you can include this in the .cc file and just forward declare contnet::BrowserContext if needed.
Thanks for the review https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc (right): https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:201: extensions::TestExtensionRegistryObserver observer( On 2014/06/19 23:48:10, Lei Zhang wrote: > you can omit extensions:: because you are already in the extensions namespace. Done. https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right): https://codereview.chromium.org/334083002/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_storage_monitor_browsertest.cc:337: extensions::TestExtensionRegistryObserver observer( On 2014/06/19 23:48:10, Lei Zhang wrote: > ditto, already in the extensions namespace Done. https://codereview.chromium.org/334083002/diff/100001/extensions/browser/test... File extensions/browser/test_extension_registry_observer.h (right): https://codereview.chromium.org/334083002/diff/100001/extensions/browser/test... extensions/browser/test_extension_registry_observer.h:9: #include "content/public/test/test_utils.h" On 2014/06/19 23:48:10, Lei Zhang wrote: > you can include this in the .cc file and just forward declare > contnet::BrowserContext if needed. Done.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/334083002/120001
The CQ bit was unchecked by limasdf@gmail.com
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/334083002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/334083002/140001
@Pawel, Could you take a look as the owner of content/public/test?
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
@Pawel, Could you take a look as the owner of content/public/test?
added @sky @sky, Could you kindly take a look as the owner of content/public/test?
I don't see any changes to content/public/test here. Is there something else I need to review?
Actually there's no change from content/public/test/. But there's presubmit error, presubmit require LGTM from the owner of content/public/test/. See http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...
ic, LGTM
Thank you very much for the review! and sorry to bother you.
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/334083002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 280415 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
