Drew Willcoxon :adw Assignee
	Description • 10 years ago

Joanne posted the logo to bug 962490 comment 99.  It's 250x61, but the newtab search implementation expects logos to be either 65x26 or 130x52, so I resized it to be 130x52.

	Matthew N. [:MattN]
	Comment 1 • 10 years ago

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

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

Sorry for the delay. Between vacation and trying to catch up on what's going on with newTab this took longer than expected.

Not really a blocker for this bug but why are we using logos of one size but making them appear as another on about:newtab (77x38)? While we're collecting logos, it seems like we should be collecting the size we want.

Is this actually an image/x-icon? Firefox is telling me it's a PNG. Please correct the mimetype if necessary.

Side note: I'm wondering why we didn't use ICOs with -moz-resolution for HiDPI on newtab like with do for the 16x16 icons. Do we think that's authors of opensearch plugins won't do that?

r=me with the mimetype fixed (assuming Firefox wasn't lying) since the other issues aren't new with this patch (although may cause churn). Hopefully you can see that this wasn't as trivial of a patch to review as one might think.

	Drew Willcoxon :adw Assignee
	Comment 2 • 10 years ago

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

(In reply to Matthew N. [:MattN] from comment #1)
> Sorry for the delay. Between vacation and trying to catch up on what's going
> on with newTab this took longer than expected.

No problem, thanks for the thoughtful comments.

> Not really a blocker for this bug but why are we using logos of one size but
> making them appear as another on about:newtab (77x38)?

The div that contains the logo image is 77x38, but the image is drawn as the div's background at 65x26.

That's just what I ended up doing to get the padding and everything right.  I started out using an <img> but had trouble getting the box model right.  I don't remember the specifics now.  I think the trouble partly involved the mouseover effect and properly centering the image.

> Is this actually an image/x-icon? Firefox is telling me it's a PNG. Please
> correct the mimetype if necessary.

Good catch, thanks.  I just copied and pasted from other search plugins and didn't notice.  I changed it to image/png.

> Side note: I'm wondering why we didn't use ICOs with -moz-resolution for
> HiDPI on newtab like with do for the 16x16 icons. Do we think that's authors
> of opensearch plugins won't do that?

No, I just didn't think of that.  I don't think I even ever heard of -moz-resolution until now.  If we can use that to avoid checking window.devicePixelRatio in the JS, that does seem a little better.  I checked the opensearch spec and we could support both ICOs as you describe and the way it works now, if we wanted.

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

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

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

Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0

Verified fixed on latest Nightly, build ID: 20140518030203.
The Yahoo logo (77x38 pixels) is now displayed on the about:newtab page if Yahoo is the default search engine.

	Drew Willcoxon :adw Assignee
	Comment 5 • 10 years ago

This is the patch that actually landed in comment 2.  It addresses the review comments on the r+'ed patch.  It applies cleanly to Beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 962490 added a search field to about:newtab and landed on 31.  This bug here adds the logo for Yahoo and landed on 32.

User impact if declined:
When Yahoo is the currently selected engine, its logo should appear to the left of the search field on about:newtab, but it won't.  about:newtab gracefully handles missing logos, though.

Testing completed (on m-c, etc.):
automated testing on m-c

Risk to taking this patch (and alternatives if risky):
very low

String or IDL/UUID changes made by this patch:
none

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

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

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

Setting back [qa+] for verification on Firefox 31 Beta and Firefox 33 Nightly.

	Petruta Horea [:phorea], Desktop QA
	Comment 8 • 10 years ago

Verified as fixed using latest Nightly 33.0a1 20140622030203 under Win 7 64-bit, Ubuntu 13.04 64-bit and Mac OSX 10.9.2.

The "Yahoo" logo is not visible on Firefox 31 beta 3 although it appears in pushlog: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=2d8266047e4e&tochange=3a7d4a2f023b

	Petruta Horea [:phorea], Desktop QA
	Comment 9 • 10 years ago

Adw, in Firefox 31 beta 4, the Yahoo logo is missing too. Any ideas? Thank you!

	Drew Willcoxon :adw Assignee
	Comment 10 • 10 years ago

I see it in this binary: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0b4/mac/en-US/Firefox%2031.0b4.dmg

I also see it in b3: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0b3/mac/en-US/Firefox%2031.0b3.dmg

	Petruta Horea [:phorea], Desktop QA
	Comment 11 • 10 years ago

(In reply to Drew Willcoxon :adw from comment #10)
This is a screenshot of Firefox 31 beta 4 opened using a clean profile. The logo is missing for us as well as for contributors participating at Firefox 31 beta 3 Testday.

I also noticed in browser inspector that placeholder="Yahoo" and hidden="" properties are used in beta compared with Nightly 33.0a1 where the logo is visible:

<input type="text" name="q" value="" id="newtab-search-text" maxlength="256" 
dir="auto" placeholder="Yahoo"/>
<input type="text" name="q" value="" id="newtab-search-text" maxlength="256" 
dir="auto" placeholder=""/>

<div hidden="" id="newtab-search-logo" style="background-image:url (&quot;data:image/png;base64,iV...CC&quot;);"/>
<div id="newtab-search-logo" style="background-image:url (&quot;data:image/png;base64,iV...CC&quot;);"/>

	Drew Willcoxon :adw Assignee
	Comment 12 • 10 years ago

OK, the problem is that bug 1009266 landed on 32 but not 31.  Since Yahoo only has a high-DPI logo, unlike the other search engines, on low-DPI displays its logo won't show up: https://hg.mozilla.org/releases/mozilla-beta/file/8800b869526e/browser/base/content/newtab/search.js#l148

We could ask for Beta approval for bug 1009266, but instead I'd like to land a low-DPI Yahoo logo since that will fix this bug and, as far as the Yahoo logo is concerned, bug 1027791 (which is also discussed in bug 1019990 comment 16).  I filed bug 1030232.

Thank you Petruta for raising the problem and explaining it further when I myself didn't see it.

	Petruta Horea [:phorea], Desktop QA
	Comment 13 • 10 years ago

Marking as verified on Firefox 31 beta 6 by verifying bug 1030232.

Thanks for the quick fix, Drew.