975228 - Create logic to merge frecency-pages and Tiles (Tile equiv 1000 frecency)

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

In the Tile Grid, some tiles will be Top Sites (based on user frecency), and others will be Sponsored or Partner tiles.  This bug should merge both in order to appear together in the grid.  

Note: tile-fetching service created in bug 975210, backend logic to provide list in bug 975211

Comment 1

[Ed Lee :Mardak]

Some behavior needs to be decided in bug 975213 that can affect the merging logic (e.g., if there's backfill of Top Sites or backfill of Sponsored Sites or History backfill)

Comment 2

[Ed Lee :Mardak]

As John99 noted in bug 975210 comment 3, the blog post notes that 30 days of normal browsing would cause the Tiles to get pushed out. https://blog.mozilla.org/advancingcontent/ (direct links to the post aren't working). This bug is currently titled as treating Directory tiles as 1000 frecency which gets replaced by typing in a site just once or visiting a same page through links/clicks 10 times.

That same post also mentions "rotating content" which would affect how this merge logic is implemented. Although this might be something that isn't part of the initial implementation.

clarkbw, what should the behavior be for regular merging of Directory and History tiles (independent of the user removing some Director tiles - bug 975213).

Comment 3

[Ed Lee :Mardak]

As mentioned in bug 965437 comment 3, we might want bug 911307 to help merge different frecencies.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Ed Lee :Mardak from comment #2)
> As John99 noted in bug 975210 comment 3, the blog post notes that 30 days of
> normal browsing would cause the Tiles to get pushed out.
> https://blog.mozilla.org/advancingcontent/ (direct links to the post aren't
> working). This bug is currently titled as treating Directory tiles as 1000
> frecency which gets replaced by typing in a site just once or visiting a
> same page through links/clicks 10 times.

Yes, that's the current replacement algorithm I understand we want to go for.  I'm not sure exactly where the 30 day number came from but it doesn't matter, we're going with an increased threshold (the one you mention) such that Directory Tiles aren't replaced with sites that don't really matter but will be replaced with sites that do matter.

> That same post also mentions "rotating content" which would affect how this
> merge logic is implemented. Although this might be something that isn't part
> of the initial implementation.

This is being looked at but not implemented yet.  The idea is that if we showed a person a YouTube tile and they didn't use it after (lets assume) 10 views we might replace that with a Vimeo tile instead.  There isn't a good system worked out for this so we're going to punt on it for now; other bugs will track that.

> clarkbw, what should the behavior be for regular merging of Directory and
> History tiles (independent of the user removing some Director tiles - bug
> 975213).

Lets stick with the 1000 frecency; typing in a site once or visiting the same page through links/clicks 10 times.  If it's too much or too little we can adjust later.

Comment 5

[Ed Lee :Mardak]

adw, I took an initial stab of implementing some dummy/debug logic for combining places and directory tiles (take 3 places and rest of directory):

https://github.com/Mardak/tiles-dev/commit/02eb1964df7fb4f50af0d33d40cbf178d42ab863

But in the process of getting things to work, I needed to set the directory link.rank.frecency = 1000 because PlacesProvider.compareLinks would have thrown an error without the rank.

This got me thinking perhaps one way to implement this merge functionality is to basically do DirectoryTiles.links.forEach(Links.onLinkChanged) where basically it's a bunch of fake link visits of appropriate frecency.

Comment 6

[Drew Willcoxon :adw]

