Media Galleries API Picasa: Make PicasaDataProvider handle async PMP and INI parsing robustly.

Media Galleries API Picasa: Make PicasaDataProvider handle async PMP and INI parsing robustly.

Depends on https://codereview.chromium.org/18562007/.

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

[tommycli, 2013-07-11 01:15:37]

vandebo: These are the changes we discussed to the PicasaDataProvider to make it
robust against invalidated data, chained async requirements, etc. 

This is contingent on the INI parser going in, so this doesn't need an immediate
review. This is next in line though.

gbillock: Since this may be a use case for your linear scheduler, you can also
volunteer to review this if interested.

[vandebo (ex-Chrome), 2013-07-11 16:56:27]

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:32: bool
need_albums_images,
Better to have this as an enum than a bool; it makes the call sites easier to
read.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:37: 
Do an early exit here.  If state is already good enough for the request, run the
callback directly.  Otherwise, drop it into the queue it needs to go in and call
the appropriate ensure method (at that point though, it's not ensure, it's do).

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:48: if
(album_table_reader_) {
This should first set the state to NO_FRESH_DATA_STATE.  Then if either of the
work tasks are outstanding, cancel it and start from the beginning. Actually,
better would be if there is anything in either of the callback queues, start
from the beginning.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:50:
EnsureAlbumListRefreshed(base::Bind(&base::DoNothing));
These DoNothing callbacks could possibly accumulate... I think there's a cleaner
way that doesn't need them.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:139: if
(parse_success) {
What to do on parse_failure ?  That's why the Itunes refresh callback has a
boolean argument, to push up parse failure.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
Then, if |albums_indexer_ready_callbacks_| isn't empty, chain to the album
indexer.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:155:
scoped_refptr<SafePicasaAlbumsIndexer> indexer,
Should this also take a parse_success argument?

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:42: void
InvalidateData();
This should be handled internally.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:51: enum
DataState {
nit: this can probably just be state instead of data state

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:52:
NO_FRESH_DATA_STATE = 0,
nit: omit = 0

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:52:
NO_FRESH_DATA_STATE = 0,
nit: STALE_DATA_STATE ?

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.cc:107:
scoped_refptr<SafePicasaAlbumTableReader>(this),
make_scoped_refptr might be easier here.

[tommycli, 2013-07-11 21:19:16]

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:32: bool
need_albums_images,
On 2013/07/11 16:56:27, vandebo wrote:
> Better to have this as an enum than a bool; it makes the call sites easier to
> read.

Done. This is kind of a strange case:

Any time we want the ALBUMS_IMAGES, we will also want the ALBUM_LIST, and
PicasaDataProvider is expected to have the ALBUM_LIST ready.

Should we make this relationship explicit by making callers pass in bitmasks
like ALBUM_LIST|ALBUMS_IMAGES ? Or just leave it as I've modified it to...

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:37: 
On 2013/07/11 16:56:27, vandebo wrote:
> Do an early exit here.  If state is already good enough for the request, run
the
> callback directly.  Otherwise, drop it into the queue it needs to go in and
call
> the appropriate ensure method (at that point though, it's not ensure, it's
do). 

The Ensure* functions perform the early exits themselves, and do it just as
efficiently (other than the overhead of the function call). It'd be kind of
duplicated to do it here also.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:48: if
(album_table_reader_) {
On 2013/07/11 16:56:27, vandebo wrote:
> This should first set the state to NO_FRESH_DATA_STATE.  Then if either of the
> work tasks are outstanding, cancel it and start from the beginning. Actually,
> better would be if there is anything in either of the callback queues, start
> from the beginning.

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:50:
EnsureAlbumListRefreshed(base::Bind(&base::DoNothing));
On 2013/07/11 16:56:27, vandebo wrote:
> These DoNothing callbacks could possibly accumulate... I think there's a
cleaner
> way that doesn't need them.

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:139: if
(parse_success) {
On 2013/07/11 16:56:27, vandebo wrote:
> What to do on parse_failure ?  That's why the Itunes refresh callback has a
> boolean argument, to push up parse failure.

Yeah - I'm not sure what the right thing to do in parse failure case is...

If it's because the data is in an in-between inconsistent state, we'd want to
try again. But we also don't want an infinite loop. Thoughts?

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
On 2013/07/11 16:56:27, vandebo wrote:
> Then, if |albums_indexer_ready_callbacks_| isn't empty, chain to the album
> indexer.

The chaining itself is one of the callbacks in the queue when it's needed. I
could also do it explicitly here. Both ways work.

Thoughts?

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:155:
scoped_refptr<SafePicasaAlbumsIndexer> indexer,
On 2013/07/11 16:56:27, vandebo wrote:
> Should this also take a parse_success argument? 

That parser doesn't have a success argument. It just throws out anything it
can't parse and returns an empty map in the extreme case.

Again, I'm not sure how to handle success/failures here. Should we retry? Should
we serve the previous data? Should we explicitly return an error to the
FileUtil?

I definitely don't want one bad INI file to prevent the program from returning
any albums images.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:42: void
InvalidateData();
On 2013/07/11 16:56:27, vandebo wrote:
> This should be handled internally.

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:51: enum
DataState {
On 2013/07/11 16:56:27, vandebo wrote:
> nit: this can probably just be state instead of data state

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:52:
NO_FRESH_DATA_STATE = 0,
On 2013/07/11 16:56:27, vandebo wrote:
> nit: omit = 0

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:52:
NO_FRESH_DATA_STATE = 0,
On 2013/07/11 16:56:27, vandebo wrote:
> nit: STALE_DATA_STATE ?

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.cc:107:
scoped_refptr<SafePicasaAlbumTableReader>(this),
On 2013/07/11 16:56:27, vandebo wrote:
> make_scoped_refptr might be easier here.

Done.

[vandebo (ex-Chrome), 2013-07-11 23:22:37]

Some quick comments before I run off.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:32: bool
need_albums_images,
On 2013/07/11 21:19:17, tommycli wrote:
> On 2013/07/11 16:56:27, vandebo wrote:
> > Better to have this as an enum than a bool; it makes the call sites easier
to
> > read.
> 
> Done. This is kind of a strange case:
> 
> Any time we want the ALBUMS_IMAGES, we will also want the ALBUM_LIST, and
> PicasaDataProvider is expected to have the ALBUM_LIST ready.
> 
> Should we make this relationship explicit by making callers pass in bitmasks
> like ALBUM_LIST|ALBUMS_IMAGES ? Or just leave it as I've modified it to...

I think it's ok the way it is.  If you want the ini data, you don't want the
other data, it's just that we need the other data to get the ini data -> it's an
implementation detail that one implies the other.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:37: 
On 2013/07/11 21:19:17, tommycli wrote:
> On 2013/07/11 16:56:27, vandebo wrote:
> > Do an early exit here.  If state is already good enough for the request, run
> the
> > callback directly.  Otherwise, drop it into the queue it needs to go in and
> call
> > the appropriate ensure method (at that point though, it's not ensure, it's
> do). 
> 
> The Ensure* functions perform the early exits themselves, and do it just as
> efficiently (other than the overhead of the function call). It'd be kind of
> duplicated to do it here also.

I really think that things will end up being a lot simpler if the ensure methods
don't take the call backs and you don't add additional callbacks (i.e. all the
callbacks in both lists should be from calls to Refresh).

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:139: if
(parse_success) {
On 2013/07/11 21:19:17, tommycli wrote:
> On 2013/07/11 16:56:27, vandebo wrote:
> > What to do on parse_failure ?  That's why the Itunes refresh callback has a
> > boolean argument, to push up parse failure.
> 
> Yeah - I'm not sure what the right thing to do in parse failure case is...
> 
> If it's because the data is in an in-between inconsistent state, we'd want to
> try again. But we also don't want an infinite loop. Thoughts?

I think we shouldn't trigger the invalidate until we think the data has finished
updating... not sure if we can do that, but I think we should aim for that.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
On 2013/07/11 21:19:17, tommycli wrote:
> On 2013/07/11 16:56:27, vandebo wrote:
> > Then, if |albums_indexer_ready_callbacks_| isn't empty, chain to the album
> > indexer.
> 
> The chaining itself is one of the callbacks in the queue when it's needed. I
> could also do it explicitly here. Both ways work.
> 
> Thoughts?

I think this makes things more complicated and harder to reason about.  I think
the callbacks should only be those passed into Refresh.

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:79:
base::Bind(&PicasaDataProvider::StartAlbumsImagesRefreshIfNecessary,
You could end up with multiple copies of this callback in
album_list_ready_callbacks_

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:30:
ALBUM_LIST_DATA,
Based on these name and my end of day fog, I don't know which one is which.  We
can probably do better on names here.

[tommycli, 2013-07-12 01:37:48]

vandebo: I added the 'success' flags we talked about and tried to simplify the
callback chaining structure along the lines we discussed.

I didn't get a chance to comb through for bugs real thoroughly, but wanted to
put something in front of you in case you wanted to look at in the morning.
Later.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:32: bool
need_albums_images,
On 2013/07/11 23:22:37, vandebo wrote:
> On 2013/07/11 21:19:17, tommycli wrote:
> > On 2013/07/11 16:56:27, vandebo wrote:
> > > Better to have this as an enum than a bool; it makes the call sites easier
> to
> > > read.
> > 
> > Done. This is kind of a strange case:
> > 
> > Any time we want the ALBUMS_IMAGES, we will also want the ALBUM_LIST, and
> > PicasaDataProvider is expected to have the ALBUM_LIST ready.
> > 
> > Should we make this relationship explicit by making callers pass in bitmasks
> > like ALBUM_LIST|ALBUMS_IMAGES ? Or just leave it as I've modified it to...
> 
> I think it's ok the way it is.  If you want the ini data, you don't want the
> other data, it's just that we need the other data to get the ini data -> it's
an
> implementation detail that one implies the other.

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:37: 
On 2013/07/11 23:22:37, vandebo wrote:
> On 2013/07/11 21:19:17, tommycli wrote:
> > On 2013/07/11 16:56:27, vandebo wrote:
> > > Do an early exit here.  If state is already good enough for the request,
run
> > the
> > > callback directly.  Otherwise, drop it into the queue it needs to go in
and
> > call
> > > the appropriate ensure method (at that point though, it's not ensure, it's
> > do). 
> > 
> > The Ensure* functions perform the early exits themselves, and do it just as
> > efficiently (other than the overhead of the function call). It'd be kind of
> > duplicated to do it here also.
> 
> I really think that things will end up being a lot simpler if the ensure
methods
> don't take the call backs and you don't add additional callbacks (i.e. all the
> callbacks in both lists should be from calls to Refresh).

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:139: if
(parse_success) {
On 2013/07/11 23:22:37, vandebo wrote:
> On 2013/07/11 21:19:17, tommycli wrote:
> > On 2013/07/11 16:56:27, vandebo wrote:
> > > What to do on parse_failure ?  That's why the Itunes refresh callback has
a
> > > boolean argument, to push up parse failure.
> > 
> > Yeah - I'm not sure what the right thing to do in parse failure case is...
> > 
> > If it's because the data is in an in-between inconsistent state, we'd want
to
> > try again. But we also don't want an infinite loop. Thoughts?
> 
> I think we shouldn't trigger the invalidate until we think the data has
finished
> updating... not sure if we can do that, but I think we should aim for that.

Done.

https://codereview.chromium.org/18986012/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
On 2013/07/11 23:22:37, vandebo wrote:
> On 2013/07/11 21:19:17, tommycli wrote:
> > On 2013/07/11 16:56:27, vandebo wrote:
> > > Then, if |albums_indexer_ready_callbacks_| isn't empty, chain to the album
> > > indexer.
> > 
> > The chaining itself is one of the callbacks in the queue when it's needed. I
> > could also do it explicitly here. Both ways work.
> > 
> > Thoughts?
> 
> I think this makes things more complicated and harder to reason about.  I
think
> the callbacks should only be those passed into Refresh.

Done.

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:79:
base::Bind(&PicasaDataProvider::StartAlbumsImagesRefreshIfNecessary,
On 2013/07/11 23:22:37, vandebo wrote:
> You could end up with multiple copies of this callback in
> album_list_ready_callbacks_

Done.

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/5629499534213120/chrome/browser...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:30:
ALBUM_LIST_DATA,
On 2013/07/11 23:22:37, vandebo wrote:
> Based on these name and my end of day fog, I don't know which one is which. 
We
> can probably do better on names here.

Done.

[vandebo (ex-Chrome), 2013-07-12 18:29:59]

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:38: switch
(data_state_) {
An open coded version is a lot shorter and I think less confusing:


  if (needed_data == LIST_OF_ALBUMS_AND_FOLDERS_DATA) {
    if (state_ != STALE_DATA_STATE) {
      ready_callback.Run(true);
      return;
    }
    album_list_ready_callbacks_.push(ready_callback);
  } else {
    if (state_ == ALBUMS_IMAGES_FRESH_STATE) {
      ready_callback.Run(true);
      return;
    }
    albums_indexer_ready_callbacks_.push(ready_callback);
  }
  DoRefreshIfNecessary();

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:112: void
PicasaDataProvider::StartAlbumListRefreshIfNecessary() {
It might be simpler to combine these two methods into one:

void DoRefreshIfNecessary() {
  DCHECK(state_ != ALBUMS_IMAGES_FRESH_STATE);
  if (state_ == STALE_DATA_STATE) {
    if (album_table_reader_)
      return;
    album_table_reader_ = ...
    album_table_reader_->Start();
  } else {
    if (album_table_reader_ || albums_indexer_ready_callbacks_.empty())
      return;
    album_inderer_ = ...
    album_indexer_->Start();
  }
}

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:113: if
(!album_table_reader_)
Is this negated?

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:143: if
(parse_success) {
You may want to handle parsing failed first..
You probably need another state to; no point it trying to reparse the same pmp
that is invalid.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:152: while
(!album_list_ready_callbacks_.empty()) {
Probably want to pull this loop into a small helper function:
RunCallbacks(std::queue<ReadyCallback>* callback, bool result);

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:41: // is
true if the caller wants the album contents also.
"is true" - needs updating.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:86: State
data_state_;
data_state_ -> state_ ?

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc (right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:289: if
(!callback.is_null()) {
Upstream changed to ensure that callback can't be null.  You can remove this
check.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:109:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get())
make a local temp variable to get nicer indenting.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:109:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get())
Why not use the current() method, you've already asserted that you're on the IO
thread

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:84:
base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
This isn't used in this CL?

[tommycli, 2013-07-12 21:37:19]

Much simplified according to your suggestions.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:38: switch
(data_state_) {
On 2013/07/12 18:29:59, vandebo wrote:
> An open coded version is a lot shorter and I think less confusing:
> 
> 
>   if (needed_data == LIST_OF_ALBUMS_AND_FOLDERS_DATA) {
>     if (state_ != STALE_DATA_STATE) {
>       ready_callback.Run(true);
>       return;
>     }
>     album_list_ready_callbacks_.push(ready_callback);
>   } else {
>     if (state_ == ALBUMS_IMAGES_FRESH_STATE) {
>       ready_callback.Run(true);
>       return;
>     }
>     albums_indexer_ready_callbacks_.push(ready_callback);
>   }
>   DoRefreshIfNecessary();

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:112: void
PicasaDataProvider::StartAlbumListRefreshIfNecessary() {
On 2013/07/12 18:29:59, vandebo wrote:
> It might be simpler to combine these two methods into one:
> 
> void DoRefreshIfNecessary() {
>   DCHECK(state_ != ALBUMS_IMAGES_FRESH_STATE);
>   if (state_ == STALE_DATA_STATE) {
>     if (album_table_reader_)
>       return;
>     album_table_reader_ = ...
>     album_table_reader_->Start();
>   } else {
>     if (album_table_reader_ || albums_indexer_ready_callbacks_.empty())
>       return;
>     album_inderer_ = ...
>     album_indexer_->Start();
>   }
> } 

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:113: if
(!album_table_reader_)
On 2013/07/12 18:29:59, vandebo wrote:
> Is this negated?

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:143: if
(parse_success) {
On 2013/07/12 18:29:59, vandebo wrote:
> You may want to handle parsing failed first..
> You probably need another state to; no point it trying to reparse the same pmp
> that is invalid.

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:152: while
(!album_list_ready_callbacks_.empty()) {
On 2013/07/12 18:29:59, vandebo wrote:
> Probably want to pull this loop into a small helper function:
> RunCallbacks(std::queue<ReadyCallback>* callback, bool result);

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:41: // is
true if the caller wants the album contents also.
On 2013/07/12 18:29:59, vandebo wrote:
> "is true" - needs updating.

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:86: State
data_state_;
On 2013/07/12 18:29:59, vandebo wrote:
> data_state_ -> state_ ?

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc (right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:289: if
(!callback.is_null()) {
On 2013/07/12 18:29:59, vandebo wrote:
> Upstream changed to ensure that callback can't be null.  You can remove this
> check.

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:109:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get())
On 2013/07/12 18:29:59, vandebo wrote:
> Why not use the current() method, you've already asserted that you're on the
IO
> thread

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:109:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get())
On 2013/07/12 18:29:59, vandebo wrote:
> make a local temp variable to get nicer indenting.

Done.

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h
(right):

https://codereview.chromium.org/18986012/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:84:
base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
On 2013/07/12 18:29:59, vandebo wrote:
> This isn't used in this CL?

This is an aborted attempt to make the utility processes explicitly
cancellable/killable. It looks to be a somewhat sizable change, so i'm backing
off and putting it in a separate CL.

[vandebo (ex-Chrome), 2013-07-12 22:17:27]

This could really use a test.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:25: void
CallAllReadyCallbacks(
nit: It sounds like this checks if a callback is ready before calling it. 
RunAllCallbacks ?

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:144: 
DCHECK state here.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
You need a return in the if loop since we set state_ below.  You may want to
move some of the stuff below above the if.  You'll want to also call the
callbacks on albums_indexer_ready_callbacks_ too.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:174: 
DCHECK state here.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:55:
ALBUM_LIST_FRESH_STATE,
ALBUM_LIST_FRESH_STATE and LIST_OF_ALBUMS_AND_FOLDERS_DATA should have a common
root.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:64: //
Should only be called if there's actually ready_callbacks waiting on data.
Method comment should describe what the method does.  This might be better as a
DCHECK at the top of the method.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:93: //
(older) utility processes are stale and ignored. Only one of these should
nit: Only one of these at a time should be non-NULL.

[tommycli, 2013-08-14 18:23:17]

vandebo:

Patchset 8->9 addresses your previous comments.
Patchset 10 adds an end-to-end browsertest.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:25: void
CallAllReadyCallbacks(
On 2013/07/12 22:17:28, vandebo wrote:
> nit: It sounds like this checks if a callback is ready before calling it. 
> RunAllCallbacks ?

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:144: 
On 2013/07/12 22:17:28, vandebo wrote:
> DCHECK state here.

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:151: }
On 2013/07/12 22:17:28, vandebo wrote:
> You need a return in the if loop since we set state_ below.  You may want to
> move some of the stuff below above the if.  You'll want to also call the
> callbacks on albums_indexer_ready_callbacks_ too.

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:174: 
On 2013/07/12 22:17:28, vandebo wrote:
> DCHECK state here.

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:55:
ALBUM_LIST_FRESH_STATE,
On 2013/07/12 22:17:28, vandebo wrote:
> ALBUM_LIST_FRESH_STATE and LIST_OF_ALBUMS_AND_FOLDERS_DATA should have a
common
> root.

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:64: //
Should only be called if there's actually ready_callbacks waiting on data.
On 2013/07/12 22:17:28, vandebo wrote:
> Method comment should describe what the method does.  This might be better as
a
> DCHECK at the top of the method.

Done.

https://codereview.chromium.org/18986012/diff/49001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:93: //
(older) utility processes are stale and ignored. Only one of these should
On 2013/07/12 22:17:28, vandebo wrote:
> nit: Only one of these at a time should be non-NULL.

Done.

[vandebo (ex-Chrome), 2013-08-14 23:48:11]

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:105:
DoRefreshIfNecessary();
This is already "IfNecessary", so why not check the callback lists inside as
well?

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:124: if
(albums_indexer_)
DCHECK state_

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:173: 
Remove extra line

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:67: void
OnAlbumListRefreshed(scoped_refptr<SafePicasaAlbumTableReader> reader,
nit: OnAlbumTableReaderDone

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:90:
std::queue<ReadyCallback> albums_indexer_ready_callbacks_;
nit: albums_index_ready_callbacks

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:29:
virtual void SetUp() OVERRIDE { InProcessBrowserTest::SetUp(); }
Omit since all you do is call the parent class's instance.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:312:

There are a lot more tests that can be added.... I think it would be worthwhile
to add at least tests that did the following:

Multiple pending requests.
An invalidation.
An invalidation while the reader/indexer is outstanding.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:36:
typedef base::Callback<void(scoped_refptr<SafePicasaAlbumTableReader>,
I don't think there's a good reason to include this in this type definition.
Just bind it into the callback before passing it down.... But this is passed
into the constructor... move the callback to the Start method?

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:37:
bool,
nit: name this since it's unclear without a name.

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
File chrome/common/media_galleries/picasa_types.h (right):

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
chrome/common/media_galleries/picasa_types.h:25: const char
kPicasaAlbumTableName[] = "albumdata";
Don't you want these to be extern with the definition in the .cc file?

[tommycli, 2013-08-15 22:52:36]

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:105:
DoRefreshIfNecessary();
On 2013/08/14 23:48:11, vandebo wrote:
> This is already "IfNecessary", so why not check the callback lists inside as
> well?

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:124: if
(albums_indexer_)
On 2013/08/14 23:48:11, vandebo wrote:
> DCHECK state_

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:173: 
On 2013/08/14 23:48:11, vandebo wrote:
> Remove extra line

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:67: void
OnAlbumListRefreshed(scoped_refptr<SafePicasaAlbumTableReader> reader,
On 2013/08/14 23:48:11, vandebo wrote:
> nit: OnAlbumTableReaderDone

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:90:
std::queue<ReadyCallback> albums_indexer_ready_callbacks_;
On 2013/08/14 23:48:11, vandebo wrote:
> nit: albums_index_ready_callbacks

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:29:
virtual void SetUp() OVERRIDE { InProcessBrowserTest::SetUp(); }
On 2013/08/14 23:48:11, vandebo wrote:
> Omit since all you do is call the parent class's instance.

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:312:

On 2013/08/14 23:48:11, vandebo wrote:
> There are a lot more tests that can be added.... I think it would be
worthwhile
> to add at least tests that did the following:
> 
> Multiple pending requests.
> An invalidation.
> An invalidation while the reader/indexer is outstanding.

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:36:
typedef base::Callback<void(scoped_refptr<SafePicasaAlbumTableReader>,
On 2013/08/14 23:48:11, vandebo wrote:
> I don't think there's a good reason to include this in this type definition.
> Just bind it into the callback before passing it down.... But this is passed
> into the constructor... move the callback to the Start method?

PicasaDataProvider also needs to keep a reference so it knows to ignore 'stale'
instances. I'm not super happy about the lifetime of this object currently
either.

It can't be unilaterally destroyed by PicasaDataProvider because it might be
doing something on a different thread.

I could make the object 'Owned' by the callback and make PicasaDataProvider keep
a raw pointer, but is that actually better?

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:37:
bool,
On 2013/08/14 23:48:11, vandebo wrote:
> nit: name this since it's unclear without a name.

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
File chrome/common/media_galleries/picasa_types.h (right):

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
chrome/common/media_galleries/picasa_types.h:25: const char
kPicasaAlbumTableName[] = "albumdata";
On 2013/08/14 23:48:11, vandebo wrote:
> Don't you want these to be extern with the definition in the .cc file?

Done. The numbers were used in a switch statement, couldn't be extern.

[tommycli, 2013-08-21 21:04:10]

vandebo: Patchset 16 changes the interface of PicasaDataProvider slightly from
Patchset 15.

[tommycli, 2013-08-21 21:11:59]

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:300:
EXPECT_EQ(
Technically correct though somewhat inconsistent formatting, but this is what
'git cl format' gave me.

[tommycli, 2013-08-22 00:13:02]

On 2013/08/21 21:11:59, tommycli wrote:
>
https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
> File
>
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
>
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:300:
> EXPECT_EQ(
> Technically correct though somewhat inconsistent formatting, but this is what
> 'git cl format' gave me.

vandebo: Ping for OWNER review. I can passoff to thestig if you're swamped
though.

[vandebo (ex-Chrome), 2013-08-22 17:48:01]

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:36:
typedef base::Callback<void(scoped_refptr<SafePicasaAlbumTableReader>,
On 2013/08/15 22:52:36, tommycli wrote:
> On 2013/08/14 23:48:11, vandebo wrote:
> > I don't think there's a good reason to include this in this type definition.
> > Just bind it into the callback before passing it down.... But this is passed
> > into the constructor... move the callback to the Start method?
> 
> PicasaDataProvider also needs to keep a reference so it knows to ignore
'stale'
> instances. I'm not super happy about the lifetime of this object currently
> either.
> 
> It can't be unilaterally destroyed by PicasaDataProvider because it might be
> doing something on a different thread.
> 
> I could make the object 'Owned' by the callback and make PicasaDataProvider
keep
> a raw pointer, but is that actually better?

Per offline discussion, Tommy looking into this again.

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
File chrome/common/media_galleries/picasa_types.h (right):

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
chrome/common/media_galleries/picasa_types.h:25: const char
kPicasaAlbumTableName[] = "albumdata";
On 2013/08/15 22:52:36, tommycli wrote:
> On 2013/08/14 23:48:11, vandebo wrote:
> > Don't you want these to be extern with the definition in the .cc file?
> 
> Done. The numbers were used in a switch statement, couldn't be extern.

Tommy looking into prevalent style.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:57: if
(state_ != STALE_DATA_STATE) {
This is correct, but in terms of making it more future proof, it'd probably be
better to check that the state_ is equal to either of the two valid states.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:106:
DCHECK(!(album_table_reader_ && albums_indexer_));
Why this invariant should be true, this method doesn't rely on it, so I don't
think you need to include the DCHECK here.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:50:
scoped_ptr<AlbumImages> FindAlbumImages(const std::string& key,
This returns images?  Please do a follow up CL to change AlbumImages to a better
name.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:138:
void WritePicasaDatabase() {
Make this a helper function with a better name (instead of method) that just
takes the two ScopedTempDirs as arguments.  That way it can get reused without
artificial inheritance.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:333:
virtual void ListCallback(bool parse_success) {
Make this take the expected value of list_callbacks (post increment) and verify
that it is correct.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:400:
data_provider()->InvalidateData();
Add TODO here, to remove this line when you start watching the database files.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:439:
class PicasaDataProviderInvalidateInflightTableReaderTest
Is this test racey?  I don't see how it works.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:457:
}
It seems like something needs to happen after the invalidate

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:479:
WriteFolderINIs();
This also seems to be racey.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:207:
base::WaitableEvent event(true, false);
Abstract this into void RunOnMediaTaskRunner(const base::Closure& closure);

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:235:
base::WaitableEvent event(true, false);
Why not use a runloop like in the browser test?

[tommycli, 2013-08-22 22:32:50]

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h
(right):

https://codereview.chromium.org/18986012/diff/75001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.h:36:
typedef base::Callback<void(scoped_refptr<SafePicasaAlbumTableReader>,
On 2013/08/22 17:48:02, vandebo wrote:
> On 2013/08/15 22:52:36, tommycli wrote:
> > On 2013/08/14 23:48:11, vandebo wrote:
> > > I don't think there's a good reason to include this in this type
definition.
> > > Just bind it into the callback before passing it down.... But this is
passed
> > > into the constructor... move the callback to the Start method?
> > 
> > PicasaDataProvider also needs to keep a reference so it knows to ignore
> 'stale'
> > instances. I'm not super happy about the lifetime of this object currently
> > either.
> > 
> > It can't be unilaterally destroyed by PicasaDataProvider because it might be
> > doing something on a different thread.
> > 
> > I could make the object 'Owned' by the callback and make PicasaDataProvider
> keep
> > a raw pointer, but is that actually better?
> 
> Per offline discussion, Tommy looking into this again.

Done.

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
File chrome/common/media_galleries/picasa_types.h (right):

https://codereview.chromium.org/18986012/diff/75001/chrome/common/media_galle...
chrome/common/media_galleries/picasa_types.h:25: const char
kPicasaAlbumTableName[] = "albumdata";
On 2013/08/22 17:48:02, vandebo wrote:
> On 2013/08/15 22:52:36, tommycli wrote:
> > On 2013/08/14 23:48:11, vandebo wrote:
> > > Don't you want these to be extern with the definition in the .cc file?
> > 
> > Done. The numbers were used in a switch statement, couldn't be extern.
> 
> Tommy looking into prevalent style. 

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:57: if
(state_ != STALE_DATA_STATE) {
On 2013/08/22 17:48:02, vandebo wrote:
> This is correct, but in terms of making it more future proof, it'd probably be
> better to check that the state_ is equal to either of the two valid states.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.cc:106:
DCHECK(!(album_table_reader_ && albums_indexer_));
On 2013/08/22 17:48:02, vandebo wrote:
> Why this invariant should be true, this method doesn't rely on it, so I don't
> think you need to include the DCHECK here.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:50:
scoped_ptr<AlbumImages> FindAlbumImages(const std::string& key,
On 2013/08/22 17:48:02, vandebo wrote:
> This returns images?  Please do a follow up CL to change AlbumImages to a
better
> name.

Done. Added a TODO in picasa_types.h.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:138:
void WritePicasaDatabase() {
On 2013/08/22 17:48:02, vandebo wrote:
> Make this a helper function with a better name (instead of method) that just
> takes the two ScopedTempDirs as arguments.  That way it can get reused without
> artificial inheritance.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:300:
EXPECT_EQ(
On 2013/08/21 21:11:59, tommycli wrote:
> Technically correct though somewhat inconsistent formatting, but this is what
> 'git cl format' gave me.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:333:
virtual void ListCallback(bool parse_success) {
On 2013/08/22 17:48:02, vandebo wrote:
> Make this take the expected value of list_callbacks (post increment) and
verify
> that it is correct.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:400:
data_provider()->InvalidateData();
On 2013/08/22 17:48:02, vandebo wrote:
> Add TODO here, to remove this line when you start watching the database files.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:439:
class PicasaDataProviderInvalidateInflightTableReaderTest
On 2013/08/22 17:48:02, vandebo wrote:
> Is this test racey?  I don't see how it works.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:457:
}
On 2013/08/22 17:48:02, vandebo wrote:
> It seems like something needs to happen after the invalidate

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:479:
WriteFolderINIs();
On 2013/08/22 17:48:02, vandebo wrote:
> This also seems to be racey.

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:207:
base::WaitableEvent event(true, false);
On 2013/08/22 17:48:02, vandebo wrote:
> Abstract this into void RunOnMediaTaskRunner(const base::Closure& closure);

Done.

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:235:
base::WaitableEvent event(true, false);
On 2013/08/22 17:48:02, vandebo wrote:
> Why not use a runloop like in the browser test?

I believe this was because Runloops can only be used when the Quit is called on
the same thread. WaitableEvents are threadsafe.

[vandebo (ex-Chrome), 2013-08-23 16:42:40]

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:235:
base::WaitableEvent event(true, false);
On 2013/08/22 22:32:51, tommycli wrote:
> On 2013/08/22 17:48:02, vandebo wrote:
> > Why not use a runloop like in the browser test?
> 
> I believe this was because Runloops can only be used when the Quit is called
on
> the same thread. WaitableEvents are threadsafe.

If calling on the wrong thread is the only issue, that's solvable....
Post the closure
Post a task, which posts a task back to the run loop and calls the quit closure.

Maybe I'm biased against using a waitable event, but it is really blocking -
nothing can happen.  At least with a RunLoop, the message loop is getting run
when it has work to do.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:86: const
base::FilePath& GetDatabasePathForTesting() const;
Do you need the getters?  I would expect that you only need the setters.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:185:
virtual void InitializeTestData() {
It looks like no class overrides this, but doesn't call it.  Move the contents
of this method to the call site (in StartTestOnMediaTaskRunner) and leave this
as an empty method that can do extra stuff in child classes.

Hmm, you override that as well. Do you always call IntializeTestData at the
start of StartTestOnMediaTaskRunner ?  If so, pull the common stuff to the
caller of StartTestOnMediaTaskRunner.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:494:
data_provider()->SetDatabasePathForTesting(database_path);
database_path = test_helper_.GetTempDirPath(), set on line 191

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:526:
data_provider()->InvalidateData();
Invalidate data is going start from the beginning, so you don't need to
AlbumMaps back to their original values.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:250:
void TearDownOnMediaTaskRunner() { picasa_data_provider_.reset(); }
I prefer that this be on multiple lines.

[tommycli, 2013-08-26 22:06:16]

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/123001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:235:
base::WaitableEvent event(true, false);
On 2013/08/23 16:42:41, vandebo wrote:
> On 2013/08/22 22:32:51, tommycli wrote:
> > On 2013/08/22 17:48:02, vandebo wrote:
> > > Why not use a runloop like in the browser test?
> > 
> > I believe this was because Runloops can only be used when the Quit is called
> on
> > the same thread. WaitableEvents are threadsafe.
> 
> If calling on the wrong thread is the only issue, that's solvable....
> Post the closure
> Post a task, which posts a task back to the run loop and calls the quit
closure.
> 
> Maybe I'm biased against using a waitable event, but it is really blocking -
> nothing can happen.  At least with a RunLoop, the message loop is getting run
> when it has work to do.

Done.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:86: const
base::FilePath& GetDatabasePathForTesting() const;
On 2013/08/23 16:42:41, vandebo wrote:
> Do you need the getters?  I would expect that you only need the setters.

Done.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:185:
virtual void InitializeTestData() {
On 2013/08/23 16:42:41, vandebo wrote:
> It looks like no class overrides this, but doesn't call it.  Move the contents
> of this method to the call site (in StartTestOnMediaTaskRunner) and leave this
> as an empty method that can do extra stuff in child classes.
> 
> Hmm, you override that as well. Do you always call IntializeTestData at the
> start of StartTestOnMediaTaskRunner ?  If so, pull the common stuff to the
> caller of StartTestOnMediaTaskRunner.

Done.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:494:
data_provider()->SetDatabasePathForTesting(database_path);
On 2013/08/23 16:42:41, vandebo wrote:
> database_path = test_helper_.GetTempDirPath(), set on line 191

Done.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:526:
data_provider()->InvalidateData();
On 2013/08/23 16:42:41, vandebo wrote:
> Invalidate data is going start from the beginning, so you don't need to
> AlbumMaps back to their original values.

Done.

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:250:
void TearDownOnMediaTaskRunner() { picasa_data_provider_.reset(); }
On 2013/08/23 16:42:41, vandebo wrote:
> I prefer that this be on multiple lines.

Done. This was a git cl format change though.

[vandebo (ex-Chrome), 2013-08-27 04:29:04]

LGTM with nits and after clang fixes.

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:523:
VerifyRefreshResults,
I don't see any reason for the extra space here (presumably added by git cl
format?)

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:158:
base::Bind(&base::RunLoop::Quit, base::Unretained(&loop)));
loop::QuitClosure()

[tommycli, 2013-08-27 18:39:17]

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc
(right):

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:523:
VerifyRefreshResults,
On 2013/08/27 04:29:04, vandebo wrote:
> I don't see any reason for the extra space here (presumably added by git cl
> format?)

I'm confused. Isn't this the same formatting at line 494?

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/18986012/diff/162001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:158:
base::Bind(&base::RunLoop::Quit, base::Unretained(&loop)));
On 2013/08/27 04:29:04, vandebo wrote:
> loop::QuitClosure()

Done.

[tommycli, 2013-08-27 18:41:05]

thestig: I believe I need your LG for gypi, common, and utility.

[commit-bot: I haz the power, 2013-08-27 18:50:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18986012/189001

[tommycli, 2013-08-27 19:51:27]

On 2013/08/27 18:50:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/tommycli%40chromium.org/18986012/189001

vandebo: patchset 24 removes spaces per our discussion. Seemed better to remove
some spaces in violation of clang format than to remove information from the
descriptive although overly long name.

[commit-bot: I haz the power, 2013-08-28 00:24:37]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-28 00:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18986012/192001

[commit-bot: I haz the power, 2013-08-28 05:36:19]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-29 21:20:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18986012/192001

[commit-bot: I haz the power, 2013-08-30 00:10:39]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[tommycli, 2013-08-30 22:36:38]

jeremy: Added /private/etc/localtime and /etc (just the directory) for
read-file-metadata in utility.sb. Need OWNER review.

[jeremy, 2013-08-30 22:40:18]

https://codereview.chromium.org/18986012/diff/185001/content/utility/utility.sb
File content/utility/utility.sb (right):

https://codereview.chromium.org/18986012/diff/185001/content/utility/utility....
content/utility/utility.sb:23: (literal "/etc"))
Per my comment in email, we generally try not to punch holes in the sandbox and
only do so as a last option.  Can you achieve this by calling the relevant
function in the sandbox warmup code? or roundtripping through the browser?

[tommycli, 2013-08-30 22:57:04]

jeremy: The warmup technique totally worked. Whoa.

https://codereview.chromium.org/18986012/diff/185001/content/utility/utility.sb
File content/utility/utility.sb (right):

https://codereview.chromium.org/18986012/diff/185001/content/utility/utility....
content/utility/utility.sb:23: (literal "/etc"))
On 2013/08/30 22:40:19, jeremy wrote:
> Per my comment in email, we generally try not to punch holes in the sandbox
and
> only do so as a last option.  Can you achieve this by calling the relevant
> function in the sandbox warmup code? or roundtripping through the browser?

Done.

[jeremy, 2013-08-30 23:02:51]

https://codereview.chromium.org/18986012/diff/179001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/18986012/diff/179001/content/common/sandbox_m...
content/common/sandbox_mac.mm:312: // Allow access to local timezones.
Can you provide a much more detailed comment about why we need it/which system
calls fail on which os versions?

The purpose of the comments in this file are to try to provide enough
information that we know when/if it's safe to remove warmup code.

[tommycli, 2013-08-30 23:14:20]

jeremy: I expanded on the comment. Thanks.

https://codereview.chromium.org/18986012/diff/179001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/18986012/diff/179001/content/common/sandbox_m...
content/common/sandbox_mac.mm:312: // Allow access to local timezones.
On 2013/08/30 23:02:51, jeremy wrote:
> Can you provide a much more detailed comment about why we need it/which system
> calls fail on which os versions?
> 
> The purpose of the comments in this file are to try to provide enough
> information that we know when/if it's safe to remove warmup code.

Done.

[jeremy, 2013-08-30 23:26:26]

OK, just a small comment fix and then we should be good to go...

Did you test the fix on 10.{6,7,8} ? Not gating my review on that, but as I
mentioned before the OSs don't always act the same :o/

https://codereview.chromium.org/18986012/diff/237001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/18986012/diff/237001/content/common/sandbox_m...
content/common/sandbox_mac.mm:315: CFTimeZoneCopySystem();
How about:

CFTimeZoneCopyZone() tries to read /etc/<?> - 10.8
Needed by Media Gallery - crbug.com/...

[tommycli, 2013-08-30 23:36:55]

jeremy: Fixed comment. I haven't tried it on the other versions of OS X. I will
be sad if this warmup doesn't work for them.

https://codereview.chromium.org/18986012/diff/237001/content/common/sandbox_m...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/18986012/diff/237001/content/common/sandbox_m...
content/common/sandbox_mac.mm:315: CFTimeZoneCopySystem();
On 2013/08/30 23:26:26, jeremy wrote:
> How about:
> 
> CFTimeZoneCopyZone() tries to read /etc/<?> - 10.8
> Needed by Media Gallery - crbug.com/...

Done.

[jeremy, 2013-08-30 23:55:54]

sandbox_mac.mm LGTM

[commit-bot: I haz the power, 2013-08-31 00:04:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18986012/243001

[tommycli, 2013-08-31 00:18:08]

jeremy: thanks for all your help today.

[commit-bot: I haz the power, 2013-08-31 03:37:49]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-03 06:06:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18986012/243001

[commit-bot: I haz the power, 2013-09-03 07:11:56]

Change committed as 220906