1444525 - Persisted window sizes should be outer, not inner

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I think I can take a look at this, in exchange for the debugging pain that Xidorn saved me ;)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, I was doing the second thing, converting to client size when loading persistent attributes, I have patches on the progress of building.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

I cannot figure out an elegant way to do the second approach, though...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(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...

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(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...

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

The remaining thing would to understand where we read/write the width/height of <window>, which probably wouldn't be a lot of places...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1444525: Extract the logic to see if we're a top-level chrome window.

Review commit: https://reviewboard.mozilla.org/r/226726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226726/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1444525: Persist XUL window size attributes as a window size, not client size.

Review commit: https://reviewboard.mozilla.org/r/226728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226728/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8957756 [details]
Bug 1444525: Extract the logic to see if we're a top-level chrome window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226726/diff/1-2/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8957757 [details]
Bug 1444525: Persist XUL window size attributes as a window size, not client size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226728/diff/1-2/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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 :-(

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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 :/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e51c36c4cecd
https://hg.mozilla.org/mozilla-central/rev/0038e9f8858b

Comment 24

[Julien Cristau [:jcristau]]

Please request beta uplift when you're comfortable doing so.

Comment 25

[Emilio Cobos Álvarez (: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.