about:newtab preloader should load content.js

RESOLVED FIXED in Firefox 33

Status

()

Firefox
New Tab Page
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=2 s=33.1 [qa-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Right now, browsers created by BrowserNewTabPreloader.jsm don't have content.js loaded into them until they move out of the preloader.  That means that search.js can't set up about:newtab's search state until the page becomes visible.

That leads to a couple of jarring visible changes when the page does become visible.  The logo of the current search provider at the start of the search field either pops in, or if the provider doesn't have a logo, then the logo div pops out, causing the search field to be slightly resized.  And the search field placeholder text pops in if the provider doesn't have a logo.

Setting up the page's search state inside the preloader may also fix some of the TART regression in bug 1004429, although I wasn't able to prove it using tryserver and the various talos tools.
Flags: firefox-backlog+

Updated

4 years ago
Whiteboard: p=2 → p=2 [qa-]

Updated

4 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Whiteboard: p=2 [qa-] → p=2 s=33.1 [qa-]
(Assignee)

Comment 1

4 years ago
Created attachment 8441193 [details] [diff] [review]
patch

This is a small patch, but it took a while to arrive at it.

The part with the SwapDocShells event lets gSearch request its initial state (via GetState) only once, instead of requesting it in init and then again, as a hedge, in setUpInitialState.  setUpInitialState would be necessary because the page might move out of the preloader while it's waiting on the response to the GetState sent in init; the response might never arrive due to swapping browsers.

https://tbpl.mozilla.org/?tree=Try&rev=ade34b271728
Attachment #8441193 - Flags: review?(ttaubert)
Comment on attachment 8441193 [details] [diff] [review]
patch

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

::: browser/modules/ContentSearch.jsm
@@ +72,5 @@
> +    // another browser's.
> +    msg.handleEvent = function (event) {
> +      this.target = event.detail;
> +    };
> +    msg.target.addEventListener("SwapDocShells", msg, true);

Took me a while to understand it, that's nice but maybe a little too magic? SwapDocShells events shouldn't happen too often, so how about calling .addEventListener() with |ContentSearch| as the handler and then iterating the event queue to update all messages with affected targets? If we're worried about performance we could as well have a WeakMap pointing to queued messages for a given browser?

I would like it even more if the EventQueue was its own object/class instead being part of |ContentSearch|. We could let it take care of SwapDocShells events then as described above and have a clearer separation of concerns.
Attachment #8441193 - Flags: review?(ttaubert) → feedback+
Comment on attachment 8441193 [details] [diff] [review]
patch

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

::: browser/modules/ContentSearch.jsm
@@ +68,5 @@
>    },
>  
>    receiveMessage: function (msg) {
> +    // Keep msg.target, a xul:browser, updated when its docshell is swapped for
> +    // another browser's.

As discussed on IRC, let's keep this solution but please refine the comment like so:

"Add a temporary event handler that exists only while the message is in the event queue. Should the message's source docShell change browsers in the meantime then we need to update |msg.target|. |event.detail| will point to the docShell's new parent <xul:browser> element."
Attachment #8441193 - Flags: feedback+ → review+
(Assignee)

Comment 4

4 years ago
Thanks, Tim.

https://hg.mozilla.org/integration/fx-team/rev/f48164c8c82c
https://hg.mozilla.org/mozilla-central/rev/f48164c8c82c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33

Comment 6

4 years ago
I just tested with this patch, and it's a great improvement! (didn't try to measure it though)

However, I noticed that when choosing Yahoo as the main search provider, a faint "blink" is still visible with the logo also when the newtab page is preloaded. I couldn't it notice with any of the other default search providers.

Is there anything different with the Yahoo provider?
(Assignee)

Comment 7

4 years ago
There's nothing different about Yahoo other than the fact that its text is really, really thin, so maybe it's an optical illusion. :-)  Are you sure you're waiting enough time to let the preloader start loading the page, plus a small but arbitrary amount to let the async message passing finish?  Not a lot of time of course, 5s after browser-delayed-startup for the first preload or 600ms for subsequent preloads, and for the message passing an additional second or three should be more than enough.

I do notice flicker when I don't let all of that finish, but I don't when I do.

Comment 8

4 years ago
I've waited enough, on some occasions more than 5s. Even more, when I wait less and don't let preload happen, there a very clear "blink" with the logo, like it was before this patch.

But when I wait (several different versions of) enough, I can notice a a very faint "blink" with the Yahoo logo but not with any of the other search providers.

Comment 9

4 years ago
Hmm.. I think I know what it is.

I was so focused on the search bar of newtab, that I didn't notice before that this "faint blink" actually happens on all the preview images as well. Now I do, and all the preview thumbnails also exhibit this faint blink, as well as the Yahoo logo, but not any of the other logos which I could notice.

I'm 99% sure this is due to bug 486918 (high quality downscaling), which, AFAIK/IIRC does downscaling in 2 phases:

1. Initial downscaling in relatively low quality (biliner or similar) to get an image to screen as quickly as possible.

