Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566713005:
Make the third party wallpaper syncable through different devices. (Closed)
|
Created:
4 years, 11 months ago by xdai1 Modified:
4 years, 11 months ago CC:
arv+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, extensions-reviews_chromium.org, oshima+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the third party wallpaper syncable through different devices.
BUG=551133
Committed: https://crrev.com/f1d980c086da850a4914892b8da992a836ac2a16
Cr-Commit-Position: refs/heads/master@{#369809}
Patch Set 1 : #Patch Set 2 : #
Total comments: 12
Patch Set 3 : Address the comments #Patch Set 4 : Add a TODO #
Messages
Total messages: 37 (17 generated)
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566713005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566713005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
xdai@chromium.org changed reviewers: + bshe@chromium.org
bshe@, could you help to review this CL please? Thanks!
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566713005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566713005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
High level comment: Have you discussed with extension api folks about this design? I worry this approach is a little bit hacky. We are basically saving another extension's data into our extension. Extensions shouldn't obtain other extensions's data I think. https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:421: var filename = Constants.ThirdPartyWallpaperPrefix + new Date().getTime(); We dont need a history of 3rd party wallpapers. Perhap just use the prefix as its name? New wallpaper will override the old one. This way, we dont need to worry about wallpapers taking too much space (especially for extensions that change to a different wallpaper every day). https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:256: thumbnailData, Constants.WallpaperDirNameEnum.THUMBNAIL); this seem to fix a different issue. do you mind land in a different CL?
Thanks for your review! Do you know who I should talk to about this? I didn't realize that I need to discuss with others before implementing this:( https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:421: var filename = Constants.ThirdPartyWallpaperPrefix + new Date().getTime(); On 2016/01/08 19:56:39, bshe wrote: > We dont need a history of 3rd party wallpapers. Perhap just use the prefix as > its name? New wallpaper will override the old one. This way, we dont need to > worry about wallpapers taking too much space (especially for extensions that > change to a different wallpaper every day). No, I'm afraid we can't use the same file name for the third party wallpaper. FileSystem API (http://www.html5rocks.com/en/tutorials/file/filesystem/) doesn't provide a way to replace/override a file. I tried to use the same file name, and when there is a new third party wallpaper, delete the old one first and then write the new one. But it's very very flaky. Sometime it succeeds, sometime it fails. Definitely not a good approach. Actually in my approach here, it does only save one third party wallpaper. The old third party wallpaper is deleted in line 318-327 or line 376-384. Because whenever the wallpaper is changed (either by third party app, or by our built-in app), the chrome.storage.onChanged will be triggered. And we could just check to see if the old wallpaper is a third party wallpaper, and if it is, we just delete it from local & sync file system to free space. I've tested on several devices on various ways and I'm pretty sure that at most one third party wallpaper is stored in the local & sync file system. https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:256: thumbnailData, Constants.WallpaperDirNameEnum.THUMBNAIL); On 2016/01/08 19:56:39, bshe wrote: > this seem to fix a different issue. do you mind land in a different CL? Sure! I will separate it in another CL.
On 2016/01/08 20:17:48, xdai1 wrote: > Thanks for your review! > > Do you know who I should talk to about this? I didn't realize that I need to > discuss with others before implementing this:( I am not too sure who to talk to, but perhaps you can start with rockot@? He might be able to answer the question or point you to the right person. A 1 pager to describe your design or even just email would be sufficient. Maybe it is just me being paranoid. But saving data from other extensions is rare. If we save and sync the data to where only chrome can access, that would be better I guess. But it would be a lot more work. > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:421: var > filename = Constants.ThirdPartyWallpaperPrefix + new Date().getTime(); > On 2016/01/08 19:56:39, bshe wrote: > > We dont need a history of 3rd party wallpapers. Perhap just use the prefix as > > its name? New wallpaper will override the old one. This way, we dont need to > > worry about wallpapers taking too much space (especially for extensions that > > change to a different wallpaper every day). > > No, I'm afraid we can't use the same file name for the third party wallpaper. > FileSystem API (http://www.html5rocks.com/en/tutorials/file/filesystem/) doesn't > provide a way to replace/override a file. I tried to use the same file name, and > when there is a new third party wallpaper, delete the old one first and then > write the new one. But it's very very flaky. Sometime it succeeds, sometime it > fails. Definitely not a good approach. > > Actually in my approach here, it does only save one third party wallpaper. The > old third party wallpaper is deleted in line 318-327 or line 376-384. Because > whenever the wallpaper is changed (either by third party app, or by our built-in > app), the chrome.storage.onChanged will be triggered. And we could just check to > see if the old wallpaper is a third party wallpaper, and if it is, we just > delete it from local & sync file system to free space. I've tested on several > devices on various ways and I'm pretty sure that at most one third party > wallpaper is stored in the local & sync file system. I see. > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:256: > thumbnailData, Constants.WallpaperDirNameEnum.THUMBNAIL); > On 2016/01/08 19:56:39, bshe wrote: > > this seem to fix a different issue. do you mind land in a different CL? > > Sure! I will separate it in another CL.
mostly looks good. just have a couple of questions. https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (left): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:299: } else { // detail.direction == 'local_to_remote' why do you want to remove this? https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:424: WallpaperUtil.storeWallpaperToSyncFS(thumbnailFilename, thumbnail); why do we want thumbnail for 3rd party wallpaper? We aren't suppose to show it in our wallpaper picker, right? https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:81: if (e.name != 'NotFoundError') would be good to add a command why we want to ignore NotFoundError
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566713005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566713005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Biao, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (left): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:299: } else { // detail.direction == 'local_to_remote' On 2016/01/13 18:43:17, bshe wrote: > why do you want to remove this? It's a different issue and it was introduced by me... I was trying to delete the custom wallpaper if it was deleted on one device (see crbug.com/496238). And when I worked on the current 3rd party wallpaper issue, I found that somehow I did something useless (but also harmless). The custom wallpaper did get removed from the local & sync file system if the device is not a powerwashed device (see crbug.com/532278 for more detail) with or without these lines of code. I guess something was messed up when I tested in crbug.com/496238, which made me believe that I fixed that bug. Anyway, Since it's unrelated to this third party wallpaper sync issue, I will separate it into another CL. Reverted the change back. https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:424: WallpaperUtil.storeWallpaperToSyncFS(thumbnailFilename, thumbnail); On 2016/01/13 18:43:17, bshe wrote: > why do we want thumbnail for 3rd party wallpaper? We aren't suppose to show it > in our wallpaper picker, right? You're right. Ideally we should not show the third party wallpaper in the wallpaper picker. Instead we should show the CurrentWallpaperSetByMessage: "The current wallpaper is set by ***, you may need to uninstall *** before selecting a different wallpaper." However, in this CL it just treats the third party wallpaper as a regular custom wallpaper. In the synced device (not the device that set the third party wallpaper), the synced third party wallpaper will show in the built-in wallpaper picker (which is wrong). The reason is we need to sync the third-party app name in order to do that and probably introduce new UMA for the third party wallpaper and it might require non-trival change, which I don't want to mess up with this CL. I plan to address this in a following up CL. Is that OK? https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:81: if (e.name != 'NotFoundError') On 2016/01/13 18:43:17, bshe wrote: > would be good to add a command why we want to ignore NotFoundError Done.
lgtm with a TODO added https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (left): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:299: } else { // detail.direction == 'local_to_remote' On 2016/01/14 23:03:51, xdai1 wrote: > On 2016/01/13 18:43:17, bshe wrote: > > why do you want to remove this? > > It's a different issue and it was introduced by me... I was trying to delete the > custom wallpaper if it was deleted on one device (see crbug.com/496238). And > when I worked on the current 3rd party wallpaper issue, I found that somehow I > did something useless (but also harmless). The custom wallpaper did get removed > from the local & sync file system if the device is not a powerwashed device (see > crbug.com/532278 for more detail) with or without these lines of code. I guess > something was messed up when I tested in crbug.com/496238, which made me believe > that I fixed that bug. > > Anyway, Since it's unrelated to this third party wallpaper sync issue, I will > separate it into another CL. Reverted the change back. > Acknowledged. https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:424: WallpaperUtil.storeWallpaperToSyncFS(thumbnailFilename, thumbnail); On 2016/01/14 23:03:51, xdai1 wrote: > On 2016/01/13 18:43:17, bshe wrote: > > why do we want thumbnail for 3rd party wallpaper? We aren't suppose to show > it > > in our wallpaper picker, right? > > You're right. Ideally we should not show the third party wallpaper in the > wallpaper picker. Instead we should show the CurrentWallpaperSetByMessage: "The > current wallpaper is set by ***, you may need to uninstall *** before selecting > a different wallpaper." > > However, in this CL it just treats the third party wallpaper as a regular custom > wallpaper. In the synced device (not the device that set the third party > wallpaper), the synced third party wallpaper will show in the built-in wallpaper > picker (which is wrong). The reason is we need to sync the third-party app name > in order to do that and probably introduce new UMA for the third party wallpaper > and it might require non-trival change, which I don't want to mess up with this > CL. I plan to address this in a following up CL. Is that OK? I might not understand you correctly, But the 3rd party app name should be synced as a part of user's profile if I remember correctly. Was it not get synced somehow and we need a new way to sync it? But anyway, I am fine with leave it in a following up CL. But please add a TODO so that we dont forget. Thanks.
Description was changed from ========== Make the third party wallpaper syncable through different devices. BUG=551133 ========== to ========== Make the third party wallpaper syncable through different devices. BUG=551133 ==========
xdai@chromium.org changed reviewers: + rockot@chromium.org
On 2016/01/15 15:52:00, bshe wrote: > lgtm with a TODO added > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (left): > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:299: } else > { // detail.direction == 'local_to_remote' > On 2016/01/14 23:03:51, xdai1 wrote: > > On 2016/01/13 18:43:17, bshe wrote: > > > why do you want to remove this? > > > > It's a different issue and it was introduced by me... I was trying to delete > the > > custom wallpaper if it was deleted on one device (see crbug.com/496238). And > > when I worked on the current 3rd party wallpaper issue, I found that somehow I > > did something useless (but also harmless). The custom wallpaper did get > removed > > from the local & sync file system if the device is not a powerwashed device > (see > > crbug.com/532278 for more detail) with or without these lines of code. I guess > > something was messed up when I tested in crbug.com/496238, which made me > believe > > that I fixed that bug. > > > > Anyway, Since it's unrelated to this third party wallpaper sync issue, I will > > separate it into another CL. Reverted the change back. > > > > Acknowledged. > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/1566713005/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:424: > WallpaperUtil.storeWallpaperToSyncFS(thumbnailFilename, thumbnail); > On 2016/01/14 23:03:51, xdai1 wrote: > > On 2016/01/13 18:43:17, bshe wrote: > > > why do we want thumbnail for 3rd party wallpaper? We aren't suppose to show > > it > > > in our wallpaper picker, right? > > > > You're right. Ideally we should not show the third party wallpaper in the > > wallpaper picker. Instead we should show the CurrentWallpaperSetByMessage: > "The > > current wallpaper is set by ***, you may need to uninstall *** before > selecting > > a different wallpaper." > > > > However, in this CL it just treats the third party wallpaper as a regular > custom > > wallpaper. In the synced device (not the device that set the third party > > wallpaper), the synced third party wallpaper will show in the built-in > wallpaper > > picker (which is wrong). The reason is we need to sync the third-party app > name > > in order to do that and probably introduce new UMA for the third party > wallpaper > > and it might require non-trival change, which I don't want to mess up with > this > > CL. I plan to address this in a following up CL. Is that OK? > > I might not understand you correctly, But the 3rd party app name should be > synced > as a part of user's profile if I remember correctly. Was it not get synced > somehow > and we need a new way to sync it? > But anyway, I am fine with leave it in a following up CL. But please add a TODO > so > that we dont forget. Thanks. No, currently the third party app name doesn't get synced. Added a TODO. I'll address it in a following up CL. rockot@, could you help review the file: chrome/common/extensions/api/wallpaper_private.json please? Thanks for your help.
lgtm
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566713005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566713005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org Link to the patchset: https://codereview.chromium.org/1566713005/#ps100001 (title: "Add a TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566713005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566713005/100001
Message was sent while issue was closed.
Description was changed from ========== Make the third party wallpaper syncable through different devices. BUG=551133 ========== to ========== Make the third party wallpaper syncable through different devices. BUG=551133 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make the third party wallpaper syncable through different devices. BUG=551133 ========== to ========== Make the third party wallpaper syncable through different devices. BUG=551133 Committed: https://crrev.com/f1d980c086da850a4914892b8da992a836ac2a16 Cr-Commit-Position: refs/heads/master@{#369809} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f1d980c086da850a4914892b8da992a836ac2a16 Cr-Commit-Position: refs/heads/master@{#369809} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
