Media Galleries API Picasa: Put INI indexing step into sandboxed utility process.

Media Galleries API Picasa: Put INI indexing step into sandboxed utility process.

Picasa parses all the INI files in the Folders to build up an index of the album contents. This sandboxes that operation into the utility process.

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

[tommycli, 2013-07-02 21:14:00]

This is similar to the iTunes Windows pref sandboxing work and the Picasa PMP
sandboxing work.

jln: Need review of chrome_utility_messages.h.
thestig: The rest.

[Lei Zhang, 2013-07-02 22:08:17]

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:22: //
Picasa INI files are named either ".picasa.ini" OR "picasa.ini" for
This sentence is a bit hard to parse. I think you meant to say:

Picasa INI files are named "picasa.ini" on Picasa for Windows before version
71.18. Later versions and Picasa for Mac uses ".picasa.ini".

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h
(right):

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:27: // INI
files fond in Folders. The SafePicasaAlbumsIndexer object is ref-counted
typo

https://codereview.chromium.org/18562007/diff/6001/chrome/utility/media_galle...
File chrome/utility/media_galleries/picasa_albums_indexer.h (right):

https://codereview.chromium.org/18562007/diff/6001/chrome/utility/media_galle...
chrome/utility/media_galleries/picasa_albums_indexer.h:46: explicit
PicasaAlbumsIndexer(const AlbumUIDSet& album_uids);
I think you need to #include chrome/common/media_galleries/picasa_types.h fo
AlbumUIDSet.

[Lei Zhang, 2013-07-02 22:10:01]

My main concern is the combined size of all the ini files. I previously thought
you were parsing them one at a time.

[tommycli, 2013-07-03 00:58:28]

thestig: Addressed your comments below.

Point taken about file size.

My original calculation was, 100 bytes of metadata per image file * 10000 images
= 10MB over IPC = meh.

We can wait for the Picasa dudes to get back to us on typical INI file sizes.

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:22: //
Picasa INI files are named either ".picasa.ini" OR "picasa.ini" for
On 2013/07/02 22:08:17, Lei Zhang wrote:
> This sentence is a bit hard to parse. I think you meant to say:
> 
> Picasa INI files are named "picasa.ini" on Picasa for Windows before version
> 71.18. Later versions and Picasa for Mac uses ".picasa.ini".

Done.

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h
(right):

https://codereview.chromium.org/18562007/diff/6001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:27: // INI
files fond in Folders. The SafePicasaAlbumsIndexer object is ref-counted
On 2013/07/02 22:08:17, Lei Zhang wrote:
> typo

Done.

https://codereview.chromium.org/18562007/diff/6001/chrome/utility/media_galle...
File chrome/utility/media_galleries/picasa_albums_indexer.h (right):

https://codereview.chromium.org/18562007/diff/6001/chrome/utility/media_galle...
chrome/utility/media_galleries/picasa_albums_indexer.h:46: explicit
PicasaAlbumsIndexer(const AlbumUIDSet& album_uids);
On 2013/07/02 22:08:17, Lei Zhang wrote:
> I think you need to #include chrome/common/media_galleries/picasa_types.h fo
> AlbumUIDSet.

Done.

[Lei Zhang, 2013-07-03 02:00:51]

I'll look at my Picasa data when I get home. This is fine assuming the amount of
data transferred over IPC is not too big.

[tommycli, 2013-07-08 22:02:53]

vandebo: thestig out on vaca this week. Can you take over this review?

thestig's primary concern: Currently we read all the .picasa.ini files and put
them into a vector of strings to be sent over a single IPC call. He was
concerned about the total size.

See the email chain: "Picasa INI File sizes".

[vandebo (ex-Chrome), 2013-07-09 15:48:39]

On 2013/07/08 22:02:53, tommycli wrote:
> vandebo: thestig out on vaca this week. Can you take over this review?
> 
> thestig's primary concern: Currently we read all the .picasa.ini files and put
> them into a vector of strings to be sent over a single IPC call. He was
> concerned about the total size.
> 
> See the email chain: "Picasa INI File sizes".

I have similar concerns.  Even if the total size isn't that big, opening
hundreds of (cold) files seems like it's going to thrash the disk - seeking all
over the place for the file meta data and content.  This may cause head of line
blocking on the IO thread and trigger jank on the first picasa access.  How hard
would it be to start a batch mode utility process and send the file content over
one by one?  Doing so will likely increase latency for picasa ini parsing, but
it will give everything else in the system a fair chance to run and eliminate
any jank issues.

[tommycli, 2013-07-09 17:17:29]

