Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23513059:
Media Galleries API Picasa: End-to-end browsertest. (Closed)
|
Created:
7 years, 3 months ago by tommycli Modified:
7 years, 2 months ago Reviewers:
vandebo (ex-Chrome) CC:
chromium-reviews, extensions-reviews_chromium.org, Lei Zhang, tzik+watch_chromium.org, Greg Billock, chromium-apps-reviews_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMedia Galleries API Picasa: End-to-end browsertest.
Adds an end-to-end browser test to Picasa.
Dependent on this fix going through:
https://codereview.chromium.org/24269007/
BUG=151701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226402
Patch Set 1 #Patch Set 2 : self review #
Total comments: 14
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 5
Patch Set 13 : #Patch Set 14 : #Messages
Total messages: 29 (0 generated)
vandebo: Ready for a review. Won't pass trybots without : https://chromiumcodereview.appspot.com/23442026/, but it works on my machine. https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... chrome/browser/media_galleries/media_galleries_preferences.cc:344: picasa::PicasaFinder::FindPicasaDatabase( Currently requires it to be 'on' for browser test to work. There's probably a way to hack it so that this doesn't have to be ON, but we should talk about this.
vandebo: Oh, and let me know if you're swamped and want me to distribute this review to greg or lei.
https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/exte... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/exte... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest(Profile* profile) { I think the js equivalent (picking out the picasa gallery) would be easy and easier to maintain. https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/exte... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:217: picasa::PicasaFinder::GetPicasaDatabasePath(); Would it make sense to just call the regular PicasaFinder method here? https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... File chrome/browser/media_galleries/fileapi/picasa_finder.h (right): https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... chrome/browser/media_galleries/fileapi/picasa_finder.h:19: base::FilePath GetPicasaDatabasePath(); This method feels a bit out of place: no comments, why would I use the more complicated Find method below when this is available... https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/browser/medi... chrome/browser/media_galleries/media_galleries_preferences.cc:344: picasa::PicasaFinder::FindPicasaDatabase( On 2013/09/16 19:35:45, tommycli wrote: > Currently requires it to be 'on' for browser test to work. There's probably a > way to hack it so that this doesn't have to be ON, but we should talk about > this. This seems fine, just don't land it before the branch cut. https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/test/data/ex... File chrome/test/data/extensions/api_test/media_galleries/picasa/test.js (right): https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/test/data/ex... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:63: function(fileEntry) { fileEntry.file(verifyFile, chrome.test.fail) }, nit: name this function instead of making it inline. https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/test/data/ex... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:102: testAlbumsListing(); Instead of chaining between the tests, can you list them all in the runTests array? https://chromiumcodereview.appspot.com/23513059/diff/3001/chrome/test/data/ex... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:168: function testAlbum2Listing() { repeated function?
vandebo: Here you go. Not urgent of course. https://codereview.chromium.org/23513059/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest(Profile* profile) { On 2013/09/17 21:18:06, vandebo wrote: > I think the js equivalent (picking out the picasa gallery) would be easy and > easier to maintain. Done. https://codereview.chromium.org/23513059/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:217: picasa::PicasaFinder::GetPicasaDatabasePath(); On 2013/09/17 21:18:06, vandebo wrote: > Would it make sense to just call the regular PicasaFinder method here? Changed some stuff here. https://codereview.chromium.org/23513059/diff/3001/chrome/browser/media_galle... File chrome/browser/media_galleries/fileapi/picasa_finder.h (right): https://codereview.chromium.org/23513059/diff/3001/chrome/browser/media_galle... chrome/browser/media_galleries/fileapi/picasa_finder.h:19: base::FilePath GetPicasaDatabasePath(); On 2013/09/17 21:18:06, vandebo wrote: > This method feels a bit out of place: no comments, why would I use the more > complicated Find method below when this is available... Done. https://codereview.chromium.org/23513059/diff/3001/chrome/test/data/extension... File chrome/test/data/extensions/api_test/media_galleries/picasa/test.js (right): https://codereview.chromium.org/23513059/diff/3001/chrome/test/data/extension... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:63: function(fileEntry) { fileEntry.file(verifyFile, chrome.test.fail) }, On 2013/09/17 21:18:06, vandebo wrote: > nit: name this function instead of making it inline. Done. https://codereview.chromium.org/23513059/diff/3001/chrome/test/data/extension... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:102: testAlbumsListing(); On 2013/09/17 21:18:06, vandebo wrote: > Instead of chaining between the tests, can you list them all in the runTests > array? Done. https://codereview.chromium.org/23513059/diff/3001/chrome/test/data/extension... chrome/test/data/extensions/api_test/media_galleries/picasa/test.js:168: function testAlbum2Listing() { On 2013/09/17 21:18:06, vandebo wrote: > repeated function? Done. https://codereview.chromium.org/23513059/diff/17001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:119: content::RunAllPendingInMessageLoop(); The Picasa gallery is not found unless I have this statement here - not sure why...
https://codereview.chromium.org/23513059/diff/17001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:119: content::RunAllPendingInMessageLoop(); On 2013/09/18 15:25:37, tommycli wrote: > The Picasa gallery is not found unless I have this statement here - not sure > why... Probably because the picasa finder runs async from preferences init. I wonder if you need the following line, won't picasa finder do the same thing? https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( It's not clear to me that you need this function at all - maybe you do because there's a race between finding the picasa gallery and getting it into preferences and the first get media file system call. If so, we should probably just fix that instead of working around it in tests. https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/picasa_finder.cc:38: return path.AppendASCII("Google").AppendASCII("Picasa2").AppendASCII( s/return/path =/ https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/picasa_finder.cc:57: namespace PicasaFinder { remove https://codereview.chromium.org/23513059/diff/35001/chrome/common/media_galle... File chrome/common/media_galleries/picasa_test_util.h (right): https://codereview.chromium.org/23513059/diff/35001/chrome/common/media_galle... chrome/common/media_galleries/picasa_test_util.h:35: bool WriteJPEGHeader(const base::FilePath& path); Maybe this should go into chrome/browser/media_galleries/media_galleries_test_util
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/18 15:49:35, vandebo wrote: > It's not clear to me that you need this function at all - maybe you do because > there's a race between finding the picasa gallery and getting it into > preferences and the first get media file system call. If so, we should probably > just fix that instead of working around it in tests. Done. Race fixed in a pending CL: https://codereview.chromium.org/24269007/ Still need function to inject DeviceID, unless I make a ScopedPicasaDeviceIDOverride. I tried that, and the result was more code than this approach. https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/picasa_finder.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/picasa_finder.cc:38: return path.AppendASCII("Google").AppendASCII("Picasa2").AppendASCII( On 2013/09/18 15:49:35, vandebo wrote: > s/return/path =/ Done. https://codereview.chromium.org/23513059/diff/35001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/picasa_finder.cc:57: namespace PicasaFinder { On 2013/09/18 15:49:35, vandebo wrote: > remove Done. https://codereview.chromium.org/23513059/diff/35001/chrome/common/media_galle... File chrome/common/media_galleries/picasa_test_util.h (right): https://codereview.chromium.org/23513059/diff/35001/chrome/common/media_galle... chrome/common/media_galleries/picasa_test_util.h:35: bool WriteJPEGHeader(const base::FilePath& path); On 2013/09/18 15:49:35, vandebo wrote: > Maybe this should go into > chrome/browser/media_galleries/media_galleries_test_util Done.
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/19 22:58:45, tommycli wrote: > On 2013/09/18 15:49:35, vandebo wrote: > > It's not clear to me that you need this function at all - maybe you do because > > there's a race between finding the picasa gallery and getting it into > > preferences and the first get media file system call. If so, we should > probably > > just fix that instead of working around it in tests. > > Done. Race fixed in a pending CL: > https://codereview.chromium.org/24269007/ > > Still need function to inject DeviceID, unless I make a > ScopedPicasaDeviceIDOverride. I tried that, and the result was more code than > this approach. Why would you need an IDOverride? Can't you just put your fake data in the directory that EnsureMediaDirectoriesExists creates? https://codereview.chromium.org/23513059/diff/47001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/23513059/diff/47001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:500: DLOG(INFO) << "OnFinderDeviceID: " << device_id; remove
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/23 16:01:31, vandebo wrote: > On 2013/09/19 22:58:45, tommycli wrote: > > On 2013/09/18 15:49:35, vandebo wrote: > > > It's not clear to me that you need this function at all - maybe you do > because > > > there's a race between finding the picasa gallery and getting it into > > > preferences and the first get media file system call. If so, we should > > probably > > > just fix that instead of working around it in tests. > > > > Done. Race fixed in a pending CL: > > https://codereview.chromium.org/24269007/ > > > > Still need function to inject DeviceID, unless I make a > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more code than > > this approach. > > Why would you need an IDOverride? Can't you just put your fake data in the > directory that EnsureMediaDirectoriesExists creates? Sry, I didn't give you enough info. In the CL after this, https://codereview.chromium.org/23456035/ , I make the Finder look in the registry for the custom Picasa database location. If it finds something in there, then overriding APP_LOCAL_DATA has no effect. I tried adding a scoped override to the Finder behavior, but it was more complicated than just overwriting the Device ID. https://codereview.chromium.org/23513059/diff/47001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/23513059/diff/47001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:500: DLOG(INFO) << "OnFinderDeviceID: " << device_id; On 2013/09/23 16:01:31, vandebo wrote: > remove Done.
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/24 00:42:36, tommycli wrote: > On 2013/09/23 16:01:31, vandebo wrote: > > On 2013/09/19 22:58:45, tommycli wrote: > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > It's not clear to me that you need this function at all - maybe you do > > because > > > > there's a race between finding the picasa gallery and getting it into > > > > preferences and the first get media file system call. If so, we should > > > probably > > > > just fix that instead of working around it in tests. > > > > > > Done. Race fixed in a pending CL: > > > https://codereview.chromium.org/24269007/ > > > > > > Still need function to inject DeviceID, unless I make a > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more code > than > > > this approach. > > > > Why would you need an IDOverride? Can't you just put your fake data in the > > directory that EnsureMediaDirectoriesExists creates? > > Sry, I didn't give you enough info. In the CL after this, > https://codereview.chromium.org/23456035/ , I make the Finder look in the > registry for the custom Picasa database location. If it finds something in > there, then overriding APP_LOCAL_DATA has no effect. > > I tried adding a scoped override to the Finder behavior, but it was more > complicated than just overwriting the Device ID. Will RegistryOverrideManager do what you need?
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/26 21:58:52, vandebo wrote: > On 2013/09/24 00:42:36, tommycli wrote: > > On 2013/09/23 16:01:31, vandebo wrote: > > > On 2013/09/19 22:58:45, tommycli wrote: > > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > > It's not clear to me that you need this function at all - maybe you do > > > because > > > > > there's a race between finding the picasa gallery and getting it into > > > > > preferences and the first get media file system call. If so, we should > > > > probably > > > > > just fix that instead of working around it in tests. > > > > > > > > Done. Race fixed in a pending CL: > > > > https://codereview.chromium.org/24269007/ > > > > > > > > Still need function to inject DeviceID, unless I make a > > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more code > > than > > > > this approach. > > > > > > Why would you need an IDOverride? Can't you just put your fake data in the > > > directory that EnsureMediaDirectoriesExists creates? > > > > Sry, I didn't give you enough info. In the CL after this, > > https://codereview.chromium.org/23456035/ , I make the Finder look in the > > registry for the custom Picasa database location. If it finds something in > > there, then overriding APP_LOCAL_DATA has no effect. > > > > I tried adding a scoped override to the Finder behavior, but it was more > > complicated than just overwriting the Device ID. > > Will RegistryOverrideManager do what you need? It would not work for Mac. I could also override the Mac preferences, but at that point, I might as well just add a scoped override to the Finder behavior...
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/26 22:02:15, tommycli wrote: > On 2013/09/26 21:58:52, vandebo wrote: > > On 2013/09/24 00:42:36, tommycli wrote: > > > On 2013/09/23 16:01:31, vandebo wrote: > > > > On 2013/09/19 22:58:45, tommycli wrote: > > > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > > > It's not clear to me that you need this function at all - maybe you do > > > > because > > > > > > there's a race between finding the picasa gallery and getting it into > > > > > > preferences and the first get media file system call. If so, we > should > > > > > probably > > > > > > just fix that instead of working around it in tests. > > > > > > > > > > Done. Race fixed in a pending CL: > > > > > https://codereview.chromium.org/24269007/ > > > > > > > > > > Still need function to inject DeviceID, unless I make a > > > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more code > > > than > > > > > this approach. > > > > > > > > Why would you need an IDOverride? Can't you just put your fake data in > the > > > > directory that EnsureMediaDirectoriesExists creates? > > > > > > Sry, I didn't give you enough info. In the CL after this, > > > https://codereview.chromium.org/23456035/ , I make the Finder look in the > > > registry for the custom Picasa database location. If it finds something in > > > there, then overriding APP_LOCAL_DATA has no effect. > > > > > > I tried adding a scoped override to the Finder behavior, but it was more > > > complicated than just overwriting the Device ID. > > > > Will RegistryOverrideManager do what you need? > > It would not work for Mac. I could also override the Mac preferences, but at > that point, I might as well just add a scoped override to the Finder behavior... I've also been working on this: https://codereview.chromium.org/19617005/
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/26 22:11:59, vandebo wrote: > On 2013/09/26 22:02:15, tommycli wrote: > > On 2013/09/26 21:58:52, vandebo wrote: > > > On 2013/09/24 00:42:36, tommycli wrote: > > > > On 2013/09/23 16:01:31, vandebo wrote: > > > > > On 2013/09/19 22:58:45, tommycli wrote: > > > > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > > > > It's not clear to me that you need this function at all - maybe you > do > > > > > because > > > > > > > there's a race between finding the picasa gallery and getting it > into > > > > > > > preferences and the first get media file system call. If so, we > > should > > > > > > probably > > > > > > > just fix that instead of working around it in tests. > > > > > > > > > > > > Done. Race fixed in a pending CL: > > > > > > https://codereview.chromium.org/24269007/ > > > > > > > > > > > > Still need function to inject DeviceID, unless I make a > > > > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more > code > > > > than > > > > > > this approach. > > > > > > > > > > Why would you need an IDOverride? Can't you just put your fake data in > > the > > > > > directory that EnsureMediaDirectoriesExists creates? > > > > > > > > Sry, I didn't give you enough info. In the CL after this, > > > > https://codereview.chromium.org/23456035/ , I make the Finder look in the > > > > registry for the custom Picasa database location. If it finds something in > > > > there, then overriding APP_LOCAL_DATA has no effect. > > > > > > > > I tried adding a scoped override to the Finder behavior, but it was more > > > > complicated than just overwriting the Device ID. > > > > > > Will RegistryOverrideManager do what you need? > > > > It would not work for Mac. I could also override the Mac preferences, but at > > that point, I might as well just add a scoped override to the Finder > behavior... > > I've also been working on this: https://codereview.chromium.org/19617005/ Yeah if it was just either mac prefs or win registry, it would be clean to do the override that way. Seeing as how it's both OSes, the cleanest thing would probably be to just override the finder behavior or inject in a fake device id. The only downside is that it makes the end-to-end test no longer test the MacPref/WinRegistry portion...
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/26 22:22:39, tommycli wrote: > On 2013/09/26 22:11:59, vandebo wrote: > > On 2013/09/26 22:02:15, tommycli wrote: > > > On 2013/09/26 21:58:52, vandebo wrote: > > > > On 2013/09/24 00:42:36, tommycli wrote: > > > > > On 2013/09/23 16:01:31, vandebo wrote: > > > > > > On 2013/09/19 22:58:45, tommycli wrote: > > > > > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > > > > > It's not clear to me that you need this function at all - maybe > you > > do > > > > > > because > > > > > > > > there's a race between finding the picasa gallery and getting it > > into > > > > > > > > preferences and the first get media file system call. If so, we > > > should > > > > > > > probably > > > > > > > > just fix that instead of working around it in tests. > > > > > > > > > > > > > > Done. Race fixed in a pending CL: > > > > > > > https://codereview.chromium.org/24269007/ > > > > > > > > > > > > > > Still need function to inject DeviceID, unless I make a > > > > > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was more > > code > > > > > than > > > > > > > this approach. > > > > > > > > > > > > Why would you need an IDOverride? Can't you just put your fake data > in > > > the > > > > > > directory that EnsureMediaDirectoriesExists creates? > > > > > > > > > > Sry, I didn't give you enough info. In the CL after this, > > > > > https://codereview.chromium.org/23456035/ , I make the Finder look in > the > > > > > registry for the custom Picasa database location. If it finds something > in > > > > > there, then overriding APP_LOCAL_DATA has no effect. > > > > > > > > > > I tried adding a scoped override to the Finder behavior, but it was more > > > > > complicated than just overwriting the Device ID. > > > > > > > > Will RegistryOverrideManager do what you need? > > > > > > It would not work for Mac. I could also override the Mac preferences, but at > > > that point, I might as well just add a scoped override to the Finder > > behavior... > > > > I've also been working on this: https://codereview.chromium.org/19617005/ > > Yeah if it was just either mac prefs or win registry, it would be clean to do > the override that way. > > Seeing as how it's both OSes, the cleanest thing would probably be to just > override the finder behavior or inject in a fake device id. The only downside is > that it makes the end-to-end test no longer test the MacPref/WinRegistry > portion... But we don't want test to behave differently when they run on system that have had picasa/itunes/what ever installed. So there's a strong preference to override everything in the EnsureMediDirectoriesExists class.
https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc (right): https://codereview.chromium.org/23513059/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:108: void SetMediaGalleriesPreferencesForPicasaTest( On 2013/09/26 22:25:10, vandebo wrote: > On 2013/09/26 22:22:39, tommycli wrote: > > On 2013/09/26 22:11:59, vandebo wrote: > > > On 2013/09/26 22:02:15, tommycli wrote: > > > > On 2013/09/26 21:58:52, vandebo wrote: > > > > > On 2013/09/24 00:42:36, tommycli wrote: > > > > > > On 2013/09/23 16:01:31, vandebo wrote: > > > > > > > On 2013/09/19 22:58:45, tommycli wrote: > > > > > > > > On 2013/09/18 15:49:35, vandebo wrote: > > > > > > > > > It's not clear to me that you need this function at all - maybe > > you > > > do > > > > > > > because > > > > > > > > > there's a race between finding the picasa gallery and getting it > > > into > > > > > > > > > preferences and the first get media file system call. If so, we > > > > should > > > > > > > > probably > > > > > > > > > just fix that instead of working around it in tests. > > > > > > > > > > > > > > > > Done. Race fixed in a pending CL: > > > > > > > > https://codereview.chromium.org/24269007/ > > > > > > > > > > > > > > > > Still need function to inject DeviceID, unless I make a > > > > > > > > ScopedPicasaDeviceIDOverride. I tried that, and the result was > more > > > code > > > > > > than > > > > > > > > this approach. > > > > > > > > > > > > > > Why would you need an IDOverride? Can't you just put your fake data > > in > > > > the > > > > > > > directory that EnsureMediaDirectoriesExists creates? > > > > > > > > > > > > Sry, I didn't give you enough info. In the CL after this, > > > > > > https://codereview.chromium.org/23456035/ , I make the Finder look in > > the > > > > > > registry for the custom Picasa database location. If it finds > something > > in > > > > > > there, then overriding APP_LOCAL_DATA has no effect. > > > > > > > > > > > > I tried adding a scoped override to the Finder behavior, but it was > more > > > > > > complicated than just overwriting the Device ID. > > > > > > > > > > Will RegistryOverrideManager do what you need? > > > > > > > > It would not work for Mac. I could also override the Mac preferences, but > at > > > > that point, I might as well just add a scoped override to the Finder > > > behavior... > > > > > > I've also been working on this: https://codereview.chromium.org/19617005/ > > > > Yeah if it was just either mac prefs or win registry, it would be clean to do > > the override that way. > > > > Seeing as how it's both OSes, the cleanest thing would probably be to just > > override the finder behavior or inject in a fake device id. The only downside > is > > that it makes the end-to-end test no longer test the MacPref/WinRegistry > > portion... > > But we don't want test to behave differently when they run on system that have > had picasa/itunes/what ever installed. So there's a strong preference to > override everything in the EnsureMediDirectoriesExists class. I think the current scheme (injecting in a fake device id to a temp dir) behaves the same on systems regardless of install history. There will be momentarily a reference to the 'real' installed database in MediaGalleriesPreferences, but that should be wiped out before the DataProvider gets spawned. If we override finder behavior, it will definitely ignore previously installed instances because the Finder will not attempt to find the 'real' installed DB and instead just return the fake one.
vandebo: Restored it to work with just the default directory. Any registry/pref overrides will come later when that functionality is enabled (it's forthcoming soon).
LGTM modulo comment. https://codereview.chromium.org/23513059/diff/88001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.h (right): https://codereview.chromium.org/23513059/diff/88001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.h:235: friend class MediaGalleriesPlatformAppBrowserTest; old?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/23513059/93001
I also changed the patch slightly between the one you approved and the one that's in CQ now. Uncheck box if any objections. https://codereview.chromium.org/23513059/diff/88001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.h (right): https://codereview.chromium.org/23513059/diff/88001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.h:235: friend class MediaGalleriesPlatformAppBrowserTest; On 2013/09/30 16:41:33, vandebo wrote: > old? Done.
https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:420: AddDefaultGalleriesIfFreshProfile(); This won't work in this location. You may have added itunes and picasa by this point so the detection for fresh profile will always say no. https://codereview.chromium.org/23513059/diff/93001/chrome/chrome_tests_unit.... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/23513059/diff/93001/chrome/chrome_tests_unit.... chrome/chrome_tests_unit.gypi:504: 'win_use_external_manifest': 1, Don't commit this.
https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:420: AddDefaultGalleriesIfFreshProfile(); On 2013/10/01 16:41:10, vandebo wrote: > This won't work in this location. You may have added itunes and picasa by this > point so the detection for fresh profile will always say no. You're right. Fix in this CL: https://codereview.chromium.org/25539003/ https://codereview.chromium.org/23513059/diff/93001/chrome/chrome_tests_unit.... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/23513059/diff/93001/chrome/chrome_tests_unit.... chrome/chrome_tests_unit.gypi:504: 'win_use_external_manifest': 1, On 2013/10/01 16:41:10, vandebo wrote: > Don't commit this. Done.
https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... File chrome/browser/media_galleries/media_galleries_preferences.cc (right): https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... chrome/browser/media_galleries/media_galleries_preferences.cc:420: AddDefaultGalleriesIfFreshProfile(); On 2013/10/01 17:11:42, tommycli wrote: > On 2013/10/01 16:41:10, vandebo wrote: > > This won't work in this location. You may have added itunes and picasa by > this > > point so the detection for fresh profile will always say no. > > You're right. Fix in this CL: > https://codereview.chromium.org/25539003/ Don't include in this CL.
On 2013/10/01 17:35:31, vandebo wrote: > https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... > File chrome/browser/media_galleries/media_galleries_preferences.cc (right): > > https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... > chrome/browser/media_galleries/media_galleries_preferences.cc:420: > AddDefaultGalleriesIfFreshProfile(); > On 2013/10/01 17:11:42, tommycli wrote: > > On 2013/10/01 16:41:10, vandebo wrote: > > > This won't work in this location. You may have added itunes and picasa by > > this > > > point so the detection for fresh profile will always say no. > > > > You're right. Fix in this CL: > > https://codereview.chromium.org/25539003/ > > Don't include in this CL. Any objections if i send to CQ? Fix is in other CL
On 2013/10/01 20:20:12, tommycli wrote: > On 2013/10/01 17:35:31, vandebo wrote: > > > https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... > > File chrome/browser/media_galleries/media_galleries_preferences.cc (right): > > > > > https://codereview.chromium.org/23513059/diff/93001/chrome/browser/media_gall... > > chrome/browser/media_galleries/media_galleries_preferences.cc:420: > > AddDefaultGalleriesIfFreshProfile(); > > On 2013/10/01 17:11:42, tommycli wrote: > > > On 2013/10/01 16:41:10, vandebo wrote: > > > > This won't work in this location. You may have added itunes and picasa by > > > this > > > > point so the detection for fresh profile will always say no. > > > > > > You're right. Fix in this CL: > > > https://codereview.chromium.org/25539003/ > > > > Don't include in this CL. > > Any objections if i send to CQ? Fix is in other CL It looks like your branch is on top of the fix branch. It looks like it might work out ok, but you may end up with a failed to apply patch... LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/23513059/134001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/23513059/134001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/23513059/134001
Message was sent while issue was closed.
Change committed as 226402 |
