Closed Bug 1367830 Opened 7 years ago Closed 5 years ago

mark hidden window as inactive (so we will soon not run the refresh driver in the hidden window)

Categories

(Core :: Layout, enhancement, P2)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: perf)

Attachments

(3 files)

Bug 1367830 - Refactor to share more code between private and non-private hidden window creation.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1367830 - Mark hidden window as inactive.
59 bytes, text/x-review-board-request
nika
: review+
Details
Allow setting chrome docshells as inactive
1.25 KB, patch
nika
: review+
Details | Diff | Splinter Review 
Right now we run the refresh driver in the hidden window, which causes some (reasonably simple) reflows during startup.

We should also ensure that PresShell::SetIsActive(false) happens on the hidden window's pres shell.

(But see also bug 71895.)
(I'm not planning to work on this further now.)
See Also: → 1352205
On the other hand, if we do bug 1352205 in full (which is sounding likely), this patch might be all we need.
Comment on attachment 8871536 [details]
Bug 1367830 - Refactor to share more code between private and non-private hidden window creation.

https://reviewboard.mozilla.org/r/143008/#review147048

::: xpfe/appshell/nsAppShellService.cpp:144
(Diff revision 2)
> -    rv = JustCreateTopWindow(nullptr, url,
> +  rv = JustCreateTopWindow(nullptr, url,
> -                             chromeMask, initialWidth, initialHeight,
> +                           chromeMask, initialWidth, initialHeight,
> -                             true, nullptr, nullptr, getter_AddRefs(newWindow));
> +                           true, nullptr, nullptr, getter_AddRefs(newWindow));
> -    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
> +  {

Is it necessary to Release() the docShell before swapping the private window? This extra scope seems ugly & unnecessary.

::: xpfe/appshell/nsAppShellService.cpp:147
(Diff revision 2)
>      if (docShell) {
> +      if (aIsPrivate) {

This is really ugly - I would ask you to merge into `if (aIsPrivate && docShell)` except that you need the `docShell` for non-private windows in part 2.

::: xpfe/appshell/nsAppShellService.cpp:154
(Diff revision 2)
> -      docShell->SetAffectPrivateSessionLifetime(false);
> +        docShell->SetAffectPrivateSessionLifetime(false);
> -    }
> +      }
> +    }
> +  }
>  
> +  if (!aIsPrivate) {

Can we flip this if statement? Other if statements here use (aIsPrivate) not (!aIsPrivate)

::: xpfe/appshell/nsAppShellService.cpp:157
(Diff revision 2)
> +  }
>  
> +  if (!aIsPrivate) {
> +    mHiddenWindow.swap(newWindow);
> +  } else {
> +    // We created the hidden private window

This comment feels silly here

::: xpfe/appshell/nsAppShellService.cpp:161
(Diff revision 2)
> +  } else {
> +    // We created the hidden private window
>      mHiddenPrivateWindow.swap(newWindow);
>    }
>  
>    // RegisterTopLevelWindow(newWindow); -- Mac only

This isn't an issue with your patch, but I still find this very confusing. What is the purpose of this comment? It appears from blame to have just floated around here since the netscape days.
Attachment #8871536 - Flags: review?(michael) → review+
Comment on attachment 8871537 [details]
Bug 1367830 - Mark hidden window as inactive.

https://reviewboard.mozilla.org/r/143010/#review147054
Attachment #8871537 - Flags: review?(michael) → review+
Summary: don't run the refresh driver in the hidden window → mark hidden window as inactive (so we will soon not run the refresh driver in the hidden window)
Comment on attachment 8871536 [details]
Bug 1367830 - Refactor to share more code between private and non-private hidden window creation.

https://reviewboard.mozilla.org/r/143008/#review147064

