Closed Bug 1145911 Opened 5 years ago Closed 5 years ago

Enable reading list by default

Categories

(Firefox Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox38 verified, firefox39 verified)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch PatchSplinter Review
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.
Attachment #8581006 - Flags: review?(florian)
Attachment #8581006 - Flags: review?(florian) → review+
Iteration: 38.3 - 23 Feb → 39.2 - 23 Mar
Attached patch Possible leak fix (obsolete) — Splinter Review
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.
Attached patch Possible test fix v2 (obsolete) — Splinter Review
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.
Attachment #8581047 - Attachment is obsolete: true
Attachment #8581143 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8581143 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Test fix v3Splinter Review
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
Attachment #8581143 - Attachment is obsolete: true
Attachment #8581148 - Flags: review?(gijskruitbosch+bugs)
Attachment #8581148 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b5bfda73ba54
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify- → qe-verify+
QA Contact: andrei.vaida
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.
Status: RESOLVED → VERIFIED
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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.