345560 - [FIX]"chromehidden" attribute being set after window layout is complete

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, we dont' set the "chromehidden" attribute until we're completely done loading and rendering the window (that is, until onload fires).  This means that if any chrome _is_ hidden, we at that point reresolve style for the whole window, reflow, etc.

Patch coming up that sets "chromehidden" right before the XUL document calls StartLayout -- at that point there are no frames yet, so when we _do_ create those we end up with the right style and layout the first time around.

This does make nsXULDocument depend on appshell, but I think that's ok, right?

This patch makes test 2 of http://www.24fun.com/downloadcenter/benchjs/benchjs.html about 30% faster over here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Note that I removed the "old value" check since content code does such a check itself anyway, so this was just duplicating code.

I also left the parts of ApplyChromeFlags that didn't have to do with this attribute running after the chrome is loaded; if I can move them earlier that would be great, but it wasn't obvious that they optimized away no-ops like the attr change does...

Comment 2

[[e-mail]]

Comment on attachment 230233 [details] [diff] [review]
Proposed patch

>+        // Before starting layout, check whether we're a toplevel chrome
>+        // window.  If we are, set our chrome flags now, so that we don't have
>+        // to restyle the whole frame tree after StartLayout.
>+        nsCOMPtr<nsISupports> container = GetContainer();
>+        nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(container);
SameCOMIdentity cannot tell that your container is a canonical nsISupports pointer so does a do_QueryInterface on it anyway. Therefore there's no point keeping around the nsISupports pointer, you might as well just do_QueryInterface(GetContainer()); [although personally I'd prefer to be able to do_QueryReferent(GetContainerWeak());]
Does it not suffice to check that your item has no parent?
Is it worth wrapping this in an if (mRootContent) check?
[ApplyChromeFlags does check too, of course]

>+  /**
>+   * Back-door method to force application of chrome flags at a particular
>+   * time.  Do NOT call this unless you know what you're doing!  In particular,
>+   * calling this when this XUL window doesn't yet have a document in its
>+   * docshell could cause problems.
>+   */
It could? It seems to me that it double-checks everything.

(In reply to comment #1)
>I also left the parts of ApplyChromeFlags that didn't have to do with this
>attribute running after the chrome is loaded; if I can move them earlier
>that would be great, but it wasn't obvious that they optimized away no-ops
>like the attr change does...
Doubtful; for instance setting the scrollbars relies on the content window.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> you might as well just do_QueryInterface(GetContainer());

Doesn't compile, I hope.  If it _does_ compile, it leaks.

> Does it not suffice to check that your item has no parent?

I'd rather not introduce such dependencies.  For example, once we allow loads in docshells that are not in any docshell tree having no parent wouldn't mean you're the chrome.

> Is it worth wrapping this in an if (mRootContent) check?

Not really, since ApplyChromeFlags does that.

> It could? It seems to me that it double-checks everything.

It does, but the very act of checking for a document would create an about:blank document in the docshell in the middle of the XUL document load.  Things _might_ still work ok.  Or they might not.

> Doubtful; for instance setting the scrollbars relies on the content window.

Ah, ok.  I'll add some comments to make sure no one removes that if check.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 230233 [details] [diff] [review]
Proposed patch

>Index: content/xul/document/src/nsXULDocument.cpp
>@@ -2945,16 +2948,35 @@ nsXULDocument::ResumeWalk()
...
>+        // Before starting layout, check whether we're a toplevel chrome
>+        // window.  If we are, set our chrome flags now, so that we don't have
>+        // to restyle the whole frame tree after StartLayout.
>+        nsCOMPtr<nsISupports> container = GetContainer();
>+        nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(container);
>+        if (item) {
>+            nsCOMPtr<nsIDocShellTreeOwner> owner;
>+            item->GetTreeOwner(getter_AddRefs(owner));
>+            nsCOMPtr<nsIXULWindow> xulWin = do_GetInterface(owner);
>+            if (xulWin) {
>+                nsCOMPtr<nsIDocShell> xulWinShell;
>+                xulWin->GetDocShell(getter_AddRefs(xulWinShell));
>+                if (SameCOMIdentity(xulWinShell, container)) {

Changing this to |SameCOMIdentity(xulWinShell, GetContainer())| should neither leak nor fail to compile.

Comment 5

[[e-mail]]

(In reply to comment #4)
>>+                if (SameCOMIdentity(xulWinShell, container)) {
>Changing this to |SameCOMIdentity(xulWinShell, GetContainer())| should neither
>leak nor fail to compile.
Based on bz's comment it will leak. Plus GetContainer() is somewhat expensive.

Comment 6

[Boris Zbarsky [:bzbarsky]]

In case people care, SameCOMIdentity(xulWinShell, GetContainer()) gives:

../../../../../mozilla/content/xul/document/src/nsXULDocument.cpp: In member 
   function `nsresult nsXULDocument::ResumeWalk()':
../../../../../mozilla/content/xul/document/src/nsXULDocument.cpp:2958: error: cannot
   convert `already_AddRefed<nsISupports>' to `nsISupports*' for argument `2' 
   to `NSCAP_BOOL SameCOMIdentity(nsISupports*, nsISupports*)'

as expected.  And since I need the container anyway to get the tree owner, I'm sticking with the original code.  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, sorry, i didn't realize that GetContainer returned an already_AddRefed.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 279171 has been marked as a duplicate of this bug. ***

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Horray!  Except for the bit about searching for duplicates before filing.

Comment 11

[[e-mail]]

(In reply to comment #8)
>Ah, sorry, i didn't realize that GetContainer returned an already_AddRefed.
You're not the first! Perhaps we should remove GetContainer and replace nsCOMPtr<nsISupports> container = GetContainer(); with
nsCOMPtr<nsIFoo> foo = do_QueryReferent(GetContainerWeak());

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, I would have realized the moment i tried to compile, so I don't think it's a big deal.