Closed Bug 990713 Opened 6 years ago Closed 5 years ago

Update directoryLinks to have actual links and images

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Mardak, Assigned: emtwo)

References

Details

(Whiteboard: p=13 s=it-32c-31a-30b.1 [qa-][talos_regression])

Attachments

(12 files, 7 obsolete files)

3.98 KB, image/png
Details
2.24 KB, image/png
Details
7.96 KB, image/png
Details
5.69 KB, image/png
Details
v1
304.61 KB, patch
adw
: review+
Details | Diff | Splinter Review
156.91 KB, image/png
Details
13.78 KB, image/png
Details
152.36 KB, image/png
Details
2.47 KB, image/png
Details
2.31 KB, image/png
Details
154.63 KB, patch
Details | Diff | Splinter Review
152.05 KB, image/png
Details
Bug 975475 will be providing the images/colors for the initial list.

Bug 975228 landed by avoiding leak failures when we actually had links to sites. The Linux/64 Debug bc1 test leaks include SpdySession31, so somehow one of the spdy-servers in the list (one/some/all of facebook.com, youtube.com, twitter.com see bug 975228 comment 38) is being loaded? favicon? prefetch?

The details of the leak from bug 975228 comment 32:

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)
Flags: firefox-backlog?
Anyone have insights into how to debug this Linux/64 Debug bc1 leak failure? We believe it's related to a SPDY site, and somehow the leaktest doesn't fail when I converted all the directoryLinks to https://www.mozilla.org/
The simple fix is to sidestep the issue by Services.prefs.setCharPref(PREF_NEWTAB_DIRECTORYSOURCE, "data:application/json,{}"); at the beginning of mochitest, so all tests that happen to open about:newtab will continue to get blank tiles.
Flags: firefox-backlog? → firefox-backlog+
Assignee: nobody → mzhilyaev
Whiteboard: p=13
Blocks: tiles-dev
Status: NEW → ASSIGNED
Whiteboard: p=13 → p=13 s=it-31c-30a-29b.2
Whiteboard: p=13 s=it-31c-30a-29b.2 → p=13 s=it-31c-30a-29b.2 [qa-]
I pushed various tryserver tests that set the url to the actual site instead of mozilla.org at various positions.

Facebook #1:   Leak https://tbpl.mozilla.org/?tree=Try&rev=af2ca7c08573
YouTube  #1:   Leak https://tbpl.mozilla.org/?tree=Try&rev=dfee822f5579
Twitter  #1: NOleak https://tbpl.mozilla.org/?tree=Try&rev=f806559e46a2
YouTube  #3: NOleak https://tbpl.mozilla.org/?tree=Try&rev=efa1bd512cd1
Twitter  #6: NOleak https://tbpl.mozilla.org/?tree=Try&rev=76e617d90463
Facebook #9: NOleak https://tbpl.mozilla.org/?tree=Try&rev=1a1bd60dfbfd

All 3 of Facebook, YouTube, and Twitter have X-Firefox-Spdy: "3.1" in the response headers, so not sure why Twitter isn't causing leaks when in position #1. I would guess that the mouse pointer is over position 1 and causes some loading?

