Themes: Remove unused IDs from ThemeProperties

Themes: Remove unused IDs from ThemeProperties

R=estade@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2667753004
Cr-Commit-Position: refs/heads/master@{#447652}
Committed: https://chromium.googlesource.com/chromium/src/+/9db88f9efd4663a70aa58b115a7b332c20a0c175

[Tom (Use chromium acct), 2017-01-31 21:25:18]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2017-01-31 21:25:26]

Patchset #1 (id:20001) has been deleted

[Tom (Use chromium acct), 2017-01-31 21:26:29]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[Tom (Use chromium acct), 2017-01-31 21:27:02]

Description was changed from

==========
Themes: Remove unused IDs from ThemeProperties
==========

to

==========
Themes: Remove unused IDs from ThemeProperties

R=estade@chromium.org
CC=erg@chromium.org
==========

[commit-bot: I haz the power, 2017-01-31 21:27:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2017-01-31 21:29:27]

estade@ ptal
erg@ FYI since we discussed this in https://codereview.chromium.org/2663993002/

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:969: (*temp_colors)[id] = color;
We could remove COLOR_NTP_SECTION too if we add a special case here.

The only thing it's used for is to provide a fallback for COLOR_NTP_HEADER when
the theme doesn't provide one

[Tom (Use chromium acct), 2017-01-31 21:30:15]

thomasanderson@google.com changed reviewers:
+ estade@chromium.org

[Tom (Use chromium acct), 2017-01-31 21:30:25]

estade@ ptal
erg@ FYI since we discussed this in https://codereview.chromium.org/2663993002/

[Elliot Glaysher, 2017-01-31 21:35:34]

erg@chromium.org changed reviewers:
+ erg@chromium.org

[Elliot Glaysher, 2017-01-31 21:35:34]

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:51: const int kThemePackVersion =
45;
Does this change need the theme pack version bumped?

[commit-bot: I haz the power, 2017-01-31 21:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:59:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tom (Use chromium acct), 2017-01-31 23:47:36]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 23:48:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-01-31 23:51:54]

compile failures look easy to fix, aside from that and NTP_SECTION removal, lg

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:969: (*temp_colors)[id] = color;
On 2017/01/31 21:29:27, Tom Anderson wrote:
> We could remove COLOR_NTP_SECTION too if we add a special case here.
> 
> The only thing it's used for is to provide a fallback for COLOR_NTP_HEADER
when
> the theme doesn't provide one

I guess I would be in favor of that. We have to special case below anyway.

[Tom (Use chromium acct), 2017-02-01 00:21:02]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[Tom (Use chromium acct), 2017-02-01 00:21:17]

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:51: const int kThemePackVersion =
45;
On 2017/01/31 21:35:34, Elliot Glaysher wrote:
> Does this change need the theme pack version bumped?

idk, but I did it anyway

https://codereview.chromium.org/2667753004/diff/40001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:969: (*temp_colors)[id] = color;
On 2017/01/31 23:51:54, Evan Stade wrote:
> On 2017/01/31 21:29:27, Tom Anderson wrote:
> > We could remove COLOR_NTP_SECTION too if we add a special case here.
> > 
> > The only thing it's used for is to provide a fallback for COLOR_NTP_HEADER
> when
> > the theme doesn't provide one
> 
> I guess I would be in favor of that. We have to special case below anyway.

Done.

[commit-bot: I haz the power, 2017-02-01 00:21:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 01:47:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 01:47:21]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-02-01 18:43:22]

lgtm

[Evan Stade, 2017-02-01 21:47:23]

lgtm

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:967: // Allow ntp_section to provide
a fallback value for ntp_header.
nit: We no longer use ntp_section, but to support legacy themes we still need to
use it as a fallback for ntp_header.

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:972: if (id != -1) {
nit: no curlies

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack_unittest.cc:373:
TEST_F(BrowserThemePackTest, UseSectionColorAsNTPHeader) {
nit: since this is sorta more subtle now, add a comment?

[Tom (Use chromium acct), 2017-02-01 23:09:22]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-02-01 23:09:23]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2667753004/#ps100001
(title: "Remove {} and update/add comments")

[Tom (Use chromium acct), 2017-02-01 23:09:30]

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:967: // Allow ntp_section to provide
a fallback value for ntp_header.
On 2017/02/01 21:47:23, Evan Stade wrote:
> nit: We no longer use ntp_section, but to support legacy themes we still need
to
> use it as a fallback for ntp_header.

Done.

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:972: if (id != -1) {
On 2017/02/01 21:47:23, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2667753004/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack_unittest.cc:373:
TEST_F(BrowserThemePackTest, UseSectionColorAsNTPHeader) {
On 2017/02/01 21:47:23, Evan Stade wrote:
> nit: since this is sorta more subtle now, add a comment?

Done.

[commit-bot: I haz the power, 2017-02-01 23:09:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 00:20:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485990562768040,
"parent_rev": "f25b3265878e57ef40f5422716f3b4f6e840a846", "commit_rev":
"9db88f9efd4663a70aa58b115a7b332c20a0c175"}

[commit-bot: I haz the power, 2017-02-02 00:21:02]

Description was changed from

==========
Themes: Remove unused IDs from ThemeProperties

R=estade@chromium.org
CC=erg@chromium.org
==========

to

==========
Themes: Remove unused IDs from ThemeProperties

R=estade@chromium.org
CC=erg@chromium.org

Review-Url: https://codereview.chromium.org/2667753004
Cr-Commit-Position: refs/heads/master@{#447652}
Committed:
https://chromium.googlesource.com/chromium/src/+/9db88f9efd4663a70aa58b115a7b...
==========

[commit-bot: I haz the power, 2017-02-02 00:21:04]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9db88f9efd4663a70aa58b115a7b...