Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1631923004:
Sync 3rd party wallpaper app name (Closed)
|
Created:
4 years, 11 months ago by xdai1 Modified:
4 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSync 3rd party wallpaper app name
BUG=551133
Committed: https://crrev.com/6a396736e33d3447575473a7cca6149ee1ba9944
Cr-Commit-Position: refs/heads/master@{#375047}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : Address tbarzic@'s comments. #
Total comments: 3
Patch Set 4 : #
Total comments: 12
Patch Set 5 : Address tbarzic@'s comments. #
Total comments: 8
Patch Set 6 : Address tbarzic@'s comments. #
Total comments: 16
Patch Set 7 : Address tbarzic@'s comments. #
Total comments: 7
Patch Set 8 : Addess tbarzic@'s comments. #
Total comments: 2
Patch Set 9 : Address tbarzic@'s comment. #
Messages
Total messages: 31 (7 generated)
Patchset #1 (id:1) has been deleted
xdai@chromium.org changed reviewers: + erg@chromium.org, tbarzic@chromium.org
erg@, tbarzic@, could you help to review this CL please? Thanks! erg@: chrome/browser/profiles/profile.cc tbarzic@: chrome/browser/chromeos/extensions/wallpaper_private_api.cc
profiles lgtm
https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:643: // to set the pref according to different types of wallpapers. I assume this prefs should be set by third-party app using wallpaper API. Can you clarify that in the comment (set perf according to type does not give too much information :) ) https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:644: if (params->file_name.find(kThirdPartyWallpaperPrefix) == std::string::npos) { Can this logic be moved to wallpaper app? E.g. add an optional bool parameter to params (like clearAppName, or isThirdPartyWallpaper), or even appName to set?
tbarzic@, please take another look, thanks for your review! https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:643: // to set the pref according to different types of wallpapers. On 2016/01/26 01:20:44, tbarzic wrote: > I assume this prefs should be set by third-party app using wallpaper API. Can > you clarify that in the comment (set perf according to type does not give too > much information :) ) Currently the 3rd party wallpaper syncing scenario works like this: suppose we have two device A and B. On device A, a third party wallpaper was set by a third party wallpaper app by calling wallpaper API, and yes during this call this pref was set. The corresponding third party wallpaper was then saved into the built-in wallpaper picker app's sandboxed sync filesystem. Then on device B, the built-in wallpaper picker app listened to the filesystem change. If there was any change, chrome.storage.onChanged was triggered to set device B's wallpaper from sync filesystem by calling wallpaper PRIVATE API (more specific, chrome.wallpaperPRIVATE.setCustomWallpaper() API, as seen in this file). chrome.wallpaperPRIVATE.setCustomWallpaper() API can also be called when a user manually set up a custom wallpaper. The naming for custom wallpapers and third party wallpapers are different, and that's the way we tell these two different types of wallpapers apart. https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:644: if (params->file_name.find(kThirdPartyWallpaperPrefix) == std::string::npos) { On 2016/01/26 01:20:44, tbarzic wrote: > Can this logic be moved to wallpaper app? > E.g. add an optional bool parameter to params (like clearAppName, or > isThirdPartyWallpaper), or even appName to set? Yes, it can be moved to wallpaper app by introducing another parameter like isThirdPartyWallpaper, but it seems a little redundant since we still use the file name to check if the current wallpaper is a custom wallpaper or a third party wallpaper... Since the file name has all the information we need, the additional parameter seems a little unnecessary? WDYT?
https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:643: // to set the pref according to different types of wallpapers. On 2016/01/26 17:57:48, xdai1 wrote: > On 2016/01/26 01:20:44, tbarzic wrote: > > I assume this prefs should be set by third-party app using wallpaper API. Can > > you clarify that in the comment (set perf according to type does not give too > > much information :) ) > > Currently the 3rd party wallpaper syncing scenario works like this: suppose we > have two device A and B. On device A, a third party wallpaper was set by a third > party wallpaper app by calling wallpaper API, and yes during this call this pref > was set. The corresponding third party wallpaper was then saved into the > built-in wallpaper picker app's sandboxed sync filesystem. > Then on device B, the built-in wallpaper picker app listened to the filesystem > change. If there was any change, chrome.storage.onChanged was triggered to set > device B's wallpaper from sync filesystem by calling wallpaper PRIVATE API (more > specific, chrome.wallpaperPRIVATE.setCustomWallpaper() API, as seen in this > file). > > chrome.wallpaperPRIVATE.setCustomWallpaper() API can also be called when a user > manually set up a custom wallpaper. The naming for custom wallpapers and third > party wallpapers are different, and that's the way we tell these two different > types of wallpapers apart. Yep, I assumed as much. What I'd like is for the comment here to say that app name should not be overwritten here because it already has the correct value, which was set using public wallpaper API (and synced using preferences). Though, I wonder if syncing wallpaper information using two mechanisms is the way to go here (sync storage and preferences). Can we add app name to params and update profile pref based on that (and leave the preference type as it was)? Otherwise, there will be a window in which info stored in the preferences won't be consistent with the actual state of the wallpaper manager. https://codereview.chromium.org/1631923004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:644: if (params->file_name.find(kThirdPartyWallpaperPrefix) == std::string::npos) { On 2016/01/26 17:57:48, xdai1 wrote: > On 2016/01/26 01:20:44, tbarzic wrote: > > Can this logic be moved to wallpaper app? > > E.g. add an optional bool parameter to params (like clearAppName, or > > isThirdPartyWallpaper), or even appName to set? > > Yes, it can be moved to wallpaper app by introducing another parameter like > isThirdPartyWallpaper, but it seems a little redundant since we still use the > file name to check if the current wallpaper is a custom wallpaper or a third > party wallpaper... Since the file name has all the information we need, the > additional parameter seems a little unnecessary? WDYT? Yes, but I would prefer not to leak file name format from wallpaper manager app into the API. Adding a flag for clearing app name, or app name to parameters (see the other comment) makes the API cleaner. The fact that the app name can be derived from the file name is implementation detail of the app, and the API should not depend on that.
tbarzic@, I've addressed your comments. Please take another look, thanks for your review. In the patch 3: 1) kCurrentWallpaperAppName preference is removed (thus chrome.wallpaperPrivate.setCustomWallpaper API proto remains the same as before) 2) Use the same syncing mechanisms as the third party wallpaper to sync the app name to avoid the possible time window
I'm not very familiar with wallpaper_manager, so you might also want to include someone who knows that code better. https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:266: Constants.ThirdPartyWallpaperPrefix) != -1) { nit: add 4 indent https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:304: }); nit: indent https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/wallpaper_private.json (right): https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... chrome/common/extensions/api/wallpaper_private.json:281: "name": "appname", nit: capital N https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:265: if (wallpaperFilename.indexOf( do you really need to branch here? Can't you just set the app name from the sync fs (it should already be '' for 1st party app)? https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:277: WallpaperUtil.storeWallpaperToLocalFS(wallpaperFilename, the wallpapers should still be stored to local fs if they came from the component wallpaper app, right? https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:311: WallpaperUtil.saveToLocalStorage(Constants.AccessLocalThirdPartyAppName, '', WallpaperUtil.saveThirdPartyAppName(''); https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (left): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:288: if (loadTimeData.valueExists('wallpaperAppName')) { I don't think you can get rid of this yet. You can't assume that the app name set in local fs is properly set (e.g. if wallpaper was set up before this change) the logic should be: function getAppName(callback) { Constants.WallpaperLocalStorage.get(thirdPartyAppName, function(items) { if (items.hasOwnProperty(thirdPartyAppName)) { callback(items[thirdPartyAppName]); } else { callback(str('wallpaperAppName') || ''); } }); getAppName(function(appName)) { if (!!appName) { // set-by message } else { // clear set-by message } } } https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:301: }); Maybe add listener to local storage to update the message when the value changes? (can you test that the message gets updated if the wallpaper is changed by another app while wallpaper manager app is active). https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:562: // picker app, we should clear the third party app name if possible. nit: remove "if possible" from the comment. Also, s/we should clear third party app name/third party app name should be cleared/
Wallpaper stuff owner bshe@ is on vacation now and will be back in 1 month. Since it's not an urgent issue, maybe I just hold it until he's back? Actually I would prefer the solution in Patch 2, which is simper and is compatible with the existing users. Your concern about the possible time gap might not that bad in my humble opinion. On 2016/01/28 20:10:48, tbarzic wrote: > I'm not very familiar with wallpaper_manager, so you might also want to include > someone who knows that code better. > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:266: > Constants.ThirdPartyWallpaperPrefix) != -1) { > nit: add 4 indent > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:304: }); > nit: indent > > https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... > File chrome/common/extensions/api/wallpaper_private.json (right): > > https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... > chrome/common/extensions/api/wallpaper_private.json:281: "name": "appname", > nit: capital N > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:265: if > (wallpaperFilename.indexOf( > do you really need to branch here? > Can't you just set the app name from the sync fs (it should already be '' for > 1st party app)? You're right. > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:277: > WallpaperUtil.storeWallpaperToLocalFS(wallpaperFilename, > the wallpapers should still be stored to local fs if they came from the > component wallpaper app, right? Yes, they only need to be stored if they come from the built-in wallpaper app so that they can display in the wallpaper app's grid. If it's a third party wallpaper, then we don't need to store it. > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:311: > WallpaperUtil.saveToLocalStorage(Constants.AccessLocalThirdPartyAppName, '', > WallpaperUtil.saveThirdPartyAppName(''); > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js > (left): > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:288: > if (loadTimeData.valueExists('wallpaperAppName')) { > I don't think you can get rid of this yet. > You can't assume that the app name set in local fs is properly set (e.g. if > wallpaper was set up before this change) > > the logic should be: > function getAppName(callback) { > Constants.WallpaperLocalStorage.get(thirdPartyAppName, function(items) { > if (items.hasOwnProperty(thirdPartyAppName)) { > callback(items[thirdPartyAppName]); > } else { > callback(str('wallpaperAppName') || ''); > } > }); > > getAppName(function(appName)) { > if (!!appName) { > // set-by message > } else { > // clear set-by message > } > } > } You're right. I didn't thought about this. In this case the profile preference kCurrentWallpaperAppName has to be kept even it's unused at all after this change... > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js > (right): > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:301: > }); > Maybe add listener to local storage to update the message when the value > changes? > (can you test that the message gets updated if the wallpaper is changed by > another app while wallpaper manager app is active). In the previous behavior, the message doesn't get updated if the wallpaper is changed by another third party app while wallpaper manager app is active. I didn't change this behavior in this CL. I can fix it if it sounds like a bug. > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:562: > // picker app, we should clear the third party app name if possible. > nit: remove "if possible" from the comment. > Also, s/we should clear third party app name/third party app name should be > cleared/
On 2016/01/30 00:49:44, xdai1 wrote: > Wallpaper stuff owner bshe@ is on vacation now and will be back in 1 month. > Since it's not an urgent issue, maybe I just hold it until he's back? > Actually I would prefer the solution in Patch 2, which is simper and is > compatible with the existing users. Your concern about the possible time gap > might not that bad in my humble opinion. > I still think that it's cleaner to have one way to sync properties. Another issue with patchset 2 I noticed while doing the review is possibility of disabling sync option in wallpaper manager app. If you used preference sync to sync the app name, it would get synced regardless of whether wallpaper sync is enabled or not. What do you think about keeping to use profile pref to store current third party app name, and just pass app name to setCustomWallpaper? > On 2016/01/28 20:10:48, tbarzic wrote: > > I'm not very familiar with wallpaper_manager, so you might also want to > include > > someone who knows that code better. > > > > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > > > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:266: > > Constants.ThirdPartyWallpaperPrefix) != -1) { > > nit: add 4 indent > > > > > https://codereview.chromium.org/1631923004/diff/60001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:304: }); > > nit: indent > > > > > https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... > > File chrome/common/extensions/api/wallpaper_private.json (right): > > > > > https://codereview.chromium.org/1631923004/diff/60001/chrome/common/extension... > > chrome/common/extensions/api/wallpaper_private.json:281: "name": "appname", > > nit: capital N > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:265: if > > (wallpaperFilename.indexOf( > > do you really need to branch here? > > Can't you just set the app name from the sync fs (it should already be '' for > > 1st party app)? > > You're right. > > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:277: > > WallpaperUtil.storeWallpaperToLocalFS(wallpaperFilename, > > the wallpapers should still be stored to local fs if they came from the > > component wallpaper app, right? > > Yes, they only need to be stored if they come from the built-in wallpaper app so > that they can display in the wallpaper app's grid. If it's a third party > wallpaper, then we don't need to store it. > Sorry, that should have said third-party app instead of component app, but thanks for explanation. Does this mean the current behavior here is wrong? > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:311: > > WallpaperUtil.saveToLocalStorage(Constants.AccessLocalThirdPartyAppName, '', > > WallpaperUtil.saveThirdPartyAppName(''); > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > File > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js > > (left): > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:288: > > if (loadTimeData.valueExists('wallpaperAppName')) { > > I don't think you can get rid of this yet. > > You can't assume that the app name set in local fs is properly set (e.g. if > > wallpaper was set up before this change) > > > > the logic should be: > > function getAppName(callback) { > > Constants.WallpaperLocalStorage.get(thirdPartyAppName, function(items) { > > if (items.hasOwnProperty(thirdPartyAppName)) { > > callback(items[thirdPartyAppName]); > > } else { > > callback(str('wallpaperAppName') || ''); > > } > > }); > > > > getAppName(function(appName)) { > > if (!!appName) { > > // set-by message > > } else { > > // clear set-by message > > } > > } > > } > > You're right. I didn't thought about this. In this case the profile preference > kCurrentWallpaperAppName has to be kept even it's unused at all after this > change... > > > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > File > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js > > (right): > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:301: > > }); > > Maybe add listener to local storage to update the message when the value > > changes? > > (can you test that the message gets updated if the wallpaper is changed by > > another app while wallpaper manager app is active). > > In the previous behavior, the message doesn't get updated if the wallpaper is > changed by another third party app while wallpaper manager app is active. I > didn't change this behavior in this CL. I can fix it if it sounds like a bug. > Yep, I noticed that, I think that is a bug though. I'm fine with not fixing it in this cl, but can you please file a bug for it? > > > > > > https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:562: > > // picker app, we should clear the third party app name if possible. > > nit: remove "if possible" from the comment. > > Also, s/we should clear third party app name/third party app name should be > > cleared/
tbarzic@, I've addressed your comments. Please take another look, thanks for your view. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:265: if (wallpaperFilename.indexOf( On 2016/01/28 20:10:48, tbarzic wrote: > do you really need to branch here? > Can't you just set the app name from the sync fs (it should already be '' for > 1st party app)? Use value from sync fs instead. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:277: WallpaperUtil.storeWallpaperToLocalFS(wallpaperFilename, On 2016/01/28 20:10:48, tbarzic wrote: > the wallpapers should still be stored to local fs if they came from the > component wallpaper app, right? Only custom wallpapers need to be stored in the local fs. Yes, the previous behavior is incorrect. In my previous CL https://codereview.chromium.org/1566713005, I just treat the third party wallpaper as a normal custom wallpaper since the app name was unsyncable. This CL is meant to address this issue as well. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:311: WallpaperUtil.saveToLocalStorage(Constants.AccessLocalThirdPartyAppName, '', On 2016/01/28 20:10:48, tbarzic wrote: > WallpaperUtil.saveThirdPartyAppName(''); Done. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (left): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:288: if (loadTimeData.valueExists('wallpaperAppName')) { On 2016/01/28 20:10:48, tbarzic wrote: > I don't think you can get rid of this yet. > You can't assume that the app name set in local fs is properly set (e.g. if > wallpaper was set up before this change) > > the logic should be: > function getAppName(callback) { > Constants.WallpaperLocalStorage.get(thirdPartyAppName, function(items) { > if (items.hasOwnProperty(thirdPartyAppName)) { > callback(items[thirdPartyAppName]); > } else { > callback(str('wallpaperAppName') || ''); > } > }); > > getAppName(function(appName)) { > if (!!appName) { > // set-by message > } else { > // clear set-by message > } > } > } Done. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:301: }); On 2016/01/28 20:10:48, tbarzic wrote: > Maybe add listener to local storage to update the message when the value > changes? > (can you test that the message gets updated if the wallpaper is changed by > another app while wallpaper manager app is active). Will do it in a following CL. https://codereview.chromium.org/1631923004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:562: // picker app, we should clear the third party app name if possible. On 2016/01/28 20:10:48, tbarzic wrote: > nit: remove "if possible" from the comment. > Also, s/we should clear third party app name/third party app name should be > cleared/ Done.
https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:637: if (params->third_party_app_name.get()) { What's the benefit of keeping to set this value if the same is kept (and read) from the apps local storage? https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/constants.js (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/constants.js:48: AccessLocalThirdPartyAppName: 'local-third-party-app-name-key', why don't you keep this with other wallpaper info, that way you'd avoid two storage change events for a single wallpaper change (first only for name, and then the rest of the info)? And source third app name _is_ part of wallpaper info. (sorry that I didn't notice wallpaperInfoKey in previous pass) https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:251: Constants.WallpaperSyncStorage.get( pass the app name as a function param (as filename and layout are) https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:277: WallpaperUtil.clearThirdPartyAppName(); the correct value should already be set at this point, so remove this https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:572: WallpaperUtil.clearThirdPartyAppName(); you should do this only when the rest of wallpaper info is updated
tbarzic@, I've addressed your comments. Please take another look, thanks for your review. https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:637: if (params->third_party_app_name.get()) { On 2016/02/02 20:59:38, tbarzic wrote: > What's the benefit of keeping to set this value if the same is kept (and read) > from the apps local storage? Although it's unused now, I thought it would be better to keep the correct value in the preference. I'm fine with reverting it back. If we don't need to keep this value correct, then we don't need to modify the proto chrome.wallpaperPrivate.setCustomWallpaper. Right? Also reverted the proto modification back. Also add a TODO here to clean up the preference after several milestones. https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/constants.js (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/constants.js:48: AccessLocalThirdPartyAppName: 'local-third-party-app-name-key', On 2016/02/02 20:59:38, tbarzic wrote: > why don't you keep this with other wallpaper info, that way you'd avoid two > storage change events for a single wallpaper change (first only for name, and > then the rest of the info)? And source third app name _is_ part of wallpaper > info. > > (sorry that I didn't notice wallpaperInfoKey in previous pass) Good point. Done. https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:251: Constants.WallpaperSyncStorage.get( On 2016/02/02 20:59:38, tbarzic wrote: > pass the app name as a function param (as filename and layout are) Done.
Mostly OK. Though, I've noticed some potential issues with pre-existing that might be worth fixing in another cls. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:639: profile->GetPrefs()->SetString(prefs::kCurrentWallpaperAppName, It should be OK to stop setting this value (but I'm fine if you want to do this later, in another cl) https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:271: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { More suitable for another cl: sync file system is updated both for thumbnails and wallpaper files, should they be handled differently (e.g. ignore changes to thumbnail changes)? https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:290: localData.url, localData.layout, localData.appName); I'd just use localData.appName || '' (instead of if (hasProperty(appName)) {set with appName} else {set with ''}) Also, localData.appName === undefined would be a good signal that the wallpaper in sync storage came from a device that does not yet support syncing third party app name. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:382: syncInfo.url, syncInfo.layout, syncInfo.appName); How about: WallpaperUtil.setCustomWallpaperFromSyncFS(url, layout, syncInfo.appName || '') https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:440: WallpaperUtil.saveWallpaperInfo( Not introduced here, but might make for a good follow-up: it looks like chrome.syncFileSystem.onFileStatusChanged listener depend on local data being set, in which case this seems a bit racy. Maybe update sync fs in a callback to saveWallpaperInfo? https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:217: function(e) { // not exists, create Not really related to this cl, so feel to ignore for now: Shouldn't this check for the error. Also, I think this does exactly what you need (writes a new file, but only if it doesn't already exist) dirEntry.getFile(wallpaperFileName, {create: true, exclusive: true}, function(fileEntry) { WallpaperUtil.writeFile(fileEntry, wallpaperData) }, errorCallback); btw, do all wallpapers have unique file path (since files are not overwritten)? https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:236: * @param {string} appName If non-empty, the current wallpaper is set by a third wdyt about changing method signature to (possibly in a follow-up) /** * @param {{fileName: string, layout: string | undefined, appName: string}} * @param {function=} onSuccess */ function(options, onSuccess) { } I'm fine if you'd prefer not to. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:267: if (!appName) { This can give false positives if the wallpaper was set by third party app on builds that do not sync file name.
tbarzic@, I've addressed your comments. Please take another look, thanks for your review. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:639: profile->GetPrefs()->SetString(prefs::kCurrentWallpaperAppName, On 2016/02/04 21:31:24, tbarzic wrote: > It should be OK to stop setting this value (but I'm fine if you want to do this > later, in another cl) I also prefer to stop setting this value. And clean up this value after several milestones. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:271: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { On 2016/02/04 21:31:24, tbarzic wrote: > More suitable for another cl: > sync file system is updated both for thumbnails and wallpaper files, should they > be handled differently (e.g. ignore changes to thumbnail changes)? Sync file system is updated both for thumbnails and wallpaper files. But they are handled in the same way. We simply store them to the local fs (see line 295-300 in this patch, or 291-295 in the new patch). But after looking into this function, I think this function needs to be rewritten. The 'added' event should only make sense for a newly setup device to sync down all custom wallpapers and thumbnails. Add a TODO here. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:382: syncInfo.url, syncInfo.layout, syncInfo.appName); On 2016/02/04 21:31:24, tbarzic wrote: > How about: > WallpaperUtil.setCustomWallpaperFromSyncFS(url, layout, syncInfo.appName || '') As explained in WallpaperUtil.setCustomWallpaperFromSyncFS() function, I think we might not need appName as a parameter. Revert it back. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:440: WallpaperUtil.saveWallpaperInfo( On 2016/02/04 21:31:24, tbarzic wrote: > Not introduced here, but might make for a good follow-up: > it looks like chrome.syncFileSystem.onFileStatusChanged listener depend on local > data being set, in which case this seems a bit racy. > Maybe update sync fs in a callback to saveWallpaperInfo? Might not true. It's the onChanged() event which is triggered by wallpaperInfo change that sync down and set the custom/third party wallpaper. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:217: function(e) { // not exists, create On 2016/02/04 21:31:24, tbarzic wrote: > Not really related to this cl, so feel to ignore for now: > Shouldn't this check for the error. > > Also, I think this does exactly what you need (writes a new file, but only if it > doesn't already exist) > dirEntry.getFile(wallpaperFileName, > {create: true, exclusive: true}, > function(fileEntry) { > WallpaperUtil.writeFile(fileEntry, wallpaperData) > }, > errorCallback); > > btw, do all wallpapers have unique file path (since files are not overwritten)? Yes, every wallpaper has its unique file path. The custom wallpaper filename uses the time (the number of milliseconds since midnight Jan 1 1970) when it was set. The third party filename uses the prefix combined with the time when it was set. I will address it in another CL. https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:267: if (!appName) { On 2016/02/04 21:31:24, tbarzic wrote: > This can give false positives if the wallpaper was set by third party app on > builds that do not sync file name. You're right. Actually on a second thought I think it's unnecessary to store the custom wallpaper here. It will be saved in the onFileStatusChanged() event anyway.
https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:271: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { On 2016/02/05 01:09:52, xdai1 wrote: > On 2016/02/04 21:31:24, tbarzic wrote: > > More suitable for another cl: > > sync file system is updated both for thumbnails and wallpaper files, should > they > > be handled differently (e.g. ignore changes to thumbnail changes)? > > Sync file system is updated both for thumbnails and wallpaper files. But they > are handled in the same way. We simply store them to the local fs (see line > 295-300 in this patch, or 291-295 in the new patch). > > But after looking into this function, I think this function needs to be > rewritten. The 'added' event should only make sense for a newly setup device to > sync down all custom wallpapers and thumbnails. Add a TODO here. Oh, yeah, I see what you mean after second reading of the code, my bad :) https://codereview.chromium.org/1631923004/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:440: WallpaperUtil.saveWallpaperInfo( On 2016/02/05 01:09:52, xdai1 wrote: > On 2016/02/04 21:31:24, tbarzic wrote: > > Not introduced here, but might make for a good follow-up: > > it looks like chrome.syncFileSystem.onFileStatusChanged listener depend on > local > > data being set, in which case this seems a bit racy. > > Maybe update sync fs in a callback to saveWallpaperInfo? > > Might not true. It's the onChanged() event which is triggered by wallpaperInfo > change that sync down and set the custom/third party wallpaper. Oh, yeah, syncFileSystem,onFileStatusChange only handles remote -> local updates. D'oh. https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:275: if (detail.status == 'synced' && detail.direction == 'remote_to_local') { nit: I think this would be easier to read: if (detail.status !== 'synced' || detail.direction != 'remote_to_local') return; https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:278: // WallpaperInfo is saved in the sync filesystem before the corresonding I'm not sure if this is necessary true. I don't think there is guarantee on ordering of change events on sync storage (where wallpaper info is stored) and sync file system (where wallpaper files are stored). I think localData.url == detail.fileEntry.name is supposed to handle this (i.e. if local data has already been updated, set the wallpaper, otherwise, expect wallpaper to be set when sync storage is updated with new wallpaper info) On the other hand if this event comes before storage.onChange, setCustomWallpaperFromSyncFS would fail when it attempts to get the wallpaper from the sync file system. https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:295: WallpaperUtil.storeWallpaperFromSyncFSToLocalFS(detail.fileEntry); Previously, local FS was not updated if call to wallpaperPrivate.setCustomWallpaper failed, are you sure we don't want to keep that behaviour?
tbarzic@, as we discussed offline, please take another look, thanks for your review. https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:275: if (detail.status == 'synced' && detail.direction == 'remote_to_local') { On 2016/02/05 22:27:37, tbarzic wrote: > nit: I think this would be easier to read: > > if (detail.status !== 'synced' || detail.direction != 'remote_to_local') > return; Done. https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:278: // WallpaperInfo is saved in the sync filesystem before the corresonding On 2016/02/05 22:27:37, tbarzic wrote: > I'm not sure if this is necessary true. I don't think there is guarantee on > ordering of change events on sync storage (where wallpaper info is stored) and > sync file system (where wallpaper files are stored). > > I think localData.url == detail.fileEntry.name is supposed to handle this (i.e. > if local data has already been updated, set the wallpaper, otherwise, expect > wallpaper to be set when sync storage is updated with new wallpaper info) > > On the other hand if this event comes before storage.onChange, > setCustomWallpaperFromSyncFS would fail when it attempts to get the wallpaper > from the sync file system. As discussed offline, modified the comment to better describe it. https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:295: WallpaperUtil.storeWallpaperFromSyncFSToLocalFS(detail.fileEntry); On 2016/02/05 22:27:37, tbarzic wrote: > Previously, local FS was not updated if call to > wallpaperPrivate.setCustomWallpaper failed, are you sure we don't want to keep > that behaviour? I prefer the current behavior. Even if the synced wallpaper is not set directly, we still should save it to the local fs to make it possible to retry/let user to manually select it from the wallpaper picker. No check mark will appear in the wallpaper picker if the synced wallpaper is not set up correctly.
lgtm https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:295: WallpaperUtil.storeWallpaperFromSyncFSToLocalFS(detail.fileEntry); On 2016/02/06 01:42:30, xdai1 wrote: > On 2016/02/05 22:27:37, tbarzic wrote: > > Previously, local FS was not updated if call to > > wallpaperPrivate.setCustomWallpaper failed, are you sure we don't want to keep > > that behaviour? > > I prefer the current behavior. Even if the synced wallpaper is not set directly, > we still should save it to the local fs to make it possible to retry/let user to > manually select it from the wallpaper picker. > No check mark will appear in the wallpaper picker if the synced wallpaper is not > set up correctly. OK. https://codereview.chromium.org/1631923004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:296: if (detail.fileEntry.name.indexOf( nit: I'll assume build in wallpaper cannot have thirdPartyPrefix somewhere in the name. Still, != 0 should probably be more precise :)
xdai@chromium.org changed reviewers: + mkearney@chromium.org
tbarzic@, thanks for your review! mkearney@, could you help do the owner review for chrome/common/extensions/api/wallpaper_private.json please? Thanks! https://codereview.chromium.org/1631923004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/1631923004/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:296: if (detail.fileEntry.name.indexOf( On 2016/02/06 01:50:14, tbarzic wrote: > nit: I'll assume build in wallpaper cannot have thirdPartyPrefix somewhere in > the name. > Still, != 0 should probably be more precise :) Done.
On 2016/02/09 18:25:02, xdai1 wrote: > mkearney@, could you help do the owner review for > chrome/common/extensions/api/wallpaper_private.json please? Thanks! > kindly ping? Thanks!
xdai@chromium.org changed reviewers: + asargent@chromium.org
Sorry I didn't notice mkearney@ is a document-only reviewer. asargent@, could you help to review the file please: chrome/common/extensions/api/wallpaper_private.json Thanks!
chrome/common/extensions/api/wallpaper_private.json lgtm
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/1631923004/#ps180001 (title: "Address tbarzic@'s comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631923004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631923004/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Sync 3rd party wallpaper app name BUG=551133 ========== to ========== Sync 3rd party wallpaper app name BUG=551133 Committed: https://crrev.com/6a396736e33d3447575473a7cca6149ee1ba9944 Cr-Commit-Position: refs/heads/master@{#375047} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6a396736e33d3447575473a7cca6149ee1ba9944 Cr-Commit-Position: refs/heads/master@{#375047} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
