More pre-MD odds and ends.

Remove more pre-MD odds and ends.

BUG=648281
Committed: https://crrev.com/b8867dff1a47afb775c2c83cf44b26e16715e2b9
Cr-Commit-Position: refs/heads/master@{#429631}

[Evan Stade, 2016-10-24 18:08:36]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 18:09:11]

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, 2016-10-24 18:36:36]

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

[commit-bot: I haz the power, 2016-10-24 18:36:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-10-24 19:14:59]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 19:15:38]

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, 2016-10-24 20:35:02]

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

[commit-bot: I haz the power, 2016-10-24 20:35:03]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Evan Stade, 2016-10-25 22:01:05]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 22:02:22]

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

[Evan Stade, 2016-10-25 22:10:31]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 22:10:54]

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, 2016-10-25 23:07:52]

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

[commit-bot: I haz the power, 2016-10-25 23:07:53]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-26 00:35:59]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2016-10-26 00:36:00]

https://codereview.chromium.org/2447553003/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.cc (right):

https://codereview.chromium.org/2447553003/diff/40001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.cc:78:
result->default_icon.Add(19, path);
I could alternatively make this a constant (E_I_ACTION_LEGACY) in constants.h,
but it needs to stop being part of the enum for the purpose of switch statements
not wanting to handle it. Since this is the only place it's used (in production
code) I didn't bother adding to constants.h.

[Peter Kasting, 2016-10-26 03:28:47]

LGTM

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (left):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:2144: ?
chrome::kThemePackMaterialDesignFilename
Do we need some kind of cleanup pass to avoid leaving the old .pak file lying
around forever?  E.g. Add something to the code that just checks for and deletes
the old file name unconditionally, leave it in for a while, take it out someday.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:29: const SkColor kDefaultColorFrame =
SkColorSetRGB(0xCC, 0xCC, 0xCC);
Nit: Can be moved above #if (common to both arms)

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:38: #if defined(OS_CHROMEOS)
Nit: This arm can be removed (same as arm below Mac)

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:65: constexpr SkColor
kDefaultColorBookmarkTextIncognito = SK_ColorWHITE;
Nit: I'm kinda inclined to combine the tab and bookmark text color constants,
but I don't have a good name.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:70: SkColorSetARGB(0x8A, 0xFF, 0xFF,
0xFF);
Nit: SkColorSetA(SK_ColorWHITE, 0x8A)

Or maybe (kDefaultColorBookmarkTextIncognito, 0x8A), but then it's kinda weird
that the non-incognito version isn't defined in terms of
kDefaultColorBookmarkText.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:109:
ExtensionAction::ActionIconSize());
Nit: I suggest using a temp for this value, since the name implies the call is
expensive.

(Separately possible: Change this function to a cheap inlined getter, maybe make
constexpr as well)

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
File
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc:128:
extension_misc::EXTENSION_ICON_BITTY);
Shouldn't this also use ExtensionAction::ActionIconSize() like
page_action_image_view.cc?  (Or that file could expose some helper to get the
right size, which both these places in turn could call)

https://codereview.chromium.org/2447553003/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.cc:77: // time of
kPageActionIcons).
Nit: Shorter: "Extension icons were 19 DIP when kPageActionIcons was supported."

[Evan Stade, 2016-10-26 14:09:16]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 14:09:30]

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, 2016-10-26 14:41:54]

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

[commit-bot: I haz the power, 2016-10-26 14:41:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2016-11-02 00:41:13]

dang, forgot about this CL because it had draft comments

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (left):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:2144: ?
chrome::kThemePackMaterialDesignFilename
On 2016/10/26 03:28:46, Peter Kasting wrote:
> Do we need some kind of cleanup pass to avoid leaving the old .pak file lying
> around forever?  E.g. Add something to the code that just checks for and
deletes
> the old file name unconditionally, leave it in for a while, take it out
someday.

I suppose that would be good. .pak files are a few MB. They would be deleted any
time you install a new theme, but people might not do that often. I added a pass
that removes the Material .pak when re-generating a .pak (which should happen
for everyone since I bumped the theme version number). Also added a test.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:29: const SkColor kDefaultColorFrame =
SkColorSetRGB(0xCC, 0xCC, 0xCC);
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Nit: Can be moved above #if (common to both arms)

