Update Pocket code to latest version (May 13th code drop)

RESOLVED FIXED in Firefox 38.0.5

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nate, Assigned: nate)

Tracking

unspecified
Firefox 41
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

4 years ago
Posted patch WedsDrop.patch (obsolete) — Splinter Review
Small code drop with minor tweaks to FxA links and a fix for navigator issue referenced in bug 1164407
Attachment #8605547 - Flags: review?(michael)
Attachment #8605547 - Flags: review?(jaws)
Attachment #8605547 - Flags: review?(dolske)
Assignee

Comment 1

4 years ago
Posted patch WedsDropv2.patch (obsolete) — Splinter Review
Attachment #8605547 - Attachment is obsolete: true
Attachment #8605547 - Flags: review?(michael)
Attachment #8605547 - Flags: review?(jaws)
Attachment #8605547 - Flags: review?(dolske)
Attachment #8605565 - Flags: review?(dolske)
Assignee

Comment 2

4 years ago
Posted patch WedsDropv3.patch (obsolete) — Splinter Review
- Fixed typo
- Fixed patch (second one didn't include both files)
Attachment #8605566 - Flags: review?(dolske)
Assignee

Comment 3

4 years ago
- Fixed typo
- Fixed patch (second one didn't include both files)
- (And this time) marked the other files as obsolete

Sorry for the comment spam
Attachment #8605565 - Attachment is obsolete: true
Attachment #8605566 - Attachment is obsolete: true
Attachment #8605565 - Flags: review?(dolske)
Attachment #8605566 - Flags: review?(dolske)
Attachment #8605568 - Flags: review?(dolske)
Comment on attachment 8605568 [details] [diff] [review]
WedsDropv3.patch

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

::: browser/components/pocket/pktApi.js
@@ +211,5 @@
>          var url = baseAPIUrl + options.path;
>          var data = options.data || {};
> +        data.locale_lang = Cc["@mozilla.org/chrome/chrome-registry;1"].
> +             getService(Ci.nsIXULChromeRegistry).
> +             getSelectedLocale("browser");

Followup from bug 1164407: you'll need to deal with the "ja-JP-mac" locale issue from that bug.

I think you can just add the same hack as in that bug.

  if (locale == "ja-JP-mac")
    locale = "ja";

Or I suppose you could instead add a "Translations.ja-JP-mac = Translations.ja" at the end of dictionary.js.
Attachment #8605568 - Flags: review?(dolske) → review-
Assignee

Comment 5

4 years ago
Dolske: Per our conversation in IRC and my additional testing:

- The dictionary loading only takes the prefix (ex 'ja' or 'ru') so even if locale_lang is 'ja' or 'ja-JP' or 'ja-JP-mac', it should work. Relevant bits: http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/panels/js/saved.js#462

- Separately, I just tested hard coding those various strings as outputs of the UILocale method and everything worked as expected for all languages. 

- As we talked about, when we eventually ship more specific languages like en-US vs en-GB, we'll need to update the regex in the following link but I've filed a internal ticket and we'll tackle when the time comes: http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/panels/js/saved.js#640

Given that, I think we should be good.
Flags: needinfo?(dolske)
Comment on attachment 8605568 [details] [diff] [review]
WedsDropv3.patch

Concur!
Flags: needinfo?(dolske)
Attachment #8605568 - Flags: review- → review+
Comment on attachment 8605568 [details] [diff] [review]
WedsDropv3.patch

[Triage Comment]

a+ for aurora/beta/release: required for Pocket launch in 38.0.5.
Attachment #8605568 - Flags: approval-mozilla-release+
Attachment #8605568 - Flags: approval-mozilla-beta+
Attachment #8605568 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/db6a6012c872
Assignee: nobody → nate
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.