Media Galleries API Picasa: Windows Custom Database Locations

Media Galleries API Picasa: Windows Custom Database Locations

This code enables Picasa to find custom Windows database locations as defined in the registry. It also provides for overridden database locations for testing.

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

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

[tommycli, 2013-09-17 18:49:34]

gbillock: Need review.

[tommycli, 2013-10-01 19:11:48]

ready for review

[Greg Billock, 2013-10-01 20:50:35]

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:27: const wchar_t
kPicasaRegistryPath[] =
Can you just move this to a local const inside the method?

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:43: std::wstring value;
Can we use string16 here?

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:81: base::FilePath path
= GetPicasaDatabasePath();
Can we split the platforms here, so like

base::FilePath path;
#if defined(OS_WIN)
path = GetPicasaDatabasePathWin();
#elif defined(OS_MACOSX)
path = GetPicasaDatabasePathMac();
#endif

return path;

Then we can either split out the impl into separate ifdef chunks without other
#elif and duplicate names.

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

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:27: const wchar_t
kPicasaRegistryPath[] =
On 2013/10/01 20:50:35, Greg Billock wrote:
> Can you just move this to a local const inside the method?

Tests use this.

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:43: std::wstring value;
On 2013/10/01 20:50:35, Greg Billock wrote:
> Can we use string16 here?

Done. Yes, that's better !

https://codereview.chromium.org/23456035/diff/9001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:81: base::FilePath path
= GetPicasaDatabasePath();
On 2013/10/01 20:50:35, Greg Billock wrote:
> Can we split the platforms here, so like
> 
> base::FilePath path;
> #if defined(OS_WIN)
> path = GetPicasaDatabasePathWin();
> #elif defined(OS_MACOSX)
> path = GetPicasaDatabasePathMac();
> #endif
> 
> return path;
> 
> Then we can either split out the impl into separate ifdef chunks without other
> #elif and duplicate names.

Done.

[vandebo (ex-Chrome), 2013-10-02 15:32:45]

drive by...

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:264:
.AppendASCII("Google").AppendASCII("Picasa2");
nit: wrapping

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:289:
.AppendASCII("Google").AppendASCII("Picasa3");
nit: wrapping

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:289:
.AppendASCII("Google").AppendASCII("Picasa3");
3 vs 2? looks like the only difference between the win and mac test, combine
them with an internal ifdef.

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:40: !key.Valid()) {
nit: previous line

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:104:
.WriteValue(picasa::kPicasaRegistryAppDataKey, path.value().c_str());
nit: wrapping

[tommycli, 2013-10-02 16:44:39]

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:264:
.AppendASCII("Google").AppendASCII("Picasa2");
On 2013/10/02 15:32:46, vandebo wrote:
> nit: wrapping

Done.

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:289:
.AppendASCII("Google").AppendASCII("Picasa3");
On 2013/10/02 15:32:46, vandebo wrote:
> nit: wrapping

Done.

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:289:
.AppendASCII("Google").AppendASCII("Picasa3");
On 2013/10/02 15:32:46, vandebo wrote:
> 3 vs 2? looks like the only difference between the win and mac test, combine
> them with an internal ifdef.

Also GetFakeAppDataPath vs GetFakeLocalAppDataPath.

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:40: !key.Valid()) {
On 2013/10/02 15:32:46, vandebo wrote:
> nit: previous line

Done.

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/23456035/diff/15001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:104:
.WriteValue(picasa::kPicasaRegistryAppDataKey, path.value().c_str());
On 2013/10/02 15:32:46, vandebo wrote:
> nit: wrapping

Done.

[tommycli, 2013-10-03 20:36:06]

gbillock: need OWNER approval/review

[tommycli, 2013-10-04 22:14:17]

On 2013/10/03 20:36:06, tommycli wrote:
> gbillock: need OWNER approval/review

gbillock: ping

[Greg Billock, 2013-10-04 23:52:50]

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:284:
PopulatePicasaTestData(metadata_root);
Should this still set the metadata_root as above to show it overrides it? Or is
the desired behavior that if the root isn't found in the default location we
look in the registry? If so, we should have the code do that.

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:57: return
path.AppendASCII("Google").AppendASCII("Picasa2").AppendASCII(
Would it be worth checking this first before looking in the registry?

[tommycli, 2013-10-05 00:18:44]

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:284:
PopulatePicasaTestData(metadata_root);
On 2013/10/04 23:52:50, Greg Billock wrote:
> Should this still set the metadata_root as above to show it overrides it? Or
is
> the desired behavior that if the root isn't found in the default location we
> look in the registry? If so, we should have the code do that.

Hmm not sure what you're asking. It's the end of the day...

The intent is that the metadata root is set to some location other than the
default, and we write that custom location to the registry.

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right):

https://codereview.chromium.org/23456035/diff/33001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa_finder.cc:57: return
path.AppendASCII("Google").AppendASCII("Picasa2").AppendASCII(
On 2013/10/04 23:52:50, Greg Billock wrote:
> Would it be worth checking this first before looking in the registry?

I believe the desired logic is to check the registry first, then the default
location.

[tommycli, 2013-10-07 21:48:56]

Hey Greg,

I confirmed that this CL replicates the Windows Picasa behavior as is.

It ALWAYS checks the registry entry:

If the registry entry is empty string, it goes to the default location.

If the registry entry is a directory that doesn't exist, it creates a new
database at C:\Users\tommycli\Google. This is strange. In this case Media
Galleries would just show an empty directory. This is somewhat up for debate,
but an explicit failure here is probably desireable.

If the registry entry is an existent directory, it overrides anything in the
default path.

I don't recommend checking the default path as a shortcut to the registry.

[tommycli, 2013-10-08 17:15:58]

On 2013/10/07 21:48:56, tommycli wrote:
> Hey Greg,
> 
> I confirmed that this CL replicates the Windows Picasa behavior as is.
> 
> It ALWAYS checks the registry entry:
> 
> If the registry entry is empty string, it goes to the default location.
> 
> If the registry entry is a directory that doesn't exist, it creates a new
> database at C:\Users\tommycli\Google. This is strange. In this case Media
> Galleries would just show an empty directory. This is somewhat up for debate,
> but an explicit failure here is probably desireable.
> 
> If the registry entry is an existent directory, it overrides anything in the
> default path.
> 
> I don't recommend checking the default path as a shortcut to the registry.

gbillock: ping.

[Greg Billock, 2013-10-08 19:22:37]

lgtm

[commit-bot: I haz the power, 2013-10-08 19:30:16]

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

[commit-bot: I haz the power, 2013-10-09 02:05:59]

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

[commit-bot: I haz the power, 2013-10-09 03:42:23]

Change committed as 227656