On 2013/07/09 15:48:39, vandebo wrote:
> On 2013/07/08 22:02:53, tommycli wrote:
> > vandebo: thestig out on vaca this week. Can you take over this review?
> > 
> > thestig's primary concern: Currently we read all the .picasa.ini files and
put
> > them into a vector of strings to be sent over a single IPC call. He was
> > concerned about the total size.
> > 
> > See the email chain: "Picasa INI File sizes".
> 
> I have similar concerns.  Even if the total size isn't that big, opening
> hundreds of (cold) files seems like it's going to thrash the disk - seeking
all
> over the place for the file meta data and content.  This may cause head of
line
> blocking on the IO thread and trigger jank on the first picasa access.  How
hard
> would it be to start a batch mode utility process and send the file content
over
> one by one?  Doing so will likely increase latency for picasa ini parsing, but
> it will give everything else in the system a fair chance to run and eliminate
> any jank issues.

No problem. I will make that work.

[tommycli, 2013-07-09 17:17:50]

On 2013/07/09 17:17:29, tommycli wrote:
> On 2013/07/09 15:48:39, vandebo wrote:
> > On 2013/07/08 22:02:53, tommycli wrote:
> > > vandebo: thestig out on vaca this week. Can you take over this review?
> > > 
> > > thestig's primary concern: Currently we read all the .picasa.ini files and
> put
> > > them into a vector of strings to be sent over a single IPC call. He was
> > > concerned about the total size.
> > > 
> > > See the email chain: "Picasa INI File sizes".
> > 
> > I have similar concerns.  Even if the total size isn't that big, opening
> > hundreds of (cold) files seems like it's going to thrash the disk - seeking
> all
> > over the place for the file meta data and content.  This may cause head of
> line
> > blocking on the IO thread and trigger jank on the first picasa access.  How
> hard
> > would it be to start a batch mode utility process and send the file content
> over
> > one by one?  Doing so will likely increase latency for picasa ini parsing,
but
> > it will give everything else in the system a fair chance to run and
eliminate
> > any jank issues.
> 
> No problem. I will make that work.

Well - it might be hard, but it seems like the correct thing to do.

[tommycli, 2013-07-09 21:52:56]

vandebo: How do you feel about my latest changes on reading the INI files?

It's now done in a callback that's repeatedly posted to the MediaTaskRunner.
Each time it processes up to 10 (synchronously). This is a simple but i think
effective way to solve the blocking issue.

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

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:64: void
SafePicasaAlbumsIndexer::ProcessFoldersBatch() {
This callback is posted repeatedly until done.

[vandebo (ex-Chrome), 2013-07-10 17:43:52]

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

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:86: if
(!folders_queue.empty()) {
Should this be folder_inis_?

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:92:
parser_state_ = FINISHED_READING_INI_FILES_STATE;
This seems ok for now.  It still has the disadvantage that you're reading all of
the inis into memory at once.  If you do the utility process in batch mode you
could send kPicasaINIReadBatchSize worth of data at a time.

[tommycli, 2013-07-10 18:43:32]

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

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:86: if
(!folders_queue.empty()) {
On 2013/07/10 17:43:52, vandebo wrote:
> Should this be folder_inis_?

No. While queue is not empty, post itself onto the task runner again.

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:92:
parser_state_ = FINISHED_READING_INI_FILES_STATE;
On 2013/07/10 17:43:52, vandebo wrote:
> This seems ok for now.  It still has the disadvantage that you're reading all
of
> the inis into memory at once.  If you do the utility process in batch mode you
> could send kPicasaINIReadBatchSize worth of data at a time.

Yeah I agree we should keep that idea in our back pocket. Lei said that for his
8000 photos, he had 330 .picasa.ini files that totaled 427KB.

[vandebo (ex-Chrome), 2013-07-10 19:54:03]

No test?

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

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:86: if
(!folders_queue.empty()) {
On 2013/07/10 18:43:32, tommycli wrote:
> On 2013/07/10 17:43:52, vandebo wrote:
> > Should this be folder_inis_?
> 
> No. While queue is not empty, post itself onto the task runner again.

Hmm, right.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:43: bool
parse_success, const std::vector<AlbumInfo>& albums,
IWYU: picasa_types.h (several places in this CL)

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:31: }
}  // namesapce

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:43: for
(AlbumMap::const_iterator it = albums.begin(); it != albums.end(); ++it) {
nit: you may omit {}'s here.

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:83:
folders_inis_.push_back(folder_ini);
It might make sense to push first to minimize the number of times the content
gets copied.

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:112:
folder_inis));
Don't you want to send folders_inis_ ?

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:34: typedef
base::Callback<void(picasa::AlbumImagesMap)> ParserCallback;
nit: ParseCallback is a bit off as a name for this. DoneCallback or something
along those lines would probably be better.

[tommycli, 2013-07-10 22:22:27]

No test because safe_itunes_* also don't have tests. Is it cool if they are
covered by the end to end test like Itunes is?

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

https://codereview.chromium.org/18562007/diff/35001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:86: if
(!folders_queue.empty()) {
On 2013/07/10 19:54:03, vandebo wrote:
> On 2013/07/10 18:43:32, tommycli wrote:
> > On 2013/07/10 17:43:52, vandebo wrote:
> > > Should this be folder_inis_?
> > 
> > No. While queue is not empty, post itself onto the task runner again.
> 
> Hmm, right.

Done.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:43: bool
parse_success, const std::vector<AlbumInfo>& albums,
On 2013/07/10 19:54:03, vandebo wrote:
> IWYU: picasa_types.h (several places in this CL)

Done.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:31: }
On 2013/07/10 19:54:03, vandebo wrote:
> }  // namesapce

