1173617 - Don't cache titlebar caption sizes unless the widget has a titlebar

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Mason Chang [Inactive] [:mchang]]

From bug 1156135, if the first chrome window we create doesn't have a titlebar, all new windows created after will look like [1]. This is because the first toplevel window created will cache the caption button sizes at [2] even if the widget is created without a titlebar. The data returned at [3] is a 0 rect because there is no caption button area, causing a negative value for the CMDBUTTONIDX_BUTTONBOX area. When we finally paint the browser [4], we calculate a very large negative width and height overtaking the whole titlebar area for the frame [5] causing the bug.

[1] https://bug1156135.bugzilla.mozilla.org/attachment.cgi?id=8612479
[2] https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#651
[3] https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsUXThemeData.cpp#164
[4] https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#2440
[5] https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsBox.cpp?from=nsIFrame::AddCSSMinSize#685

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't cache titlebar caption sizes unless we have a default OS titlebar

Updated to only cache the titlebar data if we have a normal border style from the OS. Also adds some checks to make sure that the values we do cache are sane in debug builds.

Comment 2

[Mason Chang [Inactive] [:mchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e61b68b9175

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8620714 [details] [diff] [review]
Don't cache titlebar caption sizes unless we have a default OS titlebar

Review of attachment 8620714 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsUXThemeData.cpp
@@ +222,5 @@
>    sCommandButtons[2].cx = info.rgrect[5].right - info.rgrect[5].left;
>    sCommandButtons[2].cy = info.rgrect[5].bottom - info.rgrect[5].top;
>  
> +#ifdef DEBUG
> +  for (int i = 0; i < NUM_COMMAND_BUTTONS; i++) {

please add a comment here explaining what this is for.

::: widget/windows/nsWindow.cpp
@@ +645,5 @@
>  
>    // Query for command button metric data for rendering the titlebar. We
>    // only do this once on the first window.
>    if (mWindowType == eWindowType_toplevel &&
> +      // bug 1173617, only cache titlebar if the window actually has a titlebar

nit - please remove the bug # reference and put this up top in the other commenting.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't cache titlebar caption sizes unless we have a default OS titlebar or all titlebar

Sorry for another review, had to refactor a bit to get it to work since there was a bug with the title bar in VMs / remote desktop. This caches either the first window that uses the OS default or the titlebar style all. These two options cover the test machines I have.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8621151 [details] [diff] [review]
Don't cache titlebar caption sizes unless we have a default OS titlebar or all titlebar

Review of attachment 8621151 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

A bug this blocks is tracked for 39. Is this something that will need uplift along with the other work for bug 1150622?

Comment 7

[Mason Chang [Inactive] [:mchang]]

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0514be38a07

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f567d41d849

Comment 9

[Mason Chang [Inactive] [:mchang]]

@Milan and Jet - I thought we were letting bug 1156135 and the general runtime testing test ride the trains? Can you please answer comment 6? Thanks!

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

I would certainly prefer runtime testing to ride the trains.

Comment 11

[Jet Villegas (inactive)]

(In reply to Liz Henry (:lizzard) from comment #6)
> A bug this blocks is tracked for 39. Is this something that will need uplift
> along with the other work for bug 1150622?

We'll only uplift after we've seen this work well on Nightly. We may end up shipping the video playback tests first, if these GFx bits take longer.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f567d41d849

Comment 13

[Elbart]

This seems to have fixed bug 966936.