Another note is that the leak is intermittent.
(In reply to Ed Lee :Mardak from comment #3)
> Twitter  #1: NOleak https://tbpl.mozilla.org/?tree=Try&rev=f806559e46a2
> not sure why Twitter isn't causing leaks when in position #1
Turns out Twitter in spot 1 is leaking. Just at a much lower rate:

Twitter 1 leak failure out of 14
Facebook 3 leaks in 4
YouTube 3 leaks in 4

So at least it's consistent-ish.
Depends on: 991853
Putting http://www.facebook.com/ (not https) in spot #1 does not cause the leak. https://tbpl.mozilla.org/?tree=Try&rev=544389210a65
Could y'all annotate the strings on the placeholder tiles on about:newtab to something like BBC(placeholder) ? 

It's very confusing to click on a BBC tile and get https://www.mozilla.org/en-US/#http://www.bbc.co.uk/ instead. It looks like broken behavior instead of 'hang on, we're working on this'
(In reply to :Ally Naaktgeboren from comment #6)
> Could y'all annotate the strings on the placeholder tiles on about:newtab to
> something like BBC(placeholder) ? 
But but... there's a big PLACEHOLDER image above the string. ;) This would get fixed through bug 991853 where we'll put in actual links and images to Mozilla properties for now.
(In reply to Ed Lee :Mardak from comment #7)
> (In reply to :Ally Naaktgeboren from comment #6)
> > Could y'all annotate the strings on the placeholder tiles on about:newtab to
> > something like BBC(placeholder) ? 
> But but... there's a big PLACEHOLDER image above the string. ;) This would
> get fixed through bug 991853 where we'll put in actual links and images to
> Mozilla properties for now.

Right, so it's clear that image temporary, not that the link is borked. :)
This is from the speculative connections being made[1] from tiles on the new tab page. See bug 964358 and bug 964369 comment 3 (and the patch there) where we may disable these connections by default in tests.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/sites.js?rev=f3c44cfc3900#182
Ah ha! I should have looked deeper into the newtab code. I was grepping for prefetch or preload. :(

Here are some notes from maksik:

I am running tests 24x7 to repro the leak with minimal luck so far:
1) I did manage to compile debug+refcount version of fx on linux 64bit
2) running it is a major pain, as it often either hangs or exit with cascading errors: "JavaScript Error: "source.defaultView is null".
3) sometimes the VM instance gets horcked and i have to reboot it
4) I did manage to complete a run of mochitest-browser for browser/base and broswer/components which cover 99.9% of all the tests in BC1 bucket that triggers the leak.  AND.... I saw no leaks.

The item 4) above is alerting.  I am using our github repo master branch, not the fxteam branch. Which bags the question if our changes are implicated to begin with.

Given how painful the bug is, I am going to schedule a meeting to chat of how we could be systematic about reproducing the leak.
We may still have to look for a real linux box, as VM on my poor machine does not seem to be happy running these massive tests.
Bug 992611 turns off speculative connects for tests and this should prevent the leak failures.
Depends on: 992611
Depends on: 993581
Whiteboard: p=13 s=it-31c-30a-29b.2 [qa-] → p=13 [qa-]
Duplicate of this bug: 995001
Boriss, can you update the YouTube logo and color according to these links?

https://developers.google.com/youtube/branding_guidelines
https://www.youtube.com/yt/brand/using-logo.html
Flags: needinfo?(jboriss)
Borris, here's our ready to go list so far:

+–––––––––––––––––––––––––+
|                         |
| +–––––+ +–––––+ +–––––+ |
| |  1  | |  2  | |  3  | |
| +–––––+ +–––––+ +–––––+ |
|                         |
| +–––––+ +–––––+ +–––––+ |
| |  4  | |  5  | |  6  | |
| +–––––+ +–––––+ +–––––+ |
|                         |
| +–––––+ +–––––+ +–––––+ |
| |  7  | |  8  | |  9  | |
| +–––––+ +–––––+ +–––––+ |
|                         |
+–––––––––––––––––––––––––+            

Directory Tiles with background colors:
                                                  
1. 
2. youtube.com, #e5523f
3. twitter.com, #00b5f0
4. wikipedia.com, #ffffff
5. 
6. 
7. wired.com, #1facdf
8. 
9. 

Ready Alternatives (throw these in somewhere):
* mozilla.org, #4d4e54
* amazon.com, #000000
* trulia.com

Not (Yet) Ready Alternates:
* facebook.com, #3a5898
* reddit.com, 
* weather.com,

Not Ready Alternates:

