Move legacyCreateTypeface to SkFontStyle.

skia/ext/skia_utils_base.cc

Index: skia/ext/skia_utils_base.cc
diff --git a/skia/ext/skia_utils_base.cc b/skia/ext/skia_utils_base.cc
index f026c07adecd83592f51395ff651397e5e1ff096..335eb27d70719f23de7dc2abefe6dba822085e27 100644
--- a/skia/ext/skia_utils_base.cc
+++ b/skia/ext/skia_utils_base.cc
@@ -40,6 +40,24 @@ bool ReadSkFontIdentity(base::PickleIterator* iter,
   return true;
 }
 
+bool ReadSkFontStyle(base::PickleIterator* iter, SkFontStyle* style) {
+  uint16_t reply_weight;
+  uint16_t reply_width;
+  uint16_t reply_slant;
+
+  if (!iter->ReadUInt16(&reply_weight) ||
+      !iter->ReadUInt16(&reply_width) ||
+      !iter->ReadUInt16(&reply_slant))
+    return false;
+
+  if (style) {

[dcheng, 2016/04/12 17:11:23]

Seems odd that this is an optional parameter. It doesn't need to be, does it?

(I see the other helpers allow this, but that also seems odd to me)

[bungeman-chromium, 2016/04/12 18:15:38]

I think the original intent was to avoid the locals and the copy in
font_config_ipc_linux.cc. One could just pass 'outFontIdentity' to
ReadSkFontIdentity, it would read in the data (since that always needs to be
done), and if 'outFontIdentity' were nullptr just skip setting 'identity'. It's
possible to see how this evolved from
https://codereview.chromium.org/12391070/diff/1/content/common/font_config_ip...
.

Of course, this never happens because font_config_ipc_linux.cc passes the
address of a local, ReadSkFontIdentity runs, and then if 'outFontIdentity' is
not nullptr a copy happens. Allowing the parameter to be optional was to avoid
that copy and just write directly into 'outFontIdentity' when possible. Oddly,
when this was introduced this wasn't taken advantage of
https://codereview.chromium.org/12391070/diff2/22005:54001/content/common/fon...
.


The documentation in the header file also supports this.

I can remove these checks and assert that the out parameter is not nullptr, or I
could update the calling code to just pass through its own out parameter
pointers (as appears to have been the original intention of doing things this
way), or I could just leave these as-is. Whichever way seems best, I think these
should all be consistent.

[bungeman-chromium, 2016/04/12 18:47:04]

To provide an example, taking advantage of the way this is currently written (at
PS7) looks like

--- a/content/common/font_config_ipc_linux.cc
+++ b/content/common/font_config_ipc_linux.cc
@@ -87,28 +87,12 @@ bool FontConfigIPC::matchFamilyName(const char familyName[],
   base::Pickle reply(reinterpret_cast<char*>(reply_buf), r);
   base::PickleIterator iter(reply);
   bool result;
-  if (!iter.ReadBool(&result))
+  if (!iter.ReadBool(&result) || !result)
     return false;
-  if (!result)
-    return false;
-
-  SkString     reply_family;
-  FontIdentity reply_identity;
-  SkFontStyle  reply_style;
-  if (!skia::ReadSkString(&iter, &reply_family) ||
-      !skia::ReadSkFontIdentity(&iter, &reply_identity) ||
-      !skia::ReadSkFontStyle(&iter, &reply_style)) {
-    return false;
-  }
-
-  if (outFontIdentity)
-    *outFontIdentity = reply_identity;
-  if (outFamilyName)
-    *outFamilyName = reply_family;
-  if (outStyle)
-    *outStyle = reply_style;
 
-  return true;
+  return skia::ReadSkString(&iter, outFamilyName) &&
+         skia::ReadSkFontIdentity(&iter, outFontIdentity) &&
+         skia::ReadSkFontStyle(&iter, outStyle);
 }

which is quite a bit of code removed and copies which no longer need to be made
in the common case. I'll have to ask reed why he didn't do this the first time
around (though that was years ago, so probably doesn't remember).

+    *style = SkFontStyle(reply_weight,
+                         reply_width,
+                         static_cast<SkFontStyle::Slant>(reply_slant));
+  }
+  return true;
+}
+
 bool WriteSkString(base::Pickle* pickle, const SkString& str) {
   return pickle->WriteData(str.c_str(), str.size());
 }
@@ -51,6 +69,12 @@ bool WriteSkFontIdentity(base::Pickle* pickle,
          WriteSkString(pickle, identity.fString);
 }
 
+bool WriteSkFontStyle(base::Pickle* pickle, SkFontStyle style) {
+  return pickle->WriteUInt16(style.weight()) &&
+         pickle->WriteUInt16(style.width()) &&
+         pickle->WriteUInt16(style.slant());
+}
+
 SkPixelGeometry ComputeDefaultPixelGeometry() {
     SkFontHost::LCDOrder order = SkFontHost::GetSubpixelOrder();
     if (SkFontHost::kNONE_LCDOrder == order) {