Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
673623002:
Fix regions generation on windows. (Closed)
DescriptionFix regions generation on windows.
On windows HRGN for window is generated from the SkPath using dumb
function that doesn`t respects anything except simple polygons. It's
impossible to create window with round corners on windows. Replacing
implementation of that function with a new one will fix this.
R=asvitkine@chromium.org
BUG=322360
Committed: https://crrev.com/2cb37ad893097cd5c19184c7f2b937df59b24996
Cr-Commit-Position: refs/heads/master@{#304045}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add more tests. Fix windows without mask. #
Total comments: 22
Patch Set 3 : Style fixes. #Patch Set 4 : Yet another style fix. #Patch Set 5 : CreateHRGNFromSkPath can return empty region now. #
Total comments: 12
Patch Set 6 : Fix review issues. #
Total comments: 10
Patch Set 7 : Fix build. #Patch Set 8 : Fix review issues. #
Total comments: 4
Patch Set 9 : Fix update region conditions. #
Total comments: 4
Patch Set 10 : Improve readability. Fix missed release. #Patch Set 11 : Add static casts. #Patch Set 12 : Fix build. #
Messages
Total messages: 41 (11 generated)
asvitkine@chromium.org changed reviewers: + tapted@chromium.org, wez@chromium.org
Looks like a similar thing was attempted in https://codereview.chromium.org/83293008 but later reverted. Added the people involved at that who will hopefully have more context here. https://codereview.chromium.org/673623002/diff/1/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/1/ui/gfx/path_win_unittest.cc#... ui/gfx/path_win_unittest.cc:63: }; Nit: De-indent this.
On 2014/10/22 15:14:47, Alexei Svitkine wrote: > Looks like a similar thing was attempted in > https://codereview.chromium.org/83293008 but later reverted. > > Added the people involved at that who will hopefully have more context here. > > https://codereview.chromium.org/673623002/diff/1/ui/gfx/path_win_unittest.cc > File ui/gfx/path_win_unittest.cc (right): > > https://codereview.chromium.org/673623002/diff/1/ui/gfx/path_win_unittest.cc#... > ui/gfx/path_win_unittest.cc:63: }; > Nit: De-indent this. My original patch was reverted because it returned empty regions; IIRC setPath() was failing; you should DCHECK setPath's result to make sure you're not missing that. Looks like the only difference between our patches is in how we initialise the clip region? My patch also added tests for a non-contiguous regions - can you add that as well? Finally, your test uses IsPtInRegion - why not enumerate the exact expected set of rects?
nit: You have some typos in your CL description. :)
On 2014/10/22 15:14:47, Alexei Svitkine wrote: > https://codereview.chromium.org/673623002/diff/1/ui/gfx/path_win_unittest.cc#... > ui/gfx/path_win_unittest.cc:63: }; > Nit: De-indent this. Fixed. On 2014/10/22 18:37:23, Wez wrote: > My original patch was reverted because it returned empty regions; IIRC setPath() > was failing; you should DCHECK setPath's result to make sure you're not missing > that. Looks like the only difference between our patches is in how we initialise > the clip region? I think better to return NULL, if setPath will return false. As if you pass NULL into SetWindowRgn instead of HRGN, windows does not use clipping. > My patch also added tests for a non-contiguous regions - can you add that as > well? Fixed (I used a function from your patch). > Finally, your test uses IsPtInRegion - why not enumerate the exact expected set > of rects? Fixed. On 2014/10/22 18:38:13, Wez wrote: > nit: You have some typos in your CL description. :) Fixed.
https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:5: // Author: Aleksandr Derbenev <alex-ac@yandex-team.ru> You'll need to remove this, since you're contributing this code to Chromium. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:16: std::vector<SkIRect> GetRectsFromHRGN(HRGN region) { nit: for readability, suggest blank line between the logical blocks in this function (i.e. one before each comment). https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:19: CHECK(bytes_size != 0); CHECK_NE, here and below. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:21: std::vector<char> buffer(bytes_size);; nit: duplicate ; https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:25: // Pull out the rectangles into a SkIRect vector to pass to setRects(). nit: ... to return to caller. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:28: std::transform(rects, rects + region_data->rdh.nCount, nit: why not rects.begin(), rects.end()? https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:35: // Test that rectangle with round corners stil has _round_ corners after nit: why the _emphasis_ on the second round? https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:38: // can't make round corners. nit: this comment re old implementation adds nothing. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:77: TEST(CreateHRGNFromSkPathTest, NonContiguousPath) { nit: Include a one-line comment to explain what this test is for. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:96: EXPECT_EQ(NULL, gfx::CreateHRGNFromSkPath(path)); 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.
https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:5: // Author: Aleksandr Derbenev <alex-ac@yandex-team.ru> On 2014/10/23 18:50:05, Wez wrote: > You'll need to remove this, since you're contributing this code to Chromium. Sorry, It's a copy-paste artifact. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:16: std::vector<SkIRect> GetRectsFromHRGN(HRGN region) { On 2014/10/23 18:50:05, Wez wrote: > nit: for readability, suggest blank line between the logical blocks in this > function (i.e. one before each comment). Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:19: CHECK(bytes_size != 0); On 2014/10/23 18:50:05, Wez wrote: > CHECK_NE, here and below. Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:21: std::vector<char> buffer(bytes_size);; On 2014/10/23 18:50:05, Wez wrote: > nit: duplicate ; Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:25: // Pull out the rectangles into a SkIRect vector to pass to setRects(). On 2014/10/23 18:50:05, Wez wrote: > nit: ... to return to caller. Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:28: std::transform(rects, rects + region_data->rdh.nCount, On 2014/10/23 18:50:05, Wez wrote: > nit: why not rects.begin(), rects.end()? 'rects' is an array it hasn't got begin() and end(). https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:35: // Test that rectangle with round corners stil has _round_ corners after On 2014/10/23 18:50:05, Wez wrote: > nit: why the _emphasis_ on the second round? Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:38: // can't make round corners. On 2014/10/23 18:50:05, Wez wrote: > nit: this comment re old implementation adds nothing. Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:77: TEST(CreateHRGNFromSkPathTest, NonContiguousPath) { On 2014/10/23 18:50:05, Wez wrote: > nit: Include a one-line comment to explain what this test is for. Done. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:96: EXPECT_EQ(NULL, gfx::CreateHRGNFromSkPath(path)); On 2014/10/23 18:50:05, Wez wrote: > 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. 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.
https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:28: std::transform(rects, rects + region_data->rdh.nCount, On 2014/10/24 07:05:10, alex-ac wrote: > On 2014/10/23 18:50:05, Wez wrote: > > nit: why not rects.begin(), rects.end()? > > 'rects' is an array it hasn't got begin() and end(). Acknowledged. https://codereview.chromium.org/673623002/diff/20001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:96: EXPECT_EQ(NULL, gfx::CreateHRGNFromSkPath(path)); On 2014/10/24 07:05:10, alex-ac wrote: > On 2014/10/23 18:50:05, Wez wrote: > > 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. > > 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. 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.
alex-ac@yandex-team.ru changed reviewers: + sky@chromium.org
On 2014/10/24 23:07:21, Wez wrote: > 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. Done.
LGTM w/ the nits below addressed. Thanks for fixing this bug! https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/panels/panel_frame_view.cc (right): https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/panels/panel_frame_view.cc:361: NULL); base::win::ScopedRegion new_region; if (!window_mask.isEmpty()) new_region.Set(gfx:...); would seem more readable. https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/panels/panel_frame_view.cc:364: !::EqualRgn(current_region, new_region)) { Does EqualRgn actually return true for (NULL, NULL)? MSDN doesn't document that case. https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:15: nit: no need for this blank line https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:30: sk_rects.begin(), skia::RECTToSkIRect); nit: suggest blank line here https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:76: // Check that non contiguous path is translated to region correctly. Suggest: Check that a path enclosing two non-adjacent areas is correctly translated to a non-contiguous region. https://codereview.chromium.org/673623002/diff/80001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/80001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:1136: HRGN new_region = NULL; While you're here, I'd suggest using ScopedRegion here, and then release() it below, in the SetWindowRgn call - you can then also get run of the DeleteObject for new_region. :) Same applies to current_rgn.
https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/panels/panel_frame_view.cc (right): https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/panels/panel_frame_view.cc:361: NULL); On 2014/10/27 20:28:39, Wez wrote: > base::win::ScopedRegion new_region; > if (!window_mask.isEmpty()) > new_region.Set(gfx:...); > > would seem more readable. Done. https://codereview.chromium.org/673623002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/panels/panel_frame_view.cc:364: !::EqualRgn(current_region, new_region)) { On 2014/10/27 20:28:39, Wez wrote: > Does EqualRgn actually return true for (NULL, NULL)? MSDN doesn't document that > case. EqualRgn will return ERROR if one of handles is invalid. I've added check for new_region == NULL. https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest.cc File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:15: On 2014/10/27 20:28:39, Wez wrote: > nit: no need for this blank line Done. https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:30: sk_rects.begin(), skia::RECTToSkIRect); On 2014/10/27 20:28:39, Wez wrote: > nit: suggest blank line here Done. https://codereview.chromium.org/673623002/diff/80001/ui/gfx/path_win_unittest... ui/gfx/path_win_unittest.cc:76: // Check that non contiguous path is translated to region correctly. On 2014/10/27 20:28:39, Wez wrote: > Suggest: Check that a path enclosing two non-adjacent areas is correctly > translated to a non-contiguous region. Done. https://codereview.chromium.org/673623002/diff/80001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/80001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:1136: HRGN new_region = NULL; On 2014/10/27 20:28:39, Wez wrote: > While you're here, I'd suggest using ScopedRegion here, and then release() it > below, in the SetWindowRgn call - you can then also get run of the DeleteObject > for new_region. :) Same applies to current_rgn. Done.
The CQ bit was checked by alex-ac@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/673623002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:10: #include "ui/gfx/path_win.h" Nit: This should be first include followed by an empty line. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:14: std::vector<SkIRect> GetRectsFromHRGN(HRGN region) { Add a comment explaining what this method does. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:28: std::transform(rects, rects + region_data->rdh.nCount, Nit: Please run git cl lint and include the std library headers it suggests you to include. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:84: gfx::Path path; Put the test inside the gfx namespace and remove the gfx:: prefixes. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:91: for (size_t i = 0; i < arraysize(rects) && i < region_rects.size(); ++i) Nit: Change the EXPECT_EQ above to ASSERT_EQ and then you just need to check only one of the two conditions here since they'll be guaranteed to be equivalent.
https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... File ui/gfx/path_win_unittest.cc (right): https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:10: #include "ui/gfx/path_win.h" On 2014/10/28 13:21:02, Alexei Svitkine wrote: > Nit: This should be first include followed by an empty line. Done. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:14: std::vector<SkIRect> GetRectsFromHRGN(HRGN region) { On 2014/10/28 13:21:02, Alexei Svitkine wrote: > Add a comment explaining what this method does. Done. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:28: std::transform(rects, rects + region_data->rdh.nCount, On 2014/10/28 13:21:02, Alexei Svitkine wrote: > Nit: Please run git cl lint and include the std library headers it suggests you > to include. Done. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:84: gfx::Path path; On 2014/10/28 13:21:02, Alexei Svitkine wrote: > Put the test inside the gfx namespace and remove the gfx:: prefixes. Done. https://codereview.chromium.org/673623002/diff/100001/ui/gfx/path_win_unittes... ui/gfx/path_win_unittest.cc:91: for (size_t i = 0; i < arraysize(rects) && i < region_rects.size(); ++i) On 2014/10/28 13:21:02, Alexei Svitkine wrote: > Nit: Change the EXPECT_EQ above to ASSERT_EQ and then you just need to check > only one of the two conditions here since they'll be guaranteed to be > equivalent. Done.
lgtm
https://codereview.chromium.org/673623002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/panels/panel_frame_view.cc (right): https://codereview.chromium.org/673623002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/panels/panel_frame_view.cc:363: new_region == NULL || We need to treat a NULL new region and current_region_result==ERROR as being "equal", I think, since we expect ERROR in the case that there is no current region. https://codereview.chromium.org/673623002/diff/140001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/140001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1160: !EqualRgn(current_rgn, new_region)) { Ditto here re equality of NULL region.
https://codereview.chromium.org/673623002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/panels/panel_frame_view.cc (right): https://codereview.chromium.org/673623002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/panels/panel_frame_view.cc:363: new_region == NULL || On 2014/10/28 18:21:57, Wez wrote: > We need to treat a NULL new region and current_region_result==ERROR as being > "equal", I think, since we expect ERROR in the case that there is no current > region. Done. https://codereview.chromium.org/673623002/diff/140001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/140001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1160: !EqualRgn(current_rgn, new_region)) { On 2014/10/28 18:21:57, Wez wrote: > Ditto here re equality of NULL region. Done.
lgtm
alex-ac@yandex-team.ru changed reviewers: + dimich@chromium.org - sky@chromium.org
On 2014/11/06 11:14:32, alex-ac wrote: > mailto:alex-ac@yandex-team.ru changed reviewers: > + mailto:dimich@chromium.org > - mailto:sky@chromium.org dimich: changes in panel_frame_view.cc sky: changes in hwnd_message_handler.cc
alex-ac@yandex-team.ru changed reviewers: + sky@chromium.org
On 2014/11/06 11:15:47, alex-ac wrote: > On 2014/11/06 11:14:32, alex-ac wrote: > > mailto:alex-ac@yandex-team.ru changed reviewers: > > + mailto:dimich@chromium.org > > - mailto:sky@chromium.org > > dimich: changes in panel_frame_view.cc > sky: changes in hwnd_message_handler.cc ping dimich, sky.
https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1158: if (!!current_rgn != !!new_region || Wow, could you make this any harder to read? Please convert to bools separately to make more readable. https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1161: SetWindowRgn(hwnd(), new_region, redraw); Doesn't this need to release now?
https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1158: if (!!current_rgn != !!new_region || On 2014/11/12 15:28:36, sky wrote: > Wow, could you make this any harder to read? Please convert to bools separately > to make more readable. Done. https://codereview.chromium.org/673623002/diff/160001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1161: SetWindowRgn(hwnd(), new_region, redraw); On 2014/11/12 15:28:36, sky wrote: > Doesn't this need to release now? Done.
LGTM
The CQ bit was checked by alex-ac@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/673623002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/86017)
The CQ bit was checked by alex-ac@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/673623002/200001
The CQ bit was unchecked by alex-ac@yandex-team.ru
The CQ bit was checked by alex-ac@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/673623002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2cb37ad893097cd5c19184c7f2b937df59b24996 Cr-Commit-Position: refs/heads/master@{#304045} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
