Closed Bug 671307 Opened 13 years ago Closed 13 years ago

Add Twitter to the default search plugins (en-US only)

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 9
Tracking Status
firefox7 + wontfix
firefox8 --- fixed

People

(Reporter: kev, Assigned: kev)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 5 obsolete files)

Add the Twitter search plugin to the default list of providers in the search bar. The plugin will be placed in alphabetical order (with the unordered defaults) of the Search Bar drop down.

The expected search bar query is: http://twitter.com/search/{searchterms}?partner=firefox

Note that this should not be a global addition. l10n teams can add as they see fit to their localized builds.
Attached file Twitter Search Plugin (obsolete) —
Twitter search plugin, per Twitter spec of http://twitter.com/search/[QUERY]?partner=firefox

Note that search terms cannot be defined as a param, and the URL is by design.
Kev, this is really late in the game. Is there a reason we have to take this into 6?
No, there's not, but it is something I'd like to discuss more. Traditionally we've done this kind of change with short deltas, and it is something I'd like to get some policy around.
Pushing to 7.

Stas, could you let l10n teams know this is an option that is up for consideration, pending acceptance in en-US?
Attachment #545780 - Flags: review?(asa)
Kev, when I view the data:url for the twitter icon in the browser, it looks clipped on the bottom. Is that intentional?
Milos is the productization guy now :)  CC'ing him.

