Closed Bug 1056279 Opened 6 years ago Closed 6 years ago

Turn off enhanced tiles feature for non-en-US Firefox 33 users

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
34.3
Tracking Status
firefox33 + verified

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 3 obsolete files)

There's some new functionality that shouldn't happen for non-en-US users on aurora+.

Bug 1037091 adds customize menu with items
-> remove the menu to avoid strings and leave it as a toggle to show/hide

Bug 1053530 shows the messaging on first open
-> don't show

Bug 1042214 sends pings
-> don't send ping
Blocks: 1056293
Also turning off directory links to prevent showing any content that requires the sponsored indicator from bug 1040369.

And override fetching potentially remote links such as those from bug 1042876.
Attached patch wip for aurora (obsolete) — Splinter Review
Doesn't have code to turn off bug 1053530 yet.
There's a bunch of "// Bug 1056293 will remove this code at the appropriate time" because originally I was thinking this would land on m-c, but we could just land this directly to aurora and it'll never need bug 1056293 to remove (this code from nightly).
Summary: Add functionality to turn off enhanced tiles feature for non-en-US aurora+ users → Turn off enhanced tiles feature for non-en-US aurora+ users
Attached patch v1 (obsolete) — Splinter Review
This would land directly on aurora on top of all the enhanced tiles related uplift.
Assignee: nobody → edilee
Attachment #8476447 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8477112 - Flags: review?(adw)
Points: --- → 3
Comment on attachment 8477112 [details] [diff] [review]
v1

Prioritizing r? for m-c patches first.
Attachment #8477112 - Flags: review?(adw)
Flags: firefox-backlog+
Summary: Turn off enhanced tiles feature for non-en-US aurora+ users → Turn off enhanced tiles feature for non-en-US Firefox 33 users
Blocks: 1057602
Flags: qe-verify?
Attached patch v1 (obsolete) — Splinter Review
This is new code landing directly for Firefox 33 uplift. I'm not sure how extensive of tests I should be writing.. ?
Attachment #8477112 - Attachment is obsolete: true
Attachment #8477764 - Flags: review?(adw)
Tracking because enhanced tile is a new feature.
No longer blocks: 1030832
Comment on attachment 8477764 [details] [diff] [review]
v1

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

::: browser/base/content/newtab/customize.js
@@ +47,5 @@
>  
>    showPanel: function() {
> +    if (!DirectoryLinksProvider.enabled) {
> +      gAllPages.enabled = !gAllPages.enabled;
> +      return;

So for non-en-US, the old tiny grid icon is still replaced with the new gear icon, but clicking it has the old toggling behavior instead of showing the new customize panel, right?

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +84,5 @@
>    }),
>  
>    get _linksURL() {
> +    if (!this.enabled) {
> +      return "data:text/plain,{}";

Shouldn't this be application/json?
Attachment #8477764 - Flags: review?(adw) → review+
(In reply to Ed Lee :Mardak from comment #6)
> I'm not sure how extensive of tests I should be writing.. ?

Oh, I meant to comment on this.  This patch should definitely have a test, don't you think?  As part of uplifting all of this to 33, the existing tests will come along, right?  Tests run under en-US by default, so the en-US case would already be tested.  So I think the new tests for this bug would change the locale to non-en-US, and then check:

* clicking the gear icon toggles the page instead of showing the panel
* the intro is never shown
* DirectoryLinksProvider.enabled is false
* DirectoryLinksProvider.getLinks returns an empty array
* DirectoryLinksProvider writes an empty JSON blob to disk
* DirectoryLinksProvider.reportSitesAction doesn't phone home, somehow...
(In reply to Drew Willcoxon :adw (Away 8/27–9/2) from comment #8)
> >    showPanel: function() {
> > +    if (!DirectoryLinksProvider.enabled) {
> > +      gAllPages.enabled = !gAllPages.enabled;
> So for non-en-US, the old tiny grid icon is still replaced with the new gear
> icon, but clicking it has the old toggling behavior instead of showing the
> new customize panel, right?
Correct. Reuses the now-unremoved toggle strings too.

> >    get _linksURL() {
> > +    if (!this.enabled) {
> > +      return "data:text/plain,{}";
> Shouldn't this be application/json?
Fixed.


(In reply to Drew Willcoxon :adw (Away 8/27–9/2) from comment #9)
> As part of uplifting all of this to 33, the existing tests will come along
Yup!

> So I think the new tests for this bug would change the locale to non-en-US
> * clicking the gear icon toggles the page instead of showing the panel
> * the intro is never shown
> * DirectoryLinksProvider.enabled is false
> * DirectoryLinksProvider.getLinks returns an empty array
> * DirectoryLinksProvider writes an empty JSON blob to disk
> * DirectoryLinksProvider.reportSitesAction doesn't phone home, somehow...
Done. Done. Done. Done. Done. Done!
Attachment #8477764 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Iteration: --- → 34.3
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
Verified fixed on Windows 7 64bit, Mac OS X 10.8.5 and Ubuntu 14.04 32bit using latest Aurora, build ID: 20140828004002.

Tested with the ach/ar/es-ES/fr/de and ja-JP locales and the enhanced tiles is disabled.
Clicking the gear button from the newtab page will switch to blank page and back.
Status: RESOLVED → VERIFIED
Blocks: 1073823
You need to log in before you can comment on or make changes to this bug.