Closed
Bug 1155579
Opened 9 years ago
Closed 9 years ago
Snippets - Allow multiple countries per snippet
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: giorgos, Assigned: giorgos, Mentored)
References
Details
(Whiteboard: [lang=js][good next bug])
Attachments
(1 file, 1 obsolete file)
1.10 KB,
patch
|
Details | Diff | Splinter Review |
At the moment Snippets can be optionally targeted to a country. In this case snippets service includes a 'target_geo' key like this: { 'id': 1, 'text': 'foo bar', 'icon': '..', 'url': 'https://...', 'weight': 100, 'target_geo': 'GR' } and Fennec geolocates the user and takes care of the rest. We want to change Fennec to allow a single snippet to target multiple countries. Snippet service will return a list like this: { 'id': 1, 'text': 'foo bar', 'icon': '..', 'url': 'https://...', 'weight': 100, 'target_geo': 'GR' 'countries': ['GR', 'US'] } To avoid changing the URI of the endpoint I suggest that we add the key 'countries' along with key 'target_geo'. We will remove the later 6 months after deployment.
Comment 1•9 years ago
|
||
To add support for this, we should update the message filtering logic in `updateBanner` in Snippets.js: http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#197
Mentor: margaret.leibovic
Whiteboard: [lang=js][good next bug]
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8596657 -
Flags: review?(margaret.leibovic)
Comment 3•9 years ago
|
||
Comment on attachment 8596657 [details] [diff] [review] 1155579-fennec.patch Review of attachment 8596657 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Were you able to build and test this? ::: mobile/android/components/Snippets.js @@ +196,5 @@ > // Don't add this message to the banner if it's not supposed to be shown in this country. > + if ("countries" in message && message.countries.indexOf(gCountryCode) === -1) { > + return; > + } > + else if ("target_geo" in message && message.target_geo != gCountryCode) { If you want, you could combine these two if statements with an || to have a single return statement.
Attachment #8596657 -
Flags: review?(margaret.leibovic) → review+
Updated•9 years ago
|
Assignee: nobody → giorgos
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #3) > Thanks for the patch! Were you able to build and test this? Thanks for approving! I managed to build fennec but I'm having trouble testing this: - launch fennec - go to about:config - change snippet url to my local server - reset last lastUpdateTime and lastUpdateTime gets populated again but there are no hits to my local server. Am I missing something? Also the about:config page auto refreshes every second which makes it really annoying to edit or even view the values. Is there any way to disable that?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(margaret.leibovic)
Comment 5•9 years ago
|
||
(In reply to Giorgos Logiotatidis [:giorgos] from comment #4) Sorry for the slow response! > (In reply to :Margaret Leibovic from comment #3) > > Thanks for the patch! Were you able to build and test this? > > Thanks for approving! I managed to build fennec but I'm having trouble > testing this: > - launch fennec > - go to about:config > - change snippet url to my local server > - reset last lastUpdateTime > > and lastUpdateTime gets populated again but there are no hits to my local > server. Am I missing something? We use an update timer to only ever trigger the snippets code once per 24 hours: http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest#108 http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#429 You should try setting the "browser.snippets.updateInterval" pref to some small value. > Also the about:config page auto refreshes every second which makes it really > annoying to edit or even view the values. Is there any way to disable that? Yeah, I thinks bug 1154646 should have fixed that. However, if you're making your own build, you can also edit /mobile/android/app/mobile.js to directly set prefs at build time. It also sucks that we have no unit tests for this... I should just try to make something simple for bug 971107.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 6•9 years ago
|
||
I still get no hits to my local server. Digging deeper I found out that browser.snippets.countryCode is never set either. I tried with the latest Firefox on f-droid same results. Steps to reproduce: - Download firefox from f-droid (also tried with my build and with firefox beta) - Open the app - Go to about:config - Wait a moment, 'app.update.lastUpdateTime.snippets-update-timer' pref appears initial value 0 - After a while 'app.update.lastUpdateTime.snippets-update-timer' gets a another value, e.g. 1430834774 - browser.snippets.countryCode and browser.snippets.geoLastUpdate do not exist and my local snippets server shows nothing in the logs. - I changed browser.snippets.updateInterval to 60 and restarted Firefox but nothing changed Margaret can you please confirm this behavior?
Flags: needinfo?(margaret.leibovic)
Comment 7•9 years ago
|
||
(In reply to Giorgos Logiotatidis [:giorgos] from comment #6) > I still get no hits to my local server. Digging deeper I found out that > browser.snippets.countryCode is never set either. I tried with the latest > Firefox on f-droid same results. > > Steps to reproduce: > - Download firefox from f-droid (also tried with my build and with firefox > beta) > - Open the app > - Go to about:config > - Wait a moment, 'app.update.lastUpdateTime.snippets-update-timer' pref > appears initial value 0 > - After a while 'app.update.lastUpdateTime.snippets-update-timer' gets a > another value, e.g. 1430834774 > - browser.snippets.countryCode and browser.snippets.geoLastUpdate do not > exist and my local snippets server shows nothing in the logs. > - I changed browser.snippets.updateInterval to 60 and restarted Firefox but > nothing changed > > Margaret can you please confirm this behavior? I'm not seeing this testing on a release build of Firefox. When I first opened about:config, my country code was set to CA (I've traveled to Canada recently), but after setting browser.snippets.updateInterval to 60 I saw it change to the US. Are you seeing anything in adb logcat? You can also use WebIDE to attach a debugger to Snippets.js to step through what's going on: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE Do we have data about snippets usage? I assume that we would see a drop in metrics if there is some bug in the way snippets are updated/displayed.
Flags: needinfo?(margaret.leibovic) → needinfo?(giorgos)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8596657 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
I managed to debug the problem using the awesome remote debugging feature of WebIDE. Thanks for the tip! :) The problem was that I was running an adblocker :| With the adblocker turned off, everything works as expected. I updated the patch to remove 'target_geo' entirely since meanwhile I updated the snippets service to send both 'countries' and 'target_geo'. So older Fennec releases will lookup for 'target_geo' and the new ones for 'countries'.
Flags: needinfo?(giorgos)
Comment 10•9 years ago
|
||
(In reply to Giorgos Logiotatidis [:giorgos] from comment #9) > I managed to debug the problem using the awesome remote debugging feature of > WebIDE. Thanks for the tip! :) The problem was that I was running an > adblocker :| I'm happy you figured it out, but that's sad that the snippets server was blocked! > With the adblocker turned off, everything works as expected. I updated the > patch to remove 'target_geo' entirely since meanwhile I updated the snippets > service to send both 'countries' and 'target_geo'. So older Fennec releases > will lookup for 'target_geo' and the new ones for 'countries'. Sounds good. https://hg.mozilla.org/integration/fx-team/rev/6b09f09f41e3
Assignee | ||
Comment 11•9 years ago
|
||
\o/ Thanks Margaret. Is this going to ride the trains? When is it expected to hit Release?
https://hg.mozilla.org/mozilla-central/rev/6b09f09f41e3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 13•9 years ago
|
||
(In reply to Giorgos Logiotatidis [:giorgos] from comment #11) > \o/ Thanks Margaret. Is this going to ride the trains? When is it expected > to hit Release? This will be in Firefox 40, which ships to release August 11. This is also a very simple patch, so if you need it sooner, we could probably uplift it.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•