1155191 - Please disable readling list and reader view for 38

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

As agreed, please provide a patch and an uplift request (beta only) to disable reading list in Desktop for 38.0.
We will accept it, next Friday, after beta 5 go to build.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Justin, could you handle that? Thanks

Comment 2

[Justin Dolske [:Dolske]]

(Both reading list and reader view will be disabled for 38.0, as they're features intended to launch with 38.1.)

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 WIP

This flips the 3 prefs involved. Two notes:

* Android will be leading RL/RV enabled, since they have already shipped it. So disabling RL in Desktop means adding a pref==false to firefox.js, and leaving the pref==true in all.js (This is how we had it disabled prior to bug 1148098)

* I added a couple code tweaks to forcibly disable Reading List on desktop -- even if the pref is set to true -- to prevent enthusiastic users from re-enabling it. Due to some imminent changes involving reading list, I am (overly?) paranoid about users enabling it only to later experience sad times.

Untested, and I suspect the latter means test breakage that would need fixed (by disabling unneeded tests). Mark, what do you think about the this? Too paranoid?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

LGTM, and my testing shows it does what it is supposed to. I think the level of paranoia is about right.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8593666 [details] [diff] [review]
Patch v.1 WIP

oops - see comment 4

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Because I had the question on irc and my original message is cryptic for people not involved in this, I am going to explain a bit more.
We are going to make a 38.1 release. In this release, we initially planned to ship reading list & reader view (and screen sharing).
To stabilize the 38.0 release itself, we planned to disable it in beta 5 (we are going to do in beta 6).

And when I said "We will accept it, next Friday, after beta 5 go to build." the "we" means release management (Lawrence, Liz or myself).

Hope this is more clear!

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Went ahead and disabled the broken tests and did a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7dd8752fb17

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Fix another test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=704f2ea08cc1

(There are some JP failures too. I don't _think_ they're RL related, and they seem to be hidden/disabled on mozilla-beta anyway?)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8594286 [details] [diff] [review]
Patch v.2

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

::: browser/components/readinglist/test/browser/browser.ini
@@ +1,5 @@
>  [DEFAULT]
>  support-files =
>    head.js
>  
> +;[browser_ui_enable_disable.js]

maybe:
skip-if = true # reading list is disabled.

would be better here and in xpcshell.ini?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Justin, we need this patch to land asap in m-b. Could you request the uplift? Thanks

Comment 11

[Sylvestre Ledru [:Sylvestre]]

This missed beta 6.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8594426 [details] [diff] [review]
Patch v.3

Carrying over review flag, self-approving for beta. (Sorry, I thought we had until EOD today to land this.)

Comment 13

[Justin Dolske [:Dolske]]

Landed on m-b: https://hg.mozilla.org/releases/mozilla-beta/rev/69173cc17556

Comment 14

[Sylvestre Ledru [:Sylvestre]]

(In reply to Justin Dolske [:Dolske] from comment #12)
> Carrying over review flag, self-approving for beta. (Sorry, I thought we had
> until EOD today to land this.)
No worries. It was EOD but my day in Paris ;)

Comment 15

[Andrei Vaida [:avaida]]

I can confirm that Reader View and Reading List were successfully disabled on Beta 38.0b8-build1 (20150426174329), using Windows 7 x86, Windows 8 x64, Mac OS X 10.9.5 and Ubuntu 14.04 x86.