Closed Bug 1105011 Opened 5 years ago Closed 5 years ago

Include locale in tiles upload

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- verified
fennec 35+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

According to Olivier, we need to include the locale in the tiles ping for tiles reporting to work.
Attached patch Include locale in tiles payload (obsolete) — Splinter Review
Includes locale to match the updated README at https://github.com/oyiptong/onyx.
Attachment #8529231 - Flags: review?(rnewman)
Was getting a null locale in BLM.getLocaleManager(), so I moved the tiles check after we set the locale to en-US. Was still getting null since setting the JS pref directly doesn't change the Java-side locale, so I switched that method to just set the locale using BLM.
Attachment #8529235 - Flags: review?(rnewman)
Comment on attachment 8529231 [details] [diff] [review]
Include locale in tiles payload

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

r+ with that change.

::: mobile/android/base/home/TopSitesPanel.java
@@ +236,5 @@
>  
>                          // Record tile click events on non-private tabs.
>                          final Tab tab = Tabs.getInstance().getSelectedTab();
>                          if (!tab.isPrivate()) {
> +                            final Locale locale = BrowserLocaleManager.getInstance().getCurrentLocale(getActivity());

getCurrentLocale doesn't do what you think it does -- it returns the user-selected locale if one was selected. As you found out, that means it can return null, but that doesn't mean we shouldn't be recording the click!

You probably just want to use Locale.getDefault(). Locale switching will set that, and it's the one we'll be using when we load suggested sites or fetch tiles.
Attachment #8529231 - Flags: review?(rnewman) → review+
Comment on attachment 8529235 [details] [diff] [review]
Update testDistribution to check for tiles locale

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

This is the right way to do it. Note, though, that you ought to be testing combinations:

* OS locale = en-US, no app locale.
* OS locale = other, no app locale.
* OS locale = en-US, selected app locale differs.
* OS locale = other, selected app locale same.
* OS locale = other, selected app locale differs.

The second one is the most important; r+ when you cover that.

I think it's important to know that we're sending the correct locale when you're using a non-en-US OS locale and not overriding it in Fennec!

See testOSLocale.java (as well as testDistribution), which should give you some ideas.
Attachment #8529235 - Flags: review?(rnewman) → feedback+
Blocks: 1105423
We have only provisioned Tiles for the en-US locale.

Do you want tiles for other locales as well?
(In reply to Olivier Yiptong [:oyiptong] from comment #5)
> We have only provisioned Tiles for the en-US locale.
> 
> Do you want tiles for other locales as well?

Technically, we ship to sixty (60) locales, so it is possible to provision more tiles. That said, I think the real value in the provisioning the built-in en-US tiles is checking to make sure things are working.

With that in mind, I don't think we need to provision any other locales at this time.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Olivier Yiptong [:oyiptong] from comment #5)
> > We have only provisioned Tiles for the en-US locale.
> > 
> > Do you want tiles for other locales as well?
> 
> Technically, we ship to sixty (60) locales

More than sixty ...
Updated to use Locale.getDefault().
Attachment #8529231 - Attachment is obsolete: true
Attachment #8529309 - Flags: review+
(In reply to Richard Newman [:rnewman] from comment #4)
> This is the right way to do it. Note, though, that you ought to be testing
> combinations:
> 
> * OS locale = en-US, no app locale.
> * OS locale = other, no app locale.
> * OS locale = en-US, selected app locale differs.
> * OS locale = other, selected app locale same.
> * OS locale = other, selected app locale differs.

As mentioned in IRC, I'm still kind of confused about the difference between the OS locale and the app locale. This uses Locale.setDefault(locale) to set the OS locale, but setting the app locale does the same thing (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserLocaleManager.java#426). I was hoping to do something like this:

1) Set the OS locale to en-US.
2) Set the app locale to es-MX.
3) Verify the uploaded tiles use es-MX.
4) Verify the OS locale is en-US (i.e., differs from the uploaded locale).

But since setting the OS locale and the app locale both call Locale.setDefault(), there's no way for the test to be in a state where these locales differ (meaning we can't test those "locale differs" combinations you listed).
Attachment #8529235 - Attachment is obsolete: true
Attachment #8529323 - Flags: review?(rnewman)
Removed unused code.
Attachment #8529323 - Attachment is obsolete: true
Attachment #8529323 - Flags: review?(rnewman)
Attachment #8529324 - Flags: review?(rnewman)
(In reply to Brian Nicholson (:bnicholson) from comment #9)

I could have sworn I already replied to this, but maybe just on IRC.

> As mentioned in IRC, I'm still kind of confused about the difference between
> the OS locale and the app locale. This uses Locale.setDefault(locale) to set
> the OS locale, but setting the app locale does the same thing…

That's because we can't actually set the OS locale. The closest we can get is to call Locale.setDefault really early during our process, before we launch any Fennec activities (and thus trigger BLM).

That will effectively exercise the code paths that handle OS locale setting.

The alternative is to fake everything the locale code does -- set the OS locale cache pref, trigger the same onConfigurationChanged methods that the system sends on a locale change, and send a message to Gecko.


> 1) Set the OS locale to en-US.
> 2) Set the app locale to es-MX.
> 3) Verify the uploaded tiles use es-MX.
> 4) Verify the OS locale is en-US (i.e., differs from the uploaded locale).