My understanding is that we can start the discussions among the localizers right now, however any landings and sign-offs would ideally happen after this lands in aurora.  This way the localized builds will benefit from the coordinated messaging around this when the change reaches the release channel.
(In reply to comment #6)
> Kev, when I view the data:url for the twitter icon in the browser, it looks
> clipped on the bottom. Is that intentional?

It's not, and I am thinking Twitter would want their new favicon instead. I'll re-file, this was probably my bad. Thanks for the catch.

(In reply to comment #7)
> My understanding is that we can start the discussions among the localizers
> right now, however any landings and sign-offs would ideally happen after
> this lands in aurora.  This way the localized builds will benefit from the
> coordinated messaging around this when the change reaches the release
> channel.

Perfect. Thanks much. I'll get the updated plugin in the next few minutes, and when it lands we can open the conversation up.
We have l10n freeze on aurora, so by default, this through the regular updates and thus make it in Firefox 8.

Regarding the messaging, how does this add to https://wiki.mozilla.org/L10n:Firefox/Productization#Search ? Also, in which context are we localizing this choice, twitter yes/no, or are there criteria that we can use to pick alternative providers?
Axel, I don't believe that Aurora should be locked down 100% for l10n impact changes like this. This is not the kind of change where if l10n teams don't pick it up and translate they're left with an english string or a broken feature. I intend to bring this up for Aurora 7 approval and I don't think it should be blocked on l10n grounds.

As far as this particular change is concerned, I think the discussion with l10n is "do you want twitter or not" and not "what people search do you want in the browser."  That discussion seems larger and I think it should happen but I don't think we want it tied to the current release cycle or blocking this change from landing.
Attachment #545678 - Attachment is obsolete: true
Attachment #545780 - Attachment is obsolete: true
Attachment #545963 - Flags: review?
Attachment #545780 - Flags: review?(asa)
Attachment #545963 - Flags: review? → review?(asa)
Comment on attachment 545963 [details] [diff] [review]
Add twitter to en-US default, with updated favicon (patch)

LGTM, both the patch and the birdie image.
Attachment #545963 - Flags: review?(asa) → review+
(In reply to comment #10)
> Axel, I don't believe that Aurora should be locked down 100% for l10n impact
> changes like this. This is not the kind of change where if l10n teams don't
> pick it up and translate they're left with an english string or a broken
> feature. I intend to bring this up for Aurora 7 approval and I don't think
> it should be blocked on l10n grounds.

I think this is partially a question of the messaging of new fx7 features. If the twitter search is just there, that's one thing, if it's the "oh, the key new thing", that's another story.

Also note that we're short on staff on the l10n-drivers side for two weeks of the remaining aurora period. stas, who's the main knowledge base here, is out. Milos is stepping up, but this is gonna be his first iteration of a change of this kind.

> As far as this particular change is concerned, I think the discussion with
> l10n is "do you want twitter or not" and not "what people search do you want
> in the browser."  That discussion seems larger and I think it should happen
> but I don't think we want it tied to the current release cycle or blocking
> this change from landing.

I don't think we can have the two independently. Adding twitter in 7 and then replacing it with something different in 8 sounds like a bad UX.
(In reply to comment #13)
 
> I don't think we can have the two independently. Adding twitter in 7 and
> then replacing it with something different in 8 sounds like a bad UX.

If a locale intends to do something else, then I'd recommend they don't add Twitter in 7.
Comment on attachment 545963 [details] [diff] [review]
Add twitter to en-US default, with updated favicon (patch)

Kev, looks like twitter changed their search URL since I looked at this (I think I saw something in the last week or so about a new and improved twitter search). We'll need a new patch. Can you make that happen and request review from Gavin when you've got it up. Hopefully we can get him to land it as well.
Attachment #545963 - Attachment is obsolete: true
Whee. Sent a request to Twitter asking for new syntax and to clarify what their policy is on providing a stable interface (yay irony).
Kev, we've got a week left of m-c before 8 is a wrap. If this is gonna make it, you might want to apply some pressure asap.
Attached patch Revised twitter search plugin (obsolete) — Splinter Review
Updated twitter search plugin using https and corrected URL and params, per Twitter. Ready for review.
Attachment #556885 - Flags: review?
Attachment #556885 - Flags: review? → review?(gavin.sharp)
Comment on attachment 556885 [details] [diff] [review]
Revised twitter search plugin

The icon appears to be 32x32, but it looks a bit odd at that size (seems to look ok when scaled back down to 16x16). Maybe something to follow up with them?
Attachment #556885 - Flags: review?(gavin.sharp) → review+
Attached patch Updated patch w/16x16 icon (obsolete) — Splinter Review
Per comments offline, icon in previous attachment looked to be 32x32. Revised plugin code w/16x16 twitter icon.
Attachment #556909 - Flags: review?(gavin.sharp)
Attachment #556885 - Attachment is obsolete: true
Assignee: nobody → kev
Summary: Add Twitter to the default search plugins → Add Twitter to the default search plugins (en-US only)
Comment on attachment 556909 [details] [diff] [review]
Updated patch w/16x16 icon

Looks fine syntactically, but I noticed a few issues in testing:
- searching for "http://gavinsharp.com" results in a 404
- searching for "gavin sharp" searches for "gavin+sharp"

Their redirect-to-hashbang-URLs magic seems to work fine for basic strings, but doesn't handle arbitrary input very well.

That second bug may technically be our fault - we always URL-escape parameters, even if they're placed in the non-query portion of the URL (quite an unusual setup).
Attachment #556909 - Flags: review?(gavin.sharp) → review+
backed out from inbound investigating a permaorange leak in Windows reftests. While it may appear strange both changesets in the range (this included) looked unrelated, but the backout fixed the leak, so please get a try run of this to ensure Windows XP Debug Reftests is green before pushing.
We really need a try run for a search plugin? I am... confused.
I am as well, but the other patch was looking like os x only, and since they were pushed at less than 3 minutes distance the results were coalesced, so it's hard to distinguish the culprit here. Btw with trychooser syntax should be easy to get a single result. otherwise you may merge central to fx-team, push to fx-team and then merge when it shows green on that specific test.
(In reply to Marco Bonardo [:mak] (Away 7-17 Sept.) from comment #25)
> I am as well, but the other patch was looking like os x only, and since they
> were pushed at less than 3 minutes distance the results were coalesced, so
> it's hard to distinguish the culprit here.

You could have backed them out separately. :/
Try run for e0df0462a0c9 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=e0df0462a0c9
Results (out of 47 total builds):
    success: 38
    warnings: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kneedham@mozilla.com-e0df0462a0c9
I submitted to try builds today, and both ended up with the same result. I have no clue what the results mean, as they're webgl conformance tests that are passing but expected to fail, and an image comparison test failure. both these errors seem like something that crops up intermittently. since the failures are on the reftests, is it because the search plugin changes the default content in the search dropdown (which is expected)?

Last tryserver build can be seen at https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=309da3296d74

Need some assistance debugging this, as I've never run against this kind of thing in the past, and we've never modified defaults prior to webgl (and it's associated tests) being enabled.
And, because sometimes I am not so wise with the ways of the try, I don't get a good push. After talking with Joe briefly I updated my repo and pushed a new try build. The result is at:
  
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=331d6e639c91

There's now one failure, which is an image comparison. I'm hoping this is caused by the changes the plugin causes, but I am not 100% sure, and confess I am having trouble deciphering the test output. Who would be the best person to discuss this with?
(In reply to Kev [:kev] Needham from comment #29)
reftest-no-accel doesn't seem to run on mozilla-central, so I think you can ignore it.
Updated patch used for tryserver build - https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=331d6e639c91

Per comment #30, if the reftest-no-accel isn't required, all tests are green. Can we review and land asap? 

Thanks, all.
Attachment #556909 - Attachment is obsolete: true
Attachment #558447 - Flags: review?(gavin.sharp)
Attachment #558447 - Flags: feedback?(mak77)
(In reply to Dão Gottwald [:dao] from comment #26)
> You could have backed them out separately. :/

There was too much orange that day, doing it separately would have added more than one hour of closure to the tree, with poor benefit. Btw, both patches relanded with apparent no problems (judging on the above try results), I suspect something in the meanwhile fixed the leak in reftests, since for sure one of these two patches was activating it.
Attachment #558447 - Flags: feedback?(mak77)
Whiteboard: [inbound]
Attachment #558447 - Flags: review?(gavin.sharp)
When were they re-landed? I notice they're not in today's nightly, so just wondering when it'll show up. Thankees.
http://hg.mozilla.org/mozilla-central/rev/458686d886a5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110908 Firefox/9.0a1
Status: RESOLVED → VERIFIED
We're tracking this for Firefox 7, sounds like we don't need it there...is that correct?
(In reply to Vlad [QA] from comment #35)
> Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1;
> rv:9.0a1) Gecko/20110908 Firefox/9.0a1

Vlad, on which locales has this bug been verified? Have you tested at least one other locale to ensure that Twitter doesn't appear in the list of available search engines?
I have verified this only on US-EN locale but Twitter appears in the list of available search engines.
Comment on attachment 558447 [details] [diff] [review]
Add twitter to en-US default (updated, green try)

We'd very much like to get this into Aurora (8). This is feature-ish but the risk is super low and the reward is high. In the past, we'd squeeze service changes into security updates with even less testing. I think we should probably be OK taking these kinds of things in Aurora or even early Beta going forward.
Attachment #558447 - Flags: approval-mozilla-aurora?
Adding this late is bad for feature parity on localized builds. Also, I don't remember us ever taking a change to the list of search plugins as part of a minor update.
Axel, I don't think we need feature parity here. We have different engines all over the place. Localizers should not feel the need to rush out and find a Twitter equivalent for their locale. If they want to included Twitter, that's cool, but we don't need to rush the evaluation of alternatives. If they don't want Twitter, they don't have to take it.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 9 → ---
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Status: RESOLVED → VERIFIED
(In reply to Christian Legnitto [:LegNeato] from comment #36)
> We're tracking this for Firefox 7, sounds like we don't need it there...is
> that correct?

Kev, I want to confirm there aren't contractual obligations to get this into Firefox 7
(In reply to Christian Legnitto [:LegNeato] from comment #42)
> (In reply to Christian Legnitto [:LegNeato] from comment #36)
> > We're tracking this for Firefox 7, sounds like we don't need it there...is
> > that correct?
> 
> Kev, I want to confirm there aren't contractual obligations to get this into
> Firefox 7

There are no contractual obligations to ship this in 7. There's a strong desire (on our end) to ship it ASAP, though.
Asa's correct, no contractual obligations. We definitely want to get it in as soon as possible, but it's not a 7 blocker (and they've corrected the multi-term search issue Gavin pointed out).
Comment on attachment 558447 [details] [diff] [review]
Add twitter to en-US default (updated, green try)

a=jst per todays meeting.
Attachment #558447 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa?]
Is there any messaging we have to do to localizers to let them know they don't need to take this / localize this?
http://groups.google.com/group/mozilla.dev.l10n/browse_frm/thread/1726b69b6e346a6d# has messages from both me and stas, not the most planned policy. Still hoping for Asa to chime in on that thread or open a new one, too.
Someone, please, back out this patch.
Now every time I updated my Nightly (which happens every day) - Twitter gets added to my search engines. I manually delete it, but after an update - it gets added to the list of my search engines again and again.
Mozilla, please, don't go the evil way.
Sean: that behavior isn't an intended result of this patch. You should file a bug, or contact me directly, and we can try to sort out why that's happening to you.
Verified on:
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111101 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111101 Firefox/10.0a1

The Twitter search plugin was added to the default list and it works fine. The issue mentioned in comment #49 doesn't reproduce on any builds.
Whiteboard: [qa?] → [qa!]
You need to log in before you can comment on or make changes to this bug.