:Gijs (he/him) Reporter
	Description • 8 years ago

      No description provided.

	Dão Gottwald [:dao] Assignee
	Comment 1 • 7 years ago

Bug 1322731 solves this for the "without migration" case, right?

	:Gijs (he/him) Reporter
	Comment 2 • 7 years ago

(In reply to Dão Gottwald [:dao] from comment #1)
> Bug 1322731 solves this for the "without migration" case, right?

Yes. Note that it might (and I haven't tested this) even affect the "with" migration case where visits are to the same domains as the ones we preload.

It still feels like we need a generic mechanism to create 'styled' tiles, though. I'm not entirely sure how to go about doing this. I think we can basically hyphenate the domain name, and take the starting letters of each syllable, to do the 'right' thing for most domains ("fb" for facebook, "yt" for youtube, "nyt" for new york times, etc.) -- except I am not aware of us having a (chrome-)JS-accessible API to use the hyphenation algorithm/dictionaries. All I know if is the -moz-hyphens CSS property, and I can't think of a way to use that to get the right display (ie first letters horizontally next to each other).

(The random background color is somewhat easier to do, of course...)

	Verdi [:verdi] Best to slack me
	Comment 3 • 7 years ago

Are we still going try using https://github.com/mozilla/tippy-top-sites before going with an abbreviation and color?

	Dão Gottwald [:dao] Assignee
	Comment 4 • 7 years ago

I discussed this with dolske last week and there was some confusion about what problem we're trying to solve here, and consequentially what relative importance this should have in the context of the funnelcake. AFAIK the situation is that we /initially/ show blank tiles for top sites without thumbnails (such as imported history) but use BackgroundPageThumbs.jsm to get screenshots, and we do update tiles live as we get those screenshots. Naturally this takes a few seconds depending on the user's connection. It is not the case that tiles would normally stay blank until the user manually loads pages or something. Am I missing something?

If I'm right, I think we have a rather weak case for a hacky short-term solution (e.g. first letter of domain on random background color). I think it might even add confusion as we'd progressively replace those placeholder tiles with real ones. This could hurt the metrics we'd want to test / see improved with bug 1322738 and bug 1322731 as part of this funnelcake.

	:Gijs (he/him) Reporter
	Comment 5 • 7 years ago

(clearing ni for me pending answers for comment #4)

	Verdi [:verdi] Best to slack me
	Comment 6 • 7 years ago

(In reply to Dão Gottwald [:dao] from comment #4)
> I discussed this with dolske last week and there was some confusion about
> what problem we're trying to solve here, and consequentially what relative
> importance this should have in the context of the funnelcake. AFAIK the
> situation is that we /initially/ show blank tiles for top sites without
> thumbnails (such as imported history) but use BackgroundPageThumbs.jsm to
> get screenshots, and we do update tiles live as we get those screenshots.
> Naturally this takes a few seconds depending on the user's connection. It is
> not the case that tiles would normally stay blank until the user manually
> loads pages or something. Am I missing something?

In practice, these tiles are blank for a significant amount of time. In testing, some users don't see screenshots at all in their first session. This feature, we think, helps us make Firefox instantly usable. 


> 
> If I'm right, I think we have a rather weak case for a hacky short-term
> solution (e.g. first letter of domain on random background color). I think
> it might even add confusion as we'd progressively replace those placeholder
> tiles with real ones. This could hurt the metrics we'd want to test / see
> improved with bug 1322738 and bug 1322731 as part of this funnelcake.

The UX team doesn't think a "filled-in" tile being replaced with a screenshot will be confusing. Further we think this will be a better experience than beginning with blank tiles which have confused people in testing.

	Dão Gottwald [:dao] Assignee
	Comment 7 • 7 years ago

(In reply to Verdi [:verdi] from comment #6)
> In practice, these tiles are blank for a significant amount of time.

This statement is too broad and/or imprecise. In practice, it depends on the connection like I said. The tiles load about as quick as these sites would load in a tab. With my below-average cable connection in a dense city area it really doesn't take long. In my grandparents' village it admittedly would take forever (but then again their connection is so bad that clicking on the tiles is going to be a frustrating experience too.)

> In testing, some users don't see screenshots at all in their first session.

Do you have data on this? Do we know how common this problem is?

	Dão Gottwald [:dao] Assignee
	Comment 8 • 7 years ago

I'm working on this given the tight schedule, but I think we're still not quite on the same page about what exactly the problem is, how big it is, and how big the impact of the solution is supposed to be.

	Justin Dolske [:Dolske]
	Comment 10 • 7 years ago

(quickly, since I'm about to crash)

Ok, from some quick testing we do indeed dynamically update about:newtab tiles (i.e., without reloading the page). I thought Gijs had looked at this as said it required a page reload to show up -- maybe I misunderstood.

But even on my modern hardware and connection, it takes a number of seconds after startup before that process seems to start, and takes about a second per tile to populate. I think it's entirely reasonable to expect that this process will take much longer for people with slow computers / connections. Hence the desire to show "something" while that process is completing. This seems analogous to the shift in thinking around file I/O performance some time ago -- it's not "prove it's slow", it's "prove it's fast."

I don't think we need strong evidence for testing a simple patch like this in a funnelcake. But since we're about to run it, if someone wants to whip up a patch to add a telemetry probe to measure this we could ship it to get some hard data... Although Gijs had problems actually looking at the data from previous funnelcake telemetry, so it's probably not that simple.

	:Gijs (he/him) Reporter
	Comment 11 • 7 years ago

Comment on attachment 8824285 [details] [diff] [review]
patch

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

This looks OK from the code, but it doesn't seem to work for me. I don't see it working at all - IME refreshThumbnail is only called by the time we already have a bgURL.

::: browser/base/content/newtab/sites.js
@@ +249,5 @@
> +    let bgColor = link.bgColor || "";
> +    let textContent = "";
> +    if (!bgURL &&
> +        link.type == "history" &&
> +        Services.prefs.getBoolPref("browser.newtabpage.thumbnailPlaceholder")) {

From reading the code, I can't quickly convince myself that we don't care about any other types of links (I think there's "organic" as well as the "enhanced" and "sponsored" ones, and it's not clear to me what that means), or that history entries are guaranteed to have a baseDomain, which this code seems to assume. Because any JS errors here presumably break about:newtab completely, I would suggest:

if (!bgURL && Services.prefs.getBoolPref(...)) {
  // cribbed from https://dxr.mozilla.org/mozilla-central/source/browser/modules/DirectoryLinksProvider.jsm#985
  let baseDomain = link.baseDomain || NewTabUtils.extractSite(link.url || "");
  if (baseDomain) {
    textContent = baseDomain.substr(0, 1).toUpperCase();
  }
}

which I think should be relatively safe. We could also keep the link.type check - not sure if there's a reason not apply this to other tiles with no bgURL.

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

(In reply to :Gijs from comment #11)
> This looks OK from the code, but it doesn't seem to work for me. I don't see
> it working at all - IME refreshThumbnail is only called by the time we
> already have a bgURL.

Hmmm. refreshThumbnail is called from _render which is called in the Site constructor. It doesn't at all depend on the image being available from what I can tell.

> ::: browser/base/content/newtab/sites.js
> @@ +249,5 @@
> > +    let bgColor = link.bgColor || "";
> > +    let textContent = "";
> > +    if (!bgURL &&
> > +        link.type == "history" &&
> > +        Services.prefs.getBoolPref("browser.newtabpage.thumbnailPlaceholder")) {
> 
> From reading the code, I can't quickly convince myself that we don't care
> about any other types of links (I think there's "organic" as well as the
> "enhanced" and "sponsored" ones, and it's not clear to me what that means),
> or that history entries are guaranteed to have a baseDomain, which this code
> seems to assume. Because any JS errors here presumably break about:newtab
> completely, I would suggest:

History type -> baseDomain is already assumed in _render, and I don't think it should be possible for baseDomain to be empty in that case (it would make the tile's label empty too).

I think we should keep the history type check. This is basically what this bug is about, imported history. If we want to extend this to something else then we need better understanding of what exactly it is that we want.

	Dão Gottwald [:dao] Assignee
	Comment 13 • 7 years ago

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to :Gijs from comment #11)
> > This looks OK from the code, but it doesn't seem to work for me. I don't see
> > it working at all - IME refreshThumbnail is only called by the time we
> > already have a bgURL.
> 
> Hmmm. refreshThumbnail is called from _render which is called in the Site
> constructor. It doesn't at all depend on the image being available from what
> I can tell.

So the problem is that we always have a moz-page-thumb:// URL that we try to open regardless of whether that thumbnail actually exists.

	:Gijs (he/him) Reporter
	Comment 14 • 7 years ago

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to :Gijs from comment #11)
> > > This looks OK from the code, but it doesn't seem to work for me. I don't see
> > > it working at all - IME refreshThumbnail is only called by the time we
> > > already have a bgURL.
> > 
> > Hmmm. refreshThumbnail is called from _render which is called in the Site
> > constructor. It doesn't at all depend on the image being available from what
> > I can tell.
> 
> So the problem is that we always have a moz-page-thumb:// URL that we try to
> open regardless of whether that thumbnail actually exists.

It looks like PageThumbsStorage.fileExistsForURL(url); returns a promise that resolves to a bool indicating whether we have a thumbnail or not, based on https://dxr.mozilla.org/mozilla-release/source/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#97 . Though the comment preceding it saying it "may be incorrect" is ominous...

	Dão Gottwald [:dao] Assignee
	Comment 16 • 7 years ago

I made the placeholder color a function of the base domain rather than random. This should help users recognize tiles in case the thumbnail is missing for a longer time.

	Dão Gottwald [:dao] Assignee
	Comment 17 • 7 years ago

Oops, that was still the old patch...

	:Gijs (he/him) Reporter
	Comment 18 • 7 years ago

Comment on attachment 8824397 [details] [diff] [review]
patch v3

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

Nice, thanks!

	Pulsebot
	Comment 19 • 7 years ago

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca23d313302
Implement ability to show basic placeholders for missing about:newtab thumbnails. r=gijs

	Dão Gottwald [:dao] Assignee
	Comment 20 • 7 years ago

Comment on attachment 8824397 [details] [diff] [review]
patch v3

Approval Request Comment
[Feature/Bug causing the regression]: see bug 1322718 comment 1

[User impact if declined]: about:newtab shows blank tiles on first run after importing history from another browser. It's unclear how many users are severely affected by this.

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: not yet

[Needs manual test from QE? If yes, steps to reproduce]: will be part of the funnelcake QA process

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: hardly

[Why is the change risky/not risky?]: it's pref'd off by default so it will only affect the funnelcake build. When activated, this code is also pretty straightforward with little error potential.

[String changes made/needed]: /

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9ca23d313302

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 22 • 7 years ago

Comment on attachment 8824397 [details] [diff] [review]
patch v3

Another first run change for the onboarding funnelcake. OK to uplift to beta and aurora. Should be in the beta 13 build today.

	Dão Gottwald [:dao] Assignee
	Comment 23 • 7 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fb0bcde1a7f566356097330bf5b565b33bc00fc

	Dão Gottwald [:dao] Assignee
	Comment 24 • 7 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/adb88a603efb0efe74e23cd87066317bea2c82f7

	Andrei Vaida [:avaida]
	Comment 25 • 7 years ago

Flagging this for verification (and making sure Grover is in the loop), bug description in Comment 20.

	Fahima Zulfath
	Comment 26 • 7 years ago

[testday-20170203]
The bug is verified. Not a bug hence fixed.
OPERATING SYSTEM: windows 10.0
BROWSER:Firefox beta 51.0

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 27 • 7 years ago

(In reply to fahimazulfath.a from comment #26)
> [testday-20170203]
> The bug is verified. Not a bug hence fixed.
> OPERATING SYSTEM: windows 10.0
> BROWSER:Firefox beta 51.0

This bug was set for verification by the Mozilla QA team in comment 25 (that's what the "qe-verify+" flag means). 

Please do not change the status flags until Mozilla QA has had a look.

	u554753
	Comment 28 • 7 years ago

Sorry about that. This was verified quite a while ago. Never changed the status.

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 29 • 7 years ago

(In reply to Grover Wimberly IV [:Grover-QA] from comment #28)
> Sorry about that. This was verified quite a while ago. Never changed the
> status.

Grover, could you also update the status flags so we know on which versions this was verified?

	Dão Gottwald [:dao] Assignee
	Comment 30 • 7 years ago

I'm not sure fahimazulfath.a understood what this bug was about and how to properly verify it. It needs migrated history and it needs a hidden prefs set to actually enable this feature. However, I think we can consider this verified since it was tested as part of the funnelcake QA sign-off.