#4 isn't possible. Once we override the locale default, we can't look outside the box.


> But since setting the OS locale and the app locale both call
> Locale.setDefault(), there's no way for the test to be in a state where
> these locales differ (meaning we can't test those "locale differs"
> combinations you listed).

Exactly: the "differs" states are the hypothetical "what do I do when the OS is in one locale and Fennec has an override set?" situations.

The most important scenario I called out (#2) can be partly exercised by fakery, but should be verified by hand, too.
Oh, and to address this:

> > As mentioned in IRC, I'm still kind of confused about the difference between
> > the OS locale and the app locale.

Java only knows one locale: it's the one returned by Locale.getDefault(). Each process gets its own.

Android has its own conception of the current locale. It imposes it on applications/processes implicitly at startup (your Configuration object and Locale.getDefault() return the system locale), inadvertently during unrelated configuration change callbacks (it'll reset the locale, and we have to fix it), and explicitly when the user changes the locale in Settings.

That locale is what we call the "OS locale" -- it's the one that Android will init new processes with, and occasionally push through the letterbox when you don't want it to.

Our own division between "OS locale" and "app locale" is a construction that reflects what we do in each of the circumstances above.

If there is no app locale -- that is, the user hasn't picked a different locale inside Fennec -- then we're mirroring the OS locale, and we pass through changes. If the user has picked an app locale, then we jump through hoops to undo external locale changes, and also to change the process locale during startup.

Our testing in this bug, too, ideally would reflect these: we'd somehow set the system locale, then we'd run Fennec either mirroring or overriding, and we'd verify that the right thing happens for tiles, just as it should for the rest of the application.

Without Bug 946058 and some additional work, we simply can't do that, so we have to make the best of it.
Comment on attachment 8529324 [details] [diff] [review]
Update testDistribution to check for tiles locale, v2.1

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

r+ with that change.

::: mobile/android/base/tests/testDistribution.java
@@ +133,5 @@
>          BrowserDB.setSuggestedSites(suggestedSites);
> +
> +        // Test tiles uploading for an en-US OS locale with no app locale.
> +        final Locale enUSLocale = BrowserLocaleManager.parseLocaleCode("en-US");
> +        Locale.setDefault(enUSLocale);

You can just use `Locale.US` directly.

@@ +134,5 @@
> +
> +        // Test tiles uploading for an en-US OS locale with no app locale.
> +        final Locale enUSLocale = BrowserLocaleManager.parseLocaleCode("en-US");
> +        Locale.setDefault(enUSLocale);
> +        checkTilesReporting("en-US");

You're trying to ensure that Fennec thinks the OS locale is en-US here. That means doing two things:

* Trying to change the locale itself: setDefault. You're doing that. :thumbsup:
* Tricking BLM.

To do the latter, call
                BrowserLocaleManager.storeAndNotifyOSLocale(GeckoSharedPrefs.forProfile(context), osLocale);

just as we do in GeckoApp.

You should do this both after line 137 and after line 142. That way the call to setTestLocale on 147 will be a little more meaningful.

@@ +138,5 @@
> +        checkTilesReporting("en-US");
> +
> +        // Test tiles uploading for an es-MS OS locale with no app locale.
> +        final Locale esMXLocale = BrowserLocaleManager.parseLocaleCode("es-MX");
> +        Locale.setDefault(esMXLocale);

Locale.setDefault(new Locale("es", "MX"));
Attachment #8529324 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/012715b4c18b
https://hg.mozilla.org/mozilla-central/rev/e419afb0dba7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8529309 [details] [diff] [review]
Include locale in tiles payload, v2

Approval Request Comment
[Feature/regressing bug #]: Addition to tiles reporting, bug 1105423
[User impact if declined]: Tiles metrics won't work
[Describe test coverage new/current, TBPL]: Includes test case, green on TBPL
[Risks and why]: Very low risk; just adds a field to the upload data
[String/UUID change made/needed]: None
Attachment #8529309 - Flags: approval-mozilla-beta?
Attachment #8529309 - Flags: approval-mozilla-aurora?
Attachment #8529324 - Flags: approval-mozilla-beta?
Attachment #8529324 - Flags: approval-mozilla-aurora?
After sending the tile pings with this patch applied, Olivier verified that the JSON contained the expected locale codes. With this patch, tiles tracking should be ready.
Verifying per comment 17.
Status: RESOLVED → VERIFIED
Attachment #8529309 - Flags: approval-mozilla-beta?
Attachment #8529309 - Flags: approval-mozilla-beta+
Attachment #8529309 - Flags: approval-mozilla-aurora?
Attachment #8529309 - Flags: approval-mozilla-aurora+
Attachment #8529324 - Flags: approval-mozilla-beta?
Attachment #8529324 - Flags: approval-mozilla-beta+
Attachment #8529324 - Flags: approval-mozilla-aurora?
Attachment #8529324 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 35+
You need to log in before you can comment on or make changes to this bug.