Use MediaGalleryPreferences directly in GalleryWatchStateTracker interface.

Use MediaGalleryPreferences directly in GalleryWatchStateTracker interface.

R=vandebo@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202946

[vandebo (ex-Chrome), 2013-05-22 19:45:58]

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:244:
const std::string& extension_id,
nit: undo change here.

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
(right):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:86:
void OnStorageMonitorInitialized();
This should go in the other change.

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(left):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:66:
bool GetGalleryFilePathAndId(const std::string& gallery_id,
I'm not sure anything is gained by inlining this and it leads to a bit of code
duplication.

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:386:
GalleryWatchStateTracker* state_tracker =
Extra

[Greg Billock, 2013-05-23 01:36:28]

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(left):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:66:
bool GetGalleryFilePathAndId(const std::string& gallery_id,
On 2013/05/22 19:45:58, vandebo wrote:
> I'm not sure anything is gained by inlining this and it leads to a bit of code
> duplication.

I did this to keep things straight around where the preferences were gotten
async, (which was kind of chopping this method in half). So it doesn't really
help nearly as much with this way of doing things.

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:386:
GalleryWatchStateTracker* state_tracker =
removed

On 2013/05/22 19:45:58, vandebo wrote:
> Extra

[vandebo (ex-Chrome), 2013-05-23 15:16:49]

This turns out to be simple, but nice cleanup. I'll give it the stamp once the
bots are happy.

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:25:
#include "chrome/browser/storage_monitor/storage_monitor.h"
nit: extra

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:290:
if (tracker_.get())
I don't think there's a way for tracker_ to be non-NULL when
media_galleries_privat_event_router_ is NULL.  Remove these two lines.  Add a
DCHECK if you like.

[Greg Billock, 2013-05-23 16:45:18]

+finnur for owners

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc
(right):

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.cc:25:
#include "chrome/browser/storage_monitor/storage_monitor.h"
On 2013/05/23 15:16:49, vandebo wrote:
> nit: extra

Done.

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:290:
if (tracker_.get())
On 2013/05/23 15:16:49, vandebo wrote:
> I don't think there's a way for tracker_ to be non-NULL when
> media_galleries_privat_event_router_ is NULL.  Remove these two lines.  Add a
> DCHECK if you like.

Done.

[vandebo (ex-Chrome), 2013-05-23 21:48:26]

LGTM

[Finnur, 2013-05-24 09:27:41]

LGTM, with couple of nits.

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
(right):

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:30:
// This class is owned by the MeidaGalleriesPrivateAPI, and is created on demand
nit: s/Meida/Media/

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:285:
void MediaGalleriesPrivateAPI::MaybeInitializeEventRouter() {
nit: Why not rename this function to include Tracker... or just drop EventRouter
from the name (MaybeInitialize)?

[Greg Billock, 2013-05-29 16:33:48]

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
(right):

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:30:
// This class is owned by the MeidaGalleriesPrivateAPI, and is created on demand
On 2013/05/24 09:27:41, Finnur wrote:
> nit: s/Meida/Media/

Done.

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc
(right):

https://codereview.chromium.org/15742010/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_api.cc:285:
void MediaGalleriesPrivateAPI::MaybeInitializeEventRouter() {
On 2013/05/24 09:27:41, Finnur wrote:
> nit: Why not rename this function to include Tracker... or just drop
EventRouter
> from the name (MaybeInitialize)?

Done.

[commit-bot: I haz the power, 2013-05-29 16:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/15742010/33001

[commit-bot: I haz the power, 2013-05-29 16:47:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/15742010/49001

[commit-bot: I haz the power, 2013-05-29 19:58:48]

Change committed as 202946