All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

41 Branch
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8620714 [details] [diff] [review]
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.
Attachment #8620714 - Flags: review?(jmathies)
(Assignee)

Updated

3 years ago
Blocks: 1173117

Comment 3

3 years ago
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+
(Assignee)

Comment 4

3 years ago
Created attachment 8621151 [details] [diff] [review]
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.
Attachment #8620714 - Attachment is obsolete: true
Attachment #8621151 - Flags: review?(jmathies)

Comment 5

3 years ago
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)
(Assignee)

Comment 9

3 years ago
@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
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Comment 13

3 years ago
This seems to have fixed bug 966936.

Updated

3 years ago
Duplicate of this bug: 966936
You need to log in before you can comment on or make changes to this bug.