1145911 - Enable reading list by default

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Once it's enabled, more users will start using it and when we change the name of the database nightly users will lose their reading list items.

Comment 2

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/91b402c7cd92

Comment 3

[:Gijs (he/him)]

Backed out for a bunch of leak + test orange.

remote:   https://hg.mozilla.org/integration/fx-team/rev/fe401fab6120


original push info:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=91b402c7cd92

Comment 4

[Florian Quèze [:florian]]

Attachment: Possible leak fix

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


The bc1 opt Mac failures contain several times this JS error:
chrome://browser/content/browser.js:7199 - TypeError: gNavigatorBundle.getString is not a function

Line 7199 is http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-readinglist.js?rev=fafaf621ada2#267
I don't have time to investigate that error right now.

Comment 5

[Florian Quèze [:florian]]

Attachment: Possible test fix v2

Locally this also fixes the "gNavigatorBundle.getString is not a function" errors. Pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ce7f4caa43

I afraid we'll still need to debug the browser/base/content/test/general/browser_tabfocus.js failure (I couldn't reproduce it locally); unless it is a side effect of the previous test failing.

Gijs, if the try results turn out to be green, feel free to r+/take over and land while I won't be around.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8581143 [details] [diff] [review]
Possible test fix v2

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

r=me with the comment below addressed

::: browser/base/content/browser-readinglist.js
@@ +257,5 @@
>  
>      let isInList = yield ReadingList.hasItemForURL(uri);
> +
> +    if (!this.listenerRegistered) {
> +      // The window must have been closed since we called hasItemForURL.

If this is really about window closure, why not check window.closed ?

Feels odd to use another property (nevermind one called "listenerRegistered") as a proxy for this.

Comment 7

[Florian Quèze [:florian]]

Attachment: Test fix v3

Thanks for the quick review!

I've figured out the browser_tabfocus.js failure, it's caused by browser_readerMode.js that leaves a sidebar open when it's done running.

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

Comment 8

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/9ec91e9617f3
remote:   https://hg.mozilla.org/integration/fx-team/rev/b5bfda73ba54

Comment 9

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/b5bfda73ba54

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/9ec91e9617f3

Comment 11

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-23), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5. 

Verification was done through basic smoke tests. There were a few issues encountered, but all of them are already filed and treated separately. 

Reading List is now enabled by default, via browser.readinglist.enabled.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54db0a4c777f
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc08c7dc1d51

Comment 13

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 38.0a2 (2015-03-30) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.