983723 - Yahoo search tags are not working properly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

Apparently Yahoo isn't seeing any search-related data from us, strongly implying that we haven't implemented the search codes correctly.

Mconnor said this may involve bug 923795 and asked I file this and assign to him.

Comment 1

[Mike Connor [:mconnor]]

Small update:

* Fx27 from the Play Store does not include the correct code.
* Fx28 Beta from the Play Store does include the correct partner code.
* Fx28 build1 from the FTP has the correct codes.

So there's no firedrill here, since Fx28 is working.  Tagging as regressionwindow-wanted because we should know what broke this, and what fixed it.  And we should implement some sort of automated test coverage to ensure it doesn't break again.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mike Connor [:mconnor] from comment #1)
> And we should implement some sort of automated test coverage to
> ensure it doesn't break again.

bug 969535 is filed for that, but ideally we use junit 3, which is not yet on tbpl. In the meanwhile, we can do a Robotium test and not touch the UI, porting it later, like testNativeCrypto.

Comment 3

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

(In reply to Michael Comella (:mcomella) from comment #2)
> (In reply to Mike Connor [:mconnor] from comment #1)
> > And we should implement some sort of automated test coverage to
> > ensure it doesn't break again.
> 
> bug 969535 is filed for that, but ideally we use junit 3, which is not yet
> on tbpl. In the meanwhile, we can do a Robotium test and not touch the UI,
> porting it later, like testNativeCrypto.

We can probably skip any Java test and just do a simple javascript/xpcom test. We already have a simple test for testing search engine stuff. We can copy parts of it.

Comment 4

[Mike Connor [:mconnor]]

Okay, this is actually not reliable, and I don't have clear STR for what works and what doesn't.  Assigning to Finkle to route accordingly.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

For the builds installed on my phone release 28 and beta 29 do not work. Aurora 30 does work. Though after wiping data on beta 29 I now get the partner code.

To be clear the expected str 
* type a search term in the address bar 
* select yahoo from the search list
* check the url for &fr=mozilla_mobile_search
** requires long tap copy url -> paste to view this

Comment 6

[Mike Connor [:mconnor]]

If clearing data works, I'm inclined to blame the topN feature relying on something that isn't quite deterministic.

As an added note: the topN feature seems to assume that if a user customizes the engine list that should affect partner codes.  That is not how it works in desktop, to the best of my knowledge.  I'll follow up with Joanne.

Comment 7

[Mike Connor [:mconnor]]

So, the entire premise of topN support is wrong.  It's about ship order, not customized order, so we can just rip all of that out and do what desktop does.  I'd propose that we should back out bug 923795 and make the implementation match Firefox Desktop, but with the correct mobile code.  Finkle, if that makes sense to you I can do both pieces, just assign it back to me.

Comment 8

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

Let's discuss this a bit more so others following along can get the benefit:
1. We added topN because of a partner releationship that kicks in when the partner search engine is default or second.
2. The implementation of topN assumes that the "default or second" rule is affected by the user moving the search engine as a user choice. If a user customizes the order of the search engines, topN will activate or deactive search codes.
3. The partner relationship is written such that only the default shipping order of the search engine affects the topN. The search engine must ship as default or second. Therefore the partner codes can be hard coded at build-time and are not changed if the user customizes the search engine order.

Question:
Does the "must be default" behavior for search engines also allow for users customizing the order? I assumed that if a search engine was moved out of default, the partner codes reflected the change.

Comment 9

[Mike Connor [:mconnor]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Question:
> Does the "must be default" behavior for search engines also allow for users
> customizing the order? I assumed that if a search engine was moved out of
> default, the partner codes reflected the change.

This assumption was incorrect.  The position question is "as shipped" and unaffected by user customization.

Comment 10

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

(In reply to Mike Connor [:mconnor] from comment #9)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > Question:
> > Does the "must be default" behavior for search engines also allow for users
> > customizing the order? I assumed that if a search engine was moved out of
> > default, the partner codes reflected the change.
> 
> This assumption was incorrect.  The position question is "as shipped" and
> unaffected by user customization.

Thanks for the clarification Mike. I bet that assumption was carried by others as well, and led to the incorrect implementation you mention in comment 7.

Assigning back to Mike so we can get this cleared up.

Comment 11

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

Let's see if we can get fixes on Fx29, if possible.

Comment 12

[Mike Connor [:mconnor]]

Attachment: yahooCodes

Per earlier discussion, and confirmation that this is not branch/branding-dependent, this should work (and it should be something we can uplift).

I'll file a followup on backing out topN support, since we don't need it (and it seems to be a little busted)

Comment 13

[Mike Connor [:mconnor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14b1874f1deb

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14b1874f1deb

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 8401628 [details] [diff] [review]
yahooCodes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 923795
User impact if declined: inconsistent rev credit for Yahoo
Testing completed (on m-c, etc.): baking for a while
Risk to taking this patch (and alternatives if risky): it's not working now, it won't change much
String or IDL/UUID changes made by this patch: none

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8401628 [details] [diff] [review]
yahooCodes

Approving because it is a small change.
Mike, next time, it would be nice if you could land this in before beta9... We don't like to land any non-critical bug fixes. Thanks

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eab0c808661f
https://hg.mozilla.org/releases/mozilla-beta/rev/4873bad57984

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in builds:
29.0
30.0
31.0
32.0a2 (2014-06-19)
33.0a1 (2014-06-18)
Device: Motorola Razr (Android 4.0.4) using STR from comment 5.