Done.

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:43: for
(AlbumMap::const_iterator it = albums.begin(); it != albums.end(); ++it) {
On 2013/07/10 19:54:03, vandebo wrote:
> nit: you may omit {}'s here.

Done.

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:83:
folders_inis_.push_back(folder_ini);
On 2013/07/10 19:54:03, vandebo wrote:
> It might make sense to push first to minimize the number of times the content
> gets copied.

folder_inis_ is a vector. Are you suggesting pre-allocating the memory to avoid
the copies? The actual string data itself is in the heap somewhere anyways, so I
think the copies are actually cheap.

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:112:
folder_inis));
On 2013/07/10 19:54:03, vandebo wrote:
> Don't you want to send folders_inis_ ?

Yep! thanks.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:34: typedef
base::Callback<void(picasa::AlbumImagesMap)> ParserCallback;
On 2013/07/10 19:54:03, vandebo wrote:
> nit: ParseCallback is a bit off as a name for this. DoneCallback or something
> along those lines would probably be better.

Done.

[vandebo (ex-Chrome), 2013-07-11 16:20:50]

LGTM, but please consider the comment.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:83:
folders_inis_.push_back(folder_ini);
On 2013/07/10 22:22:27, tommycli wrote:
> On 2013/07/10 19:54:03, vandebo wrote:
> > It might make sense to push first to minimize the number of times the
content
> > gets copied.
> 
> folder_inis_ is a vector. Are you suggesting pre-allocating the memory to
avoid
> the copies? The actual string data itself is in the heap somewhere anyways, so
I
> think the copies are actually cheap.

If copying a std::string was truly cheap, we wouldn't use const ref's to
std::string args.

[vandebo (ex-Chrome), 2013-07-11 16:20:50]

LGTM, but please consider the comment.

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:83:
folders_inis_.push_back(folder_ini);
On 2013/07/10 22:22:27, tommycli wrote:
> On 2013/07/10 19:54:03, vandebo wrote:
> > It might make sense to push first to minimize the number of times the
content
> > gets copied.
> 
> folder_inis_ is a vector. Are you suggesting pre-allocating the memory to
avoid
> the copies? The actual string data itself is in the heap somewhere anyways, so
I
> think the copies are actually cheap.

If copying a std::string was truly cheap, we wouldn't use const ref's to
std::string args.

[tommycli, 2013-07-11 17:24:13]

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

https://codereview.chromium.org/18562007/diff/44001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:83:
folders_inis_.push_back(folder_ini);
On 2013/07/11 16:20:50, vandebo wrote:
> On 2013/07/10 22:22:27, tommycli wrote:
> > On 2013/07/10 19:54:03, vandebo wrote:
> > > It might make sense to push first to minimize the number of times the
> content
> > > gets copied.
> > 
> > folder_inis_ is a vector. Are you suggesting pre-allocating the memory to
> avoid
> > the copies? The actual string data itself is in the heap somewhere anyways,
so
> I
> > think the copies are actually cheap.
> 
> If copying a std::string was truly cheap, we wouldn't use const ref's to
> std::string args.

Done.

[commit-bot: I haz the power, 2013-07-11 17:24:33]

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

[tommycli, 2013-07-11 17:40:01]

jln: Need security OWNER review on chrome/common/chrome_utility_messages.h.

[tommycli, 2013-07-12 00:28:34]

