Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
322503006:
Remove deprecated extension notifications from state_store.*. (Closed)
|
Created:
6 years, 6 months ago by Ted Kim Modified:
6 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemove deprecated extension notifications from state_store.*.
and use ExtensionRegisty instead.
BUG=376293
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277305
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added StateStore object to observer list #
Total comments: 24
Patch Set 3 : Edit wrong indent and odering #Patch Set 4 : Move initializing extension_registry_observer_ #
Total comments: 8
Patch Set 5 : Add StateStore::OnExtensionUninstalled as reviewer's request #
Total comments: 1
Patch Set 6 : Remove empty line #
Total comments: 9
Patch Set 7 : Remove deprecated notification #
Total comments: 2
Patch Set 8 : Change class Profile position #
Messages
Total messages: 27 (0 generated)
please review this. It's first time to try commit.
Nice try. We're using Observer pattern in here. that means we should add the instance of StateStore to the observer list. And if you didn't signed Google CLA, please sign it first. https://developers.google.com/open-source/cla/individual?csw=1 https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.cc:11: #include "content/public/browser/notification_types.h" #include "extensions/browser/extension_registry.h" https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.cc:72: content::Source<Profile>(profile)); we have to add this object to the observer. extension_registry_observer_.Add(ExtensionRegistry::Get(profile)); https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:13: #include "content/public/browser/notification_observer.h" #include "base/scoped_observer.h" https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:21: class ExtensionRegistry; https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:91: }; ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> extension_registry_observer_;
added statestore object to observer list https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.cc:11: #include "content/public/browser/notification_types.h" On 2014/06/07 05:39:02, limasdf wrote: > #include "extensions/browser/extension_registry.h" Done. https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.cc:72: content::Source<Profile>(profile)); On 2014/06/07 05:39:02, limasdf wrote: > we have to add this object to the observer. > extension_registry_observer_.Add(ExtensionRegistry::Get(profile)); Done. https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:13: #include "content/public/browser/notification_observer.h" On 2014/06/07 05:39:03, limasdf wrote: > #include "base/scoped_observer.h" Done. https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:21: On 2014/06/07 05:39:03, limasdf wrote: > class ExtensionRegistry; Done. https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... chrome/browser/extensions/state_store.h:91: }; On 2014/06/07 05:39:03, limasdf wrote: > ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> > extension_registry_observer_; Done.
On 2014/06/07 08:08:22, Ted Kim wrote: > added statestore object to observer list > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > File chrome/browser/extensions/state_store.cc (right): > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > chrome/browser/extensions/state_store.cc:11: #include > "content/public/browser/notification_types.h" > On 2014/06/07 05:39:02, limasdf wrote: > > #include "extensions/browser/extension_registry.h" > > Done. > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > chrome/browser/extensions/state_store.cc:72: content::Source<Profile>(profile)); > On 2014/06/07 05:39:02, limasdf wrote: > > we have to add this object to the observer. > > extension_registry_observer_.Add(ExtensionRegistry::Get(profile)); > > Done. > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > File chrome/browser/extensions/state_store.h (right): > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > chrome/browser/extensions/state_store.h:13: #include > "content/public/browser/notification_observer.h" > On 2014/06/07 05:39:03, limasdf wrote: > > #include "base/scoped_observer.h" > > Done. > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > chrome/browser/extensions/state_store.h:21: > On 2014/06/07 05:39:03, limasdf wrote: > > class ExtensionRegistry; > > Done. > > https://codereview.chromium.org/322503006/diff/1/chrome/browser/extensions/st... > chrome/browser/extensions/state_store.h:91: }; > On 2014/06/07 05:39:03, limasdf wrote: > > ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver> > > extension_registry_observer_; > > Done. I will request review after editing one thing.
I have added statestore object to observer list. And I initialized extension_registry_observer_ instance in StateStore constructors. plz review this.
I think you saw some presubmit warning. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:12: #include "chrome/browser/profiles/profile.h" Remove. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:71: : profile_(profile), Remove. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:79: extension_registry_observer_.Add(ExtensionRegistry::Get(profile_)); You can use |profile| instead of |profile_|. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:95: : profile_(profile), Same above. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:103: extension_registry_observer_.Add(ExtensionRegistry::Get(profile_)); here too. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:170: RemoveKeysForExtension(extension->id()); Wrong indent. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:14: #include "base/scoped_observer.h" Wrong ordering. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:17: #include "extensions/browser/extension_registry_observer.h" Wrong ordering. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:96: Profile* profile_; Remove.
I edited code as your comment. It is very helpful to me like indent, ordering. But I can't remove #include chrome/browser/profiles/profile.h in state_store.cc file. If remove include profile.h, it occur compile error. I think include profile.h in state_store.cc as chrome/browser/extensions/api/storage/managed_value_store_cache.cc and chrome/browser/extensions/api/commands/command_service.cc. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:12: #include "chrome/browser/profiles/profile.h" On 2014/06/07 16:32:22, limasdf wrote: > Remove. If it is not included profile.h here, occur compile error. ExtensionRegistry::Get(profile) function in StateStore constructor can't recognize profile just declare class Profile in StateStore.h. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:71: : profile_(profile), On 2014/06/07 16:32:22, limasdf wrote: > Remove. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:95: : profile_(profile), On 2014/06/07 16:32:23, limasdf wrote: > Same above. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:103: extension_registry_observer_.Add(ExtensionRegistry::Get(profile_)); On 2014/06/07 16:32:23, limasdf wrote: > here too. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:170: RemoveKeysForExtension(extension->id()); On 2014/06/07 16:32:23, limasdf wrote: > Wrong indent. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:14: #include "base/scoped_observer.h" On 2014/06/07 16:32:23, limasdf wrote: > Wrong ordering. > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:17: #include "extensions/browser/extension_registry_observer.h" On 2014/06/07 16:32:23, limasdf wrote: > Wrong ordering. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.h:96: Profile* profile_; On 2014/06/07 16:32:23, limasdf wrote: > Remove. Done.
This code cannot be compiled. Because extension_registry_observer_ is initizlied after task_queue_. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:12: #include "chrome/browser/profiles/profile.h" On 2014/06/08 03:05:00, Ted Kim wrote: > On 2014/06/07 16:32:22, limasdf wrote: > > Remove. > > If it is not included profile.h here, occur compile error. > ExtensionRegistry::Get(profile) function in StateStore constructor can't > recognize profile just declare class Profile in StateStore.h. Right. my bad. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:73: extension_registry_observer_(this), db_path_(db_path), task_queue_(new Delay ..), extension_registry_observer_(this) { https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:98: task_queue_(new DelayedTaskQueue()) { same above
Move initializing extension_registry_observer after initializing task_queue in StateStore. Check this out plz. Is it not compiled before? I always checked compile, unit_test before git cl upload. I think initializing extension_registry_observer after initializing task_queue logic is wrong because there is some reasons. isn't it? https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:73: extension_registry_observer_(this), On 2014/06/08 05:42:34, limasdf wrote: > db_path_(db_path), > task_queue_(new Delay ..), > extension_registry_observer_(this) { Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:73: extension_registry_observer_(this), On 2014/06/08 05:42:34, limasdf wrote: > db_path_(db_path), > task_queue_(new Delay ..), > extension_registry_observer_(this) { Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:79: extension_registry_observer_.Add(ExtensionRegistry::Get(profile_)); On 2014/06/07 16:32:22, limasdf wrote: > You can use |profile| instead of |profile_|. Done. https://codereview.chromium.org/322503006/diff/40001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:98: task_queue_(new DelayedTaskQueue()) { On 2014/06/08 05:42:34, limasdf wrote: > same above Done.
Looks good, I have one more request. Because we're here, and we don't want to visit here with same issue(migration using ExtensionRegistry). https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:97: registrar_.Add(this, Could you remove this? from line#97~99 https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:144: case chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED: Could you remove from line#144~148 https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:164: Could you add this? void StateStore::OnExtensionWillBeInstalled( content::BrowserContext* browser_context, const Extension* extension, bool is_update, bool from_ephemeral, const std::string& old_name) { RemoveKeysForExtension(extension->id()); } https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.h:77: virtual void OnExtensionUninstalled(content::BrowserContext* browser_context, Could you add this? at line#77. virtual void OnExtensionWillBeInstalled( content::BrowserContext* browser_context, const Extension* extension, bool is_update, bool from_ephemeral, const std::string& old_name) OVERRIDE;
* Add OnExtensionWillBeInstalled as your request. * Your comment : I have one more request. Because we're here, and we don't want to visit here with same issue(migration using ExtensionRegistry). ==> How can I help you? https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:97: registrar_.Add(this, On 2014/06/08 06:26:35, limasdf wrote: > Could you remove this? from line#97~99 Done. https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:144: case chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED: On 2014/06/08 06:26:35, limasdf wrote: > Could you remove from line#144~148 Done. https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.cc:164: On 2014/06/08 06:26:35, limasdf wrote: > Could you add this? > > void StateStore::OnExtensionWillBeInstalled( > content::BrowserContext* browser_context, > const Extension* extension, > bool is_update, > bool from_ephemeral, > const std::string& old_name) { > RemoveKeysForExtension(extension->id()); > } Done. https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/80001/chrome/browser/extension... chrome/browser/extensions/state_store.h:77: virtual void OnExtensionUninstalled(content::BrowserContext* browser_context, On 2014/06/08 06:26:35, limasdf wrote: > Could you add this? at line#77. > > virtual void OnExtensionWillBeInstalled( > content::BrowserContext* browser_context, > const Extension* extension, > bool is_update, > bool from_ephemeral, > const std::string& old_name) OVERRIDE; Done.
Added @devlin as a reviewer. Devlin, could you take a look? @ted is my Korean friend who want to join Chromium :) https://codereview.chromium.org/322503006/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/100001/chrome/browser/extensio... chrome/browser/extensions/state_store.h:79: remove this empty line
Remove empty line
Looks pretty good overall! Thanks for the work! :) Please also update the CL title to indicate that NOTIFICATION_EXTENSION_INSTALLED is also removed (something like "Remove deprecated extension notifications from state_store.*"). Cheers! https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's handled via the RegistryObserver. https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:140: case chrome::NOTIFICATION_SESSION_RESTORE_DONE: Since both the other cases are handled via the RegistryObserver, you can simplify this method to be: void StateStore::Observe(...) { DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || type == content::NOTIFICATION_SESSION_RESTORE_DONE); registrar_.RemoveAll(); base::MessageLoop::current->PostDelayedTask(...); } https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.h:19: class Profile; Please forward-declare content::BrowserContext.
On 2014/06/09 15:17:31, Devlin wrote: > Looks pretty good overall! Thanks for the work! :) > > Please also update the CL title to indicate that > NOTIFICATION_EXTENSION_INSTALLED is also removed (something like "Remove > deprecated extension notifications from state_store.*"). > > Cheers! > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > File chrome/browser/extensions/state_store.cc (right): > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > handled via the RegistryObserver. > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > chrome/browser/extensions/state_store.cc:140: case > chrome::NOTIFICATION_SESSION_RESTORE_DONE: > Since both the other cases are handled via the RegistryObserver, you can > simplify this method to be: > > void StateStore::Observe(...) { > DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || > type == content::NOTIFICATION_SESSION_RESTORE_DONE); > registrar_.RemoveAll(); > base::MessageLoop::current->PostDelayedTask(...); > } > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > File chrome/browser/extensions/state_store.h (right): > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > chrome/browser/extensions/state_store.h:19: class Profile; > Please forward-declare content::BrowserContext. Thanks for your very detailed review. But I want check something before upload my patch. 1. CL title update => Done. 2. simplify the Observe method => Done. 3. declare content::BrowserContext => I don't understand perfectly. Is it more explicit declaring that declare content::BrowserContext before class Profile? If I don't declare content::BrowserContext, there is no problem compile. 4. Your comment : It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's handled via the RegistryObserver. => I don't understand clearly. Could I do some test suit for testing observe EXTENSION_INSTALLED?
On 2014/06/10 05:17:57, Ted Kim wrote: > On 2014/06/09 15:17:31, Devlin wrote: > > Looks pretty good overall! Thanks for the work! :) > > > > Please also update the CL title to indicate that > > NOTIFICATION_EXTENSION_INSTALLED is also removed (something like "Remove > > deprecated extension notifications from state_store.*"). > > > > Cheers! > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > File chrome/browser/extensions/state_store.cc (right): > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > > handled via the RegistryObserver. > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > chrome/browser/extensions/state_store.cc:140: case > > chrome::NOTIFICATION_SESSION_RESTORE_DONE: > > Since both the other cases are handled via the RegistryObserver, you can > > simplify this method to be: > > > > void StateStore::Observe(...) { > > DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || > > type == content::NOTIFICATION_SESSION_RESTORE_DONE); > > registrar_.RemoveAll(); > > base::MessageLoop::current->PostDelayedTask(...); > > } > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > File chrome/browser/extensions/state_store.h (right): > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > chrome/browser/extensions/state_store.h:19: class Profile; > > Please forward-declare content::BrowserContext. > > Thanks for your very detailed review. > > But I want check something before upload my patch. > 1. CL title update => Done. > 2. simplify the Observe method => Done. > 3. declare content::BrowserContext > => I don't understand perfectly. Is it more explicit declaring that declare > content::BrowserContext before class Profile? If I don't declare > content::BrowserContext, there is no problem compile. > > 4. Your comment : It looks like we shouldn't be observing EXTENSION_INSTALLED > here, since it's > handled via the RegistryObserver. > => I don't understand clearly. Could I do some test suit for testing observe > EXTENSION_INSTALLED? 3. We have a style rule in chromium to include or forward-declare what you use. Since you use a content::BrowserContext pointer in state_store.h, it should be forward-declared, like this: namespace content { class BrowserContext; } The reason there's no compile error right now is that some class included in the .h file does it (leading to a "transitive include"). This is fine, but if we were to remove that include, we would have a compile error. Now, if everyone relied on these transitive includes, if we remove a header file somewhere, it could lead to a huge number of compile errors. As such, each individual file should directly include or declare everything it uses, with the single exception of relying on includes declared in its own .h file (e.g., if it is included in state_store.h, it does not need to be included in state_store.cc, since state_store.cc will *always* include state_store.h). I hope that explanation makes sense. :) 4. The ExtensionRegistryObserver is basically replacing the deprecated EXTENSION_*_DEPRECATED notifications, but they serve the same purpose. That is, we trigger them both at the same time. Since in state_store.cc, you register for the EXTENSION_INSTALLED notification *and also* implement the method via ExtensionRegistryObserver, it will result in two calls - one to Observe() and one to OnExtensionWillBeInstalled(). What's more, since you have a NOTREACHED/DCHECK in Observe() for any notifications that aren't a load-related notification, it will crash. There's not a specific test suite for testing observe behavior, but if you run through the browser_tests for extensions (e.g., ./out/Debug/browser_tests --gtest_filter=*Extension*), I suspect you will find one that fails with your current implementation. Hope this clears things up! :)
Thanks for your comment again. 3. forward-declare : Because of your kind explanation, I totally understood forward-declare now(http://www.chromium.org/developers/coding-style/cpp-dos-and-donts). It will be good reference to me. 4. observe issue : I understood what you mean. I will try clear this if I found some good(will fail) test with my implementation. On 2014/06/10 15:53:07, Devlin wrote: > On 2014/06/10 05:17:57, Ted Kim wrote: > > On 2014/06/09 15:17:31, Devlin wrote: > > > Looks pretty good overall! Thanks for the work! :) > > > > > > Please also update the CL title to indicate that > > > NOTIFICATION_EXTENSION_INSTALLED is also removed (something like "Remove > > > deprecated extension notifications from state_store.*"). > > > > > > Cheers! > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > File chrome/browser/extensions/state_store.cc (right): > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, > > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > > > handled via the RegistryObserver. > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > chrome/browser/extensions/state_store.cc:140: case > > > chrome::NOTIFICATION_SESSION_RESTORE_DONE: > > > Since both the other cases are handled via the RegistryObserver, you can > > > simplify this method to be: > > > > > > void StateStore::Observe(...) { > > > DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || > > > type == content::NOTIFICATION_SESSION_RESTORE_DONE); > > > registrar_.RemoveAll(); > > > base::MessageLoop::current->PostDelayedTask(...); > > > } > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > File chrome/browser/extensions/state_store.h (right): > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > chrome/browser/extensions/state_store.h:19: class Profile; > > > Please forward-declare content::BrowserContext. > > > > Thanks for your very detailed review. > > > > But I want check something before upload my patch. > > 1. CL title update => Done. > > 2. simplify the Observe method => Done. > > 3. declare content::BrowserContext > > => I don't understand perfectly. Is it more explicit declaring that declare > > content::BrowserContext before class Profile? If I don't declare > > content::BrowserContext, there is no problem compile. > > > > 4. Your comment : It looks like we shouldn't be observing EXTENSION_INSTALLED > > here, since it's > > handled via the RegistryObserver. > > => I don't understand clearly. Could I do some test suit for testing > observe > > EXTENSION_INSTALLED? > > 3. We have a style rule in chromium to include or forward-declare what you use. > Since you use a content::BrowserContext pointer in state_store.h, it should be > forward-declared, like this: > > namespace content { > class BrowserContext; > } > > The reason there's no compile error right now is that some class included in the > .h file does it (leading to a "transitive include"). This is fine, but if we > were to remove that include, we would have a compile error. Now, if everyone > relied on these transitive includes, if we remove a header file somewhere, it > could lead to a huge number of compile errors. As such, each individual file > should directly include or declare everything it uses, with the single exception > of relying on includes declared in its own .h file (e.g., if it is included in > state_store.h, it does not need to be included in state_store.cc, since > state_store.cc will *always* include state_store.h). I hope that explanation > makes sense. :) > > 4. The ExtensionRegistryObserver is basically replacing the deprecated > EXTENSION_*_DEPRECATED notifications, but they serve the same purpose. That is, > we trigger them both at the same time. Since in state_store.cc, you register > for the EXTENSION_INSTALLED notification *and also* implement the method via > ExtensionRegistryObserver, it will result in two calls - one to Observe() and > one to OnExtensionWillBeInstalled(). What's more, since you have a > NOTREACHED/DCHECK in Observe() for any notifications that aren't a load-related > notification, it will crash. > > There's not a specific test suite for testing observe behavior, but if you run > through the browser_tests for extensions (e.g., ./out/Debug/browser_tests > --gtest_filter=*Extension*), I suspect you will find one that fails with your > current implementation. > > > Hope this clears things up! :)
@Ted, I hope you use inline-comment to easily catch which one you're talking about.
On 2014/06/11 00:43:59, Ted Kim wrote: > Thanks for your comment again. > 3. forward-declare : > Because of your kind explanation, I totally understood forward-declare > now(http://www.chromium.org/developers/coding-style/cpp-dos-and-donts). > It will be good reference to me. > > 4. observe issue : > I understood what you mean. I will try clear this if I found some good(will > fail) test with my implementation. > > > On 2014/06/10 15:53:07, Devlin wrote: > > On 2014/06/10 05:17:57, Ted Kim wrote: > > > On 2014/06/09 15:17:31, Devlin wrote: > > > > Looks pretty good overall! Thanks for the work! :) > > > > > > > > Please also update the CL title to indicate that > > > > NOTIFICATION_EXTENSION_INSTALLED is also removed (something like "Remove > > > > deprecated extension notifications from state_store.*"). > > > > > > > > Cheers! > > > > > > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > > File chrome/browser/extensions/state_store.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > > chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, > > > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since > it's > > > > handled via the RegistryObserver. > > > > > > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > > chrome/browser/extensions/state_store.cc:140: case > > > > chrome::NOTIFICATION_SESSION_RESTORE_DONE: > > > > Since both the other cases are handled via the RegistryObserver, you can > > > > simplify this method to be: > > > > > > > > void StateStore::Observe(...) { > > > > DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || > > > > type == content::NOTIFICATION_SESSION_RESTORE_DONE); > > > > registrar_.RemoveAll(); > > > > base::MessageLoop::current->PostDelayedTask(...); > > > > } > > > > > > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > > File chrome/browser/extensions/state_store.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... > > > > chrome/browser/extensions/state_store.h:19: class Profile; > > > > Please forward-declare content::BrowserContext. > > > > > > Thanks for your very detailed review. > > > > > > But I want check something before upload my patch. > > > 1. CL title update => Done. > > > 2. simplify the Observe method => Done. > > > 3. declare content::BrowserContext > > > => I don't understand perfectly. Is it more explicit declaring that > declare > > > content::BrowserContext before class Profile? If I don't declare > > > content::BrowserContext, there is no problem compile. > > > > > > 4. Your comment : It looks like we shouldn't be observing > EXTENSION_INSTALLED > > > here, since it's > > > handled via the RegistryObserver. > > > => I don't understand clearly. Could I do some test suit for testing > > observe > > > EXTENSION_INSTALLED? > > > > 3. We have a style rule in chromium to include or forward-declare what you > use. > > Since you use a content::BrowserContext pointer in state_store.h, it should > be > > forward-declared, like this: > > > > namespace content { > > class BrowserContext; > > } > > > > The reason there's no compile error right now is that some class included in > the > > .h file does it (leading to a "transitive include"). This is fine, but if we > > were to remove that include, we would have a compile error. Now, if everyone > > relied on these transitive includes, if we remove a header file somewhere, it > > could lead to a huge number of compile errors. As such, each individual file > > should directly include or declare everything it uses, with the single > exception > > of relying on includes declared in its own .h file (e.g., if it is included in > > state_store.h, it does not need to be included in state_store.cc, since > > state_store.cc will *always* include state_store.h). I hope that explanation > > makes sense. :) > > > > 4. The ExtensionRegistryObserver is basically replacing the deprecated > > EXTENSION_*_DEPRECATED notifications, but they serve the same purpose. That > is, > > we trigger them both at the same time. Since in state_store.cc, you register > > for the EXTENSION_INSTALLED notification *and also* implement the method via > > ExtensionRegistryObserver, it will result in two calls - one to Observe() and > > one to OnExtensionWillBeInstalled(). What's more, since you have a > > NOTREACHED/DCHECK in Observe() for any notifications that aren't a > load-related > > notification, it will crash. > > > > There's not a specific test suite for testing observe behavior, but if you run > > through the browser_tests for extensions (e.g., ./out/Debug/browser_tests > > --gtest_filter=*Extension*), I suspect you will find one that fails with your > > current implementation. > > > > > > Hope this clears things up! :) Glad it helped. Gimme a ping when the revised patch set is up. :)
review plz. https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, I can't understand your comment. How can I patch this? On 2014/06/09 15:17:31, Devlin wrote: > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > handled via the RegistryObserver. https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:140: case chrome::NOTIFICATION_SESSION_RESTORE_DONE: On 2014/06/09 15:17:31, Devlin wrote: > Since both the other cases are handled via the RegistryObserver, you can > simplify this method to be: > > void StateStore::Observe(...) { > DCHECK(type == content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME || > type == content::NOTIFICATION_SESSION_RESTORE_DONE); > registrar_.RemoveAll(); > base::MessageLoop::current->PostDelayedTask(...); > } Done. https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.h:19: class Profile; On 2014/06/09 15:17:31, Devlin wrote: > Please forward-declare content::BrowserContext. Done.
https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, On 2014/06/14 03:36:48, Ted Kim wrote: > I can't understand your comment. How can I patch this? > > On 2014/06/09 15:17:31, Devlin wrote: > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > > handled via the RegistryObserver. > Removing line#74~#76 is enough. Because |extension_registry_observer_| observes extension's installation instead. For the details, you can see extension_service.cc and extension_registry.cc https://codereview.chromium.org/322503006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/state_store.h:19: namespace content { please make 'class Profile' come first.. class Profile; namespace content { class BrowserContext; }
@limasdf : I think that I sent wrong review message. I removed 74-76 line in state_store.cc. Check patch set 7 please.
review please https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/state_store.cc (right): https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, On 2014/06/14 03:36:48, Ted Kim wrote: > I can't understand your comment. How can I patch this? > > On 2014/06/09 15:17:31, Devlin wrote: > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > > handled via the RegistryObserver. > Done. https://codereview.chromium.org/322503006/diff/120001/chrome/browser/extensio... chrome/browser/extensions/state_store.cc:74: registrar_.Add(this, On 2014/06/14 03:58:58, limasdf wrote: > On 2014/06/14 03:36:48, Ted Kim wrote: > > I can't understand your comment. How can I patch this? > > > > On 2014/06/09 15:17:31, Devlin wrote: > > > It looks like we shouldn't be observing EXTENSION_INSTALLED here, since it's > > > handled via the RegistryObserver. > > > Removing line#74~#76 is enough. > Because |extension_registry_observer_| observes extension's installation > instead. > For the details, you can see extension_service.cc and extension_registry.cc Done. https://codereview.chromium.org/322503006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/state_store.h (right): https://codereview.chromium.org/322503006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/state_store.h:19: namespace content { On 2014/06/14 03:58:59, limasdf wrote: > please make 'class Profile' come first.. > > > class Profile; > > namespace content { > class BrowserContext; > } Done.
lgtm! Thanks for the work! :)
The CQ bit was checked by neot0000@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/neot0000@gmail.com/322503006/160001
Message was sent while issue was closed.
Change committed as 277305 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
