Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
894423003:
Mac: Optimize HistoryOverlayView using layer. (Closed)
|
Created:
5 years, 10 months ago by Andre Modified:
5 years, 10 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, rohitrao (ping after 24h) Base URL:
https://chromium.googlesource.com/chromium/src.git@themed-drawing Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Optimize HistoryOverlayView using layer.
A more efficient way to draw a simple oval with alpha.
It saves memory too since the view's layer no longer needs a backing buffer.
Also simplified fade out animation code when dismissing.
Committed: https://crrev.com/bdde23aacfd1e55858f741686f68a735bc1ebdb9
Cr-Commit-Position: refs/heads/master@{#314896}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix for rsesek #Patch Set 3 : Use CAShapeLayer #
Total comments: 2
Patch Set 4 : Fix indent #Patch Set 5 : Fix test #
Total comments: 5
Messages
Total messages: 23 (5 generated)
andresantoso@chromium.org changed reviewers: + rsesek@chromium.org
Robert, please review. This is not in any critical path, but it's an easy optimization.
https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:78: CGFloat radius = std::min(frameSize.width, frameSize.height) / 2.0; Why min here instead of just always width?
https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:78: CGFloat radius = std::min(frameSize.width, frameSize.height) / 2.0; On 2015/02/04 00:21:13, Robert Sesek wrote: > Why min here instead of just always width? height >= width always, so that would work. Done. Thinking about this some more, setting corner radius this way is not exactly the same as drawing an oval as before. But with the width and height being pretty close in this case (140x150), it's very hard to tell the difference. Do you think it's worth trying to use a CAShapeLayer instead?
On 2015/02/04 01:08:55, Andre wrote: > https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... > File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): > > https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... > chrome/browser/ui/cocoa/history_overlay_controller.mm:78: CGFloat radius = > std::min(frameSize.width, frameSize.height) / 2.0; > On 2015/02/04 00:21:13, Robert Sesek wrote: > > Why min here instead of just always width? > > height >= width always, so that would work. > Done. > Thinking about this some more, setting corner radius this way is not exactly the > same > as drawing an oval as before. But with the width and height being pretty close > in this > case (140x150), it's very hard to tell the difference. > Do you think it's worth trying to use a CAShapeLayer instead? Please do that - I don't think we should change the appearance of the history swipe bubble.
On 2015/02/04 01:18:41, erikchen wrote: > On 2015/02/04 01:08:55, Andre wrote: > > > https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... > > File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): > > > > > https://codereview.chromium.org/894423003/diff/1/chrome/browser/ui/cocoa/hist... > > chrome/browser/ui/cocoa/history_overlay_controller.mm:78: CGFloat radius = > > std::min(frameSize.width, frameSize.height) / 2.0; > > On 2015/02/04 00:21:13, Robert Sesek wrote: > > > Why min here instead of just always width? > > > > height >= width always, so that would work. > > Done. > > Thinking about this some more, setting corner radius this way is not exactly > the > > same > > as drawing an oval as before. But with the width and height being pretty close > > in this > > case (140x150), it's very hard to tell the difference. > > Do you think it's worth trying to use a CAShapeLayer instead? > > Please do that - I don't think we should change the appearance of the history > swipe bubble. Done (patch #3).
LGTM https://codereview.chromium.org/894423003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): https://codereview.chromium.org/894423003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller.mm:94: CGColorCreateGenericGray(0, shieldAlpha)); nit: indent +2
New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org
https://codereview.chromium.org/894423003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): https://codereview.chromium.org/894423003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller.mm:94: CGColorCreateGenericGray(0, shieldAlpha)); On 2015/02/04 16:08:20, Robert Sesek wrote: > nit: indent +2 Oops, done.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/894423003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Robert, PTAL at patchset 5. The unit test was failing because -animationDidStop:finished: never got called. Looks we've had a similar problems in the past, see https://codereview.chromium.org/107343008 I think the dismiss code is too complicated. I simplified it by calling removeFromSuperview on the animator, which removes the view right away while the presentation layer is animating the fade out. The unit test gets a lot simpler too.
https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right): https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:41: EXPECT_EQ(0u, [[test_view() subviews] count]); How does this ensure that the animation is no longer running when the test ends and test_view_ is deleted?
https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right): https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:41: EXPECT_EQ(0u, [[test_view() subviews] count]); On 2015/02/04 20:18:22, Robert Sesek wrote: > How does this ensure that the animation is no longer running when the test ends > and test_view_ is deleted? It does not. Is there a benefit from waiting for the animation? Or are you concerned that something may go wrong if we don't? I think we have many tests that exits without flushing the run loop, after closing a window for example. Isn't this similar? Do you have suggestions on what we should do instead?
https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right): https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:41: EXPECT_EQ(0u, [[test_view() subviews] count]); On 2015/02/04 21:01:46, Andre wrote: > On 2015/02/04 20:18:22, Robert Sesek wrote: > > How does this ensure that the animation is no longer running when the test > ends > > and test_view_ is deleted? > > It does not. > Is there a benefit from waiting for the animation? > Or are you concerned that something may go wrong if we don't? > I think we have many tests that exits without flushing the run loop, after > closing a > window for example. Isn't this similar? > Do you have suggestions on what we should do instead? Part of CocoaTest is responsible for flushing the run loop and closing open windows, but it is a bug that many tests don't clean up after themselves. I'm concerned that we have pending work in the runloop that could UAF an object and cause the test to crash. Can you wait for the animation to complete by listening for the notification?
https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right): https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:41: EXPECT_EQ(0u, [[test_view() subviews] count]); On 2015/02/04 21:21:25, Robert Sesek wrote: > On 2015/02/04 21:01:46, Andre wrote: > > On 2015/02/04 20:18:22, Robert Sesek wrote: > > > How does this ensure that the animation is no longer running when the test > > ends > > > and test_view_ is deleted? > > > > It does not. > > Is there a benefit from waiting for the animation? > > Or are you concerned that something may go wrong if we don't? > > I think we have many tests that exits without flushing the run loop, after > > closing a > > window for example. Isn't this similar? > > Do you have suggestions on what we should do instead? > > Part of CocoaTest is responsible for flushing the run loop and closing open > windows, but it is a bug that many tests don't clean up after themselves. I'm > concerned that we have pending work in the runloop that could UAF an object and > cause the test to crash. Can you wait for the animation to complete by listening > for the notification? Which notification do you mean? NSAnimationContext's completionHandler is only available on 10.7 and later. I don't think this animation actually involves the runloop much or at all, it's not done using a timer on the main thread. I'm thinking we should do it the simple way and resort to something more complicated only if the test turns out to be flaky. I'm hoping that it won't.
LGTM https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right): https://codereview.chromium.org/894423003/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:41: EXPECT_EQ(0u, [[test_view() subviews] count]); On 2015/02/04 21:32:29, Andre wrote: > On 2015/02/04 21:21:25, Robert Sesek wrote: > > On 2015/02/04 21:01:46, Andre wrote: > > > On 2015/02/04 20:18:22, Robert Sesek wrote: > > > > How does this ensure that the animation is no longer running when the test > > > ends > > > > and test_view_ is deleted? > > > > > > It does not. > > > Is there a benefit from waiting for the animation? > > > Or are you concerned that something may go wrong if we don't? > > > I think we have many tests that exits without flushing the run loop, after > > > closing a > > > window for example. Isn't this similar? > > > Do you have suggestions on what we should do instead? > > > > Part of CocoaTest is responsible for flushing the run loop and closing open > > windows, but it is a bug that many tests don't clean up after themselves. I'm > > concerned that we have pending work in the runloop that could UAF an object > and > > cause the test to crash. Can you wait for the animation to complete by > listening > > for the notification? > > Which notification do you mean? > NSAnimationContext's completionHandler is only available on 10.7 and later. > I don't think this animation actually involves the runloop much or at all, it's > not > done using a timer on the main thread. > I'm thinking we should do it the simple way and resort to something more > complicated only if the test turns out to be flaky. I'm hoping that it won't. Sorry, I was referring to -animationDidStop:finished:, but you're right. Looking at the animation code, since it doesn't set a delegate, this may not be an issue. I'm still concerned that the animation may still be running when the test ends, though. Hopefully the Valgrind/ASan bots will report any issues.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/894423003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bdde23aacfd1e55858f741686f68a735bc1ebdb9 Cr-Commit-Position: refs/heads/master@{#314896} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
