Remove uses of FontList::Derive*(..) in views payments dialogs.

Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834
Review-Url: https://codereview.chromium.org/2899653002
Cr-Original-Commit-Position: refs/heads/master@{#473764}
Committed: https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a39e4d5a2b21f
Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473859}
Committed: https://chromium.googlesource.com/chromium/src/+/17d6248cb822cf06ae06fccd65fbeebfa7b4af2a

[tapted, 2017-05-22 05:46:10]

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

[commit-bot: I haz the power, 2017-05-22 05:46:24]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-22 05:53:49]

Description was changed from

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.


BUG=721834
==========

to

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834
==========

[tapted, 2017-05-22 05:53:49]

tapted@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[commit-bot: I haz the power, 2017-05-22 06:18:36]

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

[commit-bot: I haz the power, 2017-05-22 06:18:37]

Dry run: 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_...)

[tapted, 2017-05-22 07:26:37]

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

[commit-bot: I haz the power, 2017-05-22 07:26:50]

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-05-22 08:57:23]

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

[commit-bot: I haz the power, 2017-05-22 08:57:24]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-22 11:19:41]

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

[commit-bot: I haz the power, 2017-05-22 11:19:56]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-22 11:22:30]

Hi Anthony, please take a look

[commit-bot: I haz the power, 2017-05-22 12:02:38]

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

[commit-bot: I haz the power, 2017-05-22 12:02:39]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-22 13:10:33]

lgtm w/ a question. Thanks a ton :)

https://codereview.chromium.org/2899653002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2899653002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:361:
label->SetFontList(ResourceBundle::GetSharedInstance().GetFontListWithDelta(
Our UI specs typically request that we fallback to BOLD if MEDIUM isn't
available. How do we achieve that?

Also, do we already have a way to map Chrome Typography spec to code? Let's say
our UXer asks us for a "Body 2 - secondary" label, do we have a straightforward
way of getting that? If we do, I can easily do a pass through this code to fix
this since I'm pretty sure our mocks are expressed in terms of the spec already.

[tapted, 2017-05-22 21:09:25]

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

[tapted, 2017-05-22 21:09:26]

+pkasting for the test_browser_dialog.cc changes

https://codereview.chromium.org/2899653002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2899653002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:361:
label->SetFontList(ResourceBundle::GetSharedInstance().GetFontListWithDelta(
On 2017/05/22 13:10:32, anthonyvd wrote:
> Our UI specs typically request that we fallback to BOLD if MEDIUM isn't
> available. How do we achieve that?

There is logic in the typography spec to do this for buttons -
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/harmony/harmony_...

But if these labels should really be using a MEDIUM weight font, UX need to add
one to the harmony spec ( http://crbug.com/691891 ).

Same with CONTEXT_DEPRECATED_SMALL - there is no 11pt font in the Harmony spec
and, when running with --secondary-ui-md, harmony_typography_provider.cc will
provide the default font instead.


> Also, do we already have a way to map Chrome Typography spec to code? Let's
say
> our UXer asks us for a "Body 2 - secondary" label, do we have a
straightforward
> way of getting that? If we do, I can easily do a pass through this code to fix
> this since I'm pretty sure our mocks are expressed in terms of the spec
already.

Yup - the extra arguments to the views::Label constructor are from
foo_typography.h headers. There's a "context" and a "style" (kinda like
element/class in CSS). e.g. chrome_typography.h -
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/harmony/chrome_t...

and the set of styles used in views, from ui/views/style/typography.h

You can also call views::style::GetFont, but passing them to Label encapsulates
the size, line spacing and (soon) color.

[Peter Kasting, 2017-05-22 21:22:35]

LGTM

[tapted, 2017-05-22 21:28:27]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-05-22 21:29:18]

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-05-22 21:39:02]

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

[commit-bot: I haz the power, 2017-05-22 21:39:04]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2017-05-22 23:46:26]

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

[commit-bot: I haz the power, 2017-05-22 23:47:04]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-23 00:08:21]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-05-23 00:08:27]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-23 00:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2899653002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 00:08:41]

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-05-23 00:39:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495498107324760,
"parent_rev": "661549ee2476754868691a897e3752365e204ffd", "commit_rev":
"424d27cceaba0271bc21e9c9b48a39e4d5a2b21f"}

[commit-bot: I haz the power, 2017-05-23 00:39:18]

Description was changed from

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834
==========

to

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834

Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473764}
Committed:
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...
==========

[commit-bot: I haz the power, 2017-05-23 00:39:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...

[cfroussios, 2017-05-23 08:36:06]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2901763002/ by cfroussios@chromium.org.

The reason for reverting is: Breaks compilation
see crbug.com/725407.

[tapted, 2017-05-23 09:51:32]

Description was changed from

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834

Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473764}
Committed:
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...
==========

to

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834

Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473764}
Committed:
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...
==========

[tapted, 2017-05-23 09:57:31]

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

[commit-bot: I haz the power, 2017-05-23 09:57:45]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-23 09:58:58]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-05-23 11:31:22]

gcc...

https://codereview.chromium.org/2899653002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2899653002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:86: ?
STYLE_EMPHASIZED
/b/c/goma_client/gomacc i686-pc-linux-gnu-g++
../../chrome/browser/ui/views/payments/payment_request_views_util.cc: In
function 'std::unique_ptr<views::View>
payments::{anonymous}::GetBaseProfileLabel(payments::AddressStyleType, const
string16&, const string16&, const string16&, bool)':
../../chrome/browser/ui/views/payments/payment_request_views_util.cc:86:32:
error: enumeral mismatch in conditional expression: 'ChromeTextStyle' vs
'views::style::TextStyle' [-Werror=enum-compare]
                                ? STYLE_EMPHASIZED

[tapted, 2017-05-23 11:31:35]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-23 11:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2899653002/#ps80001
(title: "hate you gcc")

[commit-bot: I haz the power, 2017-05-23 11:31:46]

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-05-23 11:35:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495539095342670,
"parent_rev": "0bc5d1bef8d597ac40ba96d4e783fc4dd0bd4958", "commit_rev":
"17d6248cb822cf06ae06fccd65fbeebfa7b4af2a"}

[commit-bot: I haz the power, 2017-05-23 11:36:11]

Description was changed from

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834

Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473764}
Committed:
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...
==========

to

==========
Remove uses of FontList::Derive*(..) in views payments dialogs.

FontList::Derive(..) is slow (typically requiring IPC with the font
server), has unpredictable results with special UI fonts on Mac, and
encourages use of magic numbers like "-1".

Instead, use context and style constants from the Chrome typography
spec, where available. When an equivalent font is not available, use
ResourceBundle to avoid a behavior change in this CL.

BUG=721834

Review-Url: https://codereview.chromium.org/2899653002
Cr-Original-Commit-Position: refs/heads/master@{#473764}
Committed:
https://chromium.googlesource.com/chromium/src/+/424d27cceaba0271bc21e9c9b48a...
Review-Url: https://codereview.chromium.org/2899653002
Cr-Commit-Position: refs/heads/master@{#473859}
Committed:
https://chromium.googlesource.com/chromium/src/+/17d6248cb822cf06ae06fccd65fb...
==========

[commit-bot: I haz the power, 2017-05-23 11:36:12]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/17d6248cb822cf06ae06fccd65fb...