Font fallback for UI

ui/gfx/win/text_analysis_source_unittest.cc

Index: ui/gfx/win/text_analysis_source_unittest.cc
diff --git a/ui/gfx/win/text_analysis_source_unittest.cc b/ui/gfx/win/text_analysis_source_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dda3ee8d77e4cbe827935281b92609ab6af1036d
--- /dev/null
+++ b/ui/gfx/win/text_analysis_source_unittest.cc
@@ -0,0 +1,137 @@
+// Copyright 2016 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/gfx/win/text_analysis_source.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mswr = Microsoft::WRL;
+
+namespace gfx {
+namespace win {
+
+namespace {
+
+DWRITE_READING_DIRECTION kReadingDirection =
+    DWRITE_READING_DIRECTION_TOP_TO_BOTTOM;
+const wchar_t* kLocale = L"hi-in";
+const wchar_t kText[] = L"sample text";
+const size_t kTextLength = _countof(kText) - 1;
+}

[msw, 2016/06/27 20:45:37]

nit: blank line above and add a comment: "}  // namespace".

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+
+class TextAnalysisSourceTest : public testing::Test {
+ public:
+  TextAnalysisSourceTest() {

[msw, 2016/06/27 20:45:37]

nit: define a dtor or use "= default;"?

[Ilya Kulshin, 2016/06/28 18:08:01]

This class does not require custom destruction logic. I'm not sure what would be
the benefit of defining a default destructor.

+    mswr::ComPtr<IDWriteFactory> factory;
+    CreateDWriteFactory(&factory);
+
+    mswr::ComPtr<IDWriteNumberSubstitution> number_substitution;
+    factory->CreateNumberSubstitution(DWRITE_NUMBER_SUBSTITUTION_METHOD_NONE,
+                                      kLocale, true /* ignoreUserOverride */,
+                                      &number_substitution);
+
+    mswr::MakeAndInitialize<TextAnalysisSource>(
+        &source_, kText, kLocale, number_substitution.Get(), kReadingDirection);
+  }
+
+  void CreateDWriteFactory(IUnknown** factory) {

[msw, 2016/06/27 20:45:37]

Can you use gfx::win::CreateDWriteFactory instead?

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+    using DWriteCreateFactoryProc = decltype(DWriteCreateFactory)*;
+    HMODULE dwrite_dll = LoadLibraryW(L"dwrite.dll");
+    if (!dwrite_dll)
+      return;
+
+    DWriteCreateFactoryProc dwrite_create_factory_proc =
+        reinterpret_cast<DWriteCreateFactoryProc>(
+            GetProcAddress(dwrite_dll, "DWriteCreateFactory"));
+    if (!dwrite_create_factory_proc)
+      return;
+
+    dwrite_create_factory_proc(DWRITE_FACTORY_TYPE_SHARED,
+                               __uuidof(IDWriteFactory), factory);
+  }
+
+  mswr::ComPtr<TextAnalysisSource> source_;

[msw, 2016/06/27 20:45:36]

nit: make private; add a protected/public accessor.

[Ilya Kulshin, 2016/06/28 18:08:01]

Made protected.

+};

[msw, 2016/06/27 20:45:37]

nit: DISALLOW_COPY_AND_ASSIGN

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+
+TEST_F(TextAnalysisSourceTest, TestGetLocaleName) {
+  UINT32 length = 0;
+  const wchar_t* locale_name = nullptr;
+  HRESULT hr;

[msw, 2016/06/27 20:45:36]

nit: init to E_FAIL or E_UNEXPECTED; ditto elsewhere.

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+  hr = source_->GetLocaleName(0, &length, &locale_name);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(kTextLength, length);
+  EXPECT_STREQ(kLocale, locale_name);
+
+  hr = source_->GetLocaleName(kTextLength, &length, &locale_name);

[msw, 2016/06/27 20:45:37]

nit: add a comment explaining why this fails? (is it because it's attempting to
determine the locale for an out-of-bounds position?)

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+
+  EXPECT_TRUE(FAILED(hr));
+}
+
+TEST_F(TextAnalysisSourceTest, TestGetNumberSubstitution) {
+  UINT32 length = 0;
+  mswr::ComPtr<IDWriteNumberSubstitution> number_substitution;
+  HRESULT hr;
+  hr = source_->GetNumberSubstitution(0, &length, &number_substitution);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(kTextLength, length);
+  EXPECT_NE(nullptr, number_substitution.Get());
+
+  hr = source_->GetNumberSubstitution(kTextLength, &length,

[msw, 2016/06/27 20:45:36]

ditto nit: add a simple comment on why this fails.

[Ilya Kulshin, 2016/06/28 18:08:01]

Done.

+                                      &number_substitution);
+
+  EXPECT_TRUE(FAILED(hr));
+}
+
+TEST_F(TextAnalysisSourceTest, TestGetParagraphReadingDirection) {
+  EXPECT_EQ(kReadingDirection, source_->GetParagraphReadingDirection());
+}
+
+TEST_F(TextAnalysisSourceTest, TestGetTextAtPosition) {
+  UINT32 length = 0;
+  const wchar_t* text = nullptr;
+  HRESULT hr;
+  hr = source_->GetTextAtPosition(0, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(kTextLength, length);
+  EXPECT_STREQ(kText, text);
+
+  hr = source_->GetTextAtPosition(5, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(kTextLength - 5, length);
+  EXPECT_STREQ(kText + 5, text);
+
+  hr = source_->GetTextAtPosition(kTextLength, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));

[msw, 2016/06/27 20:45:37]

nit: odd that this passes... should you check that it fails with kTextLength + 1
or is that simply attempting to access invalid memory?

[Ilya Kulshin, 2016/06/28 18:08:01]

My reading of
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371330(v=vs.85).aspx
is that this function should succeed and return null when called with an out of
range index.

+  EXPECT_EQ(nullptr, text);
+}
+
+TEST_F(TextAnalysisSourceTest, TestGetTextBeforePosition) {
+  UINT32 length = 0;
+  const wchar_t* text = nullptr;
+  HRESULT hr;
+  hr = source_->GetTextBeforePosition(0, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(nullptr, text);
+
+  hr = source_->GetTextBeforePosition(5, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(5u, length);
+  EXPECT_STREQ(kText, text);
+
+  hr = source_->GetTextBeforePosition(kTextLength, &text, &length);
+
+  EXPECT_TRUE(SUCCEEDED(hr));
+  EXPECT_EQ(kTextLength, length);
+  EXPECT_STREQ(kText, text);
+}
+
+}  // namespace win
+}  // namespace gfx