Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 10 years ago

We have a WIP patch that instead of swapping docShells with a <xul:browser> in the hidden window creates a <xul:browser src=about:newtab> in the target window and just links that to the newly created tab.

This is *a lot* faster than swapping docShells and doesn't seem to have any negative side effects. Push to try is still pending to see if that's true.

	Tomasz Kołodziejski [:tomasz]
	Comment 1 • 10 years ago

Can you attach a patch? I'd like to have a look.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 10 years ago

Sure.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 10 years ago

Comment on attachment 8499809 [details] [diff] [review]
0001-preload-new.patch (WIP)

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <method name="_getPreloadedBrowser">

>+            function getNotificationBox(browser) {
>+              return browser.parentNode.parentNode.parentNode.parentNode;
>+            }

You can just use this.getNotificationBox(browser) instead, right?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 10 years ago

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #3)
> >+            function getNotificationBox(browser) {
> >+              return browser.parentNode.parentNode.parentNode.parentNode;
> >+            }
> 
> You can just use this.getNotificationBox(browser) instead, right?

Cool. Will do.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

Pushed this to try and it doesn't look terrible:

https://tbpl.mozilla.org/?tree=Try&rev=dbeb6fa92919

However we seem to be hitting a slow path somehow only on Ubuntu:

http://compare-talos.mattn.ca/?oldRevs=ed5f810760cb&newRev=dbeb6fa92919&server=graphs.mozilla.org&submit=true

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=41263349,41263539,41263549,41263559,41263579,41267453,41267471,41267481,41267501,41267511&newTestIds=41263337,41263569,41263589,41263599,41263609,41267675,41267685,41267695,41267749,41268101&testName=tart&osName=Ubuntu%2032&server=graphs.mozilla.org
http://compare-talos.mattn.ca/breakdown.html?oldTestIds=41263245,41263417,41263427,41263461,41263477,41267227,41267237,41267247,41267255,41267297&newTestIds=41263291,41263437,41263463,41263491,41263501,41267565,41267575,41267587,41267609,41267629&testName=tart&osName=Ubuntu%2064&server=graphs.mozilla.org

The two detail links above show nicely that we react about 30+ms faster (on all OSs btw) to a UI interaction that opens a new tab and win possibly more on an even slower machine.

I have no clue though what causes the Linux regression. I was immediately reminded of bug 786484 that we hit when working on the first preload implementation. This wasn't Linux-only however and I can't see any erroneous paint flashing as described in the bug and its duplicates. I can't even reproduce the TART regression on a Linux VM or real machine.

The interesting thing here is that the frame rates of iconFade-open-DPI2 are affected. This test opens a new tab only once and then triggers the |fadein| attribute multiple times to measure the performance of only the animation itself. Somehow having this extra panel in the |mPanelContainer| seems to affect this.

Contrary to bug 786484 this patch also doesn't put a browser somewhere in the window but it puts it into the deck where all the other browser panels are. Maybe a simple background tab with about:newtab loaded would affect the times equally on Linux?

This will be hard to figure out without a way to reproduce on Ubuntu so I guess I will keep trying...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 10 years ago

Tested with 100 iterations of "iconFade-open-DPI2" on my Linux VM:

--- preload = off ---

Average (100): 5.24  Median: 5.0  stddev: 1.37
Average (100): 5.20  Median: 5.1  stddev: 0.65
Average (100): 4.17  Median: 3.4  stddev: 3.92

--- preload = on ---

Average (100): 7.58  Median: 7.0  stddev: 2.51
Average (100): 7.64  Median: 7.1  stddev: 1.71
Average (100): 5.92  Median: 5.0  stddev: 3.21

--- preload = off (one additional about:newtab tab) ---

Average (100): 7.95  Median: 7.3  stddev: 2.71
Average (100): 7.98  Median: 7.4  stddev: 2.16
Average (100): 5.58  Median: 4.4  stddev: 3.25

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 10 years ago

Well... not sure if that tells us something. This is in the middle between good and bad:

--- preload = off (with github.com/ttaubert in a tab) ---

Average (100): 6.66  Median: 6.3  stddev: 2.01
Average (100): 6.67  Median: 6.5  stddev: 1.43
Average (100): 4.88  Median: 4.2  stddev: 2.54

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 8 • 10 years ago

Looks like we have problems with images in a background tab? Either that is only a problem on Linux or doesn't manifest as easily on the other OSs.

--- preload = off (with google image search for "firefox") ---

Average (100): 7.64  Median: 7.2  stddev: 2.02
Average (100): 7.51  Median: 7.3  stddev: 0.98
Average (100): 6.19  Median: 4.5  stddev: 3.42

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 10 years ago

Seems to be slightly worse with a second google image search tab:

Average (100): 8.51  Median: 7.5  stddev: 3.03
Average (100): 8.16  Median: 7.7  stddev: 1.86
Average (100): 5.64  Median: 4.0  stddev: 4.34

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 10 • 10 years ago

Adding Jeff and Bas, maybe they have some idea about what's going on here? Or how to debug that further?

	Bas Schouten (:bas.schouten)
	Comment 11 • 10 years ago

I have -absolutely- no idea what the title of this bug even means :-)

	Dão Gottwald [:dao]
	Comment 12 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #5)
> However we seem to be hitting a slow path somehow only on Ubuntu:

Maybe OMTC being disabled? (layers.offmainthreadcomposition.enabled)

	Jeff Muizelaar [:jrmuizel]
	Comment 13 • 10 years ago

Can you explain what's being changed from a layout point of view and then I can try to guess at what the implications would be from a graphics point of view.

	Avi Halachmi (:avih)
	Comment 14 • 10 years ago

Is the only thing which blocks this bug is the linux perf regression which also happens if we have a background tab with images regardless of the change on this bug?

While worth investigation, this sounds to me unrelated to this bug, which means we could probably progress with this bug and deal with the linux background tab/browser issue in a followup, isn't it so?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 15 • 10 years ago

It does indeed sound like a more general problem. And given that users will probably have more than just one tab open it wouldn't affect people in reality. Still I'm not too confident... but OTOH I don't have a clue what's going on. I could maybe try getting a profile or some paint logs.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 16 • 10 years ago