2. Once it got a downscaled image to screen, does a higher quality downscale (probably bicubic or similar) in a background thread, and then swaps the image on screen with this new version. It might actually start rendering the higher quality version earlier than after phase 1, but still in a background thread.

The #2 step above is most probably this "faint blink" I noticed with the Yahoo logo.

For some reason, I think the Yahoo logo is susceptible to this process, while the other logos are not. I'm guessing that it's either due to the image size (relative to its size on screen which is one of the factors by which the high quality scaling kicks in), or some other property of the image. Maybe bits per pixel, or alpha channel, or image format (gif/jpg/etc) etc.

Other than being ugly to look at, I bet it also affects tab animation performance on slow systems to some degree.

And, to verify this theory, I went and flipped the high quality downscaling pref off - image.high_quality_downscaling.enabled , and indeed both the preview thumbnails and the Yahoo logo stopped this "blink" thingy (and the thumbnails remained at "low quality mode").

I think it might be a good time to reconsider when and how we're activating this system, as I believe on some cases it could be preferable to just skip the low quality version altogether. Needs some research though.

Ignore comment 6 then. Not because it doesn't happen - it clearly does, but because this is out of the scope of this bug. I'll try to find a better context to handle this issue.
(In reply to Drew Willcoxon :adw from comment #7)
> ... and for the message passing an
> additional second or three should be more than enough.

Ermm.. second or three? Is there a tighter estimation of this extra duration? any chance to be able to lower this value or make it a bit more deterministic?
(Assignee)

Comment 11

4 years ago
Oh wow, thanks for tracking that down.  The Yahoo logo is the only logo so far that comes in only one size, high-DPI.  It's shrunk down on low-DPI windows.  So actually there is one thing unique about it, contrary to what I said before.

Right now logos are stored in both low- and high-DPI sizes, except for Yahoo, and we choose the one appropriate to the window.  But I'm considering using -moz-resolution instead to scale up low-DPI logos in high-DPI windows.  I wonder if that would fix this.

(In reply to Avi Halachmi (:avih) from comment #10)
> Ermm.. second or three? Is there a tighter estimation of this extra
> duration? any chance to be able to lower this value or make it a bit more
> deterministic?

I just pulled those numbers out of the air to make sure you waited enough time.  It's not used anywhere or meaningful.
(In reply to Drew Willcoxon :adw from comment #11)
> Right now logos are stored in both low- and high-DPI sizes, except for
> Yahoo, and we choose the one appropriate to the window.  But I'm considering
> using -moz-resolution instead to scale up low-DPI logos in high-DPI windows.
> I wonder if that would fix this.

I think you shouldn't. because it's both not your fault (and can be fixed or reconsidered elsewhere - at the downscaling system) and because upscaling will never look as nice as downscaled images, especially on hi dpi screems. Trust me, people care about image quality ;)


> I just pulled those numbers out of the air to make sure you waited enough
> time.  It's not used anywhere or meaningful.

So is there or is there not an extra duration for the "message passing" which could go over, say, 100ms?
(Assignee)

Comment 13

4 years ago
(In reply to Avi Halachmi (:avih) from comment #12)
> I think you shouldn't.

Yeah, it looks bad.  I misunderstood how -moz-resolution works.

> So is there or is there not an extra duration for the "message passing"
> which could go over, say, 100ms?

There's an extra, nondeterministic duration.  There are a couple of things that make it nondeterministic.  First, there's the async work done inside ContentSearch to service requests, things like initializing the search service and converting image data URIs into array buffers.  Second, there's the async nature of IPC message passing.  Although now that I think about it, about:newtab is in the main process, so it's possible that these messages are actually passed synchronously.  I don't know enough about how Gecko message passing works to say whether that's the case.
(Assignee)

Comment 14

4 years ago
(In reply to Drew Willcoxon :adw from comment #13)
> Although now that I think about it, about:newtab is in the main
> process, so it's possible that these messages are actually
> passed synchronously.  I don't know enough about how Gecko
> message passing works to say whether that's the case.

To follow up, that's not the case.  Thankfully sendAsyncMessage really is async even if both message managers are in the same process.

Updated

4 years ago
Depends on: 1027791
No longer depends on: 1027791
(In reply to Drew Willcoxon :adw from comment #13)
> (In reply to Avi Halachmi (:avih) from comment #12)
> > I think you shouldn't.
> 
> Yeah, it looks bad.  I misunderstood how -moz-resolution works.

What you could do, however (again, outside the scope of this bug) is downscaling the images in advance in high quality (maybe to a canvas?), and then, at the newtab page, use those pre-rendered images, such that the 2-phases downscale system never kicks in to begin with.
(In reply to Avi Halachmi (:avih) from comment #15)
> What you could do, however (again, outside the scope of this bug) is
> downscaling the images in advance in high quality (maybe to a canvas?) ...

Even simpler, create a low-res version of the Yahoo logo and store its resource as well, and have 2 versions of the logo, like the other logos have. Problem solved.
(Assignee)

Updated

4 years ago
Depends on: 1029735

Updated

a year ago
See Also: → bug 1301837
You need to log in before you can comment on or make changes to this bug.