Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
597503007:
Sync current wallpaper (Closed)
|
Created:
6 years, 3 months ago by Ran Modified:
6 years ago Reviewers:
bshe CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd auto sync custom wallpaper support
Sync wallpaper in storage onchange events, will also upload wallpapers to server when user set custom wallpaper
BUG=416566
Committed: https://crrev.com/c82291e2586be1300d315a73b2648b2d5ad84bdd
Cr-Commit-Position: refs/heads/master@{#297876}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Messages
Total messages: 36 (9 generated)
ranj@chromium.org changed reviewers: + bshe@chromium.org
https://codereview.chromium.org/597503007/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:289: dict->SetBoolean("isSyncCustomWallpaper", true); Set it to false initially. We want to hide this feature by default. Also, name it "isExperimental" so it can be used for other experimental features that we might have. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: setWallpaperByName(localData.url, localData.layout); I think we should only set wallpaper if detail.status == 'synced' && detail.action == 'added' && detail.direction == 'remote_to_local' && localData.source == Constants.WallpaperSourceEnum.Custom && localData.url == detail.fileEntry.file().name https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:255: chrome.syncFileSystem.requestFileSystem(function(fs) {globalSyncFs = fs;}); We dont want to requestFileSystem each time storage changed. Move it outside of this function. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:285: if (newValue.source == Constants.WallpaperSourceEnum.Custom) { nit: use "else if" and move this line to previous line. e.g } else if (newValue.....) { https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:5: var globalStrings = null; define all globals and functions in WallpaperUtil namespace. This way, we are not poluting the global namespace. e.g. WallpaperUtil.globalStrings = null; https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:27: {'type' : 'image\/jpeg'}); Did you try png wallpapers? We also support png files. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:123: chrome.syncFileSystem.requestFileSystem(setWallpaperFromSyncCallback); We can probably save fs to a local variable so that we dont need to request it everytime before we use it.
Fix issues from 1st code review https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:5: var globalStrings = null; On 2014/09/24 17:07:05, bshe wrote: > define all globals and functions in WallpaperUtil namespace. This way, we are > not poluting the global namespace. > > e.g. > > WallpaperUtil.globalStrings = null; Acknowledged.
https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:27: {'type' : 'image\/jpeg'}); On 2014/09/24 17:07:05, bshe wrote: > Did you try png wallpapers? We also support png files. Yes, I tried png wallpapers and sync works as well.
https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:243: if (detail.action == 'added') { nit: detail.action == 'added' can probably combined with previous if check ? https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: function(items) { nit: indention is off. "function" should align with "Constants.Acce.." https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:250: setWallpaperByName(localData.url, localData.layout); nit: indention is off. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:5: var WallpaperUtilNamespace = { I meant to use "WallpaperUtil" namespace that is defined below. We dont need to create a new namespace. Also, avoid to use global in the variable name. Instead, you could call it "strings" and "syncFs" e.g., you could do this: WallpaperUtil.syncFs = null; WallpaperUtil.requestSyncFs = function (callback) { ... }; And for all the rest. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:12: if (WallpaperUtilNamespace.globalFlags.isSyncCustomWallpaper) isExperimental? https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:26: if (callback) nit: no need to check callback, it should always exist. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:32: if (callback) nit: same here. no need to check callback. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:45: {'type' : 'image\/jpeg'}); if png works, we probably shouldn't hard code 'image/jpeg' here. Can you try to see if you could find a way to get the MIME of original file or can you get the original file and use fileWriter.write(file) directly? https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:95: setWallpaperFromSyncLastCallFinished = false; Can you check if setWallpaperByName still being called multiple times? If not, we probably wont need this setWallpaperFromSyncLastCallFinished flag. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:108: fileEntry.file(function(file) { nit: Default indention is usually two spaces and only parameters have 4 spaces indention. You can refer to: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html and https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml for style guide. Here specifically, "fileEntry.file" should only have 2 spaces indention relative to "fs.root" in previous line. There are a bunch of other places that have wrong indention. Do you mind to go though this file and try to fix the indention? https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:524: enableSync(function() { you probably dont need to writeWallpaperToSync here. This code path only triggered when user select existing custom wallpaper (i.e. select thumbnails). Existing custom wallpaper should have scheduled to sync, we dont need to try to sync it again. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:857: enableSync(function() { It is not safe to write wallpaper to sync storage here. As setCustomWallpaper above might fail. We should only try to sync wallpaper when we know for sure that the current wallpaper has set to the custom wallpaper. We only save thumbnail of custom wallpaper in the above case. You can probably create a new function called onCustomWallpaperSuccess and in that function, call writeWallpaperToSync and saveThumbnail. You would also need to call onCustomWallpaperSuccess instead of saveThumbnail in line 853.
https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:45: {'type' : 'image\/jpeg'}); On 2014/09/25 13:41:47, bshe wrote: > if png works, we probably shouldn't hard code 'image/jpeg' here. Can you try to > see if you could find a way to get the MIME of original file or can you get the > original file and use fileWriter.write(file) directly? Do we only support jpeg and png? Do I also need to check for gif, bmp, etc..
On 2014/09/25 16:00:34, Ran wrote: > https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... > File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): > > https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... > chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:45: {'type' : > 'image\/jpeg'}); > On 2014/09/25 13:41:47, bshe wrote: > > if png works, we probably shouldn't hard code 'image/jpeg' here. Can you try > to > > see if you could find a way to get the MIME of original file or can you get > the > > original file and use fileWriter.write(file) directly? > > Do we only support jpeg and png? Do I also need to check for gif, bmp, etc.. Only jpge and png are supported.
https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:243: if (detail.action == 'added') { On 2014/09/25 13:41:46, bshe wrote: > nit: detail.action == 'added' can probably combined with previous if check ? Im going to add "deleted" in the next CL along with the sync history function. So I would prefer to leave the detail.action == 'added' along for now. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:524: enableSync(function() { On 2014/09/25 13:41:47, bshe wrote: > you probably dont need to writeWallpaperToSync here. This code path only > triggered when user select existing custom wallpaper (i.e. select thumbnails). > Existing custom wallpaper should have scheduled to sync, we dont need to try to > sync it again. I'm thinking for the existing wallpaper. If the user has thumbnails in his wallpaper library and he decide to switch back the original one, we should also sync that one.
https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:243: if (detail.action == 'added') { sounds good to me On 2014/09/25 19:47:43, Ran wrote: > On 2014/09/25 13:41:46, bshe wrote: > > nit: detail.action == 'added' can probably combined with previous if check ? > > Im going to add "deleted" in the next CL along with the sync history function. > So I would prefer to leave the detail.action == 'added' along for now. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:524: enableSync(function() { sgtm. On 2014/09/25 19:47:43, Ran wrote: > On 2014/09/25 13:41:47, bshe wrote: > > you probably dont need to writeWallpaperToSync here. This code path only > > triggered when user select existing custom wallpaper (i.e. select thumbnails). > > Existing custom wallpaper should have scheduled to sync, we dont need to try > to > > sync it again. > > I'm thinking for the existing wallpaper. If the user has thumbnails in his > wallpaper library and he decide to switch back the original one, we should also > sync that one.
https://codereview.chromium.org/597503007/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/wallpaper_private_api.cc:289: dict->SetBoolean("isSyncCustomWallpaper", true); On 2014/09/24 17:07:04, bshe wrote: > Set it to false initially. We want to hide this feature by default. Also, name > it "isExperimental" so it can be used for other experimental features that we > might have. Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: setWallpaperByName(localData.url, localData.layout); On 2014/09/24 17:07:05, bshe wrote: > I think we should only set wallpaper if > > detail.status == 'synced' && detail.action == 'added' && detail.direction == > 'remote_to_local' && > localData.source == Constants.WallpaperSourceEnum.Custom && > localData.url == detail.fileEntry.file().name Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:255: chrome.syncFileSystem.requestFileSystem(function(fs) {globalSyncFs = fs;}); On 2014/09/24 17:07:05, bshe wrote: > We dont want to requestFileSystem each time storage changed. Move it outside of > this function. Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:285: if (newValue.source == Constants.WallpaperSourceEnum.Custom) { On 2014/09/24 17:07:05, bshe wrote: > nit: use "else if" and move this line to previous line. e.g > } else if (newValue.....) { Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:5: var globalStrings = null; On 2014/09/24 17:07:05, bshe wrote: > define all globals and functions in WallpaperUtil namespace. This way, we are > not poluting the global namespace. > > e.g. > > WallpaperUtil.globalStrings = null; Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:27: {'type' : 'image\/jpeg'}); On 2014/09/24 17:07:05, bshe wrote: > Did you try png wallpapers? We also support png files. Done. https://codereview.chromium.org/597503007/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:123: chrome.syncFileSystem.requestFileSystem(setWallpaperFromSyncCallback); On 2014/09/24 17:07:05, bshe wrote: > We can probably save fs to a local variable so that we dont need to request it > everytime before we use it. Done. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: function(items) { On 2014/09/25 13:41:47, bshe wrote: > nit: indention is off. "function" should align with "Constants.Acce.." Done. https://codereview.chromium.org/597503007/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:250: setWallpaperByName(localData.url, localData.layout); On 2014/09/25 13:41:47, bshe wrote: > nit: indention is off. Done.
Patch 4
Great. It starts to look good. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:241: console.log('chrome.syncFileSystem.onFileStatusChanged fired'); //temp nit: remove line 241. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: get(Constants.AccessLocalWallpaperInfoKey, nit: change the format to: Constants.WallpaperLocalStorage.get( Constants.AccessLocalWallpaperInfoKey, function(items) .... We usually try to fit as many characters as possible in one line. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:263: }); nit: I think the above is equivalent to WallpaperUtil.enableSync(WallpaperUtil.requestSyncFs(function() {})); https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:292: WallpaperUtil.setWallpaperByName(newValue.url, newValue.layout); nit: you can probably rename the function to "setSyncCustomWallpaper" https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:11: * Run experimental features nit: Add period to the end of sentence in each comment. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:12: * @param {function=} callback The callback will be executed if 'isExperimental' since this is not an optional parameter, remove "=" after function. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:13: * flag is set to true nit: four spaces indention relative to "@param" in the above line. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:15: WallpaperUtil.enableSync = function(callback) { nit: perhaps rename this function to "enabledSyncCallback"? It would be easier to read the code. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:17: return; //temp nit: remove previous two lines. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:22: else { nit: move line 22 to line 21 to conform with chromium's style guide. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:33: * @param {function=} callback The callback will be executed on the nit: remove "=" and four spaces indention. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:40: else { nit: move else to previous line. e.g.: if () { ... } else { ... } https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:50: * @param {error} e The error will be printed to console.error Should this be an Event type? https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:52: WallpaperUtil.errorHandler = function(e) { rename to onFileSystemError is probably better. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:71: type = 'image\/jpeg'; The above check is rather hacky and there isn't seem to have a good way to get MIME type. I dont think MIME type is important here so maybe we can just use the default MIME type. The 'type' parameter is optional I believe. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:77: if (writeCallback) writeCallback(); nit: move writeCallback to next line. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:85: * @param {function=} writeCallback The callback that will be executed after do you intend to use "writeCallback" in the future? It looks like currently they are just two empty function. If you do, please rename it to "onSucess" and also add a callback "onError". Or just one callback function called onFinished. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:148: WallpaperUtil.errorHandler('fs is null'); it is easier to directly call console.error(). Plus, errorHandler should take an error Event. e.g. console.error('Failed to get syncFileSystem.'); https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:152: WallpaperUtil.errorHandler('wallpaperFilename is null'); For error message, it is more informative to say why the error happened. e.g. this would be more informative. console.error('wallpaperFilename is not provided.'); https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:167: if (!thumbnailData) { I think you should check chrome.runtime.lastError here similar to WallpaperManager.setCustomWallpaper. It would print out more useful error message. If lastError is undefined, thumbnailData is guaranteed to exist, we dont need to double check.
https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:263: }); Dooh. Sorry. It is not the same. You can remove the log, "x" parameter in the callback function and the comment though. On 2014/09/26 19:52:56, bshe wrote: > nit: I think the above is equivalent to > > WallpaperUtil.enableSync(WallpaperUtil.requestSyncFs(function() {}));
https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:241: console.log('chrome.syncFileSystem.onFileStatusChanged fired'); //temp On 2014/09/26 19:52:56, bshe wrote: > nit: remove line 241. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:246: get(Constants.AccessLocalWallpaperInfoKey, On 2014/09/26 19:52:56, bshe wrote: > nit: change the format to: > Constants.WallpaperLocalStorage.get( > Constants.AccessLocalWallpaperInfoKey, > function(items) .... > > We usually try to fit as many characters as possible in one line. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:263: }); On 2014/09/26 19:52:56, bshe wrote: > nit: I think the above is equivalent to > > WallpaperUtil.enableSync(WallpaperUtil.requestSyncFs(function() {})); Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:292: WallpaperUtil.setWallpaperByName(newValue.url, newValue.layout); On 2014/09/26 19:52:56, bshe wrote: > nit: you can probably rename the function to "setSyncCustomWallpaper" Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:11: * Run experimental features On 2014/09/26 19:52:56, bshe wrote: > nit: Add period to the end of sentence in each comment. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:12: * @param {function=} callback The callback will be executed if 'isExperimental' On 2014/09/26 19:52:56, bshe wrote: > since this is not an optional parameter, remove "=" after function. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:13: * flag is set to true On 2014/09/26 19:52:56, bshe wrote: > nit: four spaces indention relative to "@param" in the above line. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:15: WallpaperUtil.enableSync = function(callback) { On 2014/09/26 19:52:57, bshe wrote: > nit: perhaps rename this function to "enabledSyncCallback"? It would be easier > to read the code. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:17: return; //temp On 2014/09/26 19:52:57, bshe wrote: > nit: remove previous two lines. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:22: else { On 2014/09/26 19:52:57, bshe wrote: > nit: move line 22 to line 21 to conform with chromium's style guide. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:33: * @param {function=} callback The callback will be executed on the On 2014/09/26 19:52:56, bshe wrote: > nit: remove "=" and four spaces indention. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:40: else { On 2014/09/26 19:52:57, bshe wrote: > nit: move else to previous line. e.g.: > if () { > ... > } else { > ... > } Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:50: * @param {error} e The error will be printed to console.error On 2014/09/26 19:52:57, bshe wrote: > Should this be an Event type? Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:52: WallpaperUtil.errorHandler = function(e) { On 2014/09/26 19:52:56, bshe wrote: > rename to onFileSystemError is probably better. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:77: if (writeCallback) writeCallback(); On 2014/09/26 19:52:57, bshe wrote: > nit: move writeCallback to next line. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:85: * @param {function=} writeCallback The callback that will be executed after On 2014/09/26 19:52:57, bshe wrote: > do you intend to use "writeCallback" in the future? It looks like currently they > are just two empty function. If you do, please rename it to "onSucess" and also > add a callback "onError". Or just one callback function called onFinished. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:148: WallpaperUtil.errorHandler('fs is null'); On 2014/09/26 19:52:57, bshe wrote: > it is easier to directly call console.error(). > Plus, errorHandler should take an error Event. > e.g. > > console.error('Failed to get syncFileSystem.'); Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:167: if (!thumbnailData) { On 2014/09/26 19:52:56, bshe wrote: > I think you should check chrome.runtime.lastError here similar to > WallpaperManager.setCustomWallpaper. It would print out more useful error > message. If lastError is undefined, thumbnailData is guaranteed to exist, we > dont need to double check. Done. https://codereview.chromium.org/597503007/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:167: if (!thumbnailData) { On 2014/09/26 19:52:56, bshe wrote: > I think you should check chrome.runtime.lastError here similar to > WallpaperManager.setCustomWallpaper. It would print out more useful error > message. If lastError is undefined, thumbnailData is guaranteed to exist, we > dont need to double check. Done.
mostly format nits. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:251: localData.layout); nit: align this line with "local.url" in previous line https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:6: strings: null, nit: document the two variable too. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:15: WallpaperUtil.enableSyncCallback = function(callback) { nit: Sorry. I missed this in previous review, but since this is not sync specific, can you rename it enabledExperimentalFeatureCallback? https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:31: * syncFileSystem handler. nit: The callback to execute after syncFileSystem handler is available. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:49: console.error(e); Can you leave a TODO here. Ideally, we should not just print out the error event in javascript console. We should handle different errors differently. The TODO style is here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:66: },WallpaperUtil.onFileSystemError); nit: one space between "," and "WallpaperUtil" https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:79: if (!fs) return; nit: only two space indention if it is a function implementation. Also, move "return" to next line. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:82: function() { onFinished();}, //already exists Please follow the inline comment style guide for the inline comments here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:83: function(e) { //not exists, create nit: style of inline comment is not correct. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:90: WallpaperUtil.onFileSystemError); You should probably also call onFinished in the case of error. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:98: * @param {ArrayBuffer} picData Data for image file. Nit: Please capitalize the first letter of the comment. Also, avoid to use abbreviation such as picData. Since this is for wallpaper only, it would be more readable to name it as wallpaperData. e.g., this would be how I write comment for this function: Stores jpeg/png wallpaper into |localDir| in local file system. @param {ArrayBuffer} wallpaperData The wallpaper data. @param {string} saveToDir The path to store wallpaper in local file system. @param {string} fileName File name of wallpaper image. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:103: if (!picData) return; //TODO nit: new line for "return" and add more information for your TODO. Follow the TODO style from previous link. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:108: function(e) { //not exists, create nit: inline comments https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:127: * set wallpaper by file name in syncFileSystem. nit: Sets wallpaper from synced file system. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:133: wallpaperFilename, wallpaperLayout, successCall) { nit: rename successCall to onSuccess. We can also consider to have an onFinished callback function or replace the callback. I think it make sense to make sure the callback is called in either success or error cases. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:157: console.error(chrome.runtime.lastError.message); you probably should not continue to execute the following code. Please add "return" after console.error and also use curly braces to wrap them https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:171: }, function(e) {} //fail to read file, expected due to download delay nit: style of inline comment is wrong.
https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:251: localData.layout); On 2014/09/30 14:45:22, bshe wrote: > nit: align this line with "local.url" in previous line Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:6: strings: null, On 2014/09/30 14:45:23, bshe wrote: > nit: document the two variable too. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:15: WallpaperUtil.enableSyncCallback = function(callback) { On 2014/09/30 14:45:23, bshe wrote: > nit: Sorry. I missed this in previous review, but since this is not sync > specific, can you rename it enabledExperimentalFeatureCallback? Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:31: * syncFileSystem handler. On 2014/09/30 14:45:23, bshe wrote: > nit: The callback to execute after syncFileSystem handler is available. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:49: console.error(e); On 2014/09/30 14:45:23, bshe wrote: > Can you leave a TODO here. Ideally, we should not just print out the error event > in javascript console. We should handle different errors differently. > The TODO style is here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:66: },WallpaperUtil.onFileSystemError); On 2014/09/30 14:45:24, bshe wrote: > nit: one space between "," and "WallpaperUtil" Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:79: if (!fs) return; On 2014/09/30 14:45:23, bshe wrote: > nit: only two space indention if it is a function implementation. Also, move > "return" to next line. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:82: function() { onFinished();}, //already exists On 2014/09/30 14:45:23, bshe wrote: > Please follow the inline comment style guide for the inline comments here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:83: function(e) { //not exists, create On 2014/09/30 14:45:23, bshe wrote: > nit: style of inline comment is not correct. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:90: WallpaperUtil.onFileSystemError); On 2014/09/30 14:45:23, bshe wrote: > You should probably also call onFinished in the case of error. I rename "onFinished" to "onSuccess" since I only want to call this function (e.g. write current file name into storage) after successfully setting wallpaper https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:98: * @param {ArrayBuffer} picData Data for image file. On 2014/09/30 14:45:23, bshe wrote: > Nit: Please capitalize the first letter of the comment. Also, avoid to use > abbreviation such as picData. Since this is for wallpaper only, it would be more > readable to name it as wallpaperData. e.g., this would be how I write comment > for this function: > Stores jpeg/png wallpaper into |localDir| in local file system. > @param {ArrayBuffer} wallpaperData The wallpaper data. > @param {string} saveToDir The path to store wallpaper in local file system. > @param {string} fileName File name of wallpaper image. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:103: if (!picData) return; //TODO On 2014/09/30 14:45:24, bshe wrote: > nit: new line for "return" and add more information for your TODO. Follow the > TODO style from previous link. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:108: function(e) { //not exists, create On 2014/09/30 14:45:23, bshe wrote: > nit: inline comments Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:127: * set wallpaper by file name in syncFileSystem. On 2014/09/30 14:45:23, bshe wrote: > nit: Sets wallpaper from synced file system. Done. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:133: wallpaperFilename, wallpaperLayout, successCall) { On 2014/09/30 14:45:23, bshe wrote: > nit: rename successCall to onSuccess. We can also consider to have an onFinished > callback function or replace the callback. I think it make sense to make sure > the callback is called in either success or error cases. I've renamed "successCall" to "onSuccess". For now I haven't run into a case which need to call a function regardless it fails or success. I will add onFinished callback if that happens. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:157: console.error(chrome.runtime.lastError.message); On 2014/09/30 14:45:23, bshe wrote: > you probably should not continue to execute the following code. Please add > "return" after console.error and also use curly braces to wrap them Seems even there is error, the call might still success. I would like to give it a try instead of abort. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:171: }, function(e) {} //fail to read file, expected due to download delay On 2014/09/30 14:45:23, bshe wrote: > nit: style of inline comment is wrong. Done.
Fix issues from 5th code review
https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:157: console.error(chrome.runtime.lastError.message); On 2014/09/30 16:44:39, Ran wrote: > On 2014/09/30 14:45:23, bshe wrote: > > you probably should not continue to execute the following code. Please add > > "return" after console.error and also use curly braces to wrap them > > Seems even there is error, the call might still success. I would like to give it > a try instead of abort. What's the error message that is printed in the console? If there is an unexpected error, wallpaper should not success. Otherwise, it is a bug. My guess that it works because str('canceledWallpaper') is not defined in this context. I think str is only defined in wallpaper_manager.js. Can you verify please? https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:240: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { The code is not particularly very readable. You can consider to do this: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { WallpaperUtil.enabledExperimentalFeatureCallback(function() { ..... // The rest of your implementation }); }); It should be fine to listen to the file status even if experimental flag is not on. We just dont need to anything in that case. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:31: * handler is available nit: add period https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:174: reader.readAsArrayBuffer(file); nit: indention might be off from line 172 to 175. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:886: console.log('removeCustomWallpaper name ' + fileName); remove the three lines please.
Fix issues from 6th code review. Please ignore the new line "callback();" in WallpaperUtil.enabledExperimentalFeatureCallback I forgot to remove it when finish testing. I will remove it in next submission. https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/80001/chrome/browser/resources... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:157: console.error(chrome.runtime.lastError.message); On 2014/09/30 17:48:14, bshe wrote: > On 2014/09/30 16:44:39, Ran wrote: > > On 2014/09/30 14:45:23, bshe wrote: > > > you probably should not continue to execute the following code. Please add > > > "return" after console.error and also use curly braces to wrap them > > > > Seems even there is error, the call might still success. I would like to give > it > > a try instead of abort. > > What's the error message that is printed in the console? If there is an > unexpected error, wallpaper should not success. Otherwise, it is a bug. My guess > that it works because str('canceledWallpaper') is not defined in this context. I > think str is only defined > in wallpaper_manager.js. Can you verify please? Sry for the confusion, for now there is no error. I mean if there is error occurs later in some edge cases, like the one in setting system wallpaper, this call might still success. So even there is an error, we can still give it a try. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:240: chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { On 2014/09/30 17:48:14, bshe wrote: > The code is not particularly very readable. You can consider to do this: > > chrome.syncFileSystem.onFileStatusChanged.addListener(function(detail) { > WallpaperUtil.enabledExperimentalFeatureCallback(function() { > ..... // The rest of your implementation > }); > }); > > It should be fine to listen to the file status even if experimental flag is not > on. We just dont need to anything in that case. Done. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:31: * handler is available On 2014/09/30 17:48:15, bshe wrote: > nit: add period Done. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:174: reader.readAsArrayBuffer(file); On 2014/09/30 17:48:15, bshe wrote: > nit: indention might be off from line 172 to 175. Done. https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/597503007/diff/100001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:886: console.log('removeCustomWallpaper name ' + fileName); On 2014/09/30 17:48:15, bshe wrote: > remove the three lines please. Done.
Add setCustomWallpaper last error check
Good work. lgtm with more nits https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:77: WallpaperUtil.writeWallpaperToSync = function( nit: either rename this to "storePictureToSync" or rename "storePictureToLocal" to "writeWallpaperToLocal". It would be nice to keep the function name consistent. I would recommend function names like "storeWallpaperToSyncFileSystem" and "storeWallpaperToLocalFileSystem". It could increase readability. Also the order of parameters should be consistent for the two functions. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:81: return; nit: no need to check fs since requestFs already checked fs. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:113: dirEntry.getFile(fileName, nit: indention off. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:142: return; nit: remove the check for fs. requestSyncFs only execute callback when fs is not null
Fix issues from 6th code review https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/util.js (right): https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:77: WallpaperUtil.writeWallpaperToSync = function( On 2014/09/30 21:01:38, bshe wrote: > nit: either rename this to "storePictureToSync" or rename "storePictureToLocal" > to "writeWallpaperToLocal". It would be nice to keep the function name > consistent. I would recommend function names like > "storeWallpaperToSyncFileSystem" and "storeWallpaperToLocalFileSystem". It could > increase readability. > > Also the order of parameters should be consistent for the two functions. Done. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:81: return; On 2014/09/30 21:01:38, bshe wrote: > nit: no need to check fs since requestFs already checked fs. Done. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:113: dirEntry.getFile(fileName, On 2014/09/30 21:01:38, bshe wrote: > nit: indention off. Done. https://codereview.chromium.org/597503007/diff/140001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/util.js:142: return; On 2014/09/30 21:01:38, bshe wrote: > nit: remove the check for fs. requestSyncFs only execute callback when fs is not > null Done.
The CQ bit was checked by ranj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597503007/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 3fe8aee72001c2d56d6e0102ea8b88612415b97f
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c82291e2586be1300d315a73b2648b2d5ad84bdd Cr-Commit-Position: refs/heads/master@{#297876}
Message was sent while issue was closed.
Patchset #2 (id:20001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:40001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:80001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:100001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:120001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:140001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
