"Bootstrap" a toolkit-views Typography spec.

ui/views/style/typography.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/style/typography.h"
+
+#include "base/logging.h"
+#include "ui/base/default_style.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace views {
+namespace {
+
+constexpr int kHeadlineDelta = 8;
+constexpr int kDialogMessageDelta = 1;

[Peter Kasting, 2017/03/14 23:53:33]

Nit: I tend to find it easier to understand when constants appear locally in the
scope that uses them, so I'm not scanning back up through the file to figure
them out.

[tapted, 2017/03/15 08:26:24]

Done.

+
+// The default provider aims to provide values to match pre-Harmony constants
+// for the given contexts so that old UI does not change.
+class DefaultProvider : public TypographyProvider {
+ public:
+  DefaultProvider() {}
+
+  const gfx::FontList& GetFont(TextContext context, TextStyle style) override;
+  SkColor GetColor(TextContext context, TextStyle style) override;
+  int GetLineSpacing(TextContext context, TextStyle style) override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DefaultProvider);
+};
+
+const gfx::FontList& DefaultProvider::GetFont(TextContext context,
+                                              TextStyle style) {
+  int size_delta = 0;
+  gfx::Font::Weight font_weight = gfx::Font::Weight::NORMAL;

[Peter Kasting, 2017/03/14 23:53:33]

Nit: Declare this as close to first use as possible

[tapted, 2017/03/15 08:26:24]

Done.

+  switch (context) {
+    case TextContext::HEADLINE:
+      size_delta = kHeadlineDelta;
+      break;
+    case TextContext::DIALOG_TITLE:
+      size_delta = ui::kTitleFontSizeDelta;
+      break;
+    case TextContext::DIALOG_MESSAGE:
+      // Note: Not using ui::kMessageFontSizeDelta, so is 13pt in most cases.
+      size_delta = kDialogMessageDelta;
+      break;
+    case TextContext::DIALOG_TEXT_SMALL:
+    case TextContext::CONTROL_LABEL:
+    case TextContext::FIELD:
+    case TextContext::BUTTON_TEXT:
+      size_delta = ui::kLabelFontSizeDelta;
+      break;
+    case TextContext::DEPRECATED_SMALL:
+      size_delta = ResourceBundle::kSmallFontDelta;
+      break;
+  }
+  switch (style) {
+    case TextStyle::WEB_DOMAIN:
+    case TextStyle::ACTIVE_TAB:
+    case TextStyle::EXTENSION_NAME:
+      font_weight = gfx::Font::Weight::BOLD;
+      break;
+    default:
+      break;
+  }
+  constexpr gfx::Font::FontStyle kFontStyle = gfx::Font::NORMAL;
+  return ResourceBundle::GetSharedInstance().GetFontListWithDelta(
+      size_delta, kFontStyle, font_weight);
+}
+
+SkColor DefaultProvider::GetColor(TextContext context, TextStyle style) {
+  return SK_ColorBLACK;
+}
+
+int DefaultProvider::GetLineSpacing(TextContext context, TextStyle style) {
+  return 0;
+}
+
+std::unique_ptr<TypographyProvider>& Storage() {
+  CR_DEFINE_STATIC_LOCAL(std::unique_ptr<TypographyProvider>, storage,
+                         (new DefaultProvider));

[Peter Kasting, 2017/03/14 23:53:33]

Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean up on
exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support SetProvider()
below to allow this to be reset while running, in which case we do want to clean
up.

Can we avoid SetProvider() by having this create the correct type of provider
(harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming there that
we may want one or the other, but it won't change at runtime.

Then we could return a raw pointer instead of a non-const-ref-to-unique-ptr,
which raised my eyebrows.

[tapted, 2017/03/15 08:26:24]

Why do we need cleanup in that case? Threads on chromium-dev suggest we want to
move away from exit-time destructors in a more general way. If it's needed, I
was thinking views::Typography could provide a Reset() that just sets the
provider back to DefaultProvider.
I don't think we can do that -- views::Typography can't know about the other
providers the way that LayoutDelegate::Get() is able to do it for
HarmonyLayoutDelegate.
The NCRTUP was to avoid using special concepts that base::Singleton/LazyInstance
introduce, while still avoiding raw pointers so that the ownership transfer is
clear.

We could switch to LazyInstance, but base/lazy_instance.h says "This class is
useful for places you would normally use a function-level static, but you need
to have guaranteed thread-safety." - Chrome now compiles with thread-safe
statics (and this use doesn't really need to be thread-safe in any case), so I
think we can do the same thing without introducing new concepts (just
CR_DEFINE_STATIC_LOCAL I guess, but it's pretty simple).

[Peter Kasting, 2017/03/15 10:01:48]

We need to avoid leaking the thing we're replacing, since the program isn't
shutting down.
Unless we provide a delegate that does this, or split Typography across the
views/chrome boundary a la ViewsDelegate (in which case we address the enum that
way too).

Swapping providers at runtime feels error-prone (what if we race the swap and
query this thing ahead of time?), which would be another nice thing to avoid.

[tapted, 2017/03/15 11:04:06]

I think that's taken care of - Storage() exposes its local member by reference,
so assigning to it will free whatever is held in the unique_ptr (it's just the
unique_ptr that's leaked, not the thing inside it).
Hm. I wanted to explore something that provides a stronger "not going to be
null" guarantee than, e.g., ViewsDelegate. ui::NativeTheme does something like
this, but that's just per-platform (doesn't have to be delegated really). But
here we will always need a runtime delegate, since it's behind a chrome://flag

We could start with the provider being null, and assert() if it's queried before
being Set(), or Set() again when non-null. This would allow us to guarantee that
its set exactly once.

Then the 'DefaultTypographyProvider' would be installed perhaps by
TestViewsDelegate rather than being available by default. I think this will
work.

[Peter Kasting, 2017/03/15 17:52:47]

That was my point -- the unique_ptr is the thing that makes this possible.  I
was trying to get rid of the unique_ptr itself, which means disallowing changing
this at runtime.
ViewsDelegate should be guaranteed to be non-null.  All null checks in the code
should be removed.  Any test that then breaks because of not having a
ViewsDelegate must instantiate one.

Sorry if this didn't get communicated more broadly :)

[tapted, 2017/03/15 22:43:34]

I'm going to try piggy-backing off the ViewsDelegate some more.

and probably add (namespace) views::typography::GetFont(.., ..) to make the call
sites bearable :)

+  return storage;
+}
+
+}  // namespace
+
+// static
+void Typography::SetProvider(std::unique_ptr<TypographyProvider> provider) {
+  DCHECK(provider);
+  Storage() = std::move(provider);
+}
+
+// static
+const gfx::FontList& Typography::GetFont(TextContext context, TextStyle style) {
+  return Storage()->GetFont(context, style);
+}
+
+// static
+SkColor Typography::GetColor(TextContext context, TextStyle style) {
+  return Storage()->GetColor(context, style);
+}
+
+// static
+int Typography::GetLineSpacing(TextContext context, TextStyle style) {
+  return Storage()->GetLineSpacing(context, style);
+}
+
+}  // namespace views