vandebo: Added a success bool in case utility process crashes. let me know if
this seems unreasonable.

[vandebo (ex-Chrome), 2013-07-12 17:34:42]

LGTM

[tommycli, 2013-07-15 20:45:19]

jln: ping for security review.

[tommycli, 2013-07-18 15:15:52]

jln: Ping for security review.

[jln (very slow on Chromium), 2013-07-22 19:55:09]

On 2013/07/18 15:15:52, tommycli wrote:
> jln: Ping for security review.

chrome/common/chrome_utility_messages.h lgtm

[jln (very slow on Chromium), 2013-07-22 19:56:39]

(sorry for the delay, this slipped through. Don't hesitate to ping me in a
separate thread if this happens again).

[tommycli, 2013-07-22 20:10:10]

thestig: May I have an OWNER review for chrome/ ?

[Lei Zhang, 2013-07-22 21:02:44]

Per in-person discussion, this relies on the WeakPtr CL, which got reverted over
the weekend.

https://chromiumcodereview.appspot.com/18562007/diff/91001/chrome/browser/med...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h
(right):

https://chromiumcodereview.appspot.com/18562007/diff/91001/chrome/browser/med...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:34: typedef
base::Callback<void(bool success,
You should either name both callback parameters or neither.

https://chromiumcodereview.appspot.com/18562007/diff/91001/chrome/browser/med...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:82: //
Initialized on the Media Task Runner, but only accessed on the IO thread.
This comment is no longer true. |parser_state_| is being accessed on both the IO
thread and the MTR.

[vandebo (ex-Chrome), 2013-07-24 18:15:54]

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

https://codereview.chromium.org/18562007/diff/91001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:106:
DCHECK_EQ(FINISHED_READING_INI_FILES_STATE, parser_state_);
nit: shouldn't access parser_state_ from multiple threads.

[tommycli, 2013-07-25 17:02:45]

thestig: Addressed your comments.

Unfortunately the inter-patchset diffs will be confused since it's been two
weeks between them. Sorry!

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

https://codereview.chromium.org/18562007/diff/91001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:106:
DCHECK_EQ(FINISHED_READING_INI_FILES_STATE, parser_state_);
On 2013/07/24 18:15:54, vandebo wrote:
> nit: shouldn't access parser_state_ from multiple threads.

Done. Removed parser_state_ access on MediaTaskRunner. Probably not worth using
a lock.

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

https://codereview.chromium.org/18562007/diff/91001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:34: typedef
base::Callback<void(bool success,
On 2013/07/22 21:02:44, Lei Zhang wrote:
> You should either name both callback parameters or neither.

Done.

https://codereview.chromium.org/18562007/diff/91001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.h:82: //
Initialized on the Media Task Runner, but only accessed on the IO thread.
On 2013/07/22 21:02:44, Lei Zhang wrote:
> This comment is no longer true. |parser_state_| is being accessed on both the
IO
> thread and the MTR.

Done.

[Lei Zhang, 2013-07-25 21:36:56]

https://codereview.chromium.org/18562007/diff/120001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://codereview.chromium.org/18562007/diff/120001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:89:
weak_factory_.GetWeakPtr()));
I don't think this compiles?

[tommycli, 2013-07-25 21:58:06]

Whoops I must have left a file out of the patch. Sry

https://chromiumcodereview.appspot.com/18562007/diff/120001/chrome/browser/me...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://chromiumcodereview.appspot.com/18562007/diff/120001/chrome/browser/me...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:89:
weak_factory_.GetWeakPtr()));
On 2013/07/25 21:36:57, Lei Zhang wrote:
> I don't think this compiles?

Done.

[Lei Zhang, 2013-07-25 23:14:58]

lgtm

https://chromiumcodereview.appspot.com/18562007/diff/91001/chrome/browser/med...
File chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
(right):

https://chromiumcodereview.appspot.com/18562007/diff/91001/chrome/browser/med...
chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc:106:
DCHECK_EQ(FINISHED_READING_INI_FILES_STATE, parser_state_);
On 2013/07/25 17:02:51, tommycli wrote:
> On 2013/07/24 18:15:54, vandebo wrote:
> > nit: shouldn't access parser_state_ from multiple threads.
> 
> Done. Removed parser_state_ access on MediaTaskRunner. Probably not worth
using
> a lock.

Sounds good. The communication with the utility process is the only bit that's
out of our control. So the state check is more important there.

[commit-bot: I haz the power, 2013-07-26 15:08:54]

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

[commit-bot: I haz the power, 2013-07-26 17:45:39]

Change committed as 213948