1126511 - update Yahoo search plugins to use per-access point tags for better reporting/analysis

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Mike Connor [:mconnor]]

Attachment: YahooSAPDesktop

Comment 1

[Mike Connor [:mconnor]]

Attachment: YahooSAPMobile

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8555493 [details] [diff] [review]
YahooSAPMobile

>diff --git a/mobile/locales/en-US/searchplugins/yahoo.xml b/mobile/locales/en-US/searchplugins/yahoo.xml

>+<!-- phone search -->
>+<Url type="text/html" method="GET" template="https://search.yahoo.com/yhs/search" rel="searchform">

Do we need the rel="searchform" here?

>+https://search.yahoo.com/yhs/search?hspart=mozilla&hsimp=yhs-001&p=flowers

Remove?

>+<!-- Tablet Search activity -->
>+<Url type="application/x-moz-tabletsearch" method="GET" 
>+     rel="mobile" template="https://search.yahoo.com/yhs/search">

I don't know if the search activity supports phone vs tablet yet. I defer to Margaret for the answer on that.

Comment 3

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

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Do we need the rel="searchform" here?

Yeah, needed to replace the <SearchForm> being removed.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8555493 [details] [diff] [review]
YahooSAPMobile

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

::: mobile/locales/en-US/searchplugins/yahoo.xml
@@ +30,3 @@
>  </Url>
> +<!-- phone Search activity -->
> +<Url type="text/html" method="GET" rel="mobile" template="https://search.yahoo.com/yhs/search">

I filed bug 1126700 about this rel="mobile" attribute being totally unintuitive.

@@ +36,5 @@
> +  <Param name="p"  value="{searchTerms}" />
> +</Url>
> +<!-- Tablet Search activity -->
> +<Url type="application/x-moz-tabletsearch" method="GET" 
> +     rel="mobile" template="https://search.yahoo.com/yhs/search">

I filed bug 1126698 about supporting this. No harm in including it now, but it just won't do anything.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e141bcd14cf6

https://treeherder.mozilla.org/logviewer.html#?job_id=6175683&repo=mozilla-inbound

Comment 6

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/658ba2f4d879
https://hg.mozilla.org/projects/cypress/rev/da8594f6e177

Comment 7

[Mike Connor [:mconnor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/277ebbb08a47
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3187087797

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/277ebbb08a47
https://hg.mozilla.org/mozilla-central/rev/fc3187087797

Comment 9

[Francesco Lodolo [:flod]]

The tool I use to track p12n error in localizations started nagging about Yahoo on /mobile
https://l10n.mozilla-community.org/~flod/p12n/errors/?product=mobile

I think it's right
https://hg.mozilla.org/mozilla-central/raw-file/277ebbb08a47/mobile/locales/en-US/searchplugins/yahoo.xml

XML Parsing Error: non well-formed
Location: https://hg.mozilla.org/mozilla-central/raw-file/277ebbb08a47/mobile/locales/en-US/searchplugins/yahoo.xml
Line number 23, column 57:https://search.yahoo.com/yhs/search?hspart=mozilla&hsimp=yhs-001&p=flowers
--------------------------------------------------------^

Can someone check if the searchplugin is actually working? I'd be surprised.

Comment 10

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

Review comment from comment 2 was not addressed.

We really need bug 997485!

I fixed this on central:
https://hg.mozilla.org/mozilla-central/rev/5a1359495c04

Comment 11

[Mike Connor [:mconnor]]

It was initially, and then after the backout I re-imported the bugzilla patch, not the actual commit. +1 on testing.

(I owe Gavin a beer.)

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 8555492 [details] [diff] [review]
YahooSAPDesktop

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: has comprehensive test coverage on correct URL formation in all cases
[Risks and why]: basically zero
[String/UUID change made/needed]: none

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 8555493 [details] [diff] [review]
YahooSAPMobile

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:  minimal (assuming correct version is checked in)
[String/UUID change made/needed]:

This is needed as part of making Yahoo default on Android in Fx36

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8555493 [details] [diff] [review]
YahooSAPMobile

>+https://search.yahoo.com/yhs/search?hspart=mozilla&hsimp=yhs-001&p=flowers

This looks like a line that was pasted to pull out the required parameters. Is this URL supposed to be included in the patch?

Comment 15

[Mike Connor [:mconnor]]

No, I'll land or attach the corrected version.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8555492 [details] [diff] [review]
YahooSAPDesktop

Safe patch to improve our search metrics. Beta+ Aurora+

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8555493 [details] [diff] [review]
YahooSAPMobile

Safe patch for mobile search metrics. Beta+ Aurora+

Note to sheriffs - mconnor has an updated patch that needs to be landed instead of this one as per comment 15.

Comment 18

[Mike Connor [:mconnor]]

Attachment: patch for aurora/beta uplift.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43cbd2b48394
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec7123fe5e5a

I went ahead and fixed up the commit message on the desktop patch while I was at it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e32981aaf6d
https://hg.mozilla.org/releases/mozilla-beta/rev/519db3f2d282