Done.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:38: #if defined(OS_CHROMEOS)
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Nit: This arm can be removed (same as arm below Mac)

Done.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:65: constexpr SkColor
kDefaultColorBookmarkTextIncognito = SK_ColorWHITE;
On 2016/10/26 03:28:46, Peter Kasting wrote:
> Nit: I'm kinda inclined to combine the tab and bookmark text color constants,
> but I don't have a good name.

Done. (ToolbarText)

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.cc:70: SkColorSetARGB(0x8A, 0xFF, 0xFF,
0xFF);
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Nit: SkColorSetA(SK_ColorWHITE, 0x8A)
> 
> Or maybe (kDefaultColorBookmarkTextIncognito, 0x8A), but then it's kinda weird
> that the non-incognito version isn't defined in terms of
> kDefaultColorBookmarkText.

Done.

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:109:
ExtensionAction::ActionIconSize());
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Nit: I suggest using a temp for this value, since the name implies the call is
> expensive.
> 
> (Separately possible: Change this function to a cheap inlined getter, maybe
make
> constexpr as well)

How do you feel about creating a ctor for gfx::Size that has one param and
creates a square? It seems to me that this comes up a lot. (Would be a separate
CL.)

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
File
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view_interactive_uitest.cc:128:
extension_misc::EXTENSION_ICON_BITTY);
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Shouldn't this also use ExtensionAction::ActionIconSize() like
> page_action_image_view.cc?  (Or that file could expose some helper to get the
> right size, which both these places in turn could call)

Done.

https://codereview.chromium.org/2447553003/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.cc:77: // time of
kPageActionIcons).
On 2016/10/26 03:28:47, Peter Kasting wrote:
> Nit: Shorter: "Extension icons were 19 DIP when kPageActionIcons was
supported."

Done.

[Evan Stade, 2016-11-02 00:41:20]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:42:03]

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

[Evan Stade, 2016-11-02 00:43:58]

estade@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, sky@chromium.org

[Evan Stade, 2016-11-02 00:43:59]

+OWNERS. ptal at