::: xpfe/appshell/nsAppShellService.cpp:144
(Diff revision 2)
> -    rv = JustCreateTopWindow(nullptr, url,
> +  rv = JustCreateTopWindow(nullptr, url,
> -                             chromeMask, initialWidth, initialHeight,
> +                           chromeMask, initialWidth, initialHeight,
> -                             true, nullptr, nullptr, getter_AddRefs(newWindow));
> +                           true, nullptr, nullptr, getter_AddRefs(newWindow));
> -    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
> +  {

Not particularly, but it seemed like a good idea; often leads to less destructor code to keep scopes small, although it doesn't particularly matter here.  I guess I'll drop it.

::: xpfe/appshell/nsAppShellService.cpp:147
(Diff revision 2)
>      if (docShell) {
> +      if (aIsPrivate) {

Yeah, I did it this way so I wouldn't have to change it again in part 2.

::: xpfe/appshell/nsAppShellService.cpp:154
(Diff revision 2)
> -      docShell->SetAffectPrivateSessionLifetime(false);
> +        docShell->SetAffectPrivateSessionLifetime(false);
> -    }
> +      }
> +    }
> +  }
>  
> +  if (!aIsPrivate) {

Sure.

::: xpfe/appshell/nsAppShellService.cpp:157
(Diff revision 2)
> +  }
>  
> +  if (!aIsPrivate) {
> +    mHiddenWindow.swap(newWindow);
> +  } else {
> +    // We created the hidden private window

Yeah, I was trying to preserve the old comment, but I'll drop it.

::: xpfe/appshell/nsAppShellService.cpp:161
(Diff revision 2)
> +  } else {
> +    // We created the hidden private window
>      mHiddenPrivateWindow.swap(newWindow);
>    }
>  
>    // RegisterTopLevelWindow(newWindow); -- Mac only

Oops, meant to delete this comment, but forgot.  I'll do so.
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48ee194df638
Refactor to share more code between private and non-private hidden window creation.  r=mystor
https://hg.mozilla.org/integration/autoland/rev/d706d716fcbb
Mark hidden window as inactive.  r=mystor
Assignee: nobody → dbaron
I've been meaning to follow up here:  this didn't actually do much because it doesn't actually propagate to the PresShell for some reason; I've been meaning to debug that.
That's because of the test at the start of nsDocShell::SetIsActive.  Basically, the patches here so far haven't done anything yet.  (I'd meant to test, but it managed to drop off my list of things to do during the QF work week...)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox55: fixed → ---
status-firefox57: affected → ---
Target Milestone: mozilla55 → ---
With that removed, I no longer see reflows in the hidden window triggered from vsync refresh driver timer ticks.
So my patch here is causing a single mochitest failure (visible in the above try runs):

 FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_docload.html | Scenario #0 of test with ID = 'open dialog '../events/docload_wnd.html'' failed. Dupe reorder event.

From the history it looks like this is something that's been worked around before.  I'm curious if you have any advice on how to approach investigating this failure -- either adjusting the test, or figuring out whether there's a real problem as a result of the hidden window being inactive and having a slower refresh driver cycle.
Flags: needinfo?(surkov.alexander)
Sorry for delay in answering. It makes sense to disable all tests but openWndShutdownDoc in events/test_docload.html to reduce the noise, and then add lines:

gA11yEventDumpToConsole = true;
enableLogging("tree,doclifecycle");

This will provide a log that may explain why we get second reorder event on a root accessible.
Flags: needinfo?(surkov.alexander)
Priority: -- → P2
I should see what effects bug 638313 had.
This is to allow the hidden window to become inactive.

The code being removed dates to the origin of the function in bug 343515
(changeset b7836c3a63dba95c983bde1ae4402386c877640b, August 2010).

MozReview-Commit-ID: Edf2zcINpiA
Attachment #8923887 - Flags: review?(nika)
Comment on attachment 8923887 [details] [diff] [review]
Allow setting chrome docshells as inactive

Review of attachment 8923887 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a reasonable change assuming that tests still pass.
Attachment #8923887 - Flags: review?(nika) → review+
OK, in the new run there are a bunch of:

accessible/tests/mochitest/events/docload/test_docload_shutdown.html | Scenario #0 of test with ID = 'open dialog '../../events/docload/docload_wnd.html'' failed. Dupe reorder event.
See Also: → 827976

I'd forgotten about this one, but here's a new try run.

https://hg.mozilla.org/mozilla-central/rev/6692575191f3

Status: REOPENED → RESOLVED
Closed: 7 years ago → 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: