Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
867273003:
Mac: Optimize toolbar/bookmark drawing during live resize (Closed)
|
Created:
5 years, 10 months ago by Andre Modified:
5 years, 10 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, erikchen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Optimize toolbar/bookmark drawing during live resize
The default theme's background image is a subtle texture pattern that we can
scale without being easily noticed.
When using the default theme, let the layer get scaled during live resize
and redraw at the end of resize.
TabStripView is changed to no longer be a subclass of
BackgroundGradientView because we don't want to scale its layer (it may
draw a drop arrow).
It is also inefficient for it to call cr_recursivelySetNeedsDisplay.
With this optimization, the following views no longer redraw repeatedly
during live resize (when using default theme):
ToolbarView
BookmarkBarToolbarView
DownloadShelfView
OmniboxPopupTopSeparatorView
BUG=453996
Committed: https://crrev.com/4f0f726c38ce69c3fc4ee437114631c513b60152
Cr-Commit-Position: refs/heads/master@{#314661}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix for rsesek #Patch Set 3 : Rebased off 900613003 #
Total comments: 3
Patch Set 4 : Fix indent #Messages
Total messages: 22 (7 generated)
andresantoso@chromium.org changed reviewers: + rsesek@chromium.org
Robert, please review. https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... File chrome/browser/ui/cocoa/background_gradient_view.mm (left): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... chrome/browser/ui/cocoa/background_gradient_view.mm:130: removeObserver:self This looks like a bug. https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... File chrome/browser/ui/cocoa/tabs/tab_strip_view.mm (right): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... chrome/browser/ui/cocoa/tabs/tab_strip_view.mm:103: bool active = [[self window] isMainWindow] || A utility window can be key without being main, but I don't think a browser window can be.
https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... File chrome/browser/ui/cocoa/background_gradient_view.mm (right): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... chrome/browser/ui/cocoa/background_gradient_view.mm:162: NSViewLayerContentsRedrawOnSetNeedsDisplay]; nit: over-indented https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... File chrome/browser/ui/cocoa/tabs/tab_strip_view.mm (right): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... chrome/browser/ui/cocoa/tabs/tab_strip_view.mm:103: bool active = [[self window] isMainWindow] || On 2015/01/31 01:00:48, Andre wrote: > A utility window can be key without being main, but I don't think a browser > window can be. What about with child windows?
https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... File chrome/browser/ui/cocoa/background_gradient_view.mm (right): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/back... chrome/browser/ui/cocoa/background_gradient_view.mm:162: NSViewLayerContentsRedrawOnSetNeedsDisplay]; On 2015/02/02 19:32:16, Robert Sesek wrote: > nit: over-indented Done. https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... File chrome/browser/ui/cocoa/tabs/tab_strip_view.mm (right): https://codereview.chromium.org/867273003/diff/1/chrome/browser/ui/cocoa/tabs... chrome/browser/ui/cocoa/tabs/tab_strip_view.mm:103: bool active = [[self window] isMainWindow] || On 2015/02/02 19:32:16, Robert Sesek wrote: > On 2015/01/31 01:00:48, Andre wrote: > > A utility window can be key without being main, but I don't think a browser > > window can be. > > What about with child windows? I tested that the browser window stays main when showing a bubble or a sheet.
LGTM
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/867273003/20001
The CQ bit was unchecked by andresantoso@chromium.org
I found a bug with this. TabStripView was relying on BackgroundGradientView calling cr_recursivelySetNeedsDisplay to trigger redraw of its TabView subviews. I'm trying to address that first in https://crrev.com/900613003 before landing this.
Robert, PTAL at patch #3. It is rebased off https://codereview.chromium.org/900613003/
LGTM https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/background_gradient_view.mm (right): https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/background_gradient_view.mm:133: NSViewLayerContentsRedrawOnSetNeedsDisplay]; nit: over-indented
New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org
https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/background_gradient_view.mm (right): https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/background_gradient_view.mm:133: NSViewLayerContentsRedrawOnSetNeedsDisplay]; On 2015/02/04 16:13:49, Robert Sesek wrote: > nit: over-indented clang format wanted it that way, it's indenting from the start of the selector. Do you prefer patch #3?
https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/background_gradient_view.mm (right): https://codereview.chromium.org/867273003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/background_gradient_view.mm:133: NSViewLayerContentsRedrawOnSetNeedsDisplay]; On 2015/02/04 16:46:26, Andre wrote: > On 2015/02/04 16:13:49, Robert Sesek wrote: > > nit: over-indented > > clang format wanted it that way, it's indenting from the start of the selector. > Do you prefer patch #3? The most recent patch set is correct. I don't think what clang-format is doing is actually style-guide approved.
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/867273003/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_...)
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/867273003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4f0f726c38ce69c3fc4ee437114631c513b60152 Cr-Commit-Position: refs/heads/master@{#314661} |
