Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502813003:
MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. (Closed)
|
Created:
4 years, 1 month ago by tapted Modified:
4 years, 1 month ago Reviewers:
karandeepb CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout.
OnWidgetInitDone() didn't exist when size constraints were implemented
for MacViews. The non-client view is added immediately *after* the call
NativeWidget*::InitNativeWidget() so, to ensure size constraints
correctly accounted for size constraints from the NonClientView, it was
necessary to update them in OnRootViewLayout(). However, this is called
a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU
intensive within AppKit code.
OnWidgetInitDone() is a much better place for applying size constraints
from the non-client view. It was introduced in r368141 specifically to
fix problems synchronising with the client area size.
BUG=665280
TEST=Covered by existing tests.
Committed: https://crrev.com/8a4c12c91010469d7f39332b5d54e61583b5ca4d
Cr-Commit-Position: refs/heads/master@{#432821}
Patch Set 1 #
Dependent Patchsets: Messages
Total messages: 19 (11 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Description was changed from ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. To ensure it correctly matched size constraints from the ClientView, it was necessary to update in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for it. BUG= TEST=Covered by existing tests. ========== to ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. To ensure it correctly matched size constraints from the ClientView, it was necessary to update in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for it. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG= TEST=Covered by existing tests. ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. To ensure it correctly matched size constraints from the ClientView, it was necessary to update in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for it. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG= TEST=Covered by existing tests. ========== to ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. To ensure it correctly matched size constraints from the ClientView, it was necessary to update in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for it. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ==========
tapted@chromium.org changed reviewers: + karandeepb@chromium.org
Description was changed from ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. To ensure it correctly matched size constraints from the ClientView, it was necessary to update in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for it. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ========== to ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. The non-client view is added immediately *after* the call NativeWidget*::InitNativeWidget() so, to ensure size constraints correctly accounted for size constraints from the NonClientView, it was necessary to update them in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for applying size constraints from the non-client view. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ==========
Hi Karan, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/15 04:03:59, tapted wrote: > Hi Karan, please take a look. Some thoughts/questions- 1) DesktopNativeWidgetAura::OnRootViewLayout() (particularly DesktopWindowTreeHostX11::OnRootViewLayout()) also enforces size constraints in OnRootViewLayout (and not in OnWidgetInitDone). Should it also be updated? If yes, should OnRootViewLayout even exist? 2) Also, can't RootView::Layout change the non_client_view's min/max size after widget initialization and if yes, is this still correct?
On 2016/11/16 06:30:16, karandeepb wrote: > On 2016/11/15 04:03:59, tapted wrote: > > Hi Karan, please take a look. > > Some thoughts/questions- > > 1) DesktopNativeWidgetAura::OnRootViewLayout() (particularly > DesktopWindowTreeHostX11::OnRootViewLayout()) also enforces size constraints in > OnRootViewLayout (and not in OnWidgetInitDone). Should it also be updated? If > yes, should OnRootViewLayout even exist? > > 2) Also, can't RootView::Layout change the non_client_view's min/max size after > widget initialization and if yes, is this still correct? Also, sorry for the delay.
On 2016/11/16 06:30:43, karandeepb wrote: > On 2016/11/16 06:30:16, karandeepb wrote: > > On 2016/11/15 04:03:59, tapted wrote: > > > Hi Karan, please take a look. > > > > Some thoughts/questions- > > > > 1) DesktopNativeWidgetAura::OnRootViewLayout() (particularly > > DesktopWindowTreeHostX11::OnRootViewLayout()) also enforces size constraints > in > > OnRootViewLayout (and not in OnWidgetInitDone). Should it also be updated? If > > yes, should OnRootViewLayout even exist? Yeah - I think we can remove it, but min/max sizes on Linux have a bit of a rough history, so it should be a separate CL. I've made a follow-up for this in https://codereview.chromium.org/2507403002/ > > > > 2) Also, can't RootView::Layout change the non_client_view's min/max size > after > > widget initialization and if yes, is this still correct? I think that would effectively make the min/max sizes recursive, so anything relying on that would be incorrect. Min size is usually based on what the non client view returns from GetPreferredSize(), and it's GetPreferredSize() that determines the bounds used for Layout() -- if that value were to change after a call to Layout() then it wouldn't be idempotent: successive calls to Layout() would result in the window constantly growing or shrinking.
LGTM.
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. The non-client view is added immediately *after* the call NativeWidget*::InitNativeWidget() so, to ensure size constraints correctly accounted for size constraints from the NonClientView, it was necessary to update them in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for applying size constraints from the non-client view. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ========== to ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. The non-client view is added immediately *after* the call NativeWidget*::InitNativeWidget() so, to ensure size constraints correctly accounted for size constraints from the NonClientView, it was necessary to update them in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for applying size constraints from the non-client view. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. The non-client view is added immediately *after* the call NativeWidget*::InitNativeWidget() so, to ensure size constraints correctly accounted for size constraints from the NonClientView, it was necessary to update them in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for applying size constraints from the non-client view. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. ========== to ========== MacViews: Set initial window size constraints in OnWidgetInitDone rather than OnRootViewLayout. OnWidgetInitDone() didn't exist when size constraints were implemented for MacViews. The non-client view is added immediately *after* the call NativeWidget*::InitNativeWidget() so, to ensure size constraints correctly accounted for size constraints from the NonClientView, it was necessary to update them in OnRootViewLayout(). However, this is called a lot, and gfx::ApplyNSWindowSizeConstraints(..) is surprisingly CPU intensive within AppKit code. OnWidgetInitDone() is a much better place for applying size constraints from the non-client view. It was introduced in r368141 specifically to fix problems synchronising with the client area size. BUG=665280 TEST=Covered by existing tests. Committed: https://crrev.com/8a4c12c91010469d7f39332b5d54e61583b5ca4d Cr-Commit-Position: refs/heads/master@{#432821} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/8a4c12c91010469d7f39332b5d54e61583b5ca4d Cr-Commit-Position: refs/heads/master@{#432821} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
