Closed Bug 1444525 Opened 3 years ago Closed 3 years ago

Persisted window sizes should be outer, not inner

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 + wontfix
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1444525: Extract the logic to see if we're a top-level chrome window.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1444525: Persist XUL window size attributes as a window size, not client size.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
See bug 1444045 comment 22.  Or use these STR on mac:

1)  Create a new profile.
2)  Put pref("browser.tabs.drawInTitlebar", false); in the user.js for that
    profile.
3)  Start the browser.
4)  Open about:config.
5)  Toggle "browser.tabs.drawInTitlebar" to true.
6)  Quit.
7)  Edit prefs.js (NOT user.js) and remove the "browser.tabs.drawInTitlebar"
    setting in there.
8)  Repeat steps 3-7 a few times.

The browser window keeps growing in size.  It really shouldn't.

The problem is that we persist window sizes as attributes on the <window>, and bug 
1442521 changed the meaning of those...
Flags: needinfo?(xidorn+moz)
Blocks: 1444045
Note that this can be hit with OS theme or zoom level changes too, not just with manual munging of preferences.  The manual munging is just simpler to explain.
tracking-firefox60: --- → ?
Component: Audio/Video: MediaStreamGraph → XUL
I think I can take a look at this, in exchange for the debugging pain that Xidorn saved me ;)
Assignee: nobody → emilio
This is pretty messy...

If we want to persist the window outer size, we should either:
* make everything on <window> mean the outer size, including width, height, and {min,max}{width,height}, or
* change width and height immediately after reading from persistent data to reflect the inner size

I guess probably the second approach is easier...
Yeah, I was doing the second thing, converting to client size when loading persistent attributes, I have patches on the progress of building.
I cannot figure out an elegant way to do the second approach, though...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> I cannot figure out an elegant way to do the second approach, though...

It's not (too) terrible, I think... Somewhat nasty, but...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Note that this can be hit with OS theme or zoom level changes too, not just
> with manual munging of preferences.  The manual munging is just simpler to
> explain.

This can only happen if you do
1. open Firefox
2. change to a theme with thin border
3. close Firefox
4. change to a theme with thick border
and repeat from 1.

How often people would want to do this I doubt...

If we really want to persist outer size... maybe we really should have width/height mean the outer size (while keeping {min,max}{width,height} meaning the inner size)... This may be confusing but is probably the best way forward...
The remaining thing would to understand where we read/write the width/height of <window>, which probably wouldn't be a lot of places...
I think that's a bit more risky tbh. Let me try the persistent attribute thing, I'm don't think it'll be more complex.
Attachment #8957756 - Flags: review?(xidorn+moz) → review?(bzbarsky)
Attachment #8957757 - Flags: review?(xidorn+moz) → review?(bzbarsky)
Comment on attachment 8957757 [details]
Bug 1444525: Persist XUL window size attributes as a window size, not client size.

Xidorn, could you test this? I'm on Linux and the window size difference there is zero, so this patch is a no-op there.
Attachment #8957757 - Flags: feedback?(xidorn+moz)
> How often people would want to do this I doubt...

I bet it can also happen if you:

1. Open Firefox
2. Change OS zoom.
3. Close Firefox.
4. Change OS zoom back.

And how often people do that depends on what zoom levels work best for them with Firefox vs other apps...
Comment on attachment 8957756 [details]
Bug 1444525: Extract the logic to see if we're a top-level chrome window.

https://reviewboard.mozilla.org/r/226726/#review232530

::: dom/xul/XULDocument.h:204
(Diff revision 1)
>  protected:
>      virtual ~XULDocument();
>  
> +    // Returns the top-level chrome window iff this document is associated to
> +    // this.
> +    already_AddRefed<nsIXULWindow> TopLevelChromeWindowAssociatedWithThis() const;

How about GetXULWindowIfToplevelChrome() and document that it returns null if this is not a toplevel chrome document?
Attachment #8957756 - Flags: review?(bzbarsky) → review+
Comment on attachment 8957757 [details]
Bug 1444525: Persist XUL window size attributes as a window size, not client size.

https://reviewboard.mozilla.org/r/226728/#review232532

::: dom/xul/XULDocument.cpp:1753
(Diff revision 1)
>  
>      return NS_OK;
>  }
>  
> +static bool
> +ScaleAttributeToClient(Element& aElement,

Why "Scale"?  What's being scaled?

Maybe this should be called "SetAttrToClientSize"?  Or "SetAttrToInnerSize"?

And document the arguments and return value, please.

::: dom/xul/XULDocument.cpp:1775
(Diff revision 1)
> +
> +    if (!sizeDiff) {
> +        return false;
> +    }
> +
> +    nsAutoString value =

Just:

    NS_ConvertASCIItoUTF16 value(nsPrintfCString("%d", size - sizeDiff));

::: xpfe/appshell/nsIXULWindow.idl:69
(Diff revision 1)
>     * Tell this window that it has picked up a child XUL window
>     * @param aChild the child window being added
>     */
>    void addChildWindow(in nsIXULWindow aChild);
>  
> +  [noscript,notxpcom] uint32_t getClientToWindowHeightDifferenceInCSSPixels();

These need to be documented.  Especially because the use of "client" here is not at all obvious.  Maybe these should be getOuterToInnerHeightDifferenceInCSSPixels and likewise for width?

::: xpfe/appshell/nsXULWindow.cpp:343
(Diff revision 1)
>    tab.forget(aTab);
>    return NS_OK;
>  }
>  
> +static LayoutDeviceIntSize
> +GetClientToWindowSizeDifference(nsIWidget* aWindow)