(In reply to Avi Halachmi (:avih) from comment #14)
> Is the only thing which blocks this bug is the linux perf regression which
> also happens if we have a background tab with images regardless of the
> change on this bug?

Yes.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 17 • 10 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> Can you explain what's being changed from a layout point of view and then I
> can try to guess at what the implications would be from a graphics point of
> view.

TART is a test suite that roughly tests the frame rate we get when opening a new tab. The patch here wants to introduce a new version about:newtab preloading that holds a preloaded about:newtab <browser> in the <deck> that holds all browsers of all open tabs in the window. It seems that just adding this preload browser (or just having a second tab) impacts the frame rate on Ubuntu a lot.

Here is a profile of m-c:

http://people.mozilla.org/~bgirard/cleopatra/#report=21c525fdd1d0766f8e2908244311c4e821c55024

Here is a profile of m-c plus the preload patch:

http://people.mozilla.org/~bgirard/cleopatra/#report=be6bef66ec1a2d161500d3962894b4bbcc907cdf

Bas, Jeff, anything that sticks out here?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 18 • 10 years ago

nsRefreshDriver::Tick() in the second profile spends 13.9% of the time in:

__poll /build/buildd/glibc-2.19/io/../sysdeps/unix/syscall-template.S:81

whereas the first profile spends only 3.8% there. Outside of Tick() it spends ~20% whereas the first profile spends 12% only. I feel like waiting for syscalls to finish (if that's really what we do here) inside of nsRefreshDriver::Tick() would affect the frame rate? Does that sound plausible? Any idea how to find out what we're doing there?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 19 • 10 years ago

I profiled a TART test run with 10 iterations of opening a tab, running the close and open animation and then closing it again. Without the patch:

http://people.mozilla.org/~bgirard/cleopatra/#report=4d15673fa47540ed21b034785ac5722c049cb9c8

With the patch:

http://people.mozilla.org/~bgirard/cleopatra/#report=7d22553487c7d30066f7e0617beccc213e79afed

The two look very similar...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 20 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Tim Taubert [:ttaubert] from comment #5)
> > However we seem to be hitting a slow path somehow only on Ubuntu:
> 
> Maybe OMTC being disabled? (layers.offmainthreadcomposition.enabled)

I tried forcing OMTC on but that just makes TART time out. I could probably try setting it to off on non-Ubuntu and see whether that bring the same regression.

	Jeff Muizelaar [:jrmuizel]
	Comment 21 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #17)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> > Can you explain what's being changed from a layout point of view and then I
> > can try to guess at what the implications would be from a graphics point of
> > view.
> 
> TART is a test suite that roughly tests the frame rate we get when opening a
> new tab. The patch here wants to introduce a new version about:newtab
> preloading that holds a preloaded about:newtab <browser> in the <deck> that
> holds all browsers of all open tabs in the window. It seems that just adding
> this preload browser (or just having a second tab) impacts the frame rate on
> Ubuntu a lot.
> 
> Here is a profile of m-c:
> 
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=21c525fdd1d0766f8e2908244311c4e821c55024
> 
> Here is a profile of m-c plus the preload patch:
> 
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=be6bef66ec1a2d161500d3962894b4bbcc907cdf
> 
> Bas, Jeff, anything that sticks out here?

There are two few samples to make any conclusions. Can you increase the sampling frequency?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 22 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to Tim Taubert [:ttaubert] from comment #5)
> > > However we seem to be hitting a slow path somehow only on Ubuntu:
> > 
> > Maybe OMTC being disabled? (layers.offmainthreadcomposition.enabled)
> 
> I tried forcing OMTC on but that just makes TART time out. I could probably
> try setting it to off on non-Ubuntu and see whether that bring the same
> regression.

TART timing out was just me being stupid. Trying to force OMTC on did not help, same regression unfortunately.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 23 • 10 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> There are two few samples to make any conclusions. Can you increase the
> sampling frequency?

I checked the Profiler settings and applied the max. number of samples. Doesn't look too different here:

http://people.mozilla.org/~bgirard/cleopatra/#report=d77f520de594357234c2adcb1e79dcc9d2099014

http://people.mozilla.org/~bgirard/cleopatra/#report=0046802b4731f72c3d9c5d2c8dae068714666a15

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 10 years ago

The one thing I noticed is that if you look at the list of markers, it's only TART markers with preloading=off. With preloading=on however there are regular occurrences of:

Ion compiled resource://gre/modules/NewTabUtils.jsm:838
Ion compiled resource://gre/modules/NewTabUtils.jsm:924
Ion compiled self-hosted:431

I'm not sure what's causing us to recompile the JSM all the time but I checked that it only happens when the preloaded <browser> element exists. It doesn't even need to be used (so that preload is technically off) but just sit there in the background and still there are the Ion Compiler markers.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 10 years ago

Same markers with preload=off but about:newtab loaded in a pinned tab.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 10 years ago

Ok, so those markers appear once we ever loaded about:newtab in the browsing session. That's quite weird, browser.js is using the same JSM so it's not only accessed by about:newtab.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 27 • 10 years ago

Jan, since you got Ion working for JSMs in the first place, any chance you could take a look?

	Jan de Mooij [:jandem]
	Comment 28 • 10 years ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #27)
> Jan, since you got Ion working for JSMs in the first place, any chance you
> could take a look?

(In reply to Tim Taubert [:ttaubert] from comment #24)
> I'm not sure what's causing us to recompile the JSM all the time but I
> checked that it only happens when the preloaded <browser> element exists. It
> doesn't even need to be used (so that preload is technically off) but just
> sit there in the background and still there are the Ion Compiler markers.

How much time is there between recompiles? Are these scripts hot, like a few thousand calls and/or loop iterations? You can set |javascript.options.ion = false| to rule out Ion issues.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 29 • 10 years ago

(In reply to Jan de Mooij [:jandem] from comment #28)
> How much time is there between recompiles? Are these scripts hot, like a few
> thousand calls and/or loop iterations?

No, they're not that hot.

> You can set |javascript.options.ion = false| to rule out Ion issues.

Disabling "javascript.options.ion" doesn't produce those markers anymore but shows the same regression on try. Guess that's more of red herring then? But maybe it's pointing to something...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 30 • 10 years ago

I recently tried (and pushed to try):

1) Setting layers.offmainthreadcomposition.enabled=on
2) Setting javascript.options.ion=false
3) Making Thumbnails._shouldCapture() always return false
4) Making NewTabUtils.scheduleUpdateForHiddenPages() a no-op

None of that made the regression go away of course :/

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 31 • 10 years ago

I just pushed a backout of bug 1073993 to try. With the discovery that having about:newtab loaded once in a browsing session has *some* effect I wonder whether bug 1073993 had any perf impact, i.e. improved TART numbers and this patch would just make us go back to the numbers before bug 1073993 landed.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 32 • 10 years ago

No luck. Backing out bug 1073993 didn't (as expected) regress TART performance... My best guess currently is really some kind of platform issue and I'm a little clueless of what to investigate to track that down.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 10 years ago

The profiles still don't show anything interesting and I'm currently experimenting with making the difference higher. By just opening and pinning five about:newtab tabs the frame duration went from ~5ms to ~13ms. Guess I'll add some more more tabs and do even more profiling.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 34 • 10 years ago

I decided to not open new tabs because the tabs themselves could possibly affect what we're measuring. Instead I just added even more hidden "preloaded" about:newtab instances and the effect is very interesting:

for (let i=0; i<50; i++) {
  let browser = gBrowser._createBrowser(false, false);
  let notificationbox = gBrowser.getNotificationBox(browser);
  gBrowser.mPanelContainer.appendChild(notificationbox);

  browser.docShellIsActive = false;
  browser.loadURI("about:newtab");
}

Those fifty "inactive tabs" lead to ~9 times higher frame intervals. Here's a profile without those tabs:

http://people.mozilla.org/~bgirard/cleopatra/#report=fb7f02810e9e283ca449087bec94c22a36d8467a

Here's the profile with the fifty tabs:

http://people.mozilla.org/~bgirard/cleopatra/#report=31c8f112e201e592eca78b947e13496261bcec2f

They look quite different...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 35 • 10 years ago

With the fifty inactive tabs there seem to be five big refreshes (~50ms) between the "Start" and "End" markers of the iconFade-* tests. In the good version the refreshes seem to be a lot smaller and that probably affects the frame rate as well?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 36 • 10 years ago

All overhead goes away when adding ":root {display: none}" to newTab.css. Only hiding images doesn't seem to help either, I assume it's somehow related to the general painting of the page?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 37 • 10 years ago

I see lots of poll() and recvmsg() syscalls when tracing. fd=4 does a lot of that and it seems to be X11 socket. Looks exactly like what's reported in bug 516227.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 38 • 10 years ago

I went back to nsRefreshDriver::Tick() and measure what takes the longest. When adding 50 hidden about:newtab instances the ticks itself go up to ~60ms on my Linux VM. Most of the time is spent in nsViewManager::ProcessPendingUpdatesForView() iterating the ~150 widgets and calling widget->ResetWidgetBounds().

This whole process amounts to the 60ms. Doing the same on OS X is *much* faster. I see around ~0.25ms spent to reset the 150 widgets.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 39 • 10 years ago

Looks like we spend most of the time here in:

DoResetWidgetBounds() > widget->GetClientBounds()

where that's 0.5-6ms. That seems like a lot...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 40 • 10 years ago

gtk/nsWindow::GetClientOffset() seems to be at fault, that's where we spend most of the time.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 41 • 10 years ago

Looks like that was implemented in bug 668437. Timothy should hopefully know about this.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 42 • 10 years ago

Timothy, Karl, do you know why GetClientOffset() takes such a long time? Can we maybe cache those values or use gdk_window_get_frame_extents() if that's faster? The code is run for every view from the refresh driver, so it actually runs a lot. Is that code even meant to be in a hot loop?

It looks like we spend a lot of time poll()ing, could that be due to us waiting for a response from the XServer? Any idea?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 43 • 10 years ago

Using that code:

> GdkRectangle frame_extents;
> gdk_window_get_frame_extents(mGdkWindow, &frame_extents);
> return nsIntPoint(frame_extents.x, frame_extents.y);

brings a ~6x improvement. It however wrongly positions the Scratchpad menu and probably others. 10ms compared to the 0.25ms OS X takes still feels like a little too much though even if we could get that working.

	Timothy Nikkel (:tnikkel)
	Comment 44 • 10 years ago

Karl would better know why it's so slow but my guess is that it needs to talk to the xserver like you say.