(In reply to Ed Lee :Mardak from comment #5)
> This got me thinking perhaps one way to implement this merge functionality
> is to basically do DirectoryTiles.links.forEach(Links.onLinkChanged) where
> basically it's a bunch of fake link visits of appropriate frecency.

That's too much of a hack for me. :-)

There are a few reasonable ways I can think of:

1. Have DirectoryTilesProvider compose PlacesProvider.  So we'd have:

> PlacesProvider --provides links to--> DirectoryTilesProvider --provides links to--> Links

Links's provider would be DirectoryTilesProvider.  DirectoryTilesProvider.getLinks would call PlacesProvider.getLinks, merge the two lists together, and return the merged list.  Bug 911307 adds dynamic updates, so after that lands, DirectoryTilesProvider would also need to add itself as an observer of PlacesProvider so it could update its merged list with the new Places links.

2. Make DirectoryTilesProvider and PlacesProvider completely separate, have Links keep a set of providers instead of only one, and have Links merge the lists of its providers.

3. Have DirectoryTilesProvider work more like PinnedLinks and BlockedLinks than PlacesProvider.  Links.getLinks would merge directory tile links into the final list just like it now merges pinned links and drops blocked links.  Since getLinks is sync and fetching directory tiles is async, Links.populateCache would do the fetch and cache the links, and getLinks would use the cache.

#1 is how I envisioned this working when we first talked about it.  One problem is that DirectoryTilesProvider is basically forced to cache its own links.  (Either that or it has to fetch the list from disk each time.)  Right now, PlacesProvider can rely on Links caching its links, so its implementation is very simple, and it would be ideal if DirectoryTilesProvider could, too.

#2 needs the links of different providers to be comparable so Links can merge them.  To me that's not ideal, because I'd like to support a wide variety of providers, in theory, without each one knowing a lot about the others.  Providers would have to agree on either a meaningless sorting criterion, like a unit-less number, or criteria with a shared meaning, like a frecency value, even if frecencies aren't actually relevant to some providers.

#3 doesn't fit as well with the existing framework as #1 and #2.

I think I prefer #2.  Having to cache links in two places is a big problem for #1.  For #2's sorting criteria, it's not as simple as a frecency value, though.  Two links can easily have the same frecency, so we need some secondary criteria.  Within PlacesProvider, the secondary criteria is the links' IDs in the Places database.  So links are sorted first by frecency, then by Place ID.  How should frecency ties be broken now that directory tiles are included in the grid?

So does #2 sound OK?  DirectoryTilesProvider would just implement getLinks to fetch the links from disk, no caching.  Links._populateProviderCache would call getLinks on the given provider and populate its cache for that provider.  Links.populateCache, which is a public method, would call Links._populateProviderCache for each of its providers.  (Over in my bug 911307, providers would notify Links when it should call _populateProviderCache for them, which is why it's a separate method from populateCache.)  Then populateCache would merge all its caches according to the frecency-based sort criteria yet to be determined, and cache that result.  Links.getLinks would returned that merged, cached result.

Whew.

Comment 7

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #6)
> 2. Make DirectoryTilesProvider and PlacesProvider completely separate, have
> Links keep a set of providers instead of only one, and have Links merge the
> lists of its providers.
Sounds good -- that's what we were planning on. I put together a dummy/wip patch that inserted rank.frecency for DirectoryTilesProvider's links so that PlacesProvider.compareLinks would work, so this is getting towards your suggestion of comparable Links.

https://github.com/Mardak/tiles-dev/commit/02eb1964df7fb4f50af0d33d40cbf178d42ab863

For the real merge logic, Links would need to (binary? ;)) search through the sorted Places frecency links to find the appropriate spot to merge/insert these 1000-frecency equivalent links.

Comment 8

[Drew Willcoxon :adw]

(In reply to Ed Lee :Mardak from comment #7)
> Sounds good -- that's what we were planning on.

I'll stress that that means DirectoryTilesProvider doesn't need to keep its own in-memory cache of the directory tiles data like it does in the patch in bug 975211, since Links can do that.

> For the real merge logic, Links would need to (binary? ;)) search through
> the sorted Places frecency links to find the appropriate spot to
> merge/insert these 1000-frecency equivalent links.

Bug 911307 adds BinarySearch.jsm to toolkit for just that purpose.

Comment 9

[Ed Lee :Mardak]

Attachment: wip v1

Comment 10

[Olivier Yiptong [:oyiptong]]

Attachment: wip v2

Comment 11

[Olivier Yiptong [:oyiptong]]

browser/base/content/test/newtab/ chrome tests are failing. But the patches from bug 911307 failed them too.

The chrome test failures are inconsistent. In those tests, I disabled the directory tiles provider for now, because those assume directory tiles don't exist.

Comment 12

[u279076]

Ioana, can you please make sure this gets a QA Contact and tested for this iteration?

Comment 13

[Olivier Yiptong [:oyiptong]]

Anthony, Ioana, you'd probably want to test this in bug 974745 or in bug 975210, because this functionality is not surfaced to the user with this patch.

Comment 14

[Olivier Yiptong [:oyiptong]]

Attachment: v3

Adds changes from bug 975211

Comment 15

[Olivier Yiptong [:oyiptong]]

