Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14244011:
Fix custom wallpaper going back from wallpaper 2 to wallpaper 1 (Closed)
|
Created:
7 years, 8 months ago by bshe Modified:
7 years, 8 months ago Reviewers:
flackr CC:
chromium-reviews, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix custom wallpaper going back from wallpaper 2 to wallpaper 1
BUG=229036
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194913
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : flackr's review #
Total comments: 4
Patch Set 5 : #Messages
Total messages: 9 (0 generated)
Hi Rob. Could you please take a look at this small CL? Thanks!
https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js (right): https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:252: * @type {!Object} Wallpaper information inserted into the data model. @param {!Object} newElement Although I'd prefer this to be called wallpaperInfo instead of newElement since the list being inserted into contains wallpaper information. https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:258: // change event. Otherwise, wallpaper may reset to previous one as Can we not we prevent handling a change event which doesn't change the actual selected element?
Thanks for quick review. PTAL. https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js (right): https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:252: * @type {!Object} Wallpaper information inserted into the data model. On 2013/04/15 15:02:02, flackr wrote: > @param {!Object} newElement > Although I'd prefer this to be called wallpaperInfo instead of newElement since > the list being inserted into contains wallpaper information. Done. https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:258: // change event. Otherwise, wallpaper may reset to previous one as Sorry, just to confirm. Do you mean we check if the selected element is the same as previous one in the event handler? And prevent handling if it is the same as previous one? On 2013/04/15 15:02:02, flackr wrote: > Can we not we prevent handling a change event which doesn't change the actual > selected element?
https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js (right): https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:258: // change event. Otherwise, wallpaper may reset to previous one as Yes. On 2013/04/15 15:22:10, bshe wrote: > Sorry, just to confirm. Do you mean we check if the selected element is the same > as previous one in the event handler? And prevent handling if it is the same as > previous one? > > On 2013/04/15 15:02:02, flackr wrote: > > Can we not we prevent handling a change event which doesn't change the actual > > selected element? >
PTAL. Thanks for review! https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js (right): https://codereview.chromium.org/14244011/diff/1003/chrome/browser/resources/c... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:258: // change event. Otherwise, wallpaper may reset to previous one as On 2013/04/15 20:00:54, flackr wrote: > Yes. > > On 2013/04/15 15:22:10, bshe wrote: > > Sorry, just to confirm. Do you mean we check if the selected element is the > same > > as previous one in the event handler? And prevent handling if it is the same > as > > previous one? > > > > On 2013/04/15 15:02:02, flackr wrote: > > > Can we not we prevent handling a change event which doesn't change the > actual > > > selected element? > > > Done.
https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:622: WallpaperManager.prototype.onThumbnailSelectionChanged_ = function() { This method and description seem to be inaccurately described. This is handling a change of the wallpaper model which may have changed the selected thumbnail. https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:636: this.wallpaperGrid_.previousSelectedItem = selectedItem; previousSelectedItem doesn't belong in wallpaperGrid since wallpaperGrid doesn't care whether the selectedItem actually change. You could store the active/selected/displayed (whichever you think is the best name) item as a member of this class and only act if it changes. Could this not be checked against this.currentWallpaper or this.wallpaperGrid_.activeItem or would that not be correct?
PTAL. Thanks! https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js (right): https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:622: WallpaperManager.prototype.onThumbnailSelectionChanged_ = function() { On 2013/04/16 01:28:41, flackr wrote: > This method and description seem to be inaccurately described. This is handling > a change of the wallpaper model which may have changed the selected thumbnail. Done. https://codereview.chromium.org/14244011/diff/13001/chrome/browser/resources/... chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:636: this.wallpaperGrid_.previousSelectedItem = selectedItem; I can't use currentWallpaper or activeItem to replace previousSelectedItem. They are not always the same. For example, if we select a thumbnail and failed to set the corresponding wallpaper, activeItem and currentWallpaper wont change but the selectedItem changed. Then we select another item, the previousSelectedItem != currentWallpaper or activeItem in that case. You mean just move previousSelectedItem to this class, right? I have created a new onChange_ function to call this function only when selectedItem changes. On 2013/04/16 01:28:41, flackr wrote: > previousSelectedItem doesn't belong in wallpaperGrid since wallpaperGrid doesn't > care whether the selectedItem actually change. You could store the > active/selected/displayed (whichever you think is the best name) item as a > member of this class and only act if it changes. Could this not be checked > against this.currentWallpaper or this.wallpaperGrid_.activeItem or would that > not be correct?
lgtm
Message was sent while issue was closed.
Committed patchset #5 manually as r194913 (presubmit successful). |
