New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846264 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: Preview and link of the wallpaper set is not seen in Set wallpaper overlay

Project Member Reported by kebalaji@chromium.org, May 24 2018

Issue description

Chrome Version: 68.0.3438.0/10714.0.0 dev-channel Daisy,Reks and Candy
OS:Chrome OS

What steps will reproduce the problem?
(1)Sign-in to user>> After profile gets synced to the device, open Set wallpaper overlay and observe

Actual: The preview and link of wallpaper set is not seen for the first time. If we change the wallpaper and sign-out click on other user and again login, issue is reproducible. ( Refer video)

Expected: The preview and link of wallpaper set should be seen after the profile gets synced.

This is a Regression issue as same is working fine on 67.0.3396.57/10575.47.0 Beta

@Wzang: Please confirm the issue
 
ActualWallpaper.mp4
12.0 MB View Download
ExpectedWallpaper.mp4
11.2 MB View Download
Project Member

Comment 1 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8

commit 5083e4c720b5c648dc8261a8f7c56e1f9f5749b8
Author: Wenzhao Zang <wzang@chromium.org>
Date: Fri May 25 20:50:41 2018

cros: Change GURL back to std::string in SetOnlineWallpaper

This CL is exactly the revert of the diff between patch 6 and 7 of
CL 1013211:
https://chromium-review.googlesource.com/c/chromium/src/+/1013211/6..7

It was assumed that GURL and std::string are equivalent for this use
case. However GURL converts spaces in urls to "%20", therefore
wallpaper_manager.js fails to use the url to find the currently set
wallpaper. It's observed that this issue is only reproducible for
wallpapers whose urls have space.

Think it makes sense to use std::string since the url is not used for
downloading purpose. (Using url as wallpaper id is not the best
solution IMO, but changing it is a separate task.)

Bug:  846264 
Change-Id: Ie83b6d9e95dccbe44dd8fca8c0843a85fcfd9758
Reviewed-on: https://chromium-review.googlesource.com/1072700
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562003}
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/ash/public/interfaces/wallpaper.mojom
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/ash/wallpaper/wallpaper_controller_unittest.cc
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/chromeos/extensions/wallpaper_private_api.h
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/ui/ash/test_wallpaper_controller.cc
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/ui/ash/test_wallpaper_controller.h
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/ui/ash/wallpaper_controller_client.cc
[modify] https://crrev.com/5083e4c720b5c648dc8261a8f7c56e1f9f5749b8/chrome/browser/ui/ash/wallpaper_controller_client.h

Comment 2 by wzang@chromium.org, May 29 2018

Labels: Merge-Request-68
Status: Fixed (was: Assigned)
Project Member

Comment 3 by sheriffbot@chromium.org, May 30 2018

Labels: -Merge-Request-68 Hotlist-Merge-Approved Merge-Approved-68
Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, May 30 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f706f356bfe8e06118a2d06abcc04c9e689a96a2

commit f706f356bfe8e06118a2d06abcc04c9e689a96a2
Author: Wenzhao Zang <wzang@chromium.org>
Date: Wed May 30 17:48:21 2018

[Merge to M68] cros: Change GURL back to std::string in SetOnlineWallpaper

This CL is exactly the revert of the diff between patch 6 and 7 of
CL 1013211:
https://chromium-review.googlesource.com/c/chromium/src/+/1013211/6..7

It was assumed that GURL and std::string are equivalent for this use
case. However GURL converts spaces in urls to "%20", therefore
wallpaper_manager.js fails to use the url to find the currently set
wallpaper. It's observed that this issue is only reproducible for
wallpapers whose urls have space.

Think it makes sense to use std::string since the url is not used for
downloading purpose. (Using url as wallpaper id is not the best
solution IMO, but changing it is a separate task.)

TBR=wzang@chromium.org

(cherry picked from commit 5083e4c720b5c648dc8261a8f7c56e1f9f5749b8)

Bug:  846264 
Change-Id: Ie83b6d9e95dccbe44dd8fca8c0843a85fcfd9758
Reviewed-on: https://chromium-review.googlesource.com/1072700
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#562003}
Reviewed-on: https://chromium-review.googlesource.com/1079458
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#46}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/ash/public/interfaces/wallpaper.mojom
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/ash/wallpaper/wallpaper_controller_unittest.cc
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/chromeos/extensions/wallpaper_private_api.h
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/ui/ash/test_wallpaper_controller.cc
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/ui/ash/test_wallpaper_controller.h
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/ui/ash/wallpaper_controller_client.cc
[modify] https://crrev.com/f706f356bfe8e06118a2d06abcc04c9e689a96a2/chrome/browser/ui/ash/wallpaper_controller_client.h

Sign in to add a comment