(In reply to Olivier Yiptong [:oyiptong] from comment #13)
> Anthony, Ioana, you'd probably want to test this in bug 974745 or in bug
> 975210, because this functionality is not surfaced to the user with this
> patch.

Actually. I retract what I said. Sorry for the confusion.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8393916 [details] [diff] [review]
v3

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

This patch should substantially change given my comment in bug 975211 about moving the provider to browser/, so clearing the review request for now.

Comment 17

[Olivier Yiptong [:oyiptong]]

(In reply to Drew Willcoxon :adw from comment #36)
> @@ +828,5 @@
> > +      // reached by a history tile, the last tile is pushed out
> > +      aCallback(rawLinks.map((link, position) => {
> > +        link.frecency = DIRECTORY_FRECENCY;
> > +        link.lastVisitDate = rawLinks.length - position;
> > +        return link;
>
> Is link.type defined in the JSON?

Yes, the type is defined in the JSON, and can be different, depending on the link.

> @@ +77,5 @@
> > +
> > +  links = yield fetchData(provider);
> > +
> > +  // results should be unchanged
> > +  do_check_true(links.length == 1);
>
> I don't see the point of this.  You're setting the links variable to the
> array returned from fetchData, so how would it contain the new URL you
> pushed to the previous value of links?

It is a vestige of the cached implementation, and should be removed.

> @@ +116,5 @@
> > +});
> > +
> > +add_task(function test_DirectoryProvider__prefObserver_url() {
> > +  let provider = NewTabUtils._providers.directory;
> > +  Services.prefs.setCharPref(provider._prefs['tilesURL'], kDefaultTileSource);
>
> This isn't necessary since each test here calls provider.reset() at the end,
> which makes sure that the tilesURL is the default URL.

It seems that in xpcshell tests, browser/app/profile/firefox.js is not loaded, therefore the default url is never loaded. provider.reset() just deletes tilesURL.

I side stepped the issue anyway, by choosing a less confusing name for the test in the upcoming patch.

Comment 18

[Olivier Yiptong [:oyiptong]]

wrong bug =/

Comment 19

[Olivier Yiptong [:oyiptong]]

Attachment: v4

Comment 20

[Olivier Yiptong [:oyiptong]]

Attachment: v4a

Comment 21

[Ed Lee :Mardak]

browser_newtab_directory_links.js seems to be failing on some platforms:

https://tbpl.mozilla.org/?tree=Try&rev=282a3252fb75 (includes 211 and 228)
Linux Opt TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_directory_links.js | grid status = 1,,,,,,,, - Got ,,,,,,,,, expected 1,,,,,,,,

https://tbpl.mozilla.org/?tree=Try&rev=6280a49326ba (includes all tiles-dev)
Linux Opt TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_directory_links.js | grid status = 1,,,,,,,, - Got ,,,,,,,,, expected 1,,,,,,,,
Windows 7 Opt  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_directory_links.js | grid status = 1,2,,,,,,, - Got 1,,,,,,,,, expected 1,2,,,,,,, 

Not sure if related but the try push 282a32... also has these test failures:
OS X 10.8 Debug TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_update.js | grid status = 1,,,,,,,, - Got ,,,,,,,,, expected 1,,,,,,,,
Windows 8 Debug TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_update.js | grid status = 2,1,3,4,,,,, - Got 1,4,2,,,,,,, expected 2,1,3,4,,,,, 

As well as most Opt builds with test failure:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | unexpected uninterruptible reflow '_calcMouseTargetRect@chrome://browser/content/tabbrowser.xml


I wonder if there's something with the browser_newtab_directory_links.js test that isn't waiting correctly?

Comment 22

[Olivier Yiptong [:oyiptong]]

That may be the case. I'm taking a look

Comment 23

[Olivier Yiptong [:oyiptong]]

Attachment: v5

Comment 24

[Olivier Yiptong [:oyiptong]]

Fixed the browser test. Ed has made a try-server build and everything seems to be fine

Comment 25

[Drew Willcoxon :adw]

Comment on attachment 8395879 [details] [diff] [review]
v5

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

The test looks wrong in a couple of places, but I don't think this actually needs a test, so please remove it.  The test in bug 911307 should ensure that sort criteria on the directory provider's links are set correctly, and the test in bug 911307 covers merging providers.  There's nothing new beyond that to test.

::: browser/base/content/test/newtab/head.js
@@ +4,5 @@
>  const PREF_NEWTAB_ENABLED = "browser.newtabpage.enabled";
>  
>  Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, true);
> +// start with no directory links by default
> +Services.prefs.setCharPref("browser.newtabpage.directorySource", "data:application/json,{}");

Please clearUserPref this pref in the registerCleanupFunction callback several lines below this, just to leave the pref as we found it.

::: browser/components/nsBrowserGlue.js
@@ +472,5 @@
>      this._syncSearchEngines();
>  
>      WebappManager.init();
>      PageThumbs.init();
> +    DirectoryLinksProvider.init();

Nit: please move this line down so it's right above the addProvider line.

Comment 26

[Ed Lee :Mardak]

Attachment: for checkin

Comment 27

[Ed Lee :Mardak]

From clarkbw:  Currently it seems like if I keep clicking on Facebook (after ~10 clicks) a Facebook User History Tile appears and pushes the other tiles. Though the Facebook Directory Tile remains around.

The current merge logic doesn't do any de-duping between history or directory links. Perhaps we should file a followup bug maybe even start with something simple as only de-duping exact url matches?

adw, in fact there might be some odd behavior if the url is not unique after merging multiple providers?

Comment 28

[Drew Willcoxon :adw]

Yeah, duplicate URLs across different providers will ruin the merge logic.  I didn't realize the directory links might have the same URLs as history links -- but I guess that seems obvious if directory tiles can be clicked and visited.  I'll have to post yet another new patch to bug 911307.

Comment 29

[Ed Lee :Mardak]

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

Comment 30

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b - whether it was this or bug 972930 or bug 972936, something in that push caused https://tbpl.mozilla.org/php/getParsedLog.php?id=36940467&tree=Fx-Team

Comment 31

[Ed Lee :Mardak]

Most likely it was this bug but the other bugs would need to be backed out without this landing.

The test failure here:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | unexpected uninterruptible reflow '_calcMouseTargetRect@chrome://browser/content/tabbrowser.xml:5016:1|set_label@chrome://browser/content/tabbrowser.xml:4966:13|XULBrowserWindow.updateStatusField@chrome://browser/content/browser.js:12924:7|LinkTargetDisplay._showDelayed/this._timer<@chrome://browser/content/browser.js:13322:7|'

There's bug 953313 but that's slightly different:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabopen_reflows.js | unexpected uninterruptible reflow 'ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm:3339

Does anyone know why this would fail on Opt only and not Debug? I'm guessing it's because the newtab is showing some directory tiles instead of empty tiles. So one option is to have no directory tiles show for the test. Or update the test to expect extra _calcMouseTargetRect ??

Comment 32

[Ed Lee :Mardak]

Attachment: leakcheck fail

Fixes browser_tabopen_reflows.js | unexpected uninterruptible reflow '_calcMouseTargetRect by setting the data pref to empty object json.

But now try tests have a Linux/Linux64 Debug bc1 leakcheck failure that didn't exist when the patch was originally pushed https://tbpl.mozilla.org/?rev=a695139e96ee&tree=Fx-Team

https://tbpl.mozilla.org/?tree=Try&rev=5da828c50df4
TEST-UNEXPECTED-FAIL | leakcheck | 4779 bytes leaked (EventTokenBucket, Mutex, ReentrantMonitor, Service, SpdySession3, ...)
leaked 2 EventTokenBucket (216 bytes)
leaked 10 Mutex (120 bytes)
leaked 3 ReentrantMonitor (48 bytes)
leaked 1 Service (64 bytes)
leaked 1 SpdySession31 (588 bytes)
leaked 3 StringAdopt (3 bytes)
leaked 1 ThirdPartyUtil (16 bytes)
leaked 1 TransportSecurityInfo (116 bytes)
leaked 1 nsCategoryObserver (60 bytes)
leaked 1 nsCookiePermission (36 bytes)
leaked 1 nsCookieService (84 bytes)
leaked 1 nsDBusService (24 bytes)
leaked 1 nsDNSRecord (28 bytes)
leaked 1 nsDNSService (88 bytes)
leaked 5 nsDeque (260 bytes)
leaked 1 nsEffectiveTLDService (52 bytes)
leaked 1 nsHashtable (44 bytes)
leaked 1 nsHostRecord (72 bytes)
leaked 2 nsHttpAuthCache::AppDataClearObserver (32 bytes)
leaked 1 nsHttpConnection (208 bytes)
leaked 1 nsHttpConnectionInfo (44 bytes)
leaked 1 nsHttpConnectionMgr (160 bytes)
leaked 1 nsHttpConnectionMgr::nsConnectionHandle (16 bytes)
leaked 1 nsHttpHandler (504 bytes)
leaked 1 nsIDNService (72 bytes)
leaked 1 nsIOService (104 bytes)
leaked 1 nsInterfaceRequestorAgg (24 bytes)
leaked 6 nsMainThreadPtrHolder<T> (72 bytes)
leaked 1 nsNSSCertificate (56 bytes)
leaked 1 nsNSSSocketInfo (196 bytes)
leaked 1 nsNetworkManagerListener (24 bytes)
leaked 1 nsObserverService (52 bytes)
leaked 1 nsPermissionManager (100 bytes)
leaked 1 nsPrefBranch (76 bytes)
leaked 1 nsSSLStatus (52 bytes)
leaked 1 nsSiteSecurityService (60 bytes)
leaked 1 nsSocketTransport (332 bytes)
leaked 1 nsSocketTransportService (148 bytes)
leaked 1 nsStreamConverterService (72 bytes)
leaked 18 nsStringBuffer (144 bytes)
leaked 7 nsTArray_base (28 bytes)
leaked 1 nsThread (124 bytes)
leaked 1 nsTimerImpl (68 bytes)
leaked 1 nsUnicodeNormalizer (12 bytes)
leaked 5 nsWeakReference (80 bytes)

Anyone have ideas on how to debug?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

I assume it is the case that showing a non-directory-tile in tests would cause the same reflow?

It seems like it would also be dependent on the mouse cursor position, since that's the only thing that should be calling setOverLink (which ends up calling LinkTargetDisplay.update), so maybe it's luck of the draw and that opt slave had the cursor in the wrong spot? Was this failing reliably across all opt slaves?

Comment 34

[Ed Lee :Mardak]

(In reply to :Gavin Sharp (email [e-mail]) from comment #33)
> Was this failing reliably across all opt slaves?
It looks like no -- but almost. There would have been 3 rows and 3 tiles with roughly 729px width and 510px height of click targets. Any idea how big the screen resolutions are? Although if this ran after various synthesize drag/drop tests, the cursor could be placed over where tiles appear. And in that case, it's odd that not all the tests are failing -- some combination of tests disabled on certain platforms?

pass tst-linux32-spot-434
pass tst-linux32-spot-118

fail tst-linux64-spot-485
pass tst-linux64-spot-248
pass tst-linux64-spot-150

fail talos-r4-snow-132
pass talos-r4-snow-090
fail talos-r4-snow-043
fail talos-r4-snow-112

fail talos-mtnlion-r5-084
fail talos-mtnlion-r5-005
fail talos-mtnlion-r5-042
fail talos-mtnlion-r5-022

fail t-xp32-ix-028
fail t-xp32-ix-018
fail t-xp32-ix-016
fail t-xp32-ix-002

fail t-w732-ix-115
fail t-w732-ix-046
fail t-w732-ix-021
fail t-w732-ix-015

fail t-w864-ix-117
fail t-w864-ix-011
fail t-w864-ix-047
fail t-w864-ix-094

Comment 35

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/3ac8cd95308c
https://hg.mozilla.org/integration/fx-team/rev/ae535b7584fd

The second commit is to replace all links with mozilla.org#<origurl> so that each directory link is still unique but avoids leakfailure for landing.

Comment 36

[Ed Lee :Mardak]

I still don't quite understand why leaktests are failing with actual links as opposed to all links pointing to mozilla.org. At first I was thinking it was trying to fetch thumbnails for those sites, but we do have logic (in a separate patch but included in the set of commits) that skips the fetch if imageURISpec is set.

Perhaps something is fetching the favicon or prefetching a page?

Comment 37

[Ed Lee :Mardak]

oyiptong made a suggestion that perhaps for testing, we should replace the directory tiles data? Probably pointing to mochi.test. We would still need to clear the links for certain prefs as we did for this landing:
Services.prefs.setCharPref(PREF_NEWTAB_DIRECTORYSOURCE, "data:application/json,{}");

Not sure what part of the testing process we would insert ourselves, but I see testing/xpcshell/head.js turns on/off certain functionality and seems like testing/mochitest/runtests.py might have something too.

Comment 38

[Ed Lee :Mardak]

spdy yes http://spdycheck.org/#facebook.com
spdy no. http://spdycheck.org/#bbc.co.uk
spdy yes http://spdycheck.org/#youtube.com
spdy no. http://spdycheck.org/#wired.com
spdy no. http://spdycheck.org/#wikipedia.org
spdy yes http://spdycheck.org/#twitter.com
spdy no. http://spdycheck.org/#yahoo.com
spdy no. http://spdycheck.org/#amazon.com
spdy no. http://spdycheck.org/#mozilla.org

Assuming some spdy site is causing the leak, this reduces it to 3 sites of our current directoryLinks.

Comment 39

[Ed Lee :Mardak]

I started getting perf regression emails:

http://mzl.la/1hc2j7U Fx-Team-Non-PGO - Tab Animation Test - WINNT 6.1 (ix) - 4.7%
http://mzl.la/1hc1Wu3 Fx-Team-Non-PGO - Tab Animation Test - WINNT 6.2 x64 - 5.69%

I would assume from rendering 9 tiles with images instead of 9 empty tiles.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ac8cd95308c
https://hg.mozilla.org/mozilla-central/rev/ae535b7584fd

Comment 41

[Cornel Ionce [:noni] [Hubs QA]]

Tested on Windows 7 64bit, Windows 8.1 32bit, Mac OS X 10.9 and Ubuntu 12.04 32bit using latest Nightly (Build ID: 20140402030201).

While verifying this implementation I've noticed the following:

1. All the suggested Websites are currently hardcoded and lead to mozilla.org. Will these change? It's quite useless if the user isn't redirected to the correct website.

2. The Sponsored Tile Icon is shown only for the filled tiles when using a clean profile. If the user visits a Partner Website the icon is no longer shown.

3. When a user goes to a website that is already predefined using a clean profile, the website is shown twice (eg. please see attachment). Is this behaviour intended?

4. On Windows, I've noticed a border around the Sponsored Tile Tooltip (also shown in the attachment). It is not shown under Mac OS or Linux.
--

Can anyone point towards the expected behaviour for any of the above issues? It's also highly important to know if any of them could block the release of this feature.

Comment 42

[Cornel Ionce [:noni] [Hubs QA]]

Attachment: newtab_page.jpg

Comment 43

[Ed Lee :Mardak]

(In reply to Cornel Ionce [QA] from comment #41)
> 1. All the suggested Websites are currently hardcoded and lead to
> mozilla.org. Will these change?
Bug 990713

> 2. The Sponsored Tile Icon is shown only for the filled tiles when using a
> clean profile. If the user visits a Partner Website the icon is no longer
> shown.
Could you explain this? Are you saying if the user happened to have bbc.co.uk in their browsing history with a history thumbnail, the sponsored icon isn't shown?

> 3. When a user goes to a website that is already predefined using a clean
> profile, the website is shown twice (eg. please see attachment). Is this
> behaviour intended?
Can you confirm if the urls are actually the same? E.g., is it en.wikipedia.org/wiki/Main_Page in spot 1 of attachment 8401126 [details]? How did you get those history tiles to appear in front of the directory tiles? Did you type in the url? Clicked on the directory tile multiple times? Some of these will be decided in bug 988447.

> 4. On Windows, I've noticed a border around the Sponsored Tile Tooltip (also
> shown in the attachment). It is not shown under Mac OS or Linux.
Is this on both Windows 7 64bit and Windows 8.1 32bit? Filed bug 991542.

Comment 44

[Cornel Ionce [:noni] [Hubs QA]]

Thank you for the quick response.

Regarding issue 2, that's exactly what I want to say. If the user already had a partner website in his browsing history with a history thumbnail, the sponsored icon isn't shown.

For issue 3 - In spot 1 of attachment 8401126 [details] is en.wikipedia.org/wiki/Main_Page. The same behaviour for en.wikipedia.org.
I've typed the url and it appeared in the first tile. Every time I type in a url, the website is added into the first tile.

Issue 4 is reproducing on both Windows 7 64 bit and Windows 8.1 32bit.
On a Microsoft Surface Pro 2 running Windows 8.1 64bit the shadow is still there. Noticed also on the device that the tooltip is not correctly shown for the lower row, as you can see in the attachment.

Comment 45

[Cornel Ionce [:noni] [Hubs QA]]

Attachment: newtab_SurfacePro2.png

Comment 46

[Cornel Ionce [:noni] [Hubs QA]]

Marking this verified as I've added a comment in bug 975210 the remaining remark (3. If the user already had a partner website in his browsing history with a history thumbnail, the sponsored icon isn't shown).