Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(666)

Issue 2712383002: Only perform dialog Layout() once during Widget::Init(). (Closed)

Created:
3 years, 9 months ago by tapted
Modified:
3 years, 9 months ago
Reviewers:
sky
CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org, Peter Kasting
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Only perform dialog Layout() once during Widget::Init(). For Widgets that require a non-client view Layout is currently performed twice during Widget::Init(): - once before the initial bounds of the Widget are determined (when setting the RootView's ContentsView), and - once after (when setting the initial bounds). The first Layout() is unnecessary and can lead to bugs when the RootView attempts a layout at an invalid size. Layout of text in particular can be affected since giving it a size influences text wrapping, and subsequent answers for Label::GetPreferredSize() will change. In particular, on Mac and Linux, zero-sized windows are not valid, so a NativeWidget may take on a size of 1x1 until its initial bounds is determined. If RootView performs a layout at this size, it can "lock" a Label to an non-preferred, but non-zero, size. This causes Label (and gfx::RenderText) to report a new preferred size of 0x0 (since no text will fit). So when Widget initialization later tries to determine what the initial bounds of the Widget should be, the Label is not accounted for. Keeping the Label with a 0x0 size instead causes Label::GetPreferredSize() to report a meaningful preferred size in more cases. But also layout is expensive and shouldn't be done multiple times whenever a Widget is created if we can avoid it. BUG=671820 Review-Url: https://codereview.chromium.org/2712383002 Cr-Commit-Position: refs/heads/master@{#453365} Committed: https://chromium.googlesource.com/chromium/src/+/23e39591317d71645dda43792fadd858bdf892ab

Patch Set 1 : Just the test - it currently fails #

Patch Set 2 : With the fix - linux/chromeos don't like this one #

Patch Set 3 : Initial layout? #

Patch Set 4 : test desktop widgets #

Patch Set 5 : selfnits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -7 lines) Patch
M ui/views/widget/root_view.cc View 1 1 chunk +0 lines, -6 lines 0 comments Download
M ui/views/widget/root_view_unittest.cc View 1 2 3 4 2 chunks +44 lines, -0 lines 0 comments Download
M ui/views/widget/widget.cc View 1 2 3 chunks +20 lines, -1 line 0 comments Download

Messages

Total messages: 28 (24 generated)
tapted
Hi sky, please take a look (Patchset1 has tryjobs without the fix - fails on ...
3 years, 9 months ago (2017-02-27 10:04:54 UTC) #22
sky
LGTM
3 years, 9 months ago (2017-02-27 16:49:16 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2712383002/80001
3 years, 9 months ago (2017-02-27 22:31:25 UTC) #25
commit-bot: I haz the power
3 years, 9 months ago (2017-02-27 22:44:34 UTC) #28
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/23e39591317d71645dda43792fad...

Powered by Google App Engine
This is Rietveld 408576698