Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633403002:
MacViews: Add native drop shadow to dialogs on OSX < 10.10. (Closed)
|
Created:
4 years, 10 months ago by karandeepb Modified:
4 years, 10 months ago CC:
chromium-reviews, tdanderson+views_chromium.org, msw+watch_chromium.org, tfarina, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Add native drop shadow to dialogs on OSX 10.9.
On OSX 10.10 and later, the Window manager is able to gather shadow
information from the alpha channel of the composited layer, but this isn't
supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a
strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:].
This involves giving the window an opaque background and drawing over the
edges with `clear` color to make the corners round.
To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath.
This is similar to the methods in path_win.cc and path_x11.cc.
On Mac, hit testing masks are inferred from the alpha channel. Using the
window manager to draw the shadow ensures we get the shadow corresponding to
the alpha channel, and removes the need to use views::BubbleWindowTargeter.
Since, native shadows are now supported, this CL also removes raster shadows
on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS.
BUG=543671, 507965
Committed: https://crrev.com/675dee76d8e2d0d1d844665147c45f7c4468c7be
Cr-Commit-Position: refs/heads/master@{#375729}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed review comments. Reverted changes in view.cc. Implemented SchedulePaintInRect in native_widget_mac. #Patch Set 3 : Optimise drawRect by storing window mask in BridgedContentView. #
Total comments: 2
Patch Set 4 : Removed tests in CreateNSBezierPathFromSkPathTest.FillType in path_mac_unittest.mm #
Total comments: 74
Patch Set 5 : Addressed Review comments. Added unit tests for NativeWidgetMac::SchedulePaintInRect #
Total comments: 20
Patch Set 6 : Addressed review comments. Implemented quad to cubic bezier conversion. #
Total comments: 10
Patch Set 7 : Rebased. #Patch Set 8 : Addressed review comments. Removed native_widget_mac files. #
Total comments: 11
Patch Set 9 : Addressed review comments. #
Total comments: 49
Patch Set 10 : Addressed review comments. #
Messages
Total messages: 37 (18 generated)
Description was changed from ========== Changed clipping to anti aliasing type Formatted code Removed unneeded consts Changed bridged_content_view.mm Changed some #includes to #imports Changed corner radius for NO_ASSET to original value Added comments to path_mac_unittest.mm Added comments to path_mac.mm Make background white for osx <= 10.9 Set clip path in view.cc to prevent rounded corner distortion in bubbles Changed path_mac_unittest.mm Changed comment in bridged_content_view.mm Changed comment in bridged_content_view.mm Changed bridged_content_view.mm Changes to bubble_frame_view.cc Changed bubble_border.* files Corrected comment in bridged_native_widget.mm Checkout out bridged_content_view.h from master Added another test to path_mac_unittest.mm Initial version of path_mac_unittest.mm Style changes etc. to path_mac.* Style changes in path_mac.h Moved path conversion functions to path_mac.* files Have empty arrow mask if arrow paint type is none Working. Arrows handled. Some distortion on bookmark bubble. Removed isDrawingWindow_ hack. Wasn't needed. Skia function added, working version, also changed all borders on mac to NO_ASSETS Nasty code. NO_ASSETS border works, sort of Added drop shadow for OS Mavericks or earlier BUG= ========== to ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the -window an opaque background on OSX < 10.10 and snipping out the border as per -the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on -Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window -mask. BUG=543671, 581600 ==========
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the -window an opaque background on OSX < 10.10 and snipping out the border as per -the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on -Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window -mask. BUG=543671, 581600 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the -window an opaque background on OSX < 10.10 and snipping out the border as per -the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on -Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window -mask. BUG=543671, 581600, 507965 ==========
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the -window an opaque background on OSX < 10.10 and snipping out the border as per -the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on -Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window -mask. BUG=543671, 581600, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the window an opaque background on OSX < 10.10 and snipping out the border as per the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window mask. BUG=543671, 581600, 507965 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. I haven't yet fully tested these changes on other platforms. Also, I think that this will need to be split into multiple logically independent CL's. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode33 ui/gfx/path_mac.mm:33: NSBezierPath* CreateNSBezierPathFromSkPath(const SkPath& path) { A simpler implementation like that in path_win.cc also works, but it's not entirely accurate especially if we use this function for stroking (and not just filling) purposes later on. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode807 ui/views/view.cc:807: bool clip_set = false; I have added this to prevent the distortion that occurred on view like Bookmark bubble, which paint over the window boundary, and don't have rounded corners. Have checked this on OSX 10.10, yet to test it on windows. However I am not sure whether painting always resumes from root view or not. If it doesn't it may happen that the child view may still paint outside the window mask.
The stuff you did for http://crbug.com/581600 is probably the best place to start. And.. for performance it looks like we might want to use that CAShapeLayer anyway, which could make it redundant for Mac (but we should still fix the other views platforms..) Then path_mac, with the test. That might need a separate change for Skia. Note SkConvertQuadToCubic was moved out of the public Skia API in https://codereview.chromium.org/339183002 Then the rest. Still need to look at the path_mac tests, but there is lots here. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode1 ui/gfx/path_mac.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode48 ui/gfx/path_mac.mm:48: case SkPath::kQuad_Verb: { typically if any `case` has curlies in Chrome, all the cases should get them https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode54 ui/gfx/path_mac.mm:54: SkPath::ConvertQuadToCubic(quad, points); Does this function exist? Maybe you modified SkPath.h to export the internal skia function. Is it needed to implement the window shape stuff? Could we instead do: // NSBezierPath does not support quadratic bezier curves. It is straightforward // to convert to cubic using (e.g. with SkConvertQuadToCubic from SkGeometry.h) // but that's not required yet. NOTREACHED(); https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode70 ui/gfx/path_mac.mm:70: // Approximate with quads. Use two for now, increase if more precision I think we can avoid this with [NSBezierPath appendBezierPathWithArcFromPoint:...] except NSBezierPath wants a radius and SkPath gives a "weight". so we probably need to assume that abs(p0.x - p1.x) == (p0.y - p1.y). Then `radius = abs(p0.x - p1.x) / SK_ScalarRoot2Over2` (I think). https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode95 ui/gfx/path_mac.mm:95: if (!path.isInverseFillType()) { instead can we DCHECK(!path.isInverseFillType()); ? or perhaps replace the `default` case with case kInverseWinding_FillType: case kInverseEvenOdd_FillType: NOTREACHED() << "NSBezierCurve does not support inverse fill types." https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac_unittest.mm File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac_unittest.mm... ui/gfx/path_mac_unittest.mm:20: const double PI = 3.14159265358979323846; SK_ScalarPI? https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.cc:147: // On Mac, use the NO_ASSETS bubble border. WindowServer on Mac is able to nit: I think we should have the initializer still, but do #if defined(OS_MACOSX) // On Mac... shadow_ = NO_ASSETS; #endif Another option could be, in the initializer, shadow_(AdjustShadowForPlatform(shadow)) but that's probably clunkier https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:211: // it's own coordinates(i.e. with origin at (0,0)). An empty mask is returned nit: it's -> its, `coordinates(i.e. with origin at..` -> `coordinates (origin at..` An empty mask.. -> |path| is unchanged if there is no painted arrow. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:214: void GetArrowMask(const gfx::Rect& view_bounds, gfx::Path* mask) const; I think GetArrowPath is a better name for it, since it's usually used for drawing, not clipping (whereas 'mask' is typically just clipping). then mask -> path. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:232: void GetArrowMaskFromArrowBounds(const gfx::Rect& arrow_bounds, Mask/mask -> Path/path https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:167: #endif // OS_MACOSX What actually needed to change for the bookmark bubble? Can we just add NO_ASSETS to the big `if` statement for all platforms, and not worry about the rest? https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:169: if (is_dialog_shadow && is_arrow_type_ok) { There's a Chrome idiom "prefer early returns" for this kind of stuff -> reduces unnecessary indenting, but it would be nice to clean up the diff with the above change first https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:193: window_mask->addPath(arrow_mask, 0, 0); Should GetArrowMask/Path return a bool? (adding an empty path doesn't feel right..) https://codereview.chromium.org/1633403002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:333: // TODO(karandeepb) check how this can be optimised. It might be possible to paint this directly into [compositor_superview_ layer] inside BridgedNativeWidget::AcceleratedWidgetSwapCompleted. And instead of painting, it might be enough to adjust the type of |background_layer| in BridgedNativeWidget::AddCompositorSuperview() to a CAShapeLayer, and give it a background color (on Mavericks). Then, in AcceleratedWidgetSwapCompleted() just update the clip path before calling invalidateShadow https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode801 ui/views/view.cc:801: // If the View is a root view (has no parent) and if the window shape is only-do-if-root-view logic probably belongs in views::RootView E.g. you could have class View { .. protected: virtual void ApplyClip(ui::ClipRecorder* clip_recorder); }; |clip_insets_| should be removed from views::View and put into app_list::SearchResultPageView instead, which can override ApplyClip, as well as views::RootView. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode807 ui/views/view.cc:807: bool clip_set = false; On 2016/01/27 23:25:03, karandeepb wrote: > I have added this to prevent the distortion that occurred on view like Bookmark > bubble, which paint over the window boundary, and don't have rounded corners. > Have checked this on OSX 10.10, yet to test it on windows. However I am not sure > whether painting always resumes from root view or not. If it doesn't it may > happen that the child view may still paint outside the window mask. Does this fix the rounded bottom corners on the bookmark bubble on Windows/ChromeOS as well as on Mac? If so, then I think this is a good approach, and would be good to split out separately. Some graphics experts should weigh in -- there might be performance concerns with more complex clipping paths. They might also know a better place/method of achieving the fix. If we are only concerned about Mac, then I think we have the option of creating a CAShapeLayer instead of a regular CALayer for |background_layer| in BridgedNativeWidget::AddCompositorView(..). But since this is fixing a bug on other platforms - something here might be right. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode815 ui/views/view.cc:815: } else if (!clip_set) { I think this is equivalent to `else` -- |clip_set| can't change value before this is checked. (or maybe this should be `if` instead of `else if`).
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the window an opaque background on OSX < 10.10 and snipping out the border as per the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Adds clipping to root view so that child views can't paint outside the window mask. BUG=543671, 581600, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the window an opaque background on OSX < 10.10 and snipping out the border as per the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Implements SchedulePaintInRect in native_widget_mac.mm. BUG=543671, 581600, 507965 ==========
Patchset #2 (id:60001) has been deleted
PTAL Trent. I have tried to optimise drawRect a bit by taking dirtyRect into consideration and not updating the clipped border while a live resize is in progress and updating the window masks only when required. Let me know if the changes look OK to you, and whether I should start splitting this into multiple CLs. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode1 ui/gfx/path_mac.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/28 05:59:21, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode48 ui/gfx/path_mac.mm:48: case SkPath::kQuad_Verb: { On 2016/01/28 05:59:21, tapted wrote: > typically if any `case` has curlies in Chrome, all the cases should get them Done. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode54 ui/gfx/path_mac.mm:54: SkPath::ConvertQuadToCubic(quad, points); On 2016/01/28 05:59:21, tapted wrote: > Does this function exist? Maybe you modified SkPath.h to export the internal > skia function. Is it needed to implement the window shape stuff? Could we > instead do: > > // NSBezierPath does not support quadratic bezier curves. It is straightforward > // to convert to cubic using (e.g. with SkConvertQuadToCubic from SkGeometry.h) > // but that's not required yet. > NOTREACHED(); Yeah I did modify SkPath to expose the internal skia function. Here's the CL - https://codereview.chromium.org/1639143002/. There's precedent of exposing functionality in SkPath like https://codereview.chromium.org/1484373002, which was just added 2 months ago. I don't think that quadratic bezier curves are needed currently, but conic curves are needed, and currently my approach is to convert them to quadratic bezier curves. Also, converting quadratic to cubic bezier curves is straight-forward, so if needed, it can be implemented here itself. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode70 ui/gfx/path_mac.mm:70: // Approximate with quads. Use two for now, increase if more precision On 2016/01/28 05:59:21, tapted wrote: > I think we can avoid this with [NSBezierPath > appendBezierPathWithArcFromPoint:...] > > except NSBezierPath wants a radius and SkPath gives a "weight". so we probably > need to assume that abs(p0.x - p1.x) == (p0.y - p1.y). Then `radius = abs(p0.x - > p1.x) / SK_ScalarRoot2Over2` (I think). I don't think we can approximate a kConicVerb(which I think is a rational quadratic bezier curve) by a single arc, since it is used to represent conic sections. It may be possible to do for just the rounded rectangle case though. Will discuss. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac.mm#newcode95 ui/gfx/path_mac.mm:95: if (!path.isInverseFillType()) { On 2016/01/28 05:59:21, tapted wrote: > instead can we DCHECK(!path.isInverseFillType()); ? > > or perhaps replace the `default` case with > > case kInverseWinding_FillType: > case kInverseEvenOdd_FillType: > NOTREACHED() << "NSBezierCurve does not support inverse fill types." Done. https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac_unittest.mm File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/gfx/path_mac_unittest.mm... ui/gfx/path_mac_unittest.mm:20: const double PI = 3.14159265358979323846; On 2016/01/28 05:59:21, tapted wrote: > SK_ScalarPI? Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.cc:147: // On Mac, use the NO_ASSETS bubble border. WindowServer on Mac is able to On 2016/01/28 05:59:22, tapted wrote: > nit: I think we should have the initializer still, but do > > #if defined(OS_MACOSX) > // On Mac... > shadow_ = NO_ASSETS; > #endif > > Another option could be, in the initializer, > > shadow_(AdjustShadowForPlatform(shadow)) > > but that's probably clunkier Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:211: // it's own coordinates(i.e. with origin at (0,0)). An empty mask is returned On 2016/01/28 05:59:22, tapted wrote: > nit: it's -> its, `coordinates(i.e. with origin at..` -> `coordinates (origin > at..` > > An empty mask.. -> |path| is unchanged if there is no painted arrow. Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:214: void GetArrowMask(const gfx::Rect& view_bounds, gfx::Path* mask) const; On 2016/01/28 05:59:22, tapted wrote: > I think GetArrowPath is a better name for it, since it's usually used for > drawing, not clipping (whereas 'mask' is typically just clipping). then mask -> > path. Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_bord... ui/views/bubble/bubble_border.h:232: void GetArrowMaskFromArrowBounds(const gfx::Rect& arrow_bounds, On 2016/01/28 05:59:22, tapted wrote: > Mask/mask -> Path/path Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:167: #endif // OS_MACOSX On 2016/01/28 05:59:22, tapted wrote: > What actually needed to change for the bookmark bubble? > > Can we just add NO_ASSETS to the big `if` statement for all platforms, and not > worry about the rest? We also need to return a mask for bubbles with arrows if the shadow type is NO_ASSETS. Also, have changed how kBorderStrokeSize and kCornerRadius are defined. I have changed the code to include early returns. The two if statements can be combined together in a more concise way, but this seems more clearer. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:169: if (is_dialog_shadow && is_arrow_type_ok) { On 2016/01/28 05:59:22, tapted wrote: > There's a Chrome idiom "prefer early returns" for this kind of stuff -> reduces > unnecessary indenting, but it would be nice to clean up the diff with the above > change first Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:193: window_mask->addPath(arrow_mask, 0, 0); On 2016/01/28 05:59:22, tapted wrote: > Should GetArrowMask/Path return a bool? (adding an empty path doesn't feel > right..) Done. https://codereview.chromium.org/1633403002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:333: // TODO(karandeepb) check how this can be optimised. On 2016/01/28 05:59:22, tapted wrote: > It might be possible to paint this directly into [compositor_superview_ layer] > inside BridgedNativeWidget::AcceleratedWidgetSwapCompleted. > > And instead of painting, it might be enough to adjust the type of > |background_layer| in BridgedNativeWidget::AddCompositorSuperview() to a > CAShapeLayer, and give it a background color (on Mavericks). Then, in > AcceleratedWidgetSwapCompleted() just update the clip path before calling > invalidateShadow Giving background_layer a background color with the NSWindow background clear, and calling [window invalidateShadow] did not render a drop shadow. Hence it can be safe to assume that the window server is not taking the alpha channel of background_layer into consideration. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode801 ui/views/view.cc:801: // If the View is a root view (has no parent) and if the window shape is On 2016/01/28 05:59:22, tapted wrote: > only-do-if-root-view logic probably belongs in views::RootView > > E.g. you could have > > class View { > .. > protected: > virtual void ApplyClip(ui::ClipRecorder* clip_recorder); > }; > > |clip_insets_| should be removed from views::View and put into > app_list::SearchResultPageView instead, which can override ApplyClip, as well as > views::RootView. Removed the changes in view.cc. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode807 ui/views/view.cc:807: bool clip_set = false; On 2016/01/28 05:59:22, tapted wrote: > On 2016/01/27 23:25:03, karandeepb wrote: > > I have added this to prevent the distortion that occurred on view like > Bookmark > > bubble, which paint over the window boundary, and don't have rounded corners. > > Have checked this on OSX 10.10, yet to test it on windows. However I am not > sure > > whether painting always resumes from root view or not. If it doesn't it may > > happen that the child view may still paint outside the window mask. > > Does this fix the rounded bottom corners on the bookmark bubble on > Windows/ChromeOS as well as on Mac? If so, then I think this is a good approach, > and would be good to split out separately. Some graphics experts should weigh in > -- there might be performance concerns with more complex clipping paths. They > might also know a better place/method of achieving the fix. > > If we are only concerned about Mac, then I think we have the option of creating > a CAShapeLayer instead of a regular CALayer for |background_layer| in > BridgedNativeWidget::AddCompositorView(..). But since this is fixing a bug on > other platforms - something here might be right. This didn't fix the bug on other platforms since a rectangular mask is returned for BubbleBorder::SMALL_SHADOW. On OSX 10.9, the distortion around the corners on bubbles like BookmarkBubble isn't visible with the current border radius of 2 for BubbleBorder::NO_ASSETS, however, it becomes visible if we increase the border radius. This can be fixed on Mac, by setting a cornerRadius on background_layer and setting its masksToBounds to YES or by setting a mask layer on the background layer which again would depend on the window mask. However, it may be slightly computationally expensive. Alternatively, as you had recommended I can look into performing the clipping in some views::subview which does not include the raster shadow. https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc#newcode815 ui/views/view.cc:815: } else if (!clip_set) { On 2016/01/28 05:59:22, tapted wrote: > I think this is equivalent to `else` -- |clip_set| can't change value before > this is checked. (or maybe this should be `if` instead of `else if`). Done. https://codereview.chromium.org/1633403002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:342: [super viewDidEndLiveResize]; This optimisation to prevent drawing during a live resize leads to rectangular corners during a resize. Should we go ahead with it?
https://codereview.chromium.org/1633403002/diff/100001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/100001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:12: #else // __OBJC__ nit: I think we can assume only .mm files #import this, so this #else isn't needed https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:23: NSBezierPath* CreateNSBezierPathFromSkRegion(const SkRegion& region) { do we need stuff for SkRegion yet, or just SkPath? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:75: const size_t kPow2 = 1; I guess this comes from SkAutoConicToQuads. kPow2 isn't very descriptive. Perhaps kSubdivisionLevels? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:76: const size_t quadCount = 1 << kPow2; nit: quadCount -> kQuadCount https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:77: SkPoint quads[1 + 2 * quadCount]; perhaps a comment like, // The quads will share endpoints, so we need one more point than twice the number of quads. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:93: default: { NOTREACHED(); } NOTREACHED() on a new line? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:23: NSPoint GetRadialPoint(const double radius, nit: it's unusual to put `const` on arguments of primitive type in chrome. Same in CircleArea, PolygonArea, roundrectarea. (const for local variables is fine) https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:49: [path elementAtIndex:index++ associatedPoints:points]; The `index++` is easy to lose here - can this be a `for` loop? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:53: element = [path elementAtIndex:index++ associatedPoints:points]; `index++` doesn't look right on this line https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:63: DCHECK(NSEqualPoints(poly[0], poly[count - 1])); poly.front(), poly.back()? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:65: for (size_t i = 0; i < count - 1; i++) then poly.size() can be moved in here https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:96: NSBezierPath* const result = CreateNSBezierPathFromSkRegion(SkRegion()); nit: no const (const typically looks extra weird around NS types since ObjectiveC doesn't really have a concept of `const` in the way that C++ does, and there's potential for confusion whenever someone declares a const pointer rather than a pointer-to-const-object) https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:106: const NSPoint inside_points[] = { these consts look fine though, since NSPoint is a struct https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:114: const size_t kSizeRectArray = 2; = arraysize(rects)? or just use arraysize(rects) in setRects below same for kNumPoints. More in TwoRectanglesPath https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:207: // Check that the returned result has the correct bounding box. maybe mention that this is rounding to whole numbers https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:214: NSBezierPath* const polygon = [result bezierPathByFlatteningPath]; nit: no const https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:215: const double kTolerance = 0.5; Can we tighten this? I guess we should go pretty close to what passes, since these should be deterministic https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:235: // Bottom left corner. nit: remove extra space https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:236: {kCornerRadius, kCornerRadius}, maybe half the radius? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:250: {kCushion, kRectangleHeight / 2.0}}; nit: }; on a new line? (unless clang format likes this better) https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:267: {-kCushion, kRectangleHeight / 2.0}}; }: -> new line https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:347: SkPath* mask) const { mask -> path https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:18: class Path; nit: alphabetise https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:234: SkPath* mask) const; nit: mask -> path https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.h:51: // The current window mask of the window to which this view is bound. For nit: This can be more terse. E.g. // The cached window mask. Only used for non-rectangular windows on 10.9. (since, we don't support < 10.9 any more.) https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:136: // Get Notified whenever the view bounds or frame change. We should just be able to hook in via BridgedNativeWidget::OnSizeChanged(..) -- the view should be resized by then, and not yet drawn. You can use [window_ inLiveResize] there decide whether or not to do `stuff` https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:344: [self setNeedsDisplay:YES]; should this go at the end of setWindowMask? https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:362: // opaque background. This distorts windows with non rectangular shapes. To I'm not sure if `distorts` is the right word. Is it just that the corners are square? (can we say, "we set..." -> "the window is given an opaque background, but this will have square corners. To get around.." https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:368: DLOG(WARNING) << "Drawing\n"; remove https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:808: - (void)setWindowMask { I don't think this is overriding anything, so it should be declared in an @interface -- probably in the header since it's likely BridgedNativeWidget will be calling it. (Then move this impl to match declaration order) https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:808: - (void)setWindowMask { setWindowMask -> updateWindowMask https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:810: DCHECK(![self inLiveResize]); I'd make base::mac::IsOSMavericksOrEarlier() as part of the DCHECK and move the that check into BridgedNativeWidget -- it already has the version checking in CreateLayer, so this keeps those checks together. E.g. (in OnSizeChanged) something like // 10.9 is unable to generate a window shadow from the CALayer, so use Quartz. if (base::mac::IsOSMavericksOrEarlier()) [bridge_ updateWindowMask]; https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:813: if (widget->non_client_view() && base::mac::IsOSMavericksOrEarlier()) { nit: early return https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:817: if (!mask.isEmpty()) { nit: early return https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:828: stuff below here shouldn't be needed https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:505: // |rect| is relative to coordinate system of the window. is it the window or the client area? (i.e. excluding title bar). We should add a unit test for this that checks both titled and borderless windows https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:508: NSRect target_rect_window = maybe NSRect ns_rect = rect.ToCGRect(); ns_rect.origin.y = NSWidth(window_frame) - ns_rect.origin.y; then, we can assume the window and the contentView share an origin (and scale factor, and rotation, etc.), so the convertRect shouldn't be needed, but we should probably DCHECK(NSEqualRects(ns_rect, [GetNativeView() convertRect:ns_rect fromView:nil]));
PTAL Trent. Thanks. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:23: NSBezierPath* CreateNSBezierPathFromSkRegion(const SkRegion& region) { On 2016/02/05 04:51:06, tapted wrote: > do we need stuff for SkRegion yet, or just SkPath? Just SkPath. Should I remove it? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:75: const size_t kPow2 = 1; On 2016/02/05 04:51:06, tapted wrote: > I guess this comes from SkAutoConicToQuads. kPow2 isn't very descriptive. > Perhaps kSubdivisionLevels? Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:76: const size_t quadCount = 1 << kPow2; On 2016/02/05 04:51:06, tapted wrote: > nit: quadCount -> kQuadCount Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:77: SkPoint quads[1 + 2 * quadCount]; On 2016/02/05 04:51:06, tapted wrote: > perhaps a comment like, > > // The quads will share endpoints, so we need one more point than twice the > number of quads. Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:93: default: { NOTREACHED(); } On 2016/02/05 04:51:06, tapted wrote: > NOTREACHED() on a new line? This is caused by formatting done by git cl format. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:23: NSPoint GetRadialPoint(const double radius, On 2016/02/05 04:51:07, tapted wrote: > nit: it's unusual to put `const` on arguments of primitive type in chrome. Same > in CircleArea, PolygonArea, roundrectarea. (const for local variables is fine) Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:39: double CalculatePolygonArea(NSBezierPath* const path) { This function somewhat depends on the internal NSBezierPath representation. Should it be kept? https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:49: [path elementAtIndex:index++ associatedPoints:points]; On 2016/02/05 04:51:07, tapted wrote: > The `index++` is easy to lose here - can this be a `for` loop? Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:53: element = [path elementAtIndex:index++ associatedPoints:points]; On 2016/02/05 04:51:07, tapted wrote: > `index++` doesn't look right on this line Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:63: DCHECK(NSEqualPoints(poly[0], poly[count - 1])); On 2016/02/05 04:51:07, tapted wrote: > poly.front(), poly.back()? Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:65: for (size_t i = 0; i < count - 1; i++) On 2016/02/05 04:51:07, tapted wrote: > then poly.size() can be moved in here Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:96: NSBezierPath* const result = CreateNSBezierPathFromSkRegion(SkRegion()); On 2016/02/05 04:51:07, tapted wrote: > nit: no const (const typically looks extra weird around NS types since > ObjectiveC doesn't really have a concept of `const` in the way that C++ does, > and there's potential for confusion whenever someone declares a const pointer > rather than a pointer-to-const-object) Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:106: const NSPoint inside_points[] = { On 2016/02/05 04:51:07, tapted wrote: > these consts look fine though, since NSPoint is a struct Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:114: const size_t kSizeRectArray = 2; On 2016/02/05 04:51:07, tapted wrote: > = arraysize(rects)? or just use arraysize(rects) in setRects below > > same for kNumPoints. More in TwoRectanglesPath Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:207: // Check that the returned result has the correct bounding box. On 2016/02/05 04:51:07, tapted wrote: > maybe mention that this is rounding to whole numbers Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:214: NSBezierPath* const polygon = [result bezierPathByFlatteningPath]; On 2016/02/05 04:51:07, tapted wrote: > nit: no const Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:215: const double kTolerance = 0.5; On 2016/02/05 04:51:07, tapted wrote: > Can we tighten this? I guess we should go pretty close to what passes, since > these should be deterministic Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:235: // Bottom left corner. On 2016/02/05 04:51:07, tapted wrote: > nit: remove extra space Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:236: {kCornerRadius, kCornerRadius}, On 2016/02/05 04:51:07, tapted wrote: > maybe half the radius? Done. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:250: {kCushion, kRectangleHeight / 2.0}}; On 2016/02/05 04:51:07, tapted wrote: > nit: }; on a new line? (unless clang format likes this better) clang format causes this. https://codereview.chromium.org/1633403002/diff/120001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:267: {-kCushion, kRectangleHeight / 2.0}}; On 2016/02/05 04:51:07, tapted wrote: > }: -> new line clang format causes this. https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:347: SkPath* mask) const { On 2016/02/05 04:51:07, tapted wrote: > mask -> path Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:18: class Path; On 2016/02/05 04:51:07, tapted wrote: > nit: alphabetise Done. Didn't knew this was a word! https://codereview.chromium.org/1633403002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:234: SkPath* mask) const; On 2016/02/05 04:51:07, tapted wrote: > nit: mask -> path Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.h:51: // The current window mask of the window to which this view is bound. For On 2016/02/05 04:51:07, tapted wrote: > nit: This can be more terse. E.g. > > // The cached window mask. Only used for non-rectangular windows on 10.9. > > > (since, we don't support < 10.9 any more.) Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:136: // Get Notified whenever the view bounds or frame change. On 2016/02/05 04:51:07, tapted wrote: > We should just be able to hook in via BridgedNativeWidget::OnSizeChanged(..) -- > the view should be resized by then, and not yet drawn. > > You can use [window_ inLiveResize] there decide whether or not to do `stuff` Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:344: [self setNeedsDisplay:YES]; On 2016/02/05 04:51:07, tapted wrote: > should this go at the end of setWindowMask? Don't think so. Whenever we set the window mask i.e whenever the bounds change, Cocoa should have itself called setNeedsDisplay on the view. We only set it here explicitly since we had suppressed drawing during live resize. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:362: // opaque background. This distorts windows with non rectangular shapes. To On 2016/02/05 04:51:07, tapted wrote: > I'm not sure if `distorts` is the right word. Is it just that the corners are > square? (can we say, "we set..." -> "the window is given an opaque background, > but this will have square corners. To get around.." Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:368: DLOG(WARNING) << "Drawing\n"; On 2016/02/05 04:51:07, tapted wrote: > remove Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:808: - (void)setWindowMask { On 2016/02/05 04:51:08, tapted wrote: > I don't think this is overriding anything, so it should be declared in an > @interface -- probably in the header since it's likely BridgedNativeWidget will > be calling it. (Then move this impl to match declaration order) Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:808: - (void)setWindowMask { On 2016/02/05 04:51:07, tapted wrote: > setWindowMask -> updateWindowMask Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:810: DCHECK(![self inLiveResize]); On 2016/02/05 04:51:08, tapted wrote: > I'd make base::mac::IsOSMavericksOrEarlier() as part of the DCHECK and move the > that check into BridgedNativeWidget -- it already has the version checking in > CreateLayer, so this keeps those checks together. E.g. (in OnSizeChanged) > something like > > // 10.9 is unable to generate a window shadow from the CALayer, so use Quartz. > if (base::mac::IsOSMavericksOrEarlier()) > [bridge_ updateWindowMask]; Done. But we'll still need an additional check in viewDidEndLiveResize. If we want to keep the logic in bridged_content_view, we can utilise the windowDidEndLiveResize: method in NSWindowDelegate and have it call a method in bridged_native_widget. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:813: if (widget->non_client_view() && base::mac::IsOSMavericksOrEarlier()) { On 2016/02/05 04:51:07, tapted wrote: > nit: early return Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:817: if (!mask.isEmpty()) { On 2016/02/05 04:51:07, tapted wrote: > nit: early return Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:828: On 2016/02/05 04:51:08, tapted wrote: > stuff below here shouldn't be needed Done. https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:505: // |rect| is relative to coordinate system of the window. On 2016/02/05 04:51:08, tapted wrote: > is it the window or the client area? (i.e. excluding title bar). We should add a > unit test for this that checks both titled and borderless windows You are correct. It's the client area. Have corrected the implementation and added unit tests. https://codereview.chromium.org/1633403002/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:508: NSRect target_rect_window = On 2016/02/05 04:51:08, tapted wrote: > maybe > > NSRect ns_rect = rect.ToCGRect(); > ns_rect.origin.y = NSWidth(window_frame) - ns_rect.origin.y; > > then, we can assume the window and the contentView share an origin (and scale > factor, and rotation, etc.), so the convertRect shouldn't be needed, but we > should probably > > DCHECK(NSEqualRects(ns_rect, > [GetNativeView() convertRect:ns_rect fromView:nil])); Done.
nice tests! We should probably split that dirtyRect bit off and land it first. We should also ping those skia folks (maybe they even have input for this CL wrt the SkPath -> NSBezierPath conversion) https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:344: [self setNeedsDisplay:YES]; On 2016/02/08 07:47:09, karandeepb wrote: > On 2016/02/05 04:51:07, tapted wrote: > > should this go at the end of setWindowMask? > > Don't think so. Whenever we set the window mask i.e whenever the bounds change, > Cocoa should have itself called setNeedsDisplay on the view. We only set it here > explicitly since we had suppressed drawing during live resize. Acknowledged. https://codereview.chromium.org/1633403002/diff/140001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:119: for (size_t i = 0; i < kNumPoints; i++) { nit: kNumPoints -> arraysize(inside_points) (temporary isn't needed). More below. https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:184: DCHECK(![self inLiveResize]); add a DCHECK(hostedView_); too. This is to show it's not an omission to bail out if it's false like most of the methods here do https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:351: [super viewDidEndLiveResize]; nit: move before comment https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:379: gfx::ScopedNSGraphicsContextSaveGState state; nit: before this, add DCHECK(base::mac::IsOSMavericksOrEarlier())); (i.e. as a signal that this can eventually go away in X>N years) https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:60: // No of times drawRect: has been called. nit: No -> Number, drawRect: -> -[NSView drawRect:] https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1119: MockBridgedView* mock_bridged_view = [[MockBridgedView alloc] init]; scoped_nsobject (I think this is a memory leak currently) https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1122: base::RunLoop().RunUntilIdle(); Comment here, like, // Ensure the initial draw of the window is done (without the view). https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1159: MockBridgedView* mock_bridged_view = [[MockBridgedView alloc] init]; scoped_nsobject https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1178: // coordinate of rect origin is calculated as - nit: `as -` -> `as:` https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1216: @end nit: blank line before
PTAL Trent. I have split the NativeWidgetMac into a separate CL - https://codereview.chromium.org/1689483002/. Also, it seems that it's not possible to move SkPath::ConvertQuadToCubic to the public Skia interface. Hence I have implemented it in path_mac.mm. https://codereview.chromium.org/1633403002/diff/140001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:119: for (size_t i = 0; i < kNumPoints; i++) { On 2016/02/09 06:47:12, tapted wrote: > nit: kNumPoints -> arraysize(inside_points) > > (temporary isn't needed). More below. Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:184: DCHECK(![self inLiveResize]); On 2016/02/09 06:47:12, tapted wrote: > add a DCHECK(hostedView_); too. This is to show it's not an omission to bail out > if it's false like most of the methods here do Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:351: [super viewDidEndLiveResize]; On 2016/02/09 06:47:12, tapted wrote: > nit: move before comment Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:379: gfx::ScopedNSGraphicsContextSaveGState state; On 2016/02/09 06:47:12, tapted wrote: > nit: before this, add > > DCHECK(base::mac::IsOSMavericksOrEarlier())); > > (i.e. as a signal that this can eventually go away in X>N years) Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:60: // No of times drawRect: has been called. On 2016/02/09 06:47:12, tapted wrote: > nit: No -> Number, drawRect: -> -[NSView drawRect:] Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1119: MockBridgedView* mock_bridged_view = [[MockBridgedView alloc] init]; On 2016/02/09 06:47:12, tapted wrote: > scoped_nsobject (I think this is a memory leak currently) Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1122: base::RunLoop().RunUntilIdle(); On 2016/02/09 06:47:12, tapted wrote: > Comment here, like, > > // Ensure the initial draw of the window is done (without the view). Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1159: MockBridgedView* mock_bridged_view = [[MockBridgedView alloc] init]; On 2016/02/09 06:47:12, tapted wrote: > scoped_nsobject Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1178: // coordinate of rect origin is calculated as - On 2016/02/09 06:47:13, tapted wrote: > nit: `as -` -> `as:` Done. https://codereview.chromium.org/1633403002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:1216: @end On 2016/02/09 06:47:12, tapted wrote: > nit: blank line before Done.
Looking good! Is a follow-up still needed for http://crbug.com/581600 ? (the square-bottom-corner thing). I think we were exploring maskToBounds and/or transferring |windowMask_| onto the background CALayer. I think it's probably ok to land the path_mac changes with this. The CL description needs an update too. It should capture - On Mac, hit testing masks are inferred from the alpha channel. - Using the window manager to draw the shadow ensures we get the "right" shadow, and removes the need to use views::BubbleWindowTargeter - On 10.10 and later the window manager gathers shadow information from the composited later, but this isn't supported on 10.9 - For 10.9, use a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:] that gives the window a solid background and copies over the edges with `clear` to make the corners round https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:22: GFX_EXPORT NSBezierPath* CreateNSBezierPathFromSkRegion(const SkRegion& region); remove this until it's needed? Probably worthwhile moving it to a separate CL based off this and linking to it in the crbug https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:19: // Convert a quadratic bezier curve to a cubic bezier curve. nit: mention that this is based on the implementation of the private SkConvertQuadToCubic method inside Skia. https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:56: ns_points[i] = GetNSPointFromSkPoint(points[i]); I think this can read from uninitialized memory in some cases, which some bots will warn about. (i.e. RawIter only needs to guarantee that it writes to the |points| that's it's valid to read from. I think this is an improvement, but |points| should be initialized above, like SkPoint points[4] = {{0.0}}; (if that works: C++ will zero-fill per the standard once there's any initialization, but there's an associated compile warning that I'm not sure whether we enable) Since it's now used a lot less, it might be worth doing points -> sk_points ns_points -> points And in fact, you might as well ditch `GetNSPointFromSkPoint` and just use NSMakePoint here and in kConic_Verb below so it's clear that the conversion is `simple` https://codereview.chromium.org/1633403002/diff/160001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/160001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:148: #if defined(OS_MACOSX) this makes some stuff in DialogDelegate::CreateDialogFrameView redundant (the #ifdef there can be removed) https://codereview.chromium.org/1633403002/diff/160001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1633403002/diff/160001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:505: // |rect| is relative to client area of the window. needs a rebase off the other CL (probably the order that they land doesn't matter since it's a performance optimization)
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX < 10.10. This CL - -Adds drop shadows to dialogs on OSX < 10.10. It does this by giving the window an opaque background on OSX < 10.10 and snipping out the border as per the window mask. -Removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. -Implements path_mac.mm which converts Skia Paths and Regions to NSBezierPath. -Implements SchedulePaintInRect in native_widget_mac.mm. BUG=543671, 581600, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. Also, on Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. It also implements path_mac.mm which converts Skia Paths to NSBezierPath. BUG=543671, 507965 ==========
PTAL Trent. For http://crbug.com/581600, on OSX 10.9, the distortion around the corners on bubbles like BookmarkBubble isn't visible with the current border radius of 2 for BubbleBorder::NO_ASSETS, however, it becomes visible if we increase the border radius. It's easy to correct that by providing a mask layer to the compositor superview which can be updated whenever the bounds change. I'll first look if I can find a platform independent solution, and if I can't, I'll submit a follow up CL with the mask layer approach. Also, another doubt that I had was about preventing the drawing during a live resize. It leads to square corners during a resize, but isn't visible unless you zoom. Is that ok? Also, is this CL fine in its current form or should I split it? https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:22: GFX_EXPORT NSBezierPath* CreateNSBezierPathFromSkRegion(const SkRegion& region); On 2016/02/10 02:28:01, tapted wrote: > remove this until it's needed? Probably worthwhile moving it to a separate CL > based off this and linking to it in the crbug Done. https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:19: // Convert a quadratic bezier curve to a cubic bezier curve. On 2016/02/10 02:28:02, tapted wrote: > nit: mention that this is based on the implementation of the private > SkConvertQuadToCubic method inside Skia. Done. https://codereview.chromium.org/1633403002/diff/160001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:56: ns_points[i] = GetNSPointFromSkPoint(points[i]); On 2016/02/10 02:28:01, tapted wrote: > I think this can read from uninitialized memory in some cases, which some bots > will warn about. (i.e. RawIter only needs to guarantee that it writes to the > |points| that's it's valid to read from. > > I think this is an improvement, but |points| should be initialized > above, like > > SkPoint points[4] = {{0.0}}; > > (if that works: C++ will zero-fill per the standard once there's any > initialization, but there's an associated compile warning that I'm not sure > whether we enable) > > Since it's now used a lot less, it might be worth doing > > points -> sk_points > ns_points -> points > > And in fact, you might as well ditch `GetNSPointFromSkPoint` and just use > NSMakePoint here and in kConic_Verb below so it's clear that the conversion is > `simple` Done. https://codereview.chromium.org/1633403002/diff/160001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1633403002/diff/160001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:148: #if defined(OS_MACOSX) On 2016/02/10 02:28:02, tapted wrote: > this makes some stuff in DialogDelegate::CreateDialogFrameView redundant (the > #ifdef there can be removed) Done. https://codereview.chromium.org/1633403002/diff/160001/ui/views/widget/native... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/1633403002/diff/160001/ui/views/widget/native... ui/views/widget/native_widget_mac.mm:505: // |rect| is relative to client area of the window. On 2016/02/10 02:28:02, tapted wrote: > needs a rebase off the other CL (probably the order that they land doesn't > matter since it's a performance optimization) Done.
lgtm % nits we should add msw next since he is the bubble expert https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/BUILD.gn#newcode1 ui/gfx/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. CL description "Also, on Mac," -> "On Mac," It also implements path_mac.mm which converts Skia Paths to NSBezierPath. -> To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:11: @class NSBezierPath; I think only this is needed for now (no #if __OBJC__ needed) -- only objc files include this file. the SkPath forward dec can go directly below https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:17: class SkRegion; nit: remove https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:21: class SkPath; nit: I think this should appear above any forward dec between `namespace` https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:211: // its own coordinates (origin at (0,0)). hm - perhaps we should just change GetArrowRect to accept a `const gfx::Size& view_size` instead of bounds.. bounds may make sense too Maybe just, nit: `in its own coordinates` -> `local bounds` but an owner may have a different opinion. https://codereview.chromium.org/1633403002/diff/240001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1633403002/diff/240001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:883: [window_ setBackgroundColor:[NSColor whiteColor]]; I just realised.. this `else` might not be needed. (i.e. can just use the default window background?)
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. Also, on Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. It also implements path_mac.mm which converts Skia Paths to NSBezierPath. BUG=543671, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. On Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. BUG=543671, 507965 ==========
karandeepb@chromium.org changed reviewers: + msw@chromium.org
+msw for ui/gfx/* and ui/views/bubble/* review. https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:11: @class NSBezierPath; On 2016/02/11 01:47:23, tapted wrote: > I think only this is needed for now (no #if __OBJC__ needed) -- only objc files > include this file. the SkPath forward dec can go directly below Done. https://codereview.chromium.org/1633403002/diff/240001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:17: class SkRegion; On 2016/02/11 01:47:23, tapted wrote: > nit: remove Done. https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:21: class SkPath; On 2016/02/11 01:47:23, tapted wrote: > nit: I think this should appear above any forward dec between `namespace` Done. https://codereview.chromium.org/1633403002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.h:211: // its own coordinates (origin at (0,0)). On 2016/02/11 01:47:23, tapted wrote: > hm - perhaps we should just change GetArrowRect to accept a `const gfx::Size& > view_size` instead of bounds.. bounds may make sense too > > Maybe just, > > nit: `in its own coordinates` -> `local bounds` > > but an owner may have a different opinion. Done. https://codereview.chromium.org/1633403002/diff/240001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/1633403002/diff/240001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget.mm:883: [window_ setBackgroundColor:[NSColor whiteColor]]; On 2016/02/11 01:47:23, tapted wrote: > I just realised.. this `else` might not be needed. (i.e. can just use the > default window background?) Done.
https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:15: // Returns the NSBezierPath corresponding to |path|. nit: document ownership (or need to CFRelease, or similar). Maybe return a base::scoped_nsobject or similar? https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:5: #import "ui/gfx/path_mac.h" I'm not the most capable reviewer here, but I think I've covered it adequately. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:15: // implementation of the private SkConvertQuadToCubic method inside Skia. q: Can/should we expose SkConvertQuadToCubic instead? https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:40: NSPoint points[4]; nit: declare outside loop? https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:41: for (size_t i = 0; i < 4; i++) nit: maybe use arraysize(points)? https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:72: case SkPath::kConic_Verb: { nit: match decl order (move above cubic, after quad) https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:92: ConvertQuadToCubicBezier(quad, points); nit: DCHECK(NSEqualPoints([result currentPoint], points[0])); https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:99: case SkPath::kClose_Verb: { Should this use (or at least check) the returned "contour's moveTo pt": https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:119: default: nit: I think you can omit default to get compile errors if the FillType enum gets a new value. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:56: DCHECK_EQ([path elementAtIndex:element_count - 1 associatedPoints:points], optional nit: you could omit associatedPoints here and the DCHECK immediately below; the following DCHECK seems sufficient. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:64: area += poly[i].x * poly[i + 1].y - poly[i].y * poly[i + 1].x; q: don't you technically need incldue these terms once (outside the loop)? a += poly[poly.size() - 1].x * poly[0].y - poly[0].x * poly[poly.size() - 1].y (I'm just learning via https://en.wikipedia.org/wiki/Shoelace_formula) https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:81: // Returns the bounding box of |path| as a gfx::Rect. nit: put this anon namespace inside gfx, and then 'gfx::' is not needed. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:94: NSBezierPath* result = CreateNSBezierPathFromSkPath(SkPath()); Don't you need to release (CFRelease?) all the paths created here? (or use base::scoped_nsobject or something similar?) https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:118: {101, 149}, {149, 149}, {149, 101}, {25, 25}, {125, 125}}; nit: put {25, 25} after {49, 1}, bump {101, 101} to next line. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:119: const NSPoint outside_points[] = {{-1, -1}, {-1, 51}, {51, 51}, {51, -1}, EXPECT/ASSERT_EQ(arraysize(inside_points), arraysize(outside_points)); https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:122: const gfx::Rect expected_bounds(0, 0, 150, 150); nit: 'gfx::' not needed https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:149: const gfx::Rect expected_bounds(kCentreX - kRadius, kCentreY - kRadius, nit: 'gfx::' not needed https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:172: // Check area of converted path is correct upto a certain tolerance value. To nit: "up to" https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:176: NSBezierPath* polygon = [result bezierPathByFlatteningPath]; nit: delete this object at some point or use base::scoped_nsobject? https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:190: const gfx::Rect expected_bounds(kRectangleWidth, kRectangleHeight); nit: 'gfx::' not needed https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:214: const NSPoint outside_points[] = { EXPECT/ASSERT_EQ(arraysize(inside_points), arraysize(outside_points)); https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:248: // Check area of converted path is correct upto a certain tolerance value. To nit: "up to" https://codereview.chromium.org/1633403002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1633403002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:167: static const SkScalar kCornerRadius = SkIntToScalar(6); Odd, the corner radii are 4 and 2; I wonder why this was 6? https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu...
PTAL @msw. Thanks. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:15: // Returns the NSBezierPath corresponding to |path|. On 2016/02/11 23:57:37, msw wrote: > nit: document ownership (or need to CFRelease, or similar). > Maybe return a base::scoped_nsobject or similar? Done. Have documented that this returns an autoreleased pointer, which can be retained by the caller if needed. Don't think we should return a base::scoped_nsobject. It's conventional to return autoreleased pointers on Mac and use base::scoped_nsobjects as part of method arguments, when we need to transfer ownership. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:15: // implementation of the private SkConvertQuadToCubic method inside Skia. On 2016/02/11 23:57:37, msw wrote: > q: Can/should we expose SkConvertQuadToCubic instead? Tried doing this - https://codereview.chromium.org/1639143002/. But the Skia reviewers recommended implementing this in the Chromium repo itself. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:40: NSPoint points[4]; On 2016/02/11 23:57:37, msw wrote: > nit: declare outside loop? Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:41: for (size_t i = 0; i < 4; i++) On 2016/02/11 23:57:37, msw wrote: > nit: maybe use arraysize(points)? Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:72: case SkPath::kConic_Verb: { On 2016/02/11 23:57:37, msw wrote: > nit: match decl order (move above cubic, after quad) Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:92: ConvertQuadToCubicBezier(quad, points); On 2016/02/11 23:57:37, msw wrote: > nit: DCHECK(NSEqualPoints([result currentPoint], points[0])); Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:99: case SkPath::kClose_Verb: { On 2016/02/11 23:57:37, msw wrote: > Should this use (or at least check) the returned "contour's moveTo pt": > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... I don't think that the Skia behaviour here is as per the documentation. For a rectangle with end points - [(0,0), (50,50)], the point returned corresponding to the kClose_Verb is (50,50) which is weird. I'll look at filing a bug on the Skia issue tracker. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:119: default: On 2016/02/11 23:57:37, msw wrote: > nit: I think you can omit default to get compile errors if the FillType enum > gets a new value. Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:56: DCHECK_EQ([path elementAtIndex:element_count - 1 associatedPoints:points], On 2016/02/11 23:57:37, msw wrote: > optional nit: you could omit associatedPoints here and the DCHECK immediately > below; the following DCHECK seems sufficient. Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:64: area += poly[i].x * poly[i + 1].y - poly[i].y * poly[i + 1].x; On 2016/02/11 23:57:37, msw wrote: > q: don't you technically need incldue these terms once (outside the loop)? > a += poly[poly.size() - 1].x * poly[0].y - poly[0].x * poly[poly.size() - 1].y > (I'm just learning via https://en.wikipedia.org/wiki/Shoelace_formula) The last point in the poly vector is the same as the first point i.e. there are (n+1) points in the vector for an n-sided polygon. This helps eliminate the extra terms, since these are taken care of inside the loop itself. This is the same as the 3rd Alternative formula on wikipedia. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:81: // Returns the bounding box of |path| as a gfx::Rect. On 2016/02/11 23:57:38, msw wrote: > nit: put this anon namespace inside gfx, and then 'gfx::' is not needed. Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:94: NSBezierPath* result = CreateNSBezierPathFromSkPath(SkPath()); On 2016/02/11 23:57:38, msw wrote: > Don't you need to release (CFRelease?) all the paths created here? > (or use base::scoped_nsobject or something similar?) It is autoreleased when it is no longer in scope(at the end of an iteration of the main runloop). https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:118: {101, 149}, {149, 149}, {149, 101}, {25, 25}, {125, 125}}; On 2016/02/11 23:57:38, msw wrote: > nit: put {25, 25} after {49, 1}, bump {101, 101} to next line. Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:119: const NSPoint outside_points[] = {{-1, -1}, {-1, 51}, {51, 51}, {51, -1}, On 2016/02/11 23:57:38, msw wrote: > EXPECT/ASSERT_EQ(arraysize(inside_points), arraysize(outside_points)); Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:122: const gfx::Rect expected_bounds(0, 0, 150, 150); On 2016/02/11 23:57:38, msw wrote: > nit: 'gfx::' not needed Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:149: const gfx::Rect expected_bounds(kCentreX - kRadius, kCentreY - kRadius, On 2016/02/11 23:57:38, msw wrote: > nit: 'gfx::' not needed Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:172: // Check area of converted path is correct upto a certain tolerance value. To On 2016/02/11 23:57:38, msw wrote: > nit: "up to" Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:176: NSBezierPath* polygon = [result bezierPathByFlatteningPath]; On 2016/02/11 23:57:38, msw wrote: > nit: delete this object at some point or use base::scoped_nsobject? It is autoreleased when it is no longer in scope(at the end of an iteration of the main runloop). https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:190: const gfx::Rect expected_bounds(kRectangleWidth, kRectangleHeight); On 2016/02/11 23:57:38, msw wrote: > nit: 'gfx::' not needed Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:214: const NSPoint outside_points[] = { On 2016/02/11 23:57:38, msw wrote: > EXPECT/ASSERT_EQ(arraysize(inside_points), arraysize(outside_points)); Done. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:248: // Check area of converted path is correct upto a certain tolerance value. To On 2016/02/11 23:57:38, msw wrote: > nit: "up to" Done. https://codereview.chromium.org/1633403002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1633403002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:167: static const SkScalar kCornerRadius = SkIntToScalar(6); On 2016/02/11 23:57:38, msw wrote: > Odd, the corner radii are 4 and 2; I wonder why this was 6? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... Not sure about it either.
lgtm https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h File ui/gfx/path_mac.h (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.h#newc... ui/gfx/path_mac.h:15: // Returns the NSBezierPath corresponding to |path|. On 2016/02/12 03:45:51, karandeepb wrote: > On 2016/02/11 23:57:37, msw wrote: > > nit: document ownership (or need to CFRelease, or similar). > > Maybe return a base::scoped_nsobject or similar? > > Done. Have documented that this returns an autoreleased pointer, which can be > retained by the caller if needed. Don't think we should return a > base::scoped_nsobject. It's conventional to return autoreleased pointers on Mac > and use base::scoped_nsobjects as part of method arguments, when we need to > transfer ownership. I'm not familiar with this convention, but I'll trust you and Trent on it. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm File ui/gfx/path_mac.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:15: // implementation of the private SkConvertQuadToCubic method inside Skia. On 2016/02/12 03:45:51, karandeepb wrote: > On 2016/02/11 23:57:37, msw wrote: > > q: Can/should we expose SkConvertQuadToCubic instead? > > Tried doing this - https://codereview.chromium.org/1639143002/. But the Skia > reviewers recommended implementing this in the Chromium repo itself. Acknowledged. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac.mm#new... ui/gfx/path_mac.mm:99: case SkPath::kClose_Verb: { On 2016/02/12 03:45:51, karandeepb wrote: > On 2016/02/11 23:57:37, msw wrote: > > Should this use (or at least check) the returned "contour's moveTo pt": > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... > > I don't think that the Skia behaviour here is as per the documentation. For a > rectangle with end points - [(0,0), (50,50)], the point returned corresponding > to the kClose_Verb is (50,50) which is weird. I'll look at filing a bug on the > Skia issue tracker. Acknowledged. https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... File ui/gfx/path_mac_unittest.mm (right): https://codereview.chromium.org/1633403002/diff/260001/ui/gfx/path_mac_unitte... ui/gfx/path_mac_unittest.mm:64: area += poly[i].x * poly[i + 1].y - poly[i].y * poly[i + 1].x; On 2016/02/12 03:45:52, karandeepb wrote: > On 2016/02/11 23:57:37, msw wrote: > > q: don't you technically need incldue these terms once (outside the loop)? > > a += poly[poly.size() - 1].x * poly[0].y - poly[0].x * poly[poly.size() - 1].y > > (I'm just learning via https://en.wikipedia.org/wiki/Shoelace_formula) > > The last point in the poly vector is the same as the first point i.e. there are > (n+1) points in the vector for an n-sided polygon. This helps eliminate the > extra terms, since these are taken care of inside the loop itself. This is the > same as the 3rd Alternative formula on wikipedia. Acknowledged.
karandeepb@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/views/window/dialog_delegate.cc review.
LGTM
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1633403002/#ps280001 (title: "Addressed review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633403002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633403002/280001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. On Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. BUG=543671, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. On Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. BUG=543671, 507965 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. On Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. BUG=543671, 507965 ========== to ========== MacViews: Add native drop shadow to dialogs on OSX 10.9. On OSX 10.10 and later, the Window manager is able to gather shadow information from the alpha channel of the composited layer, but this isn't supported on OSX 10.9. This CL adds native shadows to OSX 10.9 by using a strategy similar to ChromeMac's -[ConstrainedWindowCustomWindow drawRect:]. This involves giving the window an opaque background and drawing over the edges with `clear` color to make the corners round. To do this, implement path_mac.mm which converts Skia Paths to NSBezierPath. This is similar to the methods in path_win.cc and path_x11.cc. On Mac, hit testing masks are inferred from the alpha channel. Using the window manager to draw the shadow ensures we get the shadow corresponding to the alpha channel, and removes the need to use views::BubbleWindowTargeter. Since, native shadows are now supported, this CL also removes raster shadows on all Mac Versions by setting all BubbleBorders on Mac to NO_ASSETS. BUG=543671, 507965 Committed: https://crrev.com/675dee76d8e2d0d1d844665147c45f7c4468c7be Cr-Commit-Position: refs/heads/master@{#375729} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/675dee76d8e2d0d1d844665147c45f7c4468c7be Cr-Commit-Position: refs/heads/master@{#375729} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
