Media Galleries API Picasa: Add file watch to invalidate database data on disk write.

Media Galleries API Picasa: Add file watch to invalidate database data on disk write.

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

R=vandebo

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

[tommycli, 2013-08-29 21:59:46]

vandebo: Based on your ITunes file watcher work. Watches the tmp directory
because Mac can't detect modifications in the actual db3 directory, but can
detect creations/deletions.

[vandebo (ex-Chrome), 2013-08-29 23:19:22]

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/data_provider_helper.cc (right):

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.cc:9: #include
"base/format_macros.h"
What's this for?

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.cc:27:
->PostTask(FROM_HERE, base::Bind(callback, path, error));
nit: PostTask( on the previous line.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/data_provider_helper.h (right):

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.h:5: #ifndef
CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_DATA_PROVIDER_HELPER_H_
Since there's nothing else in here, call this file_path_watcher_util.h

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:56: const
base::FilePath& temp_dir_path);
Probably don't need an explicit temp dir in test.

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:181:
explicit TestPicasaDataProvider(const base::FilePath& database_path,
Yea, it seems like it wouldn't be too much trouble to follow the same directory
layout for fake data.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:208:
virtual void InvalidateData() OVERRIDE {
Should this be private?

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:218:
void SetDatabasePathForTesting(const base::FilePath& database_path) {
Do you still need this?

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:229:
scoped_ptr<base::Closure> invalidate_callback_;
You don't need a scoped_ptr for Closure, just take a copy - it has an internally
ref counted object iirc.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:306:
->RefreshData(RequestedDataType(),
Don't change this formatting.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:461:

There's a race here - you want to wait until it's moved until you call
RefreshData.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:585:

And here

[tommycli, 2013-09-03 20:20:42]

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/data_provider_helper.cc (right):

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.cc:9: #include
"base/format_macros.h"
On 2013/08/29 23:19:22, vandebo wrote:
> What's this for?

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.cc:27:
->PostTask(FROM_HERE, base::Bind(callback, path, error));
On 2013/08/29 23:19:22, vandebo wrote:
> nit: PostTask( on the previous line.

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/data_provider_helper.h (right):

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/data_provider_helper.h:5: #ifndef
CHROME_BROWSER_MEDIA_GALLERIES_FILEAPI_DATA_PROVIDER_HELPER_H_
On 2013/08/29 23:19:22, vandebo wrote:
> Since there's nothing else in here, call this file_path_watcher_util.h

Done.

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider.h:56: const
base::FilePath& temp_dir_path);
On 2013/08/29 23:19:22, vandebo wrote:
> Probably don't need an explicit temp dir in test.

Done.

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:181:
explicit TestPicasaDataProvider(const base::FilePath& database_path,
On 2013/08/29 23:19:22, vandebo wrote:
> Yea, it seems like it wouldn't be too much trouble to follow the same
directory
> layout for fake data.

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:208:
virtual void InvalidateData() OVERRIDE {
On 2013/08/29 23:19:22, vandebo wrote:
> Should this be private?

No. This is explicitly called by a few tests. (I couldn't figure out a way to
avoid a race between file watcher on the FILE thread and the utility process
reporting on the Media Task Runner thread).

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:218:
void SetDatabasePathForTesting(const base::FilePath& database_path) {
On 2013/08/29 23:19:22, vandebo wrote:
> Do you still need this?

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:229:
scoped_ptr<base::Closure> invalidate_callback_;
On 2013/08/29 23:19:22, vandebo wrote:
> You don't need a scoped_ptr for Closure, just take a copy - it has an
internally
> ref counted object iirc.

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:306:
->RefreshData(RequestedDataType(),
On 2013/08/29 23:19:22, vandebo wrote:
> Don't change this formatting.

Done.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:461:

On 2013/08/29 23:19:22, vandebo wrote:
> There's a race here - you want to wait until it's moved until you call
> RefreshData.

Move is synchronous I believe.

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:585:

On 2013/08/29 23:19:22, vandebo wrote:
> And here

See above.

[vandebo (ex-Chrome), 2013-09-05 16:49:38]

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:461:

On 2013/09/03 20:20:43, tommycli wrote:
> On 2013/08/29 23:19:22, vandebo wrote:
> > There's a race here - you want to wait until it's moved until you call
> > RefreshData.
> 
> Move is synchronous I believe.

The move is synchronous, but the invalidation triggered by the file watcher is
not.  In fact, you're probably always losing the race here (because invalidation
happens on the media task runner) but starting a refresh anyway because the data
provider is in the initial state, then while you're reading the data, you might
get the invalidation from this move, which will cause you to read the data
again.

Since invalidation isn't important for this test, you could avoid the race by
writing to the database direction (instead of the temp directory) for this test.
Otherwise you should use your SetInvalidateCallback method.

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

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:541:
data_provider()->InvalidateData();
This is tricky again - You should only do one of the two things here.  If you
can ensure that the invalidate from the move will trigger before your first read
finishes (I don't think you can), then you should just move stuff into place. 
If not, the files should already be in place and you can trigger the condition
you're interested in by explicitly invalidating the data.

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_finder.cc (right):

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_finder.cc:38:
.AppendASCII(kPicasaDatabaseDirName);
nit: I would line this up with the AppendASCII on the previous line.

[tommycli, 2013-09-05 21:37:37]

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

https://codereview.chromium.org/23499006/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:461:

On 2013/09/05 16:49:38, vandebo wrote:
> On 2013/09/03 20:20:43, tommycli wrote:
> > On 2013/08/29 23:19:22, vandebo wrote:
> > > There's a race here - you want to wait until it's moved until you call
> > > RefreshData.
> > 
> > Move is synchronous I believe.
> 
> The move is synchronous, but the invalidation triggered by the file watcher is
> not.  In fact, you're probably always losing the race here (because
invalidation
> happens on the media task runner) but starting a refresh anyway because the
data
> provider is in the initial state, then while you're reading the data, you
might
> get the invalidation from this move, which will cause you to read the data
> again.
> 
> Since invalidation isn't important for this test, you could avoid the race by
> writing to the database direction (instead of the temp directory) for this
test.
> Otherwise you should use your SetInvalidateCallback method.

Oh, I see what you're saying. I took your recommendation of writing to the
database dir by default. I think that fixes and simplifies things.

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

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_data_provider_browsertest.cc:541:
data_provider()->InvalidateData();
On 2013/09/05 16:49:38, vandebo wrote:
> This is tricky again - You should only do one of the two things here.  If you
> can ensure that the invalidate from the move will trigger before your first
read
> finishes (I don't think you can), then you should just move stuff into place. 
> If not, the files should already be in place and you can trigger the condition
> you're interested in by explicitly invalidating the data.

Done.

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_finder.cc (right):

https://codereview.chromium.org/23499006/diff/19001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_finder.cc:38:
.AppendASCII(kPicasaDatabaseDirName);
On 2013/09/05 16:49:38, vandebo wrote:
> nit: I would line this up with the AppendASCII on the previous line.

Done.

[vandebo (ex-Chrome), 2013-09-06 18:01:03]

LGTM

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

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

[commit-bot: I haz the power, 2013-09-06 21:56:05]

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

[commit-bot: I haz the power, 2013-09-06 23:45:30]

Change committed as 221818