Closed Bug 1173617 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget: Win32, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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.
Attachment #8620714 - Flags: review?(jmathies)
Blocks: 1173117
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.
Attachment #8620714 - Flags: review?(jmathies) → review+
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.
Attachment #8620714 - Attachment is obsolete: true
Attachment #8621151 - Flags: review?(jmathies)
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.
Attachment #8621151 - Flags: review?(jmathies) → review+
A bug this blocks is tracked for 39. Is this something that will need uplift along with the other work for bug 1150622?
Flags: needinfo?(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!
Flags: needinfo?(milan)
Flags: needinfo?(mchang)
Flags: needinfo?(bugs)
I would certainly prefer runtime testing to ride the trains.
Flags: needinfo?(milan)
(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.
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/8f567d41d849
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
This seems to have fixed bug 966936.
Duplicate of this bug: 966936
You need to log in before you can comment on or make changes to this bug.