(In reply to Tim Taubert [:ttaubert] from comment #38)
> I went back to nsRefreshDriver::Tick() and measure what takes the longest.
> When adding 50 hidden about:newtab instances the ticks itself go up to ~60ms
> on my Linux VM. Most of the time is spent in
> nsViewManager::ProcessPendingUpdatesForView() iterating the ~150 widgets and
> calling widget->ResetWidgetBounds().

Why are there 150 widgets? I would expect there to be one widget per browser window, and then a handful more for tooltips and other popups. Each hidden about:newtab instance creates a widget? That seems wrong. If we can eliminate that that should fix this and stop other waste (like creating 150 native windows for nothing).

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 45 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #44)
> Why are there 150 widgets? I would expect there to be one widget per browser
> window, and then a handful more for tooltips and other popups. Each hidden
> about:newtab instance creates a widget? That seems wrong. If we can
> eliminate that that should fix this and stop other waste (like creating 150
> native windows for nothing).

Sorry I didn't explain that further. There are 150 widgets because I created 50 about:newtab instances for better testing and more impact. Each of those creates 3 widgets. It seems like just those 3 widgets for a single preload about:newtab instance slow down our TART talos suite by 50-100%.

	Timothy Nikkel (:tnikkel)
	Comment 46 • 10 years ago

Why do we need to create those widgets at all?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 47 • 10 years ago

Those widgets are created for a preloaded about:newtab instance. Preloaded means that it's hidden and not actually shown and we don't use those widgets for anything (yet). So can we work around that and prevent those from being created?

I'm not completely sure what these widgets are but one of them is the popup we use for the search suggestion when searching on the new tab page.

	:Gijs (he/him)
	Comment 48 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #47)
> Those widgets are created for a preloaded about:newtab instance. Preloaded
> means that it's hidden and not actually shown and we don't use those widgets
> for anything (yet). So can we work around that and prevent those from being
> created?
> 
> I'm not completely sure what these widgets are but one of them is the popup
> we use for the search suggestion when searching on the new tab page.

Is that popup marked hidden="true" initially?

	Avi Halachmi (:avih)
	Comment 49 • 10 years ago

While it's probably possible to prevent the creation of some of the content (at this case - the newtab page) popups, previous comments on this bug suggest that there's a greater issue here which also happens with normal content pages loaded at background tabs.

If this is the case, and performance of the current tab drops the more background tabs there are, maybe it'd be useful to also examine a more general fix for this issue?

For instance, if possible, "declare" all the widgets at background tabs as disabled/inactive/whatever such that they don't regress performance of the current tab?

	Timothy Nikkel (:tnikkel)
	Comment 50 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #47)
> Those widgets are created for a preloaded about:newtab instance. Preloaded
> means that it's hidden and not actually shown and we don't use those widgets
> for anything (yet). So can we work around that and prevent those from being
> created?

How/where is a preloaded about:newtab instance created? I'd like to be able to go from where they are created to figure out how/why they get a widget.

	Timothy Nikkel (:tnikkel)
	Comment 51 • 10 years ago

(In reply to Avi Halachmi (:avih) from comment #49)
> While it's probably possible to prevent the creation of some of the content
> (at this case - the newtab page) popups, previous comments on this bug
> suggest that there's a greater issue here which also happens with normal
> content pages loaded at background tabs.
> 
> If this is the case, and performance of the current tab drops the more
> background tabs there are, maybe it'd be useful to also examine a more
> general fix for this issue?
> 
> For instance, if possible, "declare" all the widgets at background tabs as
> disabled/inactive/whatever such that they don't regress performance of the
> current tab?

Background tabs don't get widgets. Foreground tabs don't get widgets. We should have one widget per browser window, and then a handful more for things like tooltips and the awesomebar drop down (and plugins sometimes). If you are seeing more widgets than this it is likely a bug.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 52 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #48)
> Is that popup marked hidden="true" initially?

I just checked and we use three <xul:panel>s on about:newtab. They're gone when marking them as hidden=true. Is that what we do with those usually? :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 53 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #51)
> Background tabs don't get widgets. Foreground tabs don't get widgets. We
> should have one widget per browser window, and then a handful more for
> things like tooltips and the awesomebar drop down (and plugins sometimes).
> If you are seeing more widgets than this it is likely a bug.

about:newtab is unfortunately XUL. It is a background tab (docShell.isActive=false) but we still create widgets for those panels. So that's a bug then?

	:Gijs (he/him)
	Comment 54 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #52)
> (In reply to :Gijs Kruitbosch from comment #48)
> > Is that popup marked hidden="true" initially?
> 
> I just checked and we use three <xul:panel>s on about:newtab. They're gone
> when marking them as hidden=true. Is that what we do with those usually? :)

Yes.

http://mxr.mozilla.org/mozilla-central/search?string=hidden%3D%22true%22&find=browser.xul&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

	:Gijs (he/him)
	Comment 55 • 10 years ago

(which isn't to say that layout shouldn't be smarter here, but, hey...)

	Timothy Nikkel (:tnikkel)
	Comment 56 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #53)
> about:newtab is unfortunately XUL. It is a background tab
> (docShell.isActive=false) but we still create widgets for those panels. So
> that's a bug then?

Okay, so they are all for panels, that makes more sense.

(In reply to :Gijs Kruitbosch from comment #55)
> (which isn't to say that layout shouldn't be smarter here, but, hey...)

I would agree with that, but if you can unblock this bug with what Gijs suggested then do it.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 57 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #56)
> (In reply to :Gijs Kruitbosch from comment #55)
> > (which isn't to say that layout shouldn't be smarter here, but, hey...)
> 
> I would agree with that, but if you can unblock this bug with what Gijs
> suggested then do it.

Yeah, that should be easy to do. Local tests are looking good, will push to try soon.

	Karl Tomlinson (:karlt)
	Comment 58 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #42)
> Timothy, Karl, do you know why GetClientOffset() takes such a long time?

It blocks on waiting for a response from the X server.

> Can we maybe cache those values or use gdk_window_get_frame_extents() if
> that's faster?

Yes, I suspect we could cache the values and watch property-notify-event for
changes.

gdk_window_get_frame_extents() does similar things, but returns offsets from
the root window origin.

> The code is run for every view from the refresh driver, so it
> actually runs a lot. Is that code even meant to be in a hot loop?

We try not to block on the X server in hot loops.

> It looks like we spend a lot of time poll()ing, could that be due to us
> waiting for a response from the XServer?

Yes, it would be.

(In reply to Tim Taubert [:ttaubert] from comment #43)
> Using that code:
> 
> > GdkRectangle frame_extents;
> > gdk_window_get_frame_extents(mGdkWindow, &frame_extents);
> > return nsIntPoint(frame_extents.x, frame_extents.y);
> 
> brings a ~6x improvement. It however wrongly positions the Scratchpad menu
> and probably others. 10ms compared to the 0.25ms OS X takes still feels like
> a little too much though even if we could get that working.

gdk_window_get_frame_extents() actually does the same thing and more in the
cases where there is something to do.

I suspect the win here is because gdk_window_get_frame_extents knows there is
nothing to do on override-redirect windows.

GetClientOffset() could do something similar by returning zero under either of the following tests:

    gint type;
    g_object_get(aWidget, "type", &type, nullptr);
    if (type == GTK_WINDOW_POPUP) {

    if (gdk_window_get_window_type(window) == GDK_WINDOW_TEMP) {

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 59 • 10 years ago

Thanks for the explanations, Karl! And thanks everyone else for helping too!

http://compare-talos.mattn.ca/?oldRevs=0b50b8074ec5&newRev=ce7ad43face2&server=graphs.mozilla.org&submit=true

We finally have perf improvements instead of regressions on Linux.

	:Gijs (he/him)
	Comment 61 • 10 years ago

Comment on attachment 8516591 [details] [diff] [review]
0001-Bug-1077652-Hide-xul-panels-by-default-when-not-open.patch

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

Please expand the commit message to detail that this is for perf reasons.

Also, if we don't have one already, can we file a followup bug about having widget/layout/whatever create its widgets lazily for popups that aren't currently being shown, even if they're not explicitly marked hidden?

::: browser/base/content/newtab/intro.js
@@ +19,5 @@
>        this._nodes[idSuffix] = document.getElementById("newtab-intro-" + idSuffix);
>      }
>  
>      this._nodes.panel.addEventListener("popupshowing", e => this._setUpPanel());
> +    this._nodes.panel.addEventListener("popuphiding", e => this._hidePanel());

Nit: Use popuphidden like in the other cases? :-)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 64 • 10 years ago

Comment on attachment 8516622 [details] [diff] [review]
0002-Bug-1077652-Link-a-preloaded-xul-browser-to-a-newly-.patch

Cancelling review until I have this leak figured out.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 65 • 10 years ago

Comment on attachment 8516629 [details] [diff] [review]
0003-Bug-1077652-Remove-old-BrowserNewTabPreloader-and-fi.patch

Cancelling review until I have this leak figured out.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 66 • 10 years ago

This patch makes us have another <xul:browser> that preloads about:newtab in the <xul:tabpanels> element, the browser has however no tab assigned. The test_tabbrowser.xul a11y test fails with the following:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/tree/test_tabbrowser.xul | Different amount of expected children of ['tabpanels node', address: [object XULElement], role: pane, address: [xpconnect wrapped nsIAccessible]]. - got 3, expected 2

It clearly doesn't expect the preloaded browser. Is this something that should be expected and we should change the test? Or would that hidden browser cause problems for screen readers? It basically is in the same place as all background tabs but has no tab assigned until the next new tab is created/added.

	Marco Zehe (:MarcoZ)
	Comment 67 • 10 years ago

Tim, is there a try-server build with this patch that I can try to see what effect this change has? I'd like to give this a manual test in addition to the probable change in our test suite.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 68 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #67)
> Tim, is there a try-server build with this patch that I can try to see what
> effect this change has? I'd like to give this a manual test in addition to
> the probable change in our test suite.

Yes, here are some try builds:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ttaubert@mozilla.com-105ebd9c49f9/

	Marco Zehe (:MarcoZ)
	Comment 69 • 10 years ago

I just gave this a quick try, and when I open a new tab, a second new tab is duplicated right next to it in my screen reader's object view of things. The accessibility tree indeed shows to identical new tab pages that both look as if one could interact with them. The accessibility APIs would allow for sending mouse clicks, and other events to this non-existent tab.

How is this thing hidden visually? Is it moved to the background via z-axis or something similar? If so, we need to find an equivalent of displa: none; or visibility: hidden; for this rather than such a pseudo-invisibility. display: none; and visibility: hidden; are the only CSS properties that screen readers (and our accessibility API) treat as truly hidden.

The way it is now, a screen reader user could find this new tab and try to send some accessibility DoAction command to the tab, possibly causing unwanted results.

Side note: When I opened a new tab for the first time in this try build, I got a Windows Security warning telling me that the firewall had blocked some activities by Nightly. I had to click "Allow" for Nightly to continue functioning properly. Not sure if that was intended or not, but it certainly made for a disconcerting user experience if I put myself in the shoes of an ordinary user.

	Marco Zehe (:MarcoZ)
	Comment 70 • 10 years ago

In other words: Just adjusting the tests isn't the right thing to do here, IMO.

	:Gijs (he/him)
	Comment 71 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #69)
> Side note: When I opened a new tab for the first time in this try build, I
> got a Windows Security warning telling me that the firewall had blocked some
> activities by Nightly. I had to click "Allow" for Nightly to continue
> functioning properly. Not sure if that was intended or not, but it certainly
> made for a disconcerting user experience if I put myself in the shoes of an
> ordinary user.

This is unrelated, fwiw.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 72 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #69)
> How is this thing hidden visually? Is it moved to the background via z-axis
> or something similar? If so, we need to find an equivalent of displa: none;
> or visibility: hidden; for this rather than such a pseudo-invisibility.
> display: none; and visibility: hidden; are the only CSS properties that
> screen readers (and our accessibility API) treat as truly hidden.

This browser isn't explicitly hidden, it's put into a <xul:deck>, so only one of a few panels. Every single panel stands for one of the tabs. The preloading browser is just never the selected panel - well until it is used and is brought to the foreground.

> The way it is now, a screen reader user could find this new tab and try to
> send some accessibility DoAction command to the tab, possibly causing
> unwanted results.

This is surprising. The preloaded about:newtab instance should behave like a background tab. Can you send commands or mouse clicks to background tabs although they are not the selected tab in the browser?

	Marco Zehe (:MarcoZ)
	Comment 73 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #72)
> (In reply to Marco Zehe (:MarcoZ) from comment #69)
> > How is this thing hidden visually? Is it moved to the background via z-axis
> > or something similar? If so, we need to find an equivalent of displa: none;
> > or visibility: hidden; for this rather than such a pseudo-invisibility.
> > display: none; and visibility: hidden; are the only CSS properties that
> > screen readers (and our accessibility API) treat as truly hidden.
> 
> This browser isn't explicitly hidden, it's put into a <xul:deck>, so only
> one of a few panels. Every single panel stands for one of the tabs. The
> preloading browser is just never the selected panel - well until it is used
> and is brought to the foreground.

So the only difference is that, as long as it has no connected tab, ctrl+Tab won't navigate to it (which it doesn't, by the way).

> This is surprising. The preloaded about:newtab instance should behave like a
> background tab. Can you send commands or mouse clicks to background tabs
> although they are not the selected tab in the browser?

Yes, one could, but the simulated mouse clicks would probably go to the foreground and then produce similarly unpredictable results.

So if we assume that this is supposed to behave like any other background tab, then I think we *can* just adjust the test and test for that extra instance to make sure the exposure is correct. Comment #70 is therefore taken back.

	Avi Halachmi (:avih)
	Comment 74 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #73)
> ...
> So if we assume that this is supposed to behave like any other background
> tab, then I think we *can* just adjust the test and test for that extra
> instance to make sure the exposure is correct. Comment #70 is therefore
> taken back.

The user isn't and shouldn't be aware of the preloaded newtab page. It's only a tool for Firefox to allow displaying a new newtab page faster - once the user opens a new tab.

Therefore I don't think that it should be exposed with a11y as well, at which case I don't think the test should be adjusted (yet).

As far as I understand from the a11y descriptions, With the current patch, the behavior is different than expected IMO, and the test fails rightly.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 75 • 10 years ago

We can file a followup to find a way to properly hide it from a11y, not a blocker.

	Marco Zehe (:MarcoZ)
	Comment 76 • 10 years ago

One thing you can try:
1. put aria-hidden="true" on this particular panel in the deck initially.
2. Remove that once you associate that particular panel with a tab. The test should then still be adjusted, but could test for the presence of the aria-hidden attribute in the object attributes of that panel accessible. This would under normal circumstances hide the preloaded panel from screen readers.

	Trevor Saunders (:tbsaunde)
	Comment 77 • 10 years ago

(In reply to Avi Halachmi (:avih) from comment #74)
> (In reply to Marco Zehe (:MarcoZ) from comment #73)
> > ...
> > So if we assume that this is supposed to behave like any other background
> > tab, then I think we *can* just adjust the test and test for that extra
> > instance to make sure the exposure is correct. Comment #70 is therefore
> > taken back.
> 
> The user isn't and shouldn't be aware of the preloaded newtab page. It's
> only a tool for Firefox to allow displaying a new newtab page faster - once
> the user opens a new tab.

it seems to me like saying this is different from having a new tab in the background you can only access by "opening a new tab" is splitting hairs.  That said in an ideal world I would probably agree with your assessment.


(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #75)
> We can file a followup to find a way to properly hide it from a11y, not a
> blocker.

yeah, please go ahead, I care far more about getting rid of hidden window nonsense than a kind of fake background tab.

	Avi Halachmi (:avih)
	Comment 78 • 10 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #77)
> (In reply to Avi Halachmi (:avih) from comment #74)
> > (In reply to Marco Zehe (:MarcoZ) from comment #73)
> > > ...
> > > So if we assume that this is supposed to behave like any other background
> > > tab, then I think we *can* just adjust the test and test for that extra
> > > instance to make sure the exposure is correct. Comment #70 is therefore
> > > taken back.
> > 
> > The user isn't and shouldn't be aware of the preloaded newtab page. It's
> > only a tool for Firefox to allow displaying a new newtab page faster - once
> > the user opens a new tab.
> 
> it seems to me like saying this is different from having a new tab in the
> background you can only access by "opening a new tab" is splitting hairs. 
> That said in an ideal world I would probably agree with your assessment.

"having a new tab in the background you can only access by "opening a new tab"" is the current implementation for the semantics I expressed.

"The user should not be aware of the existence of this hidden tab" is a fact.

It's possible that the implementation doesn't completely hide that tab, at least as far as the a11y code sees it, or that we have a bug in our a11y implementation, or that the a11y implementation just doesn't expect such case where the tab exists at the deck but should be invisible.

I think the implementation's assumption is that if it's not listed as one of the browser's tabs which the user can access, then it should be completely invisible to the user also with the a11y code.

So either the assumption is incorrect, or some more code needs to be touched in order to make this assumption correct.

	Trevor Saunders (:tbsaunde)
	Comment 79 • 10 years ago

(In reply to Avi Halachmi (:avih) from comment #78)
> (In reply to Trevor Saunders (:tbsaunde) from comment #77)
> > (In reply to Avi Halachmi (:avih) from comment #74)
> > > (In reply to Marco Zehe (:MarcoZ) from comment #73)
> > > > ...
> > > > So if we assume that this is supposed to behave like any other background
> > > > tab, then I think we *can* just adjust the test and test for that extra
> > > > instance to make sure the exposure is correct. Comment #70 is therefore
> > > > taken back.
> > > 
> > > The user isn't and shouldn't be aware of the preloaded newtab page. It's
> > > only a tool for Firefox to allow displaying a new newtab page faster - once
> > > the user opens a new tab.
> > 
> > it seems to me like saying this is different from having a new tab in the
> > background you can only access by "opening a new tab" is splitting hairs. 
> > That said in an ideal world I would probably agree with your assessment.
> 
> "having a new tab in the background you can only access by "opening a new
> tab"" is the current implementation for the semantics I expressed.
> 
> "The user should not be aware of the existence of this hidden tab" is a fact.
> 
> It's possible that the implementation doesn't completely hide that tab, at
> least as far as the a11y code sees it, or that we have a bug in our a11y
> implementation, or that the a11y implementation just doesn't expect such
> case where the tab exists at the deck but should be invisible.
> 
> I think the implementation's assumption is that if it's not listed as one of
> the browser's tabs which the user can access, then it should be completely
> invisible to the user also with the a11y code.
> 
> So either the assumption is incorrect, or some more code needs to be touched
> in order to make this assumption correct.

yes, it is more or less the case that a11y assumes every child of the deck should be accessible unless its display: none or similar, which you don't want here because you want the preloaded tab to be pre layed out I guess.  Similarly I imagine you actually want accessibility for the preloaded tab to be done ahead of time, and then just tell the user about it later, but the a11y implementation isn't setup to do that.

	alexander :surkov (:asurkov)
	Comment 80 • 10 years ago

perhaps the easiest way to make sure it works is to follow Marco's suggestion and use aria-hidden="true" but technically nothing should be bad if AT had an access to that hidden tab document, anyway AT shouldn't let the user to go into it (per states). If I understand right then Marco confirms that by running the screen readers.

	Avi Halachmi (:avih)
	Comment 81 • 10 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #79)
> ...
> Similarly I imagine you actually want accessibility for the preloaded tab to
> be done ahead of time, and then just tell the user about it later, ...

The sole goal of the preloaded newtab system is to make it _display_ faster once the user opens a new tab.

So unless there's a noticeable lag when opening a new tab with a11y - which preload might help with, then there's no need for accessibility for the preloaded tab until it becomes visible.

The rest are implementation details.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 82 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #76)
> One thing you can try:
> 1. put aria-hidden="true" on this particular panel in the deck initially.
> 2. Remove that once you associate that particular panel with a tab. The test
> should then still be adjusted, but could test for the presence of the
> aria-hidden attribute in the object attributes of that panel accessible.
> This would under normal circumstances hide the preloaded panel from screen
> readers.

I did that and pushed to try:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ttaubert@mozilla.com-1f9ee0aa3e26/

Marco, can you please try whether that yields the desired behavior? Thanks!

	Marco Zehe (:MarcoZ)
	Comment 83 • 10 years ago

Unfortunately, this does not yield the desired result yet. All looks like in the other try build.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 84 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #83)
> Unfortunately, this does not yield the desired result yet. All looks like in
> the other try build.

Hmmm ok, thanks. Looks like aria-hidden=true doesn't work for browsers then or has a different effect. Thought that would be a quick fix but we should indeed move this to a follow-up than as suggested by Gavin above.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 85 • 10 years ago

Had to fight a general problem with newtab tests timing out with the patches applied. Took me some time to figure out but try is looking good so far. Will attach a patch soon to address that.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 86 • 10 years ago

With the new preloading mechanism I ran into the following problem: when updating hidden pages (i.e. that are currently preloading) we schedule a timeout of 1s to batch smaller changes. If in that second we now create a new tab and use the preloaded browser it will still show the stale state.

This patch ensures that the test suite waits for document.hidden==false after using a preloaded newtab page and that we update a page immediately when it becomes visible should a "hidden update" have been scheduled before.

I am not sure why this wasn't a problem before but looks like we didn't run into it yet. Maybe that would also fix some of the intermittent failures that are still around without a solution.

Here's a try run of only the previous and this patch (without the new preloading parts):

https://tbpl.mozilla.org/?tree=Try&rev=f3237578900d

	:Gijs (he/him)
	Comment 87 • 10 years ago

Comment on attachment 8519069 [details] [diff] [review]
0002-Bug-1077652-Fix-hidden-page-updates-to-work-better-w.patch

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

I have questions: please re-request review and/or update the patch depending on what the answers are. :-)

::: browser/base/content/test/newtab/head.js
@@ +327,5 @@
> + * Wait until a given condition becomes true.
> + */
> +function waitUntil(condition, callback = TestRunner.next) {
> +  let retry = () => waitUntil(condition, callback);
> +  executeSoon(condition() ? callback : retry);

I'm not fond of this not having a failure timeout of some description. Until we have bug 1093566, can you copy waitForCondition from browser/components/customizableui/test/head.js instead?

@@ +356,5 @@
>    }
>  
>    // The new tab page might have been preloaded in the background.
>    if (browser.contentDocument.readyState == "complete") {
> +    waitUntil(() => !browser.contentDocument.hidden, whenNewTabLoaded);

Does this work in e10s mode and/or are any of these tests enabled in e10s (yet) ?

@@ +643,5 @@
> +    },
> +
> +    updateAfterTimeoutIfHidden() {
> +      NewTabUtils.allPages.unregister(page);
> +      setTimeout(callback, 1000);

This bit I don't understand. Does the 1000ms need to stay in sync with the one in page.js? If so, isn't this just going to race and timeout once every so often, depending on which is run first? Can we not do better here to ensure this isn't going to fail intermittently?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 88 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #87)
> > +function waitUntil(condition, callback = TestRunner.next) {
> > +  let retry = () => waitUntil(condition, callback);
> > +  executeSoon(condition() ? callback : retry);
> 
> I'm not fond of this not having a failure timeout of some description. Until
> we have bug 1093566, can you copy waitForCondition from
> browser/components/customizableui/test/head.js instead?

Yes, good idea.

> >    // The new tab page might have been preloaded in the background.
> >    if (browser.contentDocument.readyState == "complete") {
> > +    waitUntil(() => !browser.contentDocument.hidden, whenNewTabLoaded);
> 
> Does this work in e10s mode and/or are any of these tests enabled in e10s
> (yet) ?

about:newtab tests do not work in e10s at all at the moment.

> > +    updateAfterTimeoutIfHidden() {
> > +      NewTabUtils.allPages.unregister(page);
> > +      setTimeout(callback, 1000);
> 
> This bit I don't understand. Does the 1000ms need to stay in sync with the
> one in page.js? If so, isn't this just going to race and timeout once every
> so often, depending on which is run first? Can we not do better here to
> ensure this isn't going to fail intermittently?

That's a good point. I actually added the timeout only to be in sync with the normal behavior of a page. This wasn't meant as a requirement which would indeed be racy. We only want to know when a hidden update would occur so we can indeed use executeSoon() here. browser_newtab_update.js is unfortunately a little uglier than it needs to be because of the custom TestRunner implementation for newtab tests. I hope we can get rid of that in the future. I fixed it for now using promises so that it reflects what the test actually expects.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 89 • 10 years ago

Try run for the two attached patches:

https://tbpl.mozilla.org/?tree=Try&rev=02c3d3cf1b52

	:Gijs (he/him)
	Comment 90 • 10 years ago

Comment on attachment 8519566 [details] [diff] [review]
0002-Bug-1077652-Fix-hidden-page-updates-to-work-better-w.patch, v2

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

r+ but please adjust the test per whatever the answer is to my question below. :-)

::: browser/base/content/test/newtab/head.js
@@ +654,5 @@
>    let page = {
>      observe: _ => _,
> +
> +    update() {
> +      if (!aOnlyIfHidden) {

Warbl. It's confusing that this param was nuked from the real code and not the test code, but I see this method is used in other tests and so effectively this is still switching between which "callback" (update or updateAfterTimeoutIfHidden) we care about...

... in which case, shouldn't updateAfterTimeoutIfHidden have a check for aOnlyIfHidden? :-)

or should only the right method be called here anyway? In which case, can we assert that?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 91 • 10 years ago

Simplified about:newtab's page update mechanism further. Pages now have only a single update() function like before that doesn't take any parameters. If the page is hidden (document.hidden=true) then it will wait a second to batch further updates and then update the page in the background. If the page is visible we will update it immediately. Updated browser_newtab_update.js to do what it actually wants to do and use promises because this double-yielding and expecting events in a certain order is just ugly and error-prone. Have to say again that I wish Task.jsm existed when I wrote this test suite :|

https://tbpl.mozilla.org/?tree=Try&rev=470f7c2991c5

	Tomasz Kołodziejski [:tomasz]
	Comment 92 • 10 years ago

We should start using some single implementation of waitForConditionPromise. There is one in [head.js](http://hg.mozilla.org/mozilla-central/file/76b85b90a8cb/browser/base/content/test/general/head.js#l105) but it's not flexible enough (does not reject on timeout, does not let you specify the error msg). Maybe you want to make your function a standard? (and file a bug about removing the other ones)

If you want to do this, please add timeout error message and a comment that it is truly asynchronous, that is, first aConditionFn will be called on the next event loop spin.

We may as well want to do it later because modifying general head.js does not seem to belong here.

	:Gijs (he/him)
	Comment 93 • 10 years ago

(In reply to Tomasz Kołodziejski [:tomasz] from comment #92)
> We should start using some single implementation of waitForConditionPromise.
> There is one in
> [head.js](http://hg.mozilla.org/mozilla-central/file/76b85b90a8cb/browser/
> base/content/test/general/head.js#l105) but it's not flexible enough (does
> not reject on timeout, does not let you specify the error msg). Maybe you
> want to make your function a standard? (and file a bug about removing the
> other ones)
> 
> If you want to do this, please add timeout error message and a comment that
> it is truly asynchronous, that is, first aConditionFn will be called on the
> next event loop spin.
> 
> We may as well want to do it later because modifying general head.js does
> not seem to belong here.

Yes, see comment #87, and bug 1093566.

	:Gijs (he/him)
	Comment 94 • 10 years ago

Comment on attachment 8520676 [details] [diff] [review]
0002-Bug-1077652-Simplify-about-newtab-page-update-mechan.patch, v3

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

I have more questions. :-(

::: browser/base/content/test/newtab/browser_newtab_bug752841.js
@@ +46,5 @@
>      gBrowser.removeTab(newTab);
> +
> +    // Wait until the original tab is visible again.
> +    let doc = existingTab.linkedBrowser.contentDocument;
> +    yield waitForCondition(() => !doc.hidden).then(TestRunner.next);

This didn't use to call TestRunner.next, why does it need to do that now?

::: browser/base/content/test/newtab/browser_newtab_update.js
@@ -13,5 @@
> -  //
> -  // Why this weird way of yielding?  First, these two functions don't return
> -  // promises, they call TestRunner.next when done.  Second, the point at which
> -  // setLinks is done is independent of when the page update will happen, so
> -  // calling whenPagesUpdated cannot wait until that time.

You're removing this comment, but the promises generated by whenPagesUpdatedAnd still call TestRunner.next, which is weird...

::: browser/base/content/test/newtab/head.js
@@ +375,5 @@
>    }
>  
>    // The new tab page might have been preloaded in the background.
>    if (browser.contentDocument.readyState == "complete") {
> +    waitForCondition(() => !browser.contentDocument.hidden).then(whenNewTabLoaded);

This delays whenNewTabLoaded but not the return... is that OK? In other words, whenNewTabLoaded is no longer guaranteed to fire before we return...

::: toolkit/modules/NewTabUtils.jsm
@@ +994,5 @@
>        updatePages = true;
>      }
>  
> +    if (updatePages) {
> +      AllPages.update();

So this will also update the non-hidden pages? Is that OK? :-)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 95 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #94)
> ::: browser/base/content/test/newtab/browser_newtab_bug752841.js
> @@ +46,5 @@
> >      gBrowser.removeTab(newTab);
> > +
> > +    // Wait until the original tab is visible again.
> > +    let doc = existingTab.linkedBrowser.contentDocument;
> > +    yield waitForCondition(() => !doc.hidden).then(TestRunner.next);
> 
> This didn't use to call TestRunner.next, why does it need to do that now?

This test checks that about:newtab is updated correctly when prefs change that specify the number of rows and columns. The test opens a new tab every time the prefs change but also checks the old state. Due to my change to how tabs are updated the initial tab is only updated after a delay. It will probably mostly be updated when its visibility changes to document.hidden=false. That doesn't happen synchronously but is fired off a runnable so we have to actually wait for doc.hidden=false before checking the initial tab's state again one lines 29-31.

> ::: browser/base/content/test/newtab/browser_newtab_update.js
> @@ -13,5 @@
> > -  //
> > -  // Why this weird way of yielding?  First, these two functions don't return
> > -  // promises, they call TestRunner.next when done.  Second, the point at which
> > -  // setLinks is done is independent of when the page update will happen, so
> > -  // calling whenPagesUpdated cannot wait until that time.
> 
> You're removing this comment, but the promises generated by
> whenPagesUpdatedAnd still call TestRunner.next, which is weird...

Yeah, TestRunner.next() is what makes about:newtab tests continue executing. I removed the comment mainly because we don't yield that weirdly anymore but use promises now.

> ::: browser/base/content/test/newtab/head.js
> @@ +375,5 @@
> >    }
> >  
> >    // The new tab page might have been preloaded in the background.
> >    if (browser.contentDocument.readyState == "complete") {
> > +    waitForCondition(() => !browser.contentDocument.hidden).then(whenNewTabLoaded);
> 
> This delays whenNewTabLoaded but not the return... is that OK? In other
> words, whenNewTabLoaded is no longer guaranteed to fire before we return...

deferred.resolve() will resolve the Promise on the next tick anyway so there shouldn't be a problem or change in behavior. TestRunner.next() would fail if that wasn't the case because we would yield while yielding.

> ::: toolkit/modules/NewTabUtils.jsm
> @@ +994,5 @@
> >        updatePages = true;
> >      }
> >  
> > +    if (updatePages) {
> > +      AllPages.update();
> 
> So this will also update the non-hidden pages? Is that OK? :-)

I actually didn't follow why bug 911307 excluded visible pages when history updates. It might have been to not confuse users although I don't see how we usually update history when about:newtab is shown. Different day, different opinion though. I think it makes sense to keep it that way and exclude visible pages in that case. Will update the patch.

	:Gijs (he/him)
	Comment 97 • 10 years ago

Comment on attachment 8521361 [details] [diff] [review]
0002-Bug-1077652-Simplify-about-newtab-page-update-mechan.patch, v4

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

r=me assuming this is green on try. Thanks for bearing with me on all the revisions! :-|

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 98 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #97)
> r=me assuming this is green on try. Thanks for bearing with me on all the
> revisions! :-|

Will push to try before pushing to fx-team. Thanks to you as well diving into that code :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 99 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=eace38367a9f

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 100 • 10 years ago

Try is looking good. I unfortunately hit a cset causing a bc1 perma-orange but none of the newtab tests are failing.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 101 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/39ef36bf1821
https://hg.mozilla.org/integration/fx-team/rev/cdfa7492569f

	Carsten Book [:Tomcat]
	Comment 102 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/39ef36bf1821
https://hg.mozilla.org/mozilla-central/rev/cdfa7492569f

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 103 • 10 years ago

Accessing browser panels through gBrowser.mPanelContainer.childNodes breaks when we have a preloaded <browser> in the child list that has no tab assigned yet. We should just use gBrowser.browsers.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 104 • 10 years ago

browser_bug906190.js was listening for any load in a tabbrowser and got easily confused by the preloader. Should fix that test to only listen for loads associated with a tab.

	Dão Gottwald [:dao]
	Comment 105 • 10 years ago

Comment on attachment 8522702 [details] [diff] [review]
0003-Bug-1077652-Don-t-access-a-tabbrowser-s-browsers-thr.patch

>       <method name="attachFormFill">
>         <body><![CDATA[
>-          for (var i = 0; i < this.mPanelContainer.childNodes.length; ++i) {
>-            var cb = this.getBrowserAtIndex(i);
>+          for (let cb of this.browsers) {
>             cb.attachFormFill();
>           }
>         ]]></body>
>       </method>
> 
>       <method name="detachFormFill">
>         <body><![CDATA[
>-          for (var i = 0; i < this.mPanelContainer.childNodes.length; ++i) {
>-            var cb = this.getBrowserAtIndex(i);
>+          for (let cb of this.browsers) {
>             cb.detachFormFill();
>           }

Please rename cb to browser.

>       <method name="offerNewEngine">
>         <parameter name="aEngine"/>
>         <body><![CDATA[
>-          var allbrowsers = getBrowser().mPanelContainer.childNodes;
>-          for (var tab = 0; tab < allbrowsers.length; tab++) {
>-            var browser = getBrowser().getBrowserAtIndex(tab);
>+          for (let browser of getBrowser().browsers) {
>             if (browser.hiddenEngines) {
>               // XXX This will need to be changed when engines are identified by
>               // URL rather than title; see bug 335102.
>               var removeTitle = aEngine.wrappedJSObject.name;
>               for (var i = 0; i < browser.hiddenEngines.length; i++) {
>                 if (browser.hiddenEngines[i].title == removeTitle) {
>                   if (!browser.engines)
>                     browser.engines = [];
>@@ -264,19 +262,17 @@
>       </method>
> 
>       <!-- If the engine that was just added to the searchbox list was
>       autodetected on this page, move it to each browser's hidden list so it is
>       no longer offered to be added. -->
>       <method name="hideNewEngine">
>         <parameter name="aEngine"/>
>         <body><![CDATA[
>-          var allbrowsers = getBrowser().mPanelContainer.childNodes;
>-          for (var tab = 0; tab < allbrowsers.length; tab++) {
>-            var browser = getBrowser().getBrowserAtIndex(tab);
>+          for (let browser of getBrowser().browsers) {

Please use gBrowser instead of getBrowser() while you're at it.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 106 • 10 years ago

Comment on attachment 8522702 [details] [diff] [review]
0003-Bug-1077652-Don-t-access-a-tabbrowser-s-browsers-thr.patch

https://hg.mozilla.org/integration/fx-team/rev/ad527e6afe82

	Christoph Kerschbaumer [:ckerschb]
	Comment 107 • 10 years ago

Comment on attachment 8522704 [details] [diff] [review]
0004-Bug-1077652-Fix-browser_bug906190.js-to-only-listen-.patch

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

Yep, looks reasonable to me. Thanks for updating!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 108 • 10 years ago

Comment on attachment 8522704 [details] [diff] [review]
0004-Bug-1077652-Fix-browser_bug906190.js-to-only-listen-.patch

https://hg.mozilla.org/integration/fx-team/rev/723b19822595

	Ryan VanderMeulen [:RyanVM]
	Comment 109 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ad527e6afe82
https://hg.mozilla.org/mozilla-central/rev/723b19822595

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 110 • 10 years ago

By making SessionStore accept :setupSyncHandler and :update messages from <xul:browser>s without a tab assigned we can preload a <xul:browser> in the background and assign a tab later. SessionStore will have the correct sync handler and know about the current content loaded in that browser. If the browser will never be assigned to a tab the received data will simply be discarded when the browser goes away due to the use of WeakMaps in SessionStore.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 111 • 10 years ago

This fixes a few errors that were showing up on test runs.

	Dão Gottwald [:dao]
	Comment 112 • 10 years ago

Comment on attachment 8525320 [details] [diff] [review]
0006-Bug-1077652-tabbrowser-should-ignore-DOMTitleChanged.patch

Would it be unfeasible or a lot more expensive to keep the browser somewhere outside of tabbrowser and move it at the right time to avoid the need for these kind of changes?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 113 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #112)
> Would it be unfeasible or a lot more expensive to keep the browser somewhere
> outside of tabbrowser and move it at the right time to avoid the need for
> these kind of changes?

It's actually unfeasible currently and IIUIC hard to fix. Unbinding a <browser> from the tree does unfortunately destroy the frameLoader/docShell. That was my first approach, trying to use appendChild() to move it from somewhere else to the deck.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 114 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #112)
> Would it be unfeasible or a lot more expensive to keep the browser somewhere
> outside of tabbrowser and move it at the right time to avoid the need for
> these kind of changes?

It's btw the only event handler I had to change. Please note that we do the same thing already in receiveMessage() for "DOMTitleChanged".

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 115 • 10 years ago

Comment on attachment 8525316 [details] [diff] [review]
0005-Bug-1077652-SessionStore-should-accept-setupSyncHand.patch

https://hg.mozilla.org/integration/fx-team/rev/24b3d7de3682

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 116 • 10 years ago

Comment on attachment 8525320 [details] [diff] [review]
0006-Bug-1077652-tabbrowser-should-ignore-DOMTitleChanged.patch

https://hg.mozilla.org/integration/fx-team/rev/103ffeaa554d

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 117 • 10 years ago

Search tests must ignore load events not coming from tabs.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 118 • 10 years ago

browser_bug400731.js should ignore DOMContentLoaded events from anything that's not the test tab.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 119 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/24b3d7de3682
https://hg.mozilla.org/mozilla-central/rev/103ffeaa554d

	Jared Wein [:jaws] (please needinfo? me)
	Comment 120 • 10 years ago

Comment on attachment 8526180 [details] [diff] [review]
0007-Bug-1077652-Fix-search-tests-to-ignore-loads-from-br.patch

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

::: browser/components/search/test/head.js
@@ +126,5 @@
> +  return new Promise(resolve => {
> +    gBrowser.addEventListener("load", function onLoadListener(aEvent) {
> +      let cw = aEvent.target.defaultView.top;
> +      let tab = gBrowser._getTabForContentWindow(cw);
> +      if (tab) {

So this ignores loads that are not coming from a tab, but it looks like it will resolve the promise for subframes that fire onload. Is this a case where we *don't* want to use `top` and wait until aEvent.target.defaultView is the top-most frame?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 121 • 10 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #120)
> > +    gBrowser.addEventListener("load", function onLoadListener(aEvent) {
> > +      let cw = aEvent.target.defaultView.top;
> > +      let tab = gBrowser._getTabForContentWindow(cw);
> > +      if (tab) {
> 
> So this ignores loads that are not coming from a tab, but it looks like it
> will resolve the promise for subframes that fire onload. Is this a case
> where we *don't* want to use `top` and wait until aEvent.target.defaultView
> is the top-most frame?

Yeah, I think that would make it more correct, indeed.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 122 • 10 years ago

browser_bug441169.js times out on try because it catches other load events as well.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 123 • 10 years ago

Comment on attachment 8526590 [details] [diff] [review]
0007-Bug-1077652-Fix-search-tests-to-ignore-loads-from-br.patch, v2

https://hg.mozilla.org/integration/fx-team/rev/9a729fdda363

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 124 • 10 years ago

Comment on attachment 8526597 [details] [diff] [review]
0009-Bug-1077652-Fix-browser_bug441169.js-to-ignore-loads.patch

https://hg.mozilla.org/integration/fx-team/rev/80426d13feca

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 125 • 10 years ago

The only test failure left that I'm running into is:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/tree/test_dochierarchy.html | Wrong child document count of root accessible - got 2, expected 1

The test simply doesn't know about the optional preloaded <browser> element and expects fewer children of the root accessible. I took a look again at CreateDocOrRootAccessible() - to determine whether a document is active it calls nsIDocument::IsActive() which will return true even for documents sitting in the bfcache. That doesn't seem to be what we want?

Shouldn't we get the document's docShell and ask that whether it is active? That would also exclude accessibles from being created for background tabs, including the preloaded tab. As Marco said in comment #73 it doesn't seem useful to have those background documents show up in screen readers, right?

Marco/Trevor, is this something we could do or is there any value in creating accessibles for hidden documents? The content of background tabs is never visible in the UI, only the tab itself with the title and favicon is visible. I don't know how screen readers work and have never used one though, please correct me if I'm wrong.

	Marco Zehe (:MarcoZ)
	Comment 126 • 10 years ago

Frankly, I think we should deal with this in another bug, if at all. So far, background tabs that hadn't reloaded yet (e. g. after a browser restart) have never been a cause of confusion or complaint from our users. So I suggest to just adjust the test so it copes with the new situation of more children, and perhaps file a follow-up bug where we can, or not, deal with that question of background tabs. Yes, I know this is a bit contradictory to what I said in comment 73, but I ran with the try build a bit more, and it really doesn't get in the way of normal work with a screen reader.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 127 • 10 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #126)
> Frankly, I think we should deal with this in another bug, if at all.

Yeah I agree, this bug is messy enough already. I wanted to merely get an opinion before filing a new bug :)

> So far,
> background tabs that hadn't reloaded yet (e. g. after a browser restart)
> have never been a cause of confusion or complaint from our users. So I
> suggest to just adjust the test so it copes with the new situation of more
> children, and perhaps file a follow-up bug where we can, or not, deal with
> that question of background tabs. Yes, I know this is a bit contradictory to
> what I said in comment 73, but I ran with the try build a bit more, and it
> really doesn't get in the way of normal work with a screen reader.

That's good information. I think we should look into ignoring inactive docShells, will file a follow-up later.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 128 • 10 years ago

Filed bug 1103899.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 129 • 10 years ago

This patch doesn't do too much magic. It basically moves creating the <browser> element from addTab() to its own method. There is few code related to preloading itself.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 131 • 10 years ago

Removing test changes from bug 794041.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 132 • 10 years ago

Make a11y tests aware of the extra browser. Follow-up to handle this better is bug 1103899.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 133 • 10 years ago

Remove the old preloader.

	Neil Deakin
	Comment 134 • 10 years ago

Comment on attachment 8516591 [details] [diff] [review]
0001-Bug-1077652-Hide-xul-panels-by-default-when-not-open.patch

Why do you set the panel to hidden after it has closed? This will cause the panel frame and widget to be destroyed and will need to be recreated when it gets opened again.

	Carsten Book [:Tomcat]
	Comment 135 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9a729fdda363
https://hg.mozilla.org/mozilla-central/rev/80426d13feca

	Dão Gottwald [:dao]
	Comment 136 • 10 years ago

Comment on attachment 8527594 [details] [diff] [review]
0010-Bug-1077652-Introduce-new-preloading-mechanism.patch

>+      <method name="_getPreloadedBrowser">
>+        <body>
>+          <![CDATA[
>+            if (this._isPreloadingEnabled()) {
>+              // The preloaded browser might be null.
>+              let browser = this._preloadedBrowser;
>+
>+              // Consume the browser.
>+              this._preloadedBrowser = null;
>+
>+              // Attach the nsIFormFillController now that we know the browser
>+              // will be used. If we do that before and the preloaded browser
>+              // won't be consumed until shutdown then we leak a docShell.
>+              if (browser && this.hasAttribute("autocompletepopup")) {
>+                browser.setAttribute("autocompletepopup", this.getAttribute("autocompletepopup"));
>+                browser.attachFormFill();
>+              }
>+
>+              return browser;
>+            }
>+          ]]>
>+        </body>
>+      </method>

Need to always return a value. E.g.:

if (!this._isPreloadingEnabled())
  return null;

>+            // and the URL is "about:newtab". We do not support preloading for
>+            // custom newtab URLs.

Why not?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 137 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #136)
> Need to always return a value. E.g.:
> 
> if (!this._isPreloadingEnabled())
>   return null;

Will do.

> >+            // and the URL is "about:newtab". We do not support preloading for
> >+            // custom newtab URLs.
> 
> Why not?

Back in bug 875257 we disabled preloading for custom newtab pages. As a user it's easy to change browser.newtab.url and not know about the preloading part. It probably doesn't work out of the box, the page might have to be aware that it's being preloaded. It would also make future changes to the preloading system harder if suddenly we have add-ons with custom newtab pages expecting a certain behavior.

	Drew Willcoxon :adw
	Comment 138 • 10 years ago

Comment on attachment 8527595 [details] [diff] [review]
0011-Bug-1077652-Update-browser_newtab_background_capture.patch

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

::: browser/base/content/test/newtab/browser_newtab_background_captures.js
@@ -40,4 @@
>  
> -  // When newtab is loaded very quickly (which is what happens in 99% of cases)
> -  // there is no need to wait so no tests are run. Because each test requires
> -  // either a pass, fail or todo we run a dummy test here.

This comment is helpful, so as long as the ok(true) is here, I think the comment should be here, too.

@@ +64,5 @@
>  function wait(ms) {
>    setTimeout(TestRunner.next, ms);
>  }
> +
> +function waitForBrowserLoad(browser) {

Hmm, do no other newtab tests need this?  Seems like this would be broadly useful and so should be in head.js, but if not, OK. :-)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 139 • 10 years ago

Comment on attachment 8527603 [details] [diff] [review]
0014-Bug-1077652-Remove-old-BrowserNewTabPreloader.jsm-an.patch

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

I will gladly r+ any patch that removes code without breaking tests (or add-ons ;) and also present you with a beer next time I see you. Cheers!

	Trevor Saunders (:tbsaunde)
	Comment 140 • 10 years ago

Comment on attachment 8527600 [details] [diff] [review]
0012-Bug-1077652-Revert-change-from-bug-794041-that-makes.patch

nice

	Trevor Saunders (:tbsaunde)
	Comment 141 • 10 years ago

Comment on attachment 8527602 [details] [diff] [review]
0013-Bug-1077652-Let-a11y-tests-know-about-the-extra-prel.patch

>-    is(root.childDocumentCount, 1,
>+    ok(root.childDocumentCount >= 1,
>        "Wrong child document count of root accessible");

can we check its exactly two so we find out about future increases or decreases?

>+                  children: [
>+                    {
>+                      // #document ("about:newtab" preloaded)
>+                      role: ROLE_APPLICATION

I assume the document for about:newtab has role=application?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 142 • 10 years ago

Fixed _getPreloadedBrowser() to always return null.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 143 • 10 years ago

Comment on attachment 8526303 [details] [diff] [review]
0008-Bug-1077652-Fix-browser_bug400731.js-to-ignore-loads.patch

Gijs, can you maybe review this? I'm aware that safebrowsing probably isn't your specialty but the fix is very general :)

	:Gijs (he/him)
	Comment 144 • 10 years ago

Comment on attachment 8526303 [details] [diff] [review]
0008-Bug-1077652-Fix-browser_bug400731.js-to-ignore-loads.patch

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

Sure, rs=me for this test change, which seems simple enough and was randomorange waiting to happen anyway. :-)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 145 • 10 years ago

(In reply to Drew Willcoxon :adw from comment #138)
> > -  // When newtab is loaded very quickly (which is what happens in 99% of cases)
> > -  // there is no need to wait so no tests are run. Because each test requires
> > -  // either a pass, fail or todo we run a dummy test here.
> 
> This comment is helpful, so as long as the ok(true) is here, I think the
> comment should be here, too.

Good point but I think I'll just move the ok() call to inside the observer.

> @@ +64,5 @@
> >  function wait(ms) {
> >    setTimeout(TestRunner.next, ms);
> >  }
> > +
> > +function waitForBrowserLoad(browser) {
> 
> Hmm, do no other newtab tests need this?  Seems like this would be broadly
> useful and so should be in head.js, but if not, OK. :-)

I just realized that this is similar to what addNewTabPageTabPromise() is doing. I'll move waitForBrowserLoad() to head.js and let addNewTabPageTabPromise() use it too.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 146 • 10 years ago

Comment on attachment 8526303 [details] [diff] [review]
0008-Bug-1077652-Fix-browser_bug400731.js-to-ignore-loads.patch

(In reply to :Gijs Kruitbosch from comment #144)
> Sure, rs=me for this test change, which seems simple enough and was
> randomorange waiting to happen anyway. :-)

Thanks!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 147 • 10 years ago

Comment on attachment 8526303 [details] [diff] [review]
0008-Bug-1077652-Fix-browser_bug400731.js-to-ignore-loads.patch

https://hg.mozilla.org/integration/fx-team/rev/5b55d1963343

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 148 • 10 years ago

(In reply to Neil Deakin from comment #134)
> Why do you set the panel to hidden after it has closed? This will cause the
> panel frame and widget to be destroyed and will need to be recreated when it
> gets opened again.

Sure, if we destroy the panel on hide we will have to re-create it on re-opening. That doesn't necessarily seem bad to me, at least better than opening it once and having refresh driver ticks take longer until that panel goes away. Guess I'd feel more comfortable removing that with bug 1097114 fixed.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 149 • 10 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #141)
> >-    is(root.childDocumentCount, 1,
> >+    ok(root.childDocumentCount >= 1,
> >        "Wrong child document count of root accessible");
> 
> can we check its exactly two so we find out about future increases or
> decreases?

The problem is that this specific test only fails on some platforms. So sometimes I assume there is a test run before that leads to a new tab being preloaded. But sometimes that's not happening.

> >+                  children: [
> >+                    {
> >+                      // #document ("about:newtab" preloaded)
> >+                      role: ROLE_APPLICATION
> 
> I assume the document for about:newtab has role=application?

Yes, it's unfortunately still a XUL document.

	Ryan VanderMeulen [:RyanVM]
	Comment 150 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5b55d1963343

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 151 • 10 years ago

Turns out that as much as we can't promise the preloaded browser will actually be there, we can't promise that it will be the first or last child of the root accessible. This patch fixes test_dochierarchy.html to check whether |tabDoc| is one of the root's child documents.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 152 • 10 years ago

Here's a test run with the a11y failures fixed:

https://tbpl.mozilla.org/?tree=Try&rev=499e198bfb0a

	Dão Gottwald [:dao]
	Comment 153 • 10 years ago

Comment on attachment 8528340 [details] [diff] [review]
0010-Bug-1077652-Introduce-new-preloading-mechanism.patch, v2

>@@ -4264,16 +4350,19 @@
>           }
> 
>           // XXXmano: this is a temporary workaround for bug 345399
>           // We need to manually update the scroll buttons disabled state
>           // if a tab was inserted to the overflow area or removed from it
>           // without any scrolling and when the tabbar has already
>           // overflowed.
>           this.mTabstrip._updateScrollButtonsDisabledState();
>+
>+          // Preload the next about:newtab if there isn't one already.
>+          window.setTimeout(() => this.tabbrowser._createPreloadBrowser());

Why the setTimeout? Looks like an attempt to improve responsiveness, but I don't think this is going to help.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 154 • 10 years ago

Removed the setTimeout(). Disabled preloading for e10s again as we did before. Did some TART runs and preloading doesn't seem to help a lot here, the goal is to let page loads not affect parent animations anyway so I think we're fine with leaving that disabled for e10s.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 155 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/884ce04c007f
https://hg.mozilla.org/integration/fx-team/rev/afd5df8ab5af
https://hg.mozilla.org/integration/fx-team/rev/31c163560ed4
https://hg.mozilla.org/integration/fx-team/rev/00bdefd9c4fa
https://hg.mozilla.org/integration/fx-team/rev/eda93688e4b5

	Dão Gottwald [:dao]
	Comment 156 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #154)
> Disabled preloading for e10s again as we did
> before. Did some TART runs and preloading doesn't seem to help a lot here,
> the goal is to let page loads not affect parent animations anyway so I think
> we're fine with leaving that disabled for e10s.

It just dawned on me that about:newtab isn't remote even with e10s enabled, so the above doesn't make much sense to me. It seems that we should either preload regardless of e10s or get rid of preloading altogether if it doesn't help. If it really does help without e10s but not with e10s, I'd like to understand why.

	Phil Ringnalda (:philor)
	Comment 157 • 10 years ago

Linux debug, https://treeherder.mozilla.org/logviewer.html#?job_id=1356670&repo=fx-team, and Linux64 debug, https://treeherder.mozilla.org/logviewer.html#?job_id=1358334&repo=fx-team, and even once 10.8 debug, https://treeherder.mozilla.org/logviewer.html#?job_id=1356470&repo=fx-team, would very much prefer that you not do that.

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b5d29f43e52a

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 158 • 10 years ago

Wow, I pushed this to try just hours before landing. Sorry for that.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 159 • 10 years ago

(In reply to Dão Gottwald [:dao] from comment #156)
> It just dawned on me that about:newtab isn't remote even with e10s enabled,
> so the above doesn't make much sense to me. It seems that we should either
> preload regardless of e10s or get rid of preloading altogether if it doesn't
> help. If it really does help without e10s but not with e10s, I'd like to
> understand why.

I did some new measurements and looks like I got it wrong. Enabling preloading does also help e10s and I'll remove those changes with the next push after I figured out those intermittent failures.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 160 • 10 years ago

Latest try push with one more a11y test fixed:

https://tbpl.mozilla.org/?tree=Try&rev=69c4bf04e89d

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 161 • 10 years ago

Turns out we need another a11y test fix. test_media.html was timing out because test_links.html picked the wrong accessible - it was sometimes using the preloaded browser's accessible.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 162 • 10 years ago

I'm just going to merge this into part 13 after it was reviewed.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 163 • 10 years ago

Pushed to try once more:

https://tbpl.mozilla.org/?tree=Try&rev=deba29044c86

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 164 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/3e110e958354
https://hg.mozilla.org/integration/fx-team/rev/9c5abfcdf105
https://hg.mozilla.org/integration/fx-team/rev/bf5c62878b99
https://hg.mozilla.org/integration/fx-team/rev/66cd2733b4e1
https://hg.mozilla.org/integration/fx-team/rev/dca7b6d0eb79

	Carsten Book [:Tomcat]
	Comment 165 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3e110e958354
https://hg.mozilla.org/mozilla-central/rev/9c5abfcdf105
https://hg.mozilla.org/mozilla-central/rev/66cd2733b4e1
https://hg.mozilla.org/mozilla-central/rev/dca7b6d0eb79

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 166 • 10 years ago

That one got lost somehow:

https://hg.mozilla.org/mozilla-central/rev/bf5c62878b99

	Cornel Ionce [:noni] [Hubs QA]
	Comment 167 • 10 years ago

Is there any proper way to manually verify this issue?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 168 • 10 years ago

There is no way to verify the new implementation. You however can verify that all except the first newly opened tab in a window are preloaded, with the new patch even in an e10s window. You can tell if you don't see about:newtab loading but it just appears when opening a new tab.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 169 • 10 years ago

Thank you Tim!

Indeed, the about:newtab page won't load when opening a new tab with both e10s enabled/disabled.
Verified on Latest Nightly, build ID: 20141215030201.