New issue
Advanced search Search tips

Issue 834900 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Move ash unscaled resources to ash public

Project Member Reported by wutao@chromium.org, Apr 19 2018

Issue description

Currently the Ash unscaled resources includes two icons for Settings and KSV (192x192). The KSV icon needs to be accessed both at ash/components/shortcuts and chrome/browser/ui/app_list/.

Move them to ash/public/cpp/resources/unscaled_resources, so that we can access them at both Ash and Chrome.


 
Cc: est...@chromium.org
+estade FYI since something similar came up recently for strings

Comment 2 by est...@chromium.org, Apr 19 2018

right, and I use the strings target in ui/chromeos/. You might want to do the same for pngs (ui/chromeos/resources/). James said it was a goal to remove ui/chromeos/ but I think wherever those resources wind up getting moved to would probably be a good place for the resources mentioned above.
Components: UI>Shell
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 20 2018

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

commit e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc
Author: wutao <wutao@chromium.org>
Date: Fri Apr 20 18:54:08 2018

cros: Move Ash unscaled resources to ash/public

Currently the Ash unscaled resources includes two icons for Settings and
KSV (size: 192x192). The KSV icon needs to be accessed both at
ash/components/shortcut_viewer and chrome/browser/ui/app_list/.

Move them to ash/public/cpp/resources, so that we can access them at both
Ash and Chrome.

Bug:  834900 
Test: manual
Change-Id: I1caf7f52a3105016697664965bd0dce42b83529c
Reviewed-on: https://chromium-review.googlesource.com/1020416
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552412}
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/components/shortcut_viewer/BUILD.gn
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/components/shortcut_viewer/DEPS
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[add] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/public/cpp/resources/BUILD.gn
[add] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/public/cpp/resources/ash_public_unscaled_resources.grd
[rename] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/public/cpp/resources/unscaled_resources/settings_logo_192.png
[rename] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/public/cpp/resources/unscaled_resources/shortcut_viewer_logo_192.png
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/resources/BUILD.gn
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/ash/resources/ash_resources.grd
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/chrome/browser/ui/app_list/internal_app/internal_app_metadata.cc
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/chrome/browser/ui/ash/launcher/settings_window_observer.cc
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/chrome/chrome_paks.gni
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/chrome/test/BUILD.gn
[modify] https://crrev.com/e5856b5d01c3d9f0887efc4b2afef31a2bdbccdc/tools/gritsettings/resource_ids

Comment 5 by wutao@chromium.org, Apr 20 2018

Labels: Merge-Request-67
This is a fairly big change.  Is this a M67 regression or tied to a new feature?  Need some context for merge consideration.

Comment 7 by wutao@chromium.org, Apr 20 2018

Hi Oshima, WDYT do we need to merge this?

Hi Kevin,
This cl touches many files, but the changes are only +51/-17 lines.

There is a long discussion in the cl [1].
In brief, previous implementation might have extra computing, e.g. scale the size of the image twice for 200% UI scale, i.e. scale 100% resource to 200%, and then scale to the right size.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/938543

Comment 8 by wutao@chromium.org, Apr 20 2018

Labels: -Merge-Request-67
Status: Fixed (was: Available)
Discussed with oshima@ offline, we do not have to merge this (just good to do).
I will remove the merge request and close this bug.

Sign in to add a comment