1134446 - Automatically open the ReadingList sidebar the first time ReaderMode is used

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

To help introduce ReadingList, we want to automatically open the sidebar the first time ReaderMode is used (and ReadingList is enabled).

This should only happen the first time. It's not clear whether the sidebar should auto-close when ReaderMode is exited.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Michael: Thoughts on auto-closing the sidebar?

I guess one difficulty here if we auto-closed is we'd need to keep track of when a tab is closed, or a tab is switched to. Otherwise things would feel unpredictable. ie:
* open readermode
* sidebar auto-opens
* switch to another tab
* browser around for half an hour with the sidebar open
* switch to original readermode tab
* exit readermode
* sidebar suddenly closes by itself - wtf?!

Comment 2

[Mark Capella [:capella]]

Attachment: bug1134446.diff

First hack. Doesn't address auto-close.

Comment 3

[Mark Capella [:capella]]

Attachment: bug1134446.diff

Rebased here too.

Comment 4

[Mark Capella [:capella]]

Attachment: bug1134446.diff

rebased, new test updated

:-) Sorry for bug-spam

Comment 5

[Mark Capella [:capella]]

Comment on attachment 8567641 [details] [diff] [review]
bug1134446.diff

Let's wait here until bug 1133489 finalizes the base test, so I don't have to rebase again.

Comment 6

[Michael Maslaney [:mmaslaney] ([e-mail])]

My inclination is to leave it open, that way we communicate to the user that the Reading List is not dependent on the Reader View.

Comment 7

[Mark Capella [:capella]]

Attachment: bug1134446.diff

Rebased, tested.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8572062 [details] [diff] [review]
bug1134446.diff

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

::: browser/base/content/browser-readinglist.js
@@ +184,5 @@
>        }
> +
> +      case "ReadingList:ShowIntro": {
> +        if (this.enabled && !Services.prefs.getBoolPref("reader.has_shown_readinglist")) {
> +          Services.prefs.setBoolPref("reader.has_shown_readinglist", true);

Doesn't seem right for this to be setting reader.* prefs. Especially given this code doesn't necessarily know this came from ReaderView. And also, this will eventually kick off a ReadingList oriented tour (see mockup linked to in bug 1131112). So how about "browser.readinglist.introShown"?

Also, nit: use |Preferences.get("pref-name", defaultValue)|, and avoid needing a default pref set in firefox.js

::: browser/base/content/test/general/browser_readerMode.js
@@ +46,5 @@
>    is(gURLBar.value, readerUrl, "gURLBar value is about:reader URL");
>    is(gURLBar.textValue, url.substring("http://".length), "gURLBar is displaying original article URL");
>  
> +  // Readinglist button should be present, and status should be automatically
> +  // "opened", as requested in bug bug1134446.

It's not safe to assume the relevant pref is set here - you don't know what other tests have done. Safest for the test to set it to a known state first.

Also: Don't need to reference bugs - that's what blame/annotate is for.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #8)
> It's not safe to assume the relevant pref is set here - you don't know what
> other tests have done. 

Oh, in fact, this behaviour may mess up other tests - best to disable it by default in the test harness. Just need to add the pref to testing/profiles/prefs_general.js such that it won't automatically open, except for this test.

Comment 10

[Mark Capella [:capella]]

Attachment: bug1134446.diff

Comment 11

[Mark Capella [:capella]]

Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

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

Bit-rotted ... back-burner-ing this for now.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

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

Sorry I didn't get to this - I was off on sick leave for a bit.

r+ on the assumption that the bitrot doesn't mean you end up rewriting it ;)

Comment 13

[Mark Capella [:capella]]

Glad you're back!

Bitrot caused some minor test/general/browser_readerMode.js rebase mainly. Passes locally and on TRY ...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f92c424f46

... so I'll carry over the r+.

Comment 14

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/8fd82fc12d25

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8fd82fc12d25

Comment 16

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 39.0a2 (2015-04-02), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.

Comment 18

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

Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

Approval Request Comment
[Feature/regressing bug #]: needed for reading list uitour
[User impact if declined]: reading list uitour is somewhat hampered
[Describe test coverage new/current, TreeHerder]: automated testing, landed on m-c
[Risks and why]: none expected
[String/UUID change made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/887be7f12f1e

Comment 20

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.