10. lonelyplanet.com, #002f74 
11. wordpress.com, #008fc0
12. nytimes.com, #0093b9
13. eff.org, #000000
14. npr.org, #c7d2d7
(In reply to Bryan Clark (Firefox Search PM) [:clarkbw] from comment #13)
> Boriss, can you update the YouTube logo and color according to these links?
> 
> https://developers.google.com/youtube/branding_guidelines
> https://www.youtube.com/yt/brand/using-logo.html

Attached is a white logo, background is #424242
Flags: needinfo?(jboriss)
Boriss, these logo files you've provided aren't of the tile size. How should we make them fit the tile? Just take the logo file and have it centered in the tile space and fill in with background color?
Boriss, nevermind. These show up fine when centered/centered. Originally I was confused because attachment 8392628 [details] says 112x82 or smaller and these logo files are 180x86
Amazon wanted a version of the logo without the ".com" - new version attached
(In reply to Ed Lee :Mardak from comment #19)
> Boriss, nevermind. These show up fine when centered/centered. Originally I
> was confused because attachment 8392628 [details] says 112x82 or smaller and
> these logo files are 180x86

Np.  It's possible you'll need different version for retina, I imagine.  I included the original images but can make 2x size if needed.
Assignee: mzhilyaev → msamuel
Whiteboard: p=13 [qa-] → p=13 s=it-31c-30a-29b.3 [qa-]
clarkbw, is this ordering and type pairing correct?

1. amazon organic
2. youtube organic
3. wired sponsored
4. wikipedia organic
5. mozilla affiliate
6. twitter organic
7. lightbeam affiliate
8. trulia sponsored
9. ? eff affiliate ? facebook organic ?
Attached image Screenshot of WIP (obsolete) —
(In reply to Marina Samuel [:emtwo] from comment #23)
> Created attachment 8412131 [details] [diff] [review]
> WIP: Applying the assets available so far
Thanks, you can find more assets in bug 995001
Attached image Screenshot of WIP2 (obsolete) —
Attachment #8412133 - Attachment is obsolete: true
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #16)
> Created attachment 8407856 [details]
> Image: Trulia white logo without wordmark (preferred), background #9acb3c
Where did you get that color for Trulia? I see another document with #9ACA3C but their website seems to have a darker green #5EAB1F
Attached patch v1Splinter Review
There might be some image/url/title/type tweaks, but the code changes should be good.
Attachment #8412161 - Attachment is obsolete: true
Attachment #8412238 - Flags: review?(adw)
Attached image v1 screenshot
Attachment #8412162 - Attachment is obsolete: true
Is this okay Boriss? I took the lightbeam wordmark from the page and resized to 180 wide then added transparency to 86px tall.. (although technically the extra transparency above/below isn't necessary).
Attachment #8412240 - Flags: ui-review?(jboriss)
Comment on attachment 8412238 [details] [diff] [review]
v1

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

::: toolkit/content/directoryLinks.json
@@ +1,4 @@
>  {
>    "en-US": [
>      {
> +      "url": "http://www.amazon.com",

Nit: the other URLs have trailing slashes (which is more "correct" anyway).
Attachment #8412238 - Flags: review?(adw) → review+
Whiteboard: p=13 s=it-31c-30a-29b.3 [qa-] → p=13 s=it-32c-31a-30b.1 [qa-]
https://hg.mozilla.org/integration/fx-team/rev/79989d4a8d24

leave-open for potentially more image/position/title/type/url changes.
Keywords: leave-open
Removing from iteration until final decisions are made regarding potentially more image/position/title/type/url changes.
Status: ASSIGNED → NEW
Whiteboard: p=13 s=it-32c-31a-30b.1 [qa-] → p=13 [qa-]
Here's the "final" order/type/text/url

= Order =

1. Facebook
2. YouTube
3. Wired
4. Lightbeam
5. MoFo
6. Wikipedia
7. Trulia
8. Amazon
9. Webmaker

= Types =

- Organic -

Facebook
YouTube
Wikipedia
Amazon

- Affiliate -

Lightbeam
MoFo
Webmaker

- (Trial) Sponsored -

Wired
Trulia

= Text:URL = 

Facebook :	https://www.facebook.com/
YouTube  : 	http://www.youtube.com/
Wired 	 :      http://www.wired.com/
Lightbeam:	https://www.mozilla.org/en-US/lightbeam/
MoFo     :      http://www.mozilla.org/en-US/about/
Wikipedia:	http://en.wikipedia.org/wiki/Main_Page
Trulia   :      http://www.trulia.com/?ecampaign=tiles
Amazon   :      http://www.amazon.com/gp/bit/amazonbookmark.html?tag=mozilla-directory-tiles-20&partner=Mozilla
Webmaker	 :      https://webmaker.org/

Only reason for the "final" in quotes is the Wired creative and the Mozilla center tile are being reviewed right now.  Everything else should be good.
Attached patch comment 38 patch (obsolete) — Splinter Review
Attachment #8414783 - Flags: review?(adw)
Attached image comment 38 screenshot (obsolete) —
Attached patch comment 38 patch (obsolete) — Splinter Review
Attachment #8414783 - Attachment is obsolete: true
Attachment #8414783 - Flags: review?(adw)
Attachment #8414785 - Flags: review?(adw)
Attachment #8414785 - Flags: review?(adw) → review+
this was backed out:
https://hg.mozilla.org/integration/fx-team/rev/24cf989be9e3

tracked in bug 1000459.

To add more to this, we have a performance regression from this patch, 5.33% TART regression on win7 and winXP.  When we backed it out, the regression went away:
http://graphs.mozilla.org/graph.html#tests=[[293,132,25]]&sel=none&displayrange=7&datatype=running

I had done some retriggers to verify it was a real regression and not a random spike of electricity or some noise:
https://tbpl.mozilla.org/?tree=Fx-Team&startdate=2014-04-29&enddate=2014-04-31&jobname=Windows%207%2032-bit%20fx-team%20talos%20svgr
oh and this also had a 2.22% regression on linux32
Whiteboard: p=13 [qa-] → p=13 [qa-][talos_regression]
jmaher, the regression is the same as bug 990212 where the change that actually triggers the regression is the one line change to turn on the feature in bug 1000459. I believe the plan is to populate the tiles for TART to have the test run consistently (no matter what we change the list of links) and be more "realistic" where users will be seeing tiles with content as opposed to all empty.
ah, thanks!  I had blinders on and just filed things, thanks for clarifying the relationship!
Attached image LonelyPlanet logo
Proposed background: #1D4F8D

Disclaimer: I do some work for LonelyPlanet and I asked their designers to prepare a logo.
Updated various ordering, inserted bbc to spot 9 (moving webmaker to 10), Wired -> WIRED, new WIRED logo, Mozilla Foundation -> The Nonprofit Behind Firefox, updated urls, crushed images
Attachment #8414785 - Attachment is obsolete: true
Attached image 2014-05-08 screenshot
Attachment #8414784 - Attachment is obsolete: true
(In reply to Piotr Okoński from comment #46)
> Created attachment 8417963 [details]
> LonelyPlanet logo
> 
> Proposed background: #1D4F8D
> 
> Disclaimer: I do some work for LonelyPlanet and I asked their designers to
> prepare a logo.

Thanks Piotr, this is great!  I sent a message to someone who has our contact at Lonely Planet so we can keep in touch with them.
https://hg.mozilla.org/integration/fx-team/rev/4d19381c20ee
Status: NEW → ASSIGNED
Keywords: leave-open
Whiteboard: p=13 [qa-][talos_regression] → p=13 s=it-32c-31a-30b.1 [qa-][talos_regression]
https://hg.mozilla.org/mozilla-central/rev/4d19381c20ee
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Attachment #8412240 - Flags: ui-review?(jboriss)
You need to log in before you can comment on or make changes to this bug.