rdevlin.cronin:
  chrome/browser/extensions/*
  chrome/common/extensions/api/*
  extensions/*
sky:
  chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc
  chrome/common/chrome_constants.cc
  chrome/common/chrome_constants.h

[Evan Stade, 2016-11-02 00:44:14]

Description was changed from

==========
More pre-MD odds and ends.

BUG=648281
==========

to

==========
Remove more pre-MD odds and ends.

BUG=648281
==========

[Peter Kasting, 2016-11-02 00:47:44]

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/2447553003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:109:
ExtensionAction::ActionIconSize());
On 2016/11/02 00:41:13, Evan Stade wrote:
> On 2016/10/26 03:28:47, Peter Kasting wrote:
> > Nit: I suggest using a temp for this value, since the name implies the call
is
> > expensive.
> > 
> > (Separately possible: Change this function to a cheap inlined getter, maybe
> make
> > constexpr as well)
> 
> How do you feel about creating a ctor for gfx::Size that has one param and
> creates a square? It seems to me that this comes up a lot. (Would be a
separate
> CL.)

I'd ask danakj this.

It does come up a lot, so I'm weakly supportive, although I dunno if it's a
little unintuitive (then again, what else would it mean but "make a square"?).

[Devlin, 2016-11-02 00:52:12]

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
File extensions/common/constants.h (left):

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
extensions/common/constants.h:200: EXTENSION_ICON_ACTION = 19,
I think I'd prefer we keep this, and just update it to also be 16. 
Conceptually, it's use remains the same - it's the size of the extension action
in the toolbar.

[commit-bot: I haz the power, 2016-11-02 01:19:26]

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

[commit-bot: I haz the power, 2016-11-02 01:19:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2016-11-02 03:05:14]

chrome_constants LGTM, but get a local owner for the DVLOG change.

https://codereview.chromium.org/2447553003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc
(right):

https://codereview.chromium.org/2447553003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc:138:
DVLOG(1) << "Loading content for a profile without "
Please ping the owner of this if they are ok with the cange. chromeos seems to
favor logging more in certain cases as otherwise they have no hope of tracking
down issues.

[Evan Stade, 2016-11-02 12:58:35]

estade@chromium.org changed reviewers:
+ achuith@chromium.org

[Evan Stade, 2016-11-02 12:58:36]

https://codereview.chromium.org/2447553003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc
(right):

https://codereview.chromium.org/2447553003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/signin/merge_session_throttling_utils.cc:138:
DVLOG(1) << "Loading content for a profile without "
On 2016/11/02 03:05:14, sky wrote:
> Please ping the owner of this if they are ok with the cange. chromeos seems to
> favor logging more in certain cases as otherwise they have no hope of tracking
> down issues.

+achuith@chromium.org, wdyt? I made this change because this warning was
spamming my console and making it hard to read test output. Rest of the file
seems to use DVLOG.

[Evan Stade, 2016-11-02 13:14:05]

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
File extensions/common/constants.h (left):

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
extensions/common/constants.h:200: EXTENSION_ICON_ACTION = 19,
On 2016/11/02 00:52:12, Devlin wrote:
> I think I'd prefer we keep this, and just update it to also be 16. 
> Conceptually, it's use remains the same - it's the size of the extension
action
> in the toolbar.

I think that creates a mismatch between this value, which describes a semantic
usage, and the rest of the values in this enum, which are ambivalent about how
they're applied. The logical connection between 16 and extension actions is
provided by ExtensionAction::ActionIconSize().

Nevertheless, done.

[Evan Stade, 2016-11-02 13:14:17]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 13:14:34]

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, 2016-11-02 13:47:36]

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

[commit-bot: I haz the power, 2016-11-02 13:47:37]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Evan Stade, 2016-11-02 18:05:23]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:06:00]

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, 2016-11-02 19:17:57]

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

[commit-bot: I haz the power, 2016-11-02 19:17:57]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-02 23:16:52]

extensions lgtm.  Sorry for the churn.

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
File extensions/common/constants.h (left):

https://codereview.chromium.org/2447553003/diff/100001/extensions/common/cons...
extensions/common/constants.h:200: EXTENSION_ICON_ACTION = 19,
On 2016/11/02 13:14:05, Evan Stade wrote:
> On 2016/11/02 00:52:12, Devlin wrote:
> > I think I'd prefer we keep this, and just update it to also be 16. 
> > Conceptually, it's use remains the same - it's the size of the extension
> action
> > in the toolbar.
> 
> I think that creates a mismatch between this value, which describes a semantic
> usage, and the rest of the values in this enum, which are ambivalent about how
> they're applied. The logical connection between 16 and extension actions is
> provided by ExtensionAction::ActionIconSize().
> 
> Nevertheless, done.

You've convinced me.  For some reason in the original patch, I swear I saw more
usages of ICON_BITTY than of using ExtensionAction::ActionIconSize().

[Evan Stade, 2016-11-02 23:37:20]

estade@chromium.org changed reviewers:
- achuith@chromium.org

[Evan Stade, 2016-11-02 23:37:20]

removed logging change, moving achuithb to cc

[Evan Stade, 2016-11-02 23:37:28]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-02 23:37:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2447553003/#ps160001
(title: "remove enum value")

[commit-bot: I haz the power, 2016-11-02 23:38:07]

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, 2016-11-03 00:20:20]

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

[commit-bot: I haz the power, 2016-11-03 00:20:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2016-11-03 15:55:37]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-03 15:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2447553003/#ps180001
(title: "fix typo")

[commit-bot: I haz the power, 2016-11-03 15:56:00]

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, 2016-11-03 17:34:00]

Description was changed from

==========
Remove more pre-MD odds and ends.

BUG=648281
==========

to

==========
Remove more pre-MD odds and ends.

BUG=648281
==========

[commit-bot: I haz the power, 2016-11-03 17:34:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-03 17:48:10]

Description was changed from

==========
Remove more pre-MD odds and ends.

BUG=648281
==========

to

==========
Remove more pre-MD odds and ends.

BUG=648281

Committed: https://crrev.com/b8867dff1a47afb775c2c83cf44b26e16715e2b9
Cr-Commit-Position: refs/heads/master@{#429631}
==========

[commit-bot: I haz the power, 2016-11-03 17:48:11]

Patchset 10 (id:??) landed as
https://crrev.com/b8867dff1a47afb775c2c83cf44b26e16715e2b9
Cr-Commit-Position: refs/heads/master@{#429631}