Again, not sure about the naming.

::: xpfe/appshell/nsXULWindow.cpp:1648
(Diff revision 1)
>        }
>      }
>    }
>  
>    if ((mPersistentAttributesDirty & PAD_SIZE) && gotRestoredBounds) {
> -    LayoutDeviceIntSize winDiff = GetWindowOuterInnerDiff(mWindow);
> +    // NOTE: We persist _window_ sizes, not client sizes, see bug 1444525 & co.

Again, the "client" here is not at all obvious.  And "window sizes" is sadly ambiguous given the existence of DOM Window...  It would be clearer to say that the size we're persisting includes operating system decorations.

::: xpfe/appshell/nsXULWindow.cpp:1650
(Diff revision 1)
>    }
>  
>    if ((mPersistentAttributesDirty & PAD_SIZE) && gotRestoredBounds) {
> -    LayoutDeviceIntSize winDiff = GetWindowOuterInnerDiff(mWindow);
> +    // NOTE: We persist _window_ sizes, not client sizes, see bug 1444525 & co.
>      if (persistString.Find("width") >= 0) {
> -      auto width = rect.Width() - winDiff.width;
> +      auto width = rect.Width();

So I'm a little confused about this.  Shouldn't we be setting the attr value to the "inner" size, still?  I don't see why we're not doing that.
Attachment #8957757 - Flags: review?(bzbarsky) → review-
Patches now should be a bit more believable... I guess that's what I get for taking this when I'm on the only desktop platform where this returns 0 :-(
I thought a bit about whether we can make layout consider width/height on <window> as outer size. I guess it is not impossible, but it has its implication as well, specifically, width/height properties would mean different thing than width/height attributes, which can be confusing, and that also means we need some special case in test_windowminmaxsize.xul to check outer size when the attributes are used.

Also if we have width/height attributes to mean the outer size, we probably also want {min,max}{width,height} to mean outer size, otherwise everything would simply become more confusing, and harder to test.

So maybe persisting width/height differently should be the way forward... We need special-casing somewhere anyway :/
Flags: needinfo?(xidorn+moz)
So, I suspect bug 1445519 is enough to fix bug 1444045 (given that they are both macOS-only, and have pretty similar phenomenon), but we may still want to persist outer size given the reasoning in comment 0 and comment 13.
Comment on attachment 8957757 [details]
Bug 1444525: Persist XUL window size attributes as a window size, not client size.

https://reviewboard.mozilla.org/r/226728/#review233632

::: dom/xul/XULDocument.cpp:1195
(Diff revision 2)
> +    }
> +
> +    int32_t multiplier =
> +        aDirection == ConversionDirection::InnerToOuter ? 1 : - 1;
> +
> +    aInOutString.Assign(

CopyASCIItoUTF16(nsPrintfCString(stuff), aInOutString);

::: dom/xul/XULDocument.cpp:1845
(Diff revision 2)
>              RefPtr<Element> element = aElements.SafeObjectAt(i);
>              if (!element) {
>                   continue;
>              }
>  
> +            // Scale attributes from window size to client size for top-level

Again, comment should talk about outer vs inner, not client vs window, I think.

And s/Scale/Convert/

::: dom/xul/XULDocument.cpp:1850
(Diff revision 2)
> +            // Scale attributes from window size to client size for top-level
> +            // windows, see bug 1444525 & co.
> +            if (element->IsXULElement(nsGkAtoms::window) &&
> +                (attr == nsGkAtoms::width || attr == nsGkAtoms::height)) {
> +                if (nsCOMPtr<nsIXULWindow> win = GetXULWindowIfToplevelChrome()) {
> +                    nsAutoString maybeScaledValue = value;

maybeConvertedValue ?

::: xpfe/appshell/nsIXULWindow.idl:70
(Diff revision 2)
>     * @param aChild the child window being added
>     */
>    void addChildWindow(in nsIXULWindow aChild);
>  
>    /**
> +   * Returns the difference between the inner window size (client size) and the

Might be a good idea to make it clear that this is a nonnegative number.
Attachment #8957757 - Flags: review?(bzbarsky) → review+
No longer blocks: 1444045
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51c36c4cecd
Extract the logic to see if we're a top-level chrome window. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0038e9f8858b
Persist XUL window size attributes as the outer size, not inner size. r=bz
https://hg.mozilla.org/mozilla-central/rev/e51c36c4cecd
https://hg.mozilla.org/mozilla-central/rev/0038e9f8858b
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request beta uplift when you're comfortable doing so.
tracking-firefox60: ? → +
Flags: needinfo?(emilio)
Comment on attachment 8957757 [details]
Bug 1444525: Persist XUL window size attributes as a window size, not client size.

Whoops, sorry for the lag! Bug 1442521 was backed out in beta, no need for uplift.
Flags: needinfo?(emilio)
See Also: → 1446343
You need to log in before you can comment on or make changes to this bug.