Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11535008:
Implement mediaGalleriesPrivate api to notify extensions about gallery changed events. (Closed)
|
Created:
8 years ago by kmadhusu Modified:
7 years, 11 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement mediaGalleriesPrivate api to notify extensions about gallery changed events.
BUG=144491
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175347
Patch Set 1 : '' #
Total comments: 62
Patch Set 2 : Addressed review comments #
Total comments: 40
Patch Set 3 : Addressed review comments #
Total comments: 27
Patch Set 4 : Addressed review comments #
Total comments: 6
Patch Set 5 : Addressed review comments #
Total comments: 8
Patch Set 6 : Rebase + Addressed review comments #
Total comments: 2
Patch Set 7 : Addressed review comments #
Total comments: 6
Patch Set 8 : Addressed review comments #Patch Set 9 : Disable SetupGalleryWatch browser test on ChromeOS #Messages
Total messages: 34 (0 generated)
https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:28: // constructed, destructed and operated on the FILE thread. You can just say "this class lives on the foo thread". https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:31: typedef std::set<std::string> ExtensionIdSet; This seems to be used in 1 place inside the .cc file. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h:8: #include "base/lazy_instance.h" Classes derived from ProfileKeyedServiceFactory must be Singletons. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h:22: // Returns the MediaGalleriesPrivateAPI for |profile|, creating it if All the other Factories put this above GetInstance(). https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:22: #include "chrome/common/extensions/extension.h" Is this include needed? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:99: DCHECK(thread_checker_.CalledOnValidThread()); Why not DCHECK() OnRemovableStorageAttached() as well? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:37: if (!manager) How can this happen when HasForProfile() just returned true? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:76: content::BrowserThread::PostTask( This block looks a lot like the previous one. You can simplify this to: Extension* extension = NULL; if (type == A) { ... extension = foo; } else if (type == B) { ... extension = bar; } DCHECK(extension); PostTask(...); https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:83: NOTREACHED(); You don't need this. You can write the code as: if (type == A) { ... } else { DCHECK_EQ(B, type); ... } https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h:26: // content::NotificationDelegate implementation. You meant to say NotificationObserver. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h:38: base::ThreadChecker thread_checker_; I think you are going overboard with ThreadCheckers. a) NotificationObserver::Observe() isn't going to get called on any other thread. If it does get called on the wrong thread, then Chrome will likely crash in other horrible ways before it gets to your code. BUt let's say you really wanted to check this. You still don't need a ThreadChecker. You can be more specific and just check to see if you are on the UI thread. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/event_names.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/event_names.h:108: nit: extra newline? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/med... File chrome/browser/media_gallery/media_galleries_test_util.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/med... chrome/browser/media_gallery/media_galleries_test_util.h:29: bool AddNewFileInGallery(int gallery_directory_key); This shouldn't be part of EnsureMediaDirectoriesExists. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/common/exte... File chrome/common/extensions/api/media_galleries_private.idl (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/common/exte... chrome/common/extensions/api/media_galleries_private.idl:38: static void onGalleryChanged(ModifiedGalleryDetails details); GalleryChangeDetails to be consistent with DeviceAttachmentDetails.
https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:185: bool MediaGalleryWatchFunctionBase::RunImpl() { You shouldn't have to write this function twice. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:210: content::BrowserThread::PostTask( Can you PostTaskAndReplyWithResult() instead? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:227: scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue()); Look for an AddGalleryWatchResult class in out/Debug/gen/chrome/common/extensions/api/media_galleries_private.h You should fill in the class members and call fooResult.ToValue().release(); https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:89: class MediaGalleriesPrivateRemoveGalleryWatchFunction I don't think the two Function classes should share a base class. Unlike the AddGalleryWatch, RemoveGalleryWatch is synchronous. The only thing MediaGalleriesPrivateRemoveGalleryWatchFunction::HandleResponse() does is return true, and you can just do that in RunImpl(). https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/common/exte... File chrome/common/extensions/api/media_galleries_private.idl (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/common/exte... chrome/common/extensions/api/media_galleries_private.idl:48: void (AddGalleryWatchResult result); nit: fits on the previous line.
https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:69: WatcherMap::const_iterator iter = gallery_watchers_.find(watch_path); It's easier to understand if you write the code as: if (iter != gallery_watchers_.end()) { // Already watched iter->second->AddExtension(extension_id); return true; } // Need to add new watcher scoped_ptr<GalleryFilePathWatcher> watch( new GalleryFilePathWatcher(profile_, gallery_id, watch_path, extension_id)); if (!watch->SetupWatch()) return false; gallery_watchers_[watch_path] = watch.release(); return true; https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:53: typedef std::map<std::string, int> ExtensionUsageRegistry; This map is too wimpy to a registry. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:58: // Gallery file path watcher delegate to handle the gallery change You should mention this class does recursive watches. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:63: class GalleryFilePathWatcher { Can this go in the .cc file instead? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:100: scoped_ptr<base::files::FilePathWatcher> file_watcher_; Does this need to be a scoped_ptr? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:102: // The gallery file path, e.g "C:/My Pictures". nit: C:\, not C:/ https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:108: ExtensionUsageRegistry extensions_; |extensions_watch_count_map_| ? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:111: unsigned int ref_count_; Do you have to manually refcount? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:114: base::Time last_gallery_changed_event_; This should be per-extension. Extension AA has a watch for /foo, and an event happens at T=0. If extension BB adds a watch for /foo at T=1, and an event happens at T=2, BB will never be notified. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.cc:16: base::LazyInstance<GalleryWatchManagerFactory>::Leaky Why does this have to leak? https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.h:21: class GalleryWatchManagerFactory { Can you just replace this with a map in gallery_watch_manager.cc's anonymous namespace and expose the Get/Has function via static methods? Then you can make the GalleryWatchManager's ctor private, since this is really the intended way to create a GalleryWatchManager. https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://chromiumcodereview.appspot.com/11535008/diff/23001/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:45: scoped_ptr<MediaGalleryExtensionNotificationObserver> Why both with a separate observer? Composition isn't always the right solution. There's 300+ classes that all inherit from content::NotificationObserver. Very few of them create a separate observer class.
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:69: WatcherMap::const_iterator iter = gallery_watchers_.find(watch_path); On 2012/12/15 04:13:21, Lei Zhang wrote: > It's easier to understand if you write the code as: > > if (iter != gallery_watchers_.end()) { > // Already watched > iter->second->AddExtension(extension_id); > return true; > } > > // Need to add new watcher > scoped_ptr<GalleryFilePathWatcher> watch( > new GalleryFilePathWatcher(profile_, gallery_id, watch_path, extension_id)); > if (!watch->SetupWatch()) > return false; > gallery_watchers_[watch_path] = watch.release(); > return true; Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:28: // constructed, destructed and operated on the FILE thread. On 2012/12/15 01:11:54, Lei Zhang wrote: > You can just say "this class lives on the foo thread". Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:31: typedef std::set<std::string> ExtensionIdSet; On 2012/12/15 01:11:54, Lei Zhang wrote: > This seems to be used in 1 place inside the .cc file. Removed typedef. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:53: typedef std::map<std::string, int> ExtensionUsageRegistry; On 2012/12/15 04:13:21, Lei Zhang wrote: > This map is too wimpy to a registry. ExtensionUsageRegistry => ExtensionWatchCountMap https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:58: // Gallery file path watcher delegate to handle the gallery change On 2012/12/15 04:13:21, Lei Zhang wrote: > You should mention this class does recursive watches. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:63: class GalleryFilePathWatcher { On 2012/12/15 04:13:21, Lei Zhang wrote: > Can this go in the .cc file instead? Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:100: scoped_ptr<base::files::FilePathWatcher> file_watcher_; On 2012/12/15 04:13:21, Lei Zhang wrote: > Does this need to be a scoped_ptr? Not needed. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:102: // The gallery file path, e.g "C:/My Pictures". On 2012/12/15 04:13:21, Lei Zhang wrote: > nit: C:\, not C:/ Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:108: ExtensionUsageRegistry extensions_; On 2012/12/15 04:13:21, Lei Zhang wrote: > |extensions_watch_count_map_| ? Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:111: unsigned int ref_count_; On 2012/12/15 04:13:21, Lei Zhang wrote: > Do you have to manually refcount? Fixed. GalleryFilePathWatch inherits from RefCounted class. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:114: base::Time last_gallery_changed_event_; On 2012/12/15 04:13:21, Lei Zhang wrote: > This should be per-extension. > > Extension AA has a watch for /foo, and an event happens at T=0. If extension BB > adds a watch for /foo at T=1, and an event happens at T=2, BB will never be > notified. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.cc (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.cc:16: base::LazyInstance<GalleryWatchManagerFactory>::Leaky On 2012/12/15 04:13:21, Lei Zhang wrote: > Why does this have to leak? This class is operated on the file thread. The destructor of the lazy instance is always called on the UI thread. In future, someone may try to do non-threadsafe operations in the destructor. In order to avoid all these confusions, I made this as a leaky lazy instance. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager_factory.h:21: class GalleryWatchManagerFactory { On 2012/12/15 04:13:21, Lei Zhang wrote: > Can you just replace this with a map in gallery_watch_manager.cc's anonymous > namespace and expose the Get/Has function via static methods? Then you can make > the GalleryWatchManager's ctor private, since this is really the intended way to > create a GalleryWatchManager. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:185: bool MediaGalleryWatchFunctionBase::RunImpl() { On 2012/12/15 02:00:29, Lei Zhang wrote: > You shouldn't have to write this function twice. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:210: content::BrowserThread::PostTask( On 2012/12/15 02:00:29, Lei Zhang wrote: > Can you PostTaskAndReplyWithResult() instead? Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:227: scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue()); On 2012/12/15 02:00:29, Lei Zhang wrote: > Look for an AddGalleryWatchResult class in > out/Debug/gen/chrome/common/extensions/api/media_galleries_private.h > > You should fill in the class members and call fooResult.ToValue().release(); Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:45: scoped_ptr<MediaGalleryExtensionNotificationObserver> On 2012/12/15 04:13:21, Lei Zhang wrote: > Why both with a separate observer? Composition isn't always the right solution. > There's 300+ classes that all inherit from content::NotificationObserver. Very > few of them create a separate observer class. GalleryWatchManager needs to perform some clean up operations when an extension is destroyed. GalleryWatchManager lives on the file thread. NotificationObserver::Observe function is called on the UI thread. If GalleryWatchManager inherits from NotificationObserver, we are introducing a function in GalleryWatchManager which gets called on the UI thread. I have to either use a lock or weak pointer to do the clean up operations. Inheritance solution makes this complex. With composition, it is simple and easier to understand and manage the objects. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:89: class MediaGalleriesPrivateRemoveGalleryWatchFunction On 2012/12/15 02:00:29, Lei Zhang wrote: > I don't think the two Function classes should share a base class. Unlike the > AddGalleryWatch, RemoveGalleryWatch is synchronous. The only thing > MediaGalleriesPrivateRemoveGalleryWatchFunction::HandleResponse() does is return > true, and you can just do that in RunImpl(). Good catch. Fixed. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h:8: #include "base/lazy_instance.h" On 2012/12/15 01:11:54, Lei Zhang wrote: > Classes derived from ProfileKeyedServiceFactory must be Singletons. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api_factory.h:22: // Returns the MediaGalleriesPrivateAPI for |profile|, creating it if On 2012/12/15 01:11:54, Lei Zhang wrote: > All the other Factories put this above GetInstance(). Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:22: #include "chrome/common/extensions/extension.h" On 2012/12/15 01:11:54, Lei Zhang wrote: > Is this include needed? Not required. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:99: DCHECK(thread_checker_.CalledOnValidThread()); On 2012/12/15 01:11:54, Lei Zhang wrote: > Why not DCHECK() OnRemovableStorageAttached() as well? Fixed. Forgot to add the DCHECK in that function. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:37: if (!manager) On 2012/12/15 01:11:54, Lei Zhang wrote: > How can this happen when HasForProfile() just returned true? You are right. |manager| can never be null here. Fixed. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:76: content::BrowserThread::PostTask( On 2012/12/15 01:11:54, Lei Zhang wrote: > This block looks a lot like the previous one. You can simplify this to: > > Extension* extension = NULL; > if (type == A) { > ... > extension = foo; > } else if (type == B) { > ... > extension = bar; > } > DCHECK(extension); > PostTask(...); Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.cc:83: NOTREACHED(); On 2012/12/15 01:11:54, Lei Zhang wrote: > You don't need this. You can write the code as: > if (type == A) { > ... > } else { > DCHECK_EQ(B, type); > ... > } Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h:26: // content::NotificationDelegate implementation. On 2012/12/15 01:11:54, Lei Zhang wrote: > You meant to say NotificationObserver. oops. Fixed. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_gallery_extension_notification_observer.h:38: base::ThreadChecker thread_checker_; On 2012/12/15 01:11:54, Lei Zhang wrote: > I think you are going overboard with ThreadCheckers. a) > NotificationObserver::Observe() isn't going to get called on any other thread. > If it does get called on the wrong thread, then Chrome will likely crash in > other horrible ways before it gets to your code. BUt let's say you really wanted > to check this. You still don't need a ThreadChecker. You can be more specific > and just check to see if you are on the UI thread. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... File chrome/browser/extensions/event_names.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/extensions... chrome/browser/extensions/event_names.h:108: On 2012/12/15 01:11:54, Lei Zhang wrote: > nit: extra newline? Removed. https://codereview.chromium.org/11535008/diff/23001/chrome/browser/media_gall... File chrome/browser/media_gallery/media_galleries_test_util.h (right): https://codereview.chromium.org/11535008/diff/23001/chrome/browser/media_gall... chrome/browser/media_gallery/media_galleries_test_util.h:29: bool AddNewFileInGallery(int gallery_directory_key); On 2012/12/15 01:11:54, Lei Zhang wrote: > This shouldn't be part of EnsureMediaDirectoriesExists. Removed. Added this function to the media_galleries_watch_apitest.cc https://codereview.chromium.org/11535008/diff/23001/chrome/common/extensions/... File chrome/common/extensions/api/media_galleries_private.idl (right): https://codereview.chromium.org/11535008/diff/23001/chrome/common/extensions/... chrome/common/extensions/api/media_galleries_private.idl:38: static void onGalleryChanged(ModifiedGalleryDetails details); On 2012/12/15 01:11:54, Lei Zhang wrote: > GalleryChangeDetails to be consistent with DeviceAttachmentDetails. Done. https://codereview.chromium.org/11535008/diff/23001/chrome/common/extensions/... chrome/common/extensions/api/media_galleries_private.idl:48: void (AddGalleryWatchResult result); On 2012/12/15 02:00:29, Lei Zhang wrote: > nit: fits on the previous line. Done.
https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:56: void OnExtensionDestroyed(const std::string& extension_id); If you also make this a static function like OnProfileShutdown(), then you won't need HandleExtensionDestroyedOnFileThread(). https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:40: registry->GetPreferences(const_cast<Profile*>(profile)); Why do you take a Profile*, pass it in as a const Profile*, and then cast away the const? https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:58: GalleryWatchManager* GetGalleryWatchManagerOnFileThread(const Profile* profile, Is this really needed? Add can just do: GalleryWatchManager* manager = GalleryWatchManager::GetForProfile(profile); and Remove can just do: if (GalleryWatchManager::HasForProfile(profile))) GalleryWatchManager::GetForProfile(profile)->StopGalleryWatch(...) https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:214: if (!args_->GetString(0, &gallery_id) || (gallery_id.empty())) Again, look in out/Debug/gen/chrome/common/extensions/api/media_galleries_private.h for the auto-generated class to use. This could should start out as: scoped_ptr<Foo::Params> params(Foo::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); Same with the add watcher case above. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:62: virtual ~MediaGalleriesPrivateAddGalleryWatchFunction() {} Put the implementation in the .cc file. Same for remove. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:68: void HandleResponse(const std::string& gallery_id, This can be private I think. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( Why can't you use MediaGalleriesPrivateEventRouter::DispatchEvent()? https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:51: const char kGalleryChangedEventReceived[] = "gallery_changed_event_recevied"; typo https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:81: bool AddNewFileInGallery(int gallery_directory_key) { Calls to this should ASSERT_TRUE(). https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:88: PathService::Get(gallery_directory_key, &gallery_dir); Check return value here. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:297: #if !defined(OS_WIN) Isn't lines 295-297 just #else ? https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/common/exte... File chrome/common/extensions/api/media_galleries_private.idl (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/common/exte... chrome/common/extensions/api/media_galleries_private.idl:27: DOMString galleryId; Can't you make galleryId a long in the IDL file here to avoid both int to string conversions in JS and string to int conversions in C++?
https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:49: const_cast<Profile*>(profile))->event_router(); GalleryWatchManager shouldn't promise to use a const Profile* and then cast away its const-ness. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:61: // Gallery file path watcher delegate to handle the gallery change From this comment, I still have trouble understanding what the exact responsibility of this class is. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:74: // ExtensionWatchInfoMap and initiate the watch operation. What does "initiate the watch operation" mean? How does it relate to "sets up the watch operation" below? https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:95: struct ExtensionWatchInfo { Are you keeping track of how many times a particular extension tries to add a particular gallery watch? i.e. if extension foo calls AddGalleryWatch for /bar twice, then it takes 2 remove AddGalleryWatch calls to stop watching /bar. You shouldn't have to do this. If extension foo calls AddGallery() twice for a particular gallery, the second call is a no-op. Whether a given extension is watching a given gallery should be an on/off state. https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:210: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); Structs should not do anything other can carry value. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://chromiumcodereview.appspot.com/11535008/diff/27002/chrome/browser/ext... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:25: // The profile-keyed service that manages the gallery watchers. This class profile-keyed services inherit from ProfileKeyedService.
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:49: const_cast<Profile*>(profile))->event_router(); On 2012/12/18 01:45:16, Lei Zhang wrote: > GalleryWatchManager shouldn't promise to use a const Profile* and then cast away > its const-ness. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:61: // Gallery file path watcher delegate to handle the gallery change On 2012/12/18 01:45:16, Lei Zhang wrote: > From this comment, I still have trouble understanding what the exact > responsibility of this class is. How about this? // This class does a recursive watch on the gallery file path and sends notifications to the // extensions about the gallery changed event. This class lives on the file thread. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:74: // ExtensionWatchInfoMap and initiate the watch operation. On 2012/12/18 01:45:16, Lei Zhang wrote: > What does "initiate the watch operation" mean? How does it relate to "sets up > the watch operation" below? This function adds an extension reference to the watched gallery. Fixed the comment. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:95: struct ExtensionWatchInfo { On 2012/12/18 01:45:16, Lei Zhang wrote: > Are you keeping track of how many times a particular extension tries to add a > particular gallery watch? Yes > i.e. if extension foo calls AddGalleryWatch for /bar > twice, then it takes 2 remove AddGalleryWatch calls to stop watching /bar. > > You shouldn't have to do this. If extension foo calls AddGallery() twice for a > particular gallery, the second call is a no-op. Whether a given extension is > watching a given gallery should be an on/off state. When we have multiple instances of the same extension, we need to count the total number of references. (FYI) All the instances have the same extension id. We receive the extension destroyed event only when the last instance of the extension is destroyed. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:210: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2012/12/18 01:45:16, Lei Zhang wrote: > Structs should not do anything other can carry value. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:25: // The profile-keyed service that manages the gallery watchers. This class On 2012/12/18 01:45:16, Lei Zhang wrote: > profile-keyed services inherit from ProfileKeyedService. Removed the class header comments and added a couple of lines in file header comments. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:56: void OnExtensionDestroyed(const std::string& extension_id); On 2012/12/18 00:47:01, Lei Zhang wrote: > If you also make this a static function like OnProfileShutdown(), then you won't > need HandleExtensionDestroyedOnFileThread(). OnProfileShutdown() implementation modifies the global variable where as OnExtensionDestroyed() modifies the non-static class member variable. Therefore, it cannot be a static function of this class. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:40: registry->GetPreferences(const_cast<Profile*>(profile)); On 2012/12/18 00:47:01, Lei Zhang wrote: > Why do you take a Profile*, pass it in as a const Profile*, and then cast away > the const? Fixed. Passing Profile* as param. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:58: GalleryWatchManager* GetGalleryWatchManagerOnFileThread(const Profile* profile, On 2012/12/18 00:47:01, Lei Zhang wrote: > Is this really needed? > Removed. > Add can just do: > GalleryWatchManager* manager = GalleryWatchManager::GetForProfile(profile); > > and Remove can just do: > if (GalleryWatchManager::HasForProfile(profile))) > GalleryWatchManager::GetForProfile(profile)->StopGalleryWatch(...) Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:214: if (!args_->GetString(0, &gallery_id) || (gallery_id.empty())) On 2012/12/18 00:47:01, Lei Zhang wrote: > Again, look in > out/Debug/gen/chrome/common/extensions/api/media_galleries_private.h for the > auto-generated class to use. This could should start out as: > > scoped_ptr<Foo::Params> params(Foo::Params::Create(*args_)); > EXTENSION_FUNCTION_VALIDATE(params.get()); > > Same with the add watcher case above. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:62: virtual ~MediaGalleriesPrivateAddGalleryWatchFunction() {} On 2012/12/18 00:47:01, Lei Zhang wrote: > Put the implementation in the .cc file. Same for remove. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:68: void HandleResponse(const std::string& gallery_id, On 2012/12/18 00:47:01, Lei Zhang wrote: > This can be private I think. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( On 2012/12/18 00:47:01, Lei Zhang wrote: > Why can't you use MediaGalleriesPrivateEventRouter::DispatchEvent()? DispatchEvent uses BroadcastEvent(). BroadcastEvent() sends the notifications to all the listeners of this event. Let us assume we have an extension(ext_A) that have added a listener for the OnGalleryChanged() event. When a gallery change event happens, BroadcastEvent() dispatches the event to the ext_A even though it has not called addGalleryWatch() function. Therefore, we should call DispatchEventToExtension() function to send the events instead of BroadcastEvent(). https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:51: const char kGalleryChangedEventReceived[] = "gallery_changed_event_recevied"; On 2012/12/18 00:47:01, Lei Zhang wrote: > typo Fixed. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:81: bool AddNewFileInGallery(int gallery_directory_key) { On 2012/12/18 00:47:01, Lei Zhang wrote: > Calls to this should ASSERT_TRUE(). Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:88: PathService::Get(gallery_directory_key, &gallery_dir); On 2012/12/18 00:47:01, Lei Zhang wrote: > Check return value here. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_watch_apitest.cc:297: #if !defined(OS_WIN) On 2012/12/18 00:47:01, Lei Zhang wrote: > Isn't lines 295-297 just #else ? During some other review, reviewer said I should prefer #if..#endif test blocks instead of #if...#else..#endif blocks. https://codereview.chromium.org/11535008/diff/27002/chrome/common/extensions/... File chrome/common/extensions/api/media_galleries_private.idl (right): https://codereview.chromium.org/11535008/diff/27002/chrome/common/extensions/... chrome/common/extensions/api/media_galleries_private.idl:27: DOMString galleryId; On 2012/12/18 00:47:01, Lei Zhang wrote: > Can't you make galleryId a long in the IDL file here to avoid both int to string > conversions in JS and string to int conversions in C++? Done.
https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:61: // Gallery file path watcher delegate to handle the gallery change On 2012/12/18 21:32:39, kmadhusu wrote: > On 2012/12/18 01:45:16, Lei Zhang wrote: > > From this comment, I still have trouble understanding what the exact > > responsibility of this class is. > > How about this? > // This class does a recursive watch on the gallery file path and sends > notifications to the > // extensions about the gallery changed event. This class lives on the file > thread. Your comment still does not answer the question of who are "the extensions"? How about: This class does a recursive watch on a gallery file path and holds a list of extensions that are watching the gallery. When there is file system activity within the gallery, GalleryFilePathWatcher notifies the interested extensions. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( On 2012/12/18 21:32:39, kmadhusu wrote: > On 2012/12/18 00:47:01, Lei Zhang wrote: > > Why can't you use MediaGalleriesPrivateEventRouter::DispatchEvent()? > > DispatchEvent uses BroadcastEvent(). BroadcastEvent() sends the notifications to > all the listeners of this event. Let us assume we have an extension(ext_A) that > have added a listener for the OnGalleryChanged() event. When a gallery change > event happens, BroadcastEvent() dispatches the event to the ext_A even though it > has not called addGalleryWatch() function. Therefore, we should call > DispatchEventToExtension() function to send the events instead of > BroadcastEvent(). This warrants a comment so future readers understands the behavior. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:98: base::Time last_gallery_changed_event; So if you can have multiple instances of an extension, then doesn't each instance need its own |last_gallery_changed_event| ? You may want to replace ExtensionWatchInfoMap with a std::multi_map<std::string, base::Time>. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:298: if (!gallery_watchers_.empty()) { Put this in DeleteAllWatchers(). https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:299: // User closed the extension/browser without stoping the gallery watchers. I don't find this comment useful. No user will ever think about removing gallery watchers before closing their browser. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:351: if (iter->second->HasOneRef()) { HasOneRef() is rarely used in our code base. I think this is just more manual refcounting. WatcherMap probably should have a raw pointer and the watchers it hold can have a callback to GalleryWatchManager when they get destroyed. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:7: // is temporary and will be moved to a permanent, public place in the near You should reference a bug for this. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:18: #include "base/time.h" nit: move to .cc file https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:24: class GalleryFilePathWatcher; Why did this get moved out of GalleryWatchManager? Nobody else uses GalleryFilePathWatcher. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:39: ~GalleryWatchManager(); private? https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:50: // |extension_id|. |watch_path| specifies the gallery absolute path. nit: gallery absolute path -> absolute path of the gallery https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:161: HandleResponse(gallery_pref_id, false); It looks like many other async extension APIs will just set |error_| and return false. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:56: uint64 gallery_id, This is actually MediaGalleryPrefId. Ditto elsewhere. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( You probably want to set event->restrict_to_profile.
https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:98: base::Time last_gallery_changed_event; On 2012/12/19 01:03:47, Lei Zhang wrote: > So if you can have multiple instances of an extension, then doesn't each > instance need its own |last_gallery_changed_event| ? > > You may want to replace ExtensionWatchInfoMap with a std::multi_map<std::string, > base::Time>. Come to think of it, this is a very weird case. Even with a multimap, you know there's some instance of the extension that needs the event notification, but there's no way to send the event to only that instance.
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:61: // Gallery file path watcher delegate to handle the gallery change On 2012/12/19 01:03:47, Lei Zhang wrote: > On 2012/12/18 21:32:39, kmadhusu wrote: > > On 2012/12/18 01:45:16, Lei Zhang wrote: > > > From this comment, I still have trouble understanding what the exact > > > responsibility of this class is. > > > > How about this? > > // This class does a recursive watch on the gallery file path and sends > > notifications to the > > // extensions about the gallery changed event. This class lives on the file > > thread. > > Your comment still does not answer the question of who are "the extensions"? How > about: > > This class does a recursive watch on a gallery file path and holds a list of > extensions that are watching the gallery. When there is file system activity > within the gallery, GalleryFilePathWatcher notifies the interested extensions. sure. Done. https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/27002/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( On 2012/12/19 01:03:47, Lei Zhang wrote: > On 2012/12/18 21:32:39, kmadhusu wrote: > > On 2012/12/18 00:47:01, Lei Zhang wrote: > > > Why can't you use MediaGalleriesPrivateEventRouter::DispatchEvent()? > > > > DispatchEvent uses BroadcastEvent(). BroadcastEvent() sends the notifications > to > > all the listeners of this event. Let us assume we have an extension(ext_A) > that > > have added a listener for the OnGalleryChanged() event. When a gallery change > > event happens, BroadcastEvent() dispatches the event to the ext_A even though > it > > has not called addGalleryWatch() function. Therefore, we should call > > DispatchEventToExtension() function to send the events instead of > > BroadcastEvent(). > > This warrants a comment so future readers understands the behavior. Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:298: if (!gallery_watchers_.empty()) { On 2012/12/19 01:03:47, Lei Zhang wrote: > Put this in DeleteAllWatchers(). Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:299: // User closed the extension/browser without stoping the gallery watchers. On 2012/12/19 01:03:47, Lei Zhang wrote: > I don't find this comment useful. No user will ever think about removing gallery > watchers before closing their browser. Removed. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:351: if (iter->second->HasOneRef()) { On 2012/12/19 01:03:47, Lei Zhang wrote: > HasOneRef() is rarely used in our code base. I think this is just more manual > refcounting. WatcherMap probably should have a raw pointer and the watchers it > hold can have a callback to GalleryWatchManager when they get destroyed. Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:7: // is temporary and will be moved to a permanent, public place in the near On 2012/12/19 01:03:47, Lei Zhang wrote: > You should reference a bug for this. Is it okay to specify the metabug crbug.com/144491 or do you want me to file a separate bug to move all the mediaGalleriesPrivate.* api to mediaGalleries.* api? https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:18: #include "base/time.h" On 2012/12/19 01:03:47, Lei Zhang wrote: > nit: move to .cc file Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:24: class GalleryFilePathWatcher; On 2012/12/19 01:03:47, Lei Zhang wrote: > Why did this get moved out of GalleryWatchManager? Nobody else uses > GalleryFilePathWatcher. Fixed. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:39: ~GalleryWatchManager(); On 2012/12/19 01:03:47, Lei Zhang wrote: > private? Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:50: // |extension_id|. |watch_path| specifies the gallery absolute path. On 2012/12/19 01:03:47, Lei Zhang wrote: > nit: gallery absolute path -> absolute path of the gallery Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:161: HandleResponse(gallery_pref_id, false); On 2012/12/19 01:03:47, Lei Zhang wrote: > It looks like many other async extension APIs will just set |error_| and return > false. Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:56: uint64 gallery_id, On 2012/12/19 01:03:47, Lei Zhang wrote: > This is actually MediaGalleryPrefId. Ditto elsewhere. Done. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:70: scoped_ptr<extensions::Event> event(new extensions::Event( On 2012/12/19 01:03:47, Lei Zhang wrote: > You probably want to set event->restrict_to_profile. Done.
Any thoughts on multi_map vs counting? https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:7: // is temporary and will be moved to a permanent, public place in the near On 2012/12/19 21:55:55, kmadhusu wrote: > On 2012/12/19 01:03:47, Lei Zhang wrote: > > You should reference a bug for this. > > Is it okay to specify the metabug crbug.com/144491 or do you want me to file a > separate bug to move all the mediaGalleriesPrivate.* api to mediaGalleries.* > api? Let's file a new bug. Otherwise, some people will just look at the status and think this isn't done. https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:131: base::Closure no_references_callback_; how about |on_destroyed_callback_| ? https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:389: WatcherMap::iterator iter = gallery_watchers_.find(watch_path); If you are not going to DCHECK that |watch_path| is in |gallery_watchers_|, then just do gallery_watchers_.erase(watch_path); https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:36: const char kInvalidGalleryIDError[] = "Specified invalid gallery identifier"; Just say "Invalid gallery ID"
https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:98: base::Time last_gallery_changed_event; On 2012/12/19 01:13:25, Lei Zhang wrote: > On 2012/12/19 01:03:47, Lei Zhang wrote: > > So if you can have multiple instances of an extension, then doesn't each > > instance need its own |last_gallery_changed_event| ? > > > > You may want to replace ExtensionWatchInfoMap with a > std::multi_map<std::string, > > base::Time>. > > Come to think of it, this is a very weird case. Even with a multimap, you know > there's some instance of the extension that needs the event notification, but > there's no way to send the event to only that instance. The chances of hitting this use case is very less. Lets use the counting method now. If we find more corner cases, we will use a multi-map or find some other way to distinguish multiple instances of the same extension. https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/17049/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:7: // is temporary and will be moved to a permanent, public place in the near On 2012/12/19 22:35:50, Lei Zhang wrote: > On 2012/12/19 21:55:55, kmadhusu wrote: > > On 2012/12/19 01:03:47, Lei Zhang wrote: > > > You should reference a bug for this. > > > > Is it okay to specify the metabug crbug.com/144491 or do you want me to file a > > separate bug to move all the mediaGalleriesPrivate.* api to mediaGalleries.* > > api? > > Let's file a new bug. Otherwise, some people will just look at the status and > think this isn't done. Done. https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:131: base::Closure no_references_callback_; On 2012/12/19 22:35:50, Lei Zhang wrote: > how about |on_destroyed_callback_| ? Done. https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:389: WatcherMap::iterator iter = gallery_watchers_.find(watch_path); On 2012/12/19 22:35:50, Lei Zhang wrote: > If you are not going to DCHECK that |watch_path| is in |gallery_watchers_|, then > just do gallery_watchers_.erase(watch_path); Done. https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/58007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:36: const char kInvalidGalleryIDError[] = "Specified invalid gallery identifier"; On 2012/12/19 22:35:50, Lei Zhang wrote: > Just say "Invalid gallery ID" Done.
+mpcomplete as a reviewer from the extensions team. We talked in person about persisting watches for background event pages between restarts. Can we defer that to a later CL?
On 2012/12/19 23:17:42, Lei Zhang wrote: > +mpcomplete as a reviewer from the extensions team. > > We talked in person about persisting watches for background event pages between > restarts. Can we defer that to a later CL? Yes. We can do that in a followup CL.
mpcomplete@: ping
Quick comment for you to digest while I finish the rest of the review. https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:48: MediaGalleriesPrivateAPIFactory::GetForProfile(profile)->event_router(); This is not safe. |profile| could have been deleted since posting the task to the UI thread. The source of the problem is passing Profile pointers around on the File thread. Profile can't be accessed on the File thread, so having pointers is just asking for trouble. (If you need a Profile identifier, I recommend just using a void* to clarify that it shouldn't be dereferenced.) Additionally, rather than giving the GalleryWatchManager a Profile, you probably want to give it a WeakPtr to the MediaGalleriesPrivateEventRouter. That way, you know when the event router is deleted.
https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:100: unsigned int watch_count; Don't use unsigned here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:168: ExtensionWatchInfoMap::value_type(extension_id, ExtensionWatchInfo())); nit: change ExtensionWatchInfo's constructor to start watch_count at 0 and you can change the whole if/else block to: extension_watch_info_map_[extension_id].watch_count++; https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:73: event->restrict_to_profile = profile_; Is there a separate MediaGalleriesPrivateEventRouter created in incognito mode? If not, you might not want this line here. If an extension uses "split" incognito mode, it has 2 processes running: one for the regular profile, one for incognito. The incognito one won't see the events for the regular one (and vice versa), if you set restrict_to_profile.
mpcomplete@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:48: MediaGalleriesPrivateAPIFactory::GetForProfile(profile)->event_router(); On 2013/01/03 21:53:41, Matt Perry wrote: > This is not safe. |profile| could have been deleted since posting the task to > the UI thread. > > The source of the problem is passing Profile pointers around on the File thread. > Profile can't be accessed on the File thread, so having pointers is just asking > for trouble. (If you need a Profile identifier, I recommend just using a void* > to clarify that it shouldn't be dereferenced.) > > Additionally, rather than giving the GalleryWatchManager a Profile, you probably > want to give it a WeakPtr to the MediaGalleriesPrivateEventRouter. That way, you > know when the event router is deleted. Fixed. MediaGalleriesPrivateEventRouter supports weak pointers. GalleryFilePathWatcher has a weak pointer reference to MediaGalleriesPrivateEventRouter. GalleryWatchManager uses "void* profile_id" instead of "Profile* profile". https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:100: unsigned int watch_count; On 2013/01/03 23:20:34, Matt Perry wrote: > Don't use unsigned here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... Changed to int. https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:168: ExtensionWatchInfoMap::value_type(extension_id, ExtensionWatchInfo())); On 2013/01/03 23:20:34, Matt Perry wrote: > nit: change ExtensionWatchInfo's constructor to start watch_count at 0 and you > can change the whole if/else block to: > extension_watch_info_map_[extension_id].watch_count++; Done. https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc (right): https://codereview.chromium.org/11535008/diff/57007/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_event_router.cc:73: event->restrict_to_profile = profile_; On 2013/01/03 23:20:34, Matt Perry wrote: > Is there a separate MediaGalleriesPrivateEventRouter created in incognito mode? > If not, you might not want this line here. If an extension uses "split" > incognito mode, it has 2 processes running: one for the regular profile, one for > incognito. The incognito one won't see the events for the regular one (and vice > versa), if you set restrict_to_profile. Currently, we are not creating a separate MediaGalleriesPrivateEventRouter for Incognito mode. Therefore, I removed this statement.
lgtm https://codereview.chromium.org/11535008/diff/71003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/71003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:154: ExtensionWatchInfoMap::iterator it = lines 154-159 are unnecessary. map[key] automatically inserts a default-constructed object if it doesn't already exist.
https://codereview.chromium.org/11535008/diff/71003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc (right): https://codereview.chromium.org/11535008/diff/71003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.cc:154: ExtensionWatchInfoMap::iterator it = On 2013/01/04 18:02:38, Matt Perry wrote: > lines 154-159 are unnecessary. map[key] automatically inserts a > default-constructed object if it doesn't already exist. Done.
https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:60: explicit GalleryWatchManager(); nit: no need for explicit. https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:134: event_router(); This looks a bit weird. Can you add a comment to explain why you do this? https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:38: MediaGalleriesPrivateEventRouter* event_router(); nit: event_router -> GetEventRouter()
https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/gallery_watch_manager.h:60: explicit GalleryWatchManager(); On 2013/01/04 22:56:28, Lei Zhang wrote: > nit: no need for explicit. Done. https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:134: event_router(); On 2013/01/04 22:56:28, Lei Zhang wrote: > This looks a bit weird. Can you add a comment to explain why you do this? Added a comment and a private function "MaybeInitializeEventRouter". https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h (right): https://codereview.chromium.org/11535008/diff/73003/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.h:38: MediaGalleriesPrivateEventRouter* event_router(); On 2013/01/04 22:56:28, Lei Zhang wrote: > nit: event_router -> GetEventRouter() Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11535008/96001
thestig@: MediaGalleriesPrivateGalleryWatchApiTest.SetupGalleryWatch fails on ChromeOS because there are no default galleries. Therefore, made changes to media_galleries_watch_apitest.cc to not to run this browser test on ChromeOS. PTAL. Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11535008/88003
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11535008/88003
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11535008/88003
Message was sent while issue was closed.
Change committed as 175347 |
