Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(111)

Issue 2568403004: Remove some theme images that are not used any more. (Closed)

Created:
4 years ago by Evan Stade
Modified:
4 years ago
Reviewers:
Peter Kasting, Bret, oshima
CC:
chromium-reviews, oshima+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove some theme images that are not used any more. These aren't used by the default theme, which renders programmatically or with colors. (For example, the ntp background image was a 42x42 white square, but we don't need it because we set the bg color to white.) I left the IDR_s in place because they're still needed to identify custom theme images. We could replace them with a C++ enum, but since they occupy the same namespace as other themed/not-themed images (ints), leaving them here seems the best way to avoid potential collisions. BUG=522168 Committed: https://crrev.com/5cc5609d072e90e2b1fe5d9de04bf00d45c5a5c6 Cr-Commit-Position: refs/heads/master@{#438989}

Patch Set 1 #

Patch Set 2 : a few more #

Patch Set 3 : delete some theming code too #

Total comments: 6

Patch Set 4 : simplifaction #

Patch Set 5 : fix #

Messages

Total messages: 32 (19 generated)
Evan Stade
4 years ago (2016-12-13 16:51:22 UTC) #5
Evan Stade
wait, hold up. At least one of these is used as a base for tinting ...
4 years ago (2016-12-13 16:54:12 UTC) #7
Evan Stade
ok, PTAL Peter and Bret. https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc File chrome/browser/themes/browser_theme_pack.cc (left): https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc#oldcode1254 chrome/browser/themes/browser_theme_pack.cc:1254: int prs_base_id = 0; ...
4 years ago (2016-12-13 17:40:11 UTC) #11
Bret
lgtm
4 years ago (2016-12-13 22:58:46 UTC) #14
Evan Stade
Peter, in case I wasn't clear, I was hoping you'd take a look at this ...
4 years ago (2016-12-14 04:10:00 UTC) #16
Peter Kasting
Yeah, I just hadn't reviewed yet. LGTM https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc#newcode1253 chrome/browser/themes/browser_theme_pack.cc:1253: // thing ...
4 years ago (2016-12-14 04:27:54 UTC) #17
oshima
c/a/theme lgtm
4 years ago (2016-12-14 21:42:35 UTC) #18
Evan Stade
https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/2568403004/diff/40001/chrome/browser/themes/browser_theme_pack.cc#newcode1253 chrome/browser/themes/browser_theme_pack.cc:1253: // thing and just use theme colors. On 2016/12/14 ...
4 years ago (2016-12-15 03:31:14 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2568403004/60001
4 years ago (2016-12-15 03:31:37 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/333592)
4 years ago (2016-12-15 04:01:00 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2568403004/80001
4 years ago (2016-12-15 23:34:37 UTC) #27
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2016-12-16 02:21:40 UTC) #30
commit-bot: I haz the power
4 years ago (2016-12-16 02:23:37 UTC) #32
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/5cc5609d072e90e2b1fe5d9de04bf00d45c5a5c6
Cr-Commit-Position: refs/heads/master@{#438989}

Powered by Google App Engine
This is Rietveld 408576698