Closed Bug 1009299 Opened 6 years ago Closed 6 years ago

Add Yahoo logo to search plugin so about:newtab can use it

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

(Whiteboard: p=2 s=it-32c-31a-30b.2 [qa!])

Attachments

(3 files)

Attached patch patchSplinter Review
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.
Attachment #8421405 - Flags: review?(MattN+bmo)
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa?]
Flags: firefox-backlog+
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa?] → p=2 s=it-32c-31a-30b.2 [qa?]
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa?] → p=2 s=it-32c-31a-30b.2 [qa+]
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.
Attachment #8421405 - Flags: review?(MattN+bmo) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/eaea131b47d7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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.
Status: RESOLVED → VERIFIED
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa+] → p=2 s=it-32c-31a-30b.2 [qa!]
QA Contact: cornel.ionce
Attached patch Beta patchSplinter Review
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
Attachment #8441075 - Flags: approval-mozilla-beta?
Attachment #8441075 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting back [qa+] for verification on Firefox 31 Beta and Firefox 33 Nightly.
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa!] → p=2 s=it-32c-31a-30b.2 [qa+]
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
Adw, in Firefox 31 beta 4, the Yahoo logo is missing too. Any ideas? Thank you!
Flags: needinfo?(adw)
Depends on: 1029652
Attached image noYahooLogo.png
(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;);"/>
Depends on: 1030232
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.
Marking as verified on Firefox 31 beta 6 by verifying bug 1030232.

Thanks for the quick fix, Drew.
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa+] → p=2 s=it-32c-31a-30b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.