Fix regions generation on windows.

ui/gfx/path_win_unittest.cc

Index: ui/gfx/path_win_unittest.cc
diff --git a/ui/gfx/path_win_unittest.cc b/ui/gfx/path_win_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d059b1955294fd5b06411796152cfc1aaf87401f
--- /dev/null
+++ b/ui/gfx/path_win_unittest.cc
@@ -0,0 +1,98 @@
+// Copyright 2014 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.
+// Copyright (c) 2014 Yandex LLC. All rights reserved.
+// Author: Aleksandr Derbenev <alex-ac@yandex-team.ru>

[Wez, 2014/10/23 18:50:05]

You'll need to remove this, since you're contributing this code to Chromium.

[alex-ac, 2014/10/24 07:05:11]

Sorry, It's a copy-paste artifact.

+
+#include "base/win/scoped_gdi_object.h"
+#include "skia/ext/skia_utils_win.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkRRect.h"
+#include "ui/gfx/path.h"
+#include "ui/gfx/path_win.h"
+
+namespace {
+
+std::vector<SkIRect> GetRectsFromHRGN(HRGN region) {

[Wez, 2014/10/23 18:50:05]

nit: for readability, suggest blank line between the logical blocks in this
function (i.e. one before each comment).

[alex-ac, 2014/10/24 07:05:11]

Done.

+  // Determine the size of output buffer required to receive the region.
+  DWORD bytes_size = GetRegionData(region, 0, NULL);
+  CHECK(bytes_size != 0);

[Wez, 2014/10/23 18:50:05]

CHECK_NE, here and below.

[alex-ac, 2014/10/24 07:05:11]

Done.

+  // Fetch the Windows RECTs that comprise the region.
+  std::vector<char> buffer(bytes_size);;

[Wez, 2014/10/23 18:50:05]

nit: duplicate ;

[alex-ac, 2014/10/24 07:05:10]

Done.

+  LPRGNDATA region_data = reinterpret_cast<LPRGNDATA>(buffer.data());
+  DWORD result = GetRegionData(region, bytes_size, region_data);
+  CHECK(result == bytes_size);
+  // Pull out the rectangles into a SkIRect vector to pass to setRects().

[Wez, 2014/10/23 18:50:05]

nit: ... to return to caller.

[alex-ac, 2014/10/24 07:05:10]

Done.

+  const LPRECT rects = reinterpret_cast<LPRECT>(&region_data->Buffer[0]);
+  std::vector<SkIRect> sk_rects(region_data->rdh.nCount);
+  std::transform(rects, rects + region_data->rdh.nCount,

[Wez, 2014/10/23 18:50:05]

nit: why not rects.begin(), rects.end()?

[alex-ac, 2014/10/24 07:05:10]

'rects' is an array it hasn't got begin() and end().

[Wez, 2014/10/24 23:07:20]

Acknowledged.

+                 sk_rects.begin(), skia::RECTToSkIRect);
+  return sk_rects;
+}
+
+}  // namespace
+
+// Test that rectangle with round corners stil has _round_ corners after

[Wez, 2014/10/23 18:50:05]

nit: why the _emphasis_ on the second round?

[alex-ac, 2014/10/24 07:05:11]

Done.

+// converting from SkPath to the HRGN.
+// Old implementation of the CreateHRGNFromSkPath operates with polygons and
+// can't make round corners.

[Wez, 2014/10/23 18:50:05]

nit: this comment re old implementation adds nothing.

[alex-ac, 2014/10/24 07:05:10]

Done.

+TEST(CreateHRGNFromSkPathTest, RoundCornerTest) {
+  const SkIRect rects[] = {
+      { 17, 0, 33, 1},
+      { 12, 1, 38, 2},
+      { 11, 2, 39, 3},
+      { 9, 3, 41, 4},
+      { 8, 4, 42, 5},
+      { 6, 5, 44, 6},
+      { 5, 6, 45, 8},
+      { 4, 8, 46, 9},
+      { 3, 9, 47, 11},
+      { 2, 11, 48, 12},
+      { 1, 12, 49, 17},
+      { 0, 17, 50, 33},
+      { 1, 33, 49, 38},
+      { 2, 38, 48, 39},
+      { 3, 39, 47, 41},
+      { 4, 41, 46, 42},
+      { 5, 42, 45, 44},
+      { 6, 44, 44, 45},
+      { 8, 45, 42, 46},
+      { 9, 46, 41, 47},
+      { 11, 47, 39, 48},
+      { 12, 48, 38, 49},
+      { 17, 49, 33, 50},
+  };
+
+  gfx::Path path;
+  SkRRect rrect;
+  rrect.setRectXY(SkRect::MakeWH(50, 50), 20, 20);
+  path.addRRect(rrect);
+  base::win::ScopedRegion region(gfx::CreateHRGNFromSkPath(path));
+  const std::vector<SkIRect>& region_rects = GetRectsFromHRGN(region);
+  EXPECT_EQ(arraysize(rects), region_rects.size());
+  for (size_t i = 0; i < arraysize(rects) && i < region_rects.size(); ++i)
+    EXPECT_EQ(rects[i], region_rects[i]);
+}
+
+TEST(CreateHRGNFromSkPathTest, NonContiguousPath) {

[Wez, 2014/10/23 18:50:05]

nit: Include a one-line comment to explain what this test is for.

[alex-ac, 2014/10/24 07:05:10]

Done.

+  const SkIRect rects[] = {
+      { 0, 0, 50, 50},
+      { 100, 100, 150, 150},
+  };
+
+  gfx::Path path;
+  for (const SkIRect& rect : rects) {
+    path.addRect(SkRect::Make(rect));
+  }
+  base::win::ScopedRegion region(gfx::CreateHRGNFromSkPath(path));
+  const std::vector<SkIRect>& region_rects = GetRectsFromHRGN(region);
+  EXPECT_EQ(arraysize(rects), region_rects.size());
+  for (size_t i = 0; i < arraysize(rects) && i < region_rects.size(); ++i)
+    EXPECT_EQ(rects[i], region_rects[i]);
+}
+
+TEST(CreateHRGNFromSkPathTest, EmptyPath) {
+  gfx::Path path;
+  EXPECT_EQ(NULL, gfx::CreateHRGNFromSkPath(path));

[Wez, 2014/10/23 18:50:05]

This test is wrong; an empty path should return a valid empty region, not NULL -
NULL indicates no-region, i.e. that a windows should retain its natural shape.

[alex-ac, 2014/10/24 07:05:10]

 There are many NonClientFrameView implementations where GetWindowMask() methods
are empty. Path is empty for those views. If empty region will be passed into
SetWindowRgn, than window will be hidden completely. Otherwise if NULL will be
passed into SetWindowRgn, than window will be drawn without clipping. I think
that NULL is right value for empty path.

[Wez, 2014/10/24 23:07:20]

There are only actually four calls to the API, though:
1. ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:   
GetWidget()->non_client_view()->GetWindowMask(size, path);
2. ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:     
widget->non_client_view()->GetWindowMask(bounds_.size(), &window_mask);
3. ui/views/win/hwnd_message_handler.cc:   
delegate_->GetWindowMask(gfx::Size(window_rect.right - window_rect.left,
4. ui/views/window/non_client_view.cc:  frame_view_->GetWindowMask(size,
window_mask);

Of those only #2 and #3 are actual applications of the mask to a region, and #2
already special-cases the empty case as per the GetWindowMask requirements.

So I think it makes sense to change the SkPath to Hrgn API to return an empty
region for an empty path, document that behaviour in the comment on the
function, and fix call-site #3 to special-case empty to mean no shape should be
applied.

+}
+