1123517 - [ReadingList] Implement basic sidebar that lists unread ReadingList items

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

As an initial step for the Reading List, we'll have a sidebar to list items in the ReadingList. See bug 1120007 for mockups.

As the first step for that, we need a basic sidebar that gives us a minimal framework to build on. Separate bugs will handle things such as thumbnails, opening, deleting, adding, etc.

This will include:
* 40 items (arbitrary number, scaling to 1000 is a separate bug)
* Only unread items
* Show only title and URL
* Clicking loads the URL in a tab (no Reader Mode integration yet)

This can largely be done separately from any backend module to sync the list, if we mock up a basic API.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We have three different visual designs for this:
* The current design in Firefox
* The design mocked up in bug 989102 (and partially implemented in bug 1011014) for all sidebars
* The design mocked up in bug 1120007 for just the reading sidebar 

Which are we going with?

Comment 2

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

For the Reading List, we are moving forward with the design in bug 1120007. Unification of sidebars will be another project entirely, which will most likely have history and bookmarks adopt the new style.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Mark/Drew: You may be interested in the ReadingList.jsm API I've mocked up here.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8556010 [details] [diff] [review]
WiP v1

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

Seems straightforward to me!  Thanks for including the link to the server API.  Makes it easier to understand the JSM design.

::: browser/components/readinglist/ReadingList.jsm
@@ +294,5 @@
> +   * Fetch items matching a set of conditions, in a sorted list.
> +   * @return {Promise}
> +   * @resolves {[Item]}
> +   */
> +  getItems(sort = "addedOn", conditions = {unread: false}, limit: {start: 0, end: null}) {

Think the colon after limit should be an equals sign.

It might be better for this to take one "options" object instead of many parameters.  You'd include only the options that you're interested in.

@@ +316,5 @@
> +  /**
> +   * Find an item based on its URL.
> +   *
> +   * TODO: Does this match original or resolved URL, or both?
> +   * TODO: Should this just be a generic findItem API?

Maybe!  I don't know.  Might make sense to combine getItemByID and getItemByURL into one getItem that takes an options object.

Comment 5

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

Comment on attachment 8556010 [details] [diff] [review]
WiP v1

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

LGTM - most of my comments are made without context so take them for what they cost ya!

::: browser/components/readinglist/ReadingList.jsm
@@ +21,5 @@
> +  parentLogger.addAppender(new Log.ConsoleAppender(formatter));
> +  parentLogger.addAppender(new Log.DumpAppender(formatter));
> +})();
> +
> +let logger =  Log.repository.getLogger("readinglist.backend");

I think |log| is a better named (but YMMV, so whatever :)

@@ +53,5 @@
> +  /**
> +   * @type {nsIURL}
> +   */
> +  get originalUrl() {
> +    try {

this seems like a future foot-gun - can a reading list item really have an invalid originalUrl?  Even if it can, I'd expect this._data.url to be null (ie, the silent catching of the exception seems dangerous)

@@ +73,5 @@
> +  get resolvedUrl() {
> +    try {
> +      return Services.io.newURI(this._data.resolved_url, null, null);
> +    } catch (e) {}
> +    return this.originalUrl;

ditto here - if .resolved_url is null we should just return null rather than expecting an invalid one.

@@ +234,5 @@
> +  _init() {
> +    logger.debug("Init");
> +
> +    // Initialize mock data
> +    if (gMockData) {

IMO, Object.freeze and additional hacks for mocking is overkill and is solving a problem that doesn't really exist in practice, and just forces addon authors to find even more fragile ways of doing what they want to do. I know for sure others in the team disagree strongly about this, but I'm just preparing for "told you so" in the future ;)

@@ +262,5 @@
> +
> +  /**
> +   * Notify all registered event listeners of an event.
> +   */
> +  _notifyListeners(eventName, ...args) {

why not nsIObserverService?

@@ +331,5 @@
> +
> +
> +/**
> + * Mock data for dev/testing, until the real backend gets built.
> + */

somewhat related to the above, I think it might make sense to ensure the a test can add this mock data rather than making it inline - it seems likely to change the API (eg, the mock data might need to be passed into the ctor or something).  IOW, an xpcshell test should be added sooner rather than later even if it doesn't test much yet.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: MozReview Request: bz://1123517/Unfocused

/r/3893 - Bug 1123517 - Refactor sidebar code.
/r/3895 - Bug 1123517 - [ReadingList] Implement basic sidebar that lists unread ReadingList items.

Pull down these commits:

hg pull review -r d5b363c664d4e4c0fd5a8d7ccafe8d57d97c3a26

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note that this is intentionally lacking in numerous areas:
* Tests aren't enough, we can flesh those out more later. So some extent this is blocked on other work anyway.
* Visual polish is reasonably true to the designs. One notable issue is lack of fitting in on all platforms. eg, sidebar header background color. And the mockups only deal with OSX.
* Various other things missing already covered by existing bugs

But, we benefit more by landing this earlier, and doing thing in numerous followups. My main concern here has been to constructing a reasonable foundation that everything else can build on (sidebar, urlbar, bookmarks, readermode, uitour, data storage, syncing, etc).

The first patch here is an unfortunate necessity - it was either refactor, or hack around 10+ year old code and be left with fragility and code duplication. And before you ask, no one else knows that code either ;) It's only a refactoring, and existing tests seem to cover it good enough.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #7)
> But, we benefit more by landing this earlier,

Which is to say, I'd like to convert as many review comments to followup bugs as possible.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8564809 [details]
MozReview Request: bz://1123517/Unfocused

/r/3893 - Bug 1123517 - Refactor sidebar code.
/r/3895 - Bug 1123517 - [ReadingList] Implement basic sidebar that lists unread ReadingList items.

Pull down these commits:

hg pull review -r 0a0f3af061e29f49eac4bfea2c583d7fbbd7391b

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 1 - v1 (sidebar refactor)

Reverting to splinter, following reviewboard awkwardness.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 2 - v1 (readinglist sidebar)

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8564986 [details] [diff] [review]
Part 1 - v1 (sidebar refactor)

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

f+ for now; could be changed to r+ after discussing or fixing the behavior of the uninit method.

From having reviewed only this patch, I haven't seen yet what prompted the need for this large refactoring, but the new code is definitely much more readable.

I think it would have been nice to have this patch in its own bug, but it's probably too late at this point.

::: browser/base/content/browser-sidebar.js
@@ +57,5 @@
> +
> +  uninit() {
> +    let enumerator = Services.wm.getEnumerator(null);
> +    enumerator.getNext();
> +    if (enumerator.hasMoreElements()) {

The code you moved was testing "!enumerator.hasMoreElements()". Have you intentionally removed the '!'?

I don't really understand what this code is attempting to do with that enumerator, and it's been there 'forever' - CVS history says "2002-11-04 16:29 Fix some bugs, hook up cross-session sidebar persistence.", no bug number -, but... my guess is it's checking that we are shutting down, vs just closing a window.

@@ +192,5 @@
> +        if (broadcaster != sidebarBroadcaster) {
> +          broadcaster.removeAttribute("checked");
> +        } else {
> +          sidebarBroadcaster.setAttribute("checked", "true");
> +        }

nit: I don't think the added {} improve readability here, and it's not consistent with 2 ifs within 10 lines before/after this that don't have {}.

@@ +209,5 @@
> +      let url = sidebarBroadcaster.getAttribute("sidebarurl");
> +      this.browser.setAttribute("src", url); // kick off async load
> +
> +      // We set this attribute here in addition to setting it on the <browser>
> +      // element itself, because the code in gBrowserInit.onUnload persists this

You probably want to update this line; this code is now in SidebarUI.uninit.

@@ +268,5 @@
> +    this._title.value = "";
> +    this._box.hidden = true;
> +    this._splitter.hidden = true;
> +    gBrowser.selectedBrowser.focus();
> +  },

nit: trailing comma.

@@ +296,5 @@
> + * @deprecated
> + */
> +function toggleSidebar(commandID, forceOpen = false) {
> +  Deprecated.warning("toggleSidebar() is deprecated, please use SidebarUI.toggle() or SidebarUI.show() instead",
> +                     "https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Sidebar");

We should ensure this page gets updated.

::: browser/base/content/browser.js
@@ +1705,5 @@
>    case "Search":
>      BrowserSearch.webSearch();
>      break;
>    case "Bookmarks":
> +    SidebarUI.toggle('viewBookmarksSidebar');

nit: touching this line seems a good opportunity to change the single quotes to double quotes.

@@ +5305,5 @@
>  
>      // Tell the Web Panels sidebar to load the bookmark.
> +    if (SidebarUI.browser.docShell && SidebarUI.browser.contentDocument &&
> +        SidebarUI.browser.contentDocument.getElementById("web-panels-browser")) {
> +      SidebarUI.browser.contentWindow.loadWebPanel(aURI);

I think you should either re-indent the whole function, or keep the 4 space indent on this line, so that the whole function has consistent indent.

@@ +5323,5 @@
>  function asyncOpenWebPanel(event)
>  {
> +    if (gWebPanelURI && SidebarUI.browser.contentDocument &&
> +        SidebarUI.browser.contentDocument.getElementById("web-panels-browser")) {
> +      SidebarUI.browser.contentWindow.loadWebPanel(gWebPanelURI);

Same here, mixed 2-space and 4-space indent in the same function isn't nice.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8564987 [details] [diff] [review]
Part 2 - v1 (readinglist sidebar)

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

::: browser/base/content/browser-readinglist.js
@@ +1,5 @@
> +/*
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +*/

You can remove the /* */ around this header, the file will go through the preprocessor.

@@ +56,5 @@
> +  hideSidebar() {
> +    if (this.isSidebarOpen) {
> +      SidebarUI.hide();
> +    }
> +  },

nit: trailing comma.

::: browser/base/content/browser-sets.inc
@@ +424,5 @@
>           command="viewHistorySidebar"/>
>  
> +    <key id="key_readingListSidebar"
> +         key="&readingList.sidebar.commandKey;"
> +         modifiers="accel,shift"

I wondered if that keyboard shortcut was already in use. Turns out it is, for "Firefox -> Services -> Search with Google"... and that opens Safari!

::: browser/base/content/test/BrowserUITestUtils.jsm
@@ +42,5 @@
> +   * @param {Element} target - Expected target of the event.
> +   * @returns {Promise} A Promise that resolves to the received event, or
> +   *                    rejects with an Error.
> +   */
> +  waitForEvent(subject, eventName, timeoutMs, target) {

This seems very similar to promiseWaitForEvent which is already in the scope from: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#113

Do we really need to have 2 different versions?

@@ +46,5 @@
> +  waitForEvent(subject, eventName, timeoutMs, target) {
> +    return new Promise((resolve, reject) => {
> +      function listener(event) {
> +        if (target && target !== event.target) {
> +            return;

nit: fix indent.

::: browser/components/readinglist/ReadingList.jsm
@@ +30,5 @@
> +
> +/**
> + * Represents an item in the Reading List.
> + * @constructor
> + * @see https://github.com/mozilla-services/readinglist/wiki/API-Design-proposal#data-model

Is this documentation expected to move to MDN at some point?

@@ +174,5 @@
> +   * Array of scraped or captured summary images for this page.
> +   * @type {[nsIURL]}
> +   */
> +  get images() {
> +    return null;

Should this return an empty array rather than null? Doesn't really matter anyway, as this is obviously not the final code.

@@ +203,5 @@
> +    return this.resolvedTitle;
> +  },
> +
> +  /**
> +   * Domain portion of the URL, with prefixes stripped. For display puposes.

spelling: pu_r_poses

@@ +207,5 @@
> +   * Domain portion of the URL, with prefixes stripped. For display puposes.
> +   * @type {string}
> +   */
> +  get domain() {
> +    let host = this.resolvedUrl.host;

Should this use nsIEffectiveTLDService.getBaseDomain(this.resolvedUrl, 1) ?

@@ +226,5 @@
> +   * Get the value that should be used for a JSON representation of this Item.
> +   */
> +  toJSON() {
> +    return this._data;
> +  },

nit: trailing comma. (last time I mentioned it; there are also trailing commas in several other places in this patch).

@@ +258,5 @@
> +    }
> +  },
> +
> +  get status() {
> +    return "ok";

What's the use case for this?

@@ +263,5 @@
> +  },
> +
> +  /**
> +   * Add an event listener.
> +   * @param {object} listener - Listener object start notifying.

It seems a word is missing in this line.

@@ +303,5 @@
> +   *                            filtering items.
> +   * @return {Promise}
> +   * @resolves {number}
> +   */
> +  getNumItems(conditions = {unread: false}) {

Could probably do with a TODO comment pointing out that filtering isn't implemented yet.

::: browser/components/readinglist/moz.build
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +JAR_MANIFESTS += ['jar.mn']
> +
> +EXTRA_JS_MODULES.readinglist += [

Do we really need ReadingList.jsm to be in its own subfolder?

::: browser/components/readinglist/sidebar.js
@@ +82,5 @@
> +    let itemNode = this.itemNodesById.get(item.id);
> +    itemNode.remove();
> +    this.itemNodesById.delete(item.id);
> +    this.itemsById.delete(item.id);
> +    //this.ensureListItems();

Please remove this line if it's just dead code, or replace it with a comment explaining what's missing.

@@ +376,5 @@
> +      this.selectedItem.focus();
> +    } else if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
> +      let selectedItem = this.selectedItem;
> +      if (selectedItem) {
> +        this.activeItem = this.selectedItem;``

Remove the trailing '``'.

::: browser/components/readinglist/sidebar.xhtml
@@ +6,5 @@
> +<!DOCTYPE html>
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +  <head>
> +    <script src="chrome://browser/content/readinglist/sidebar.js" type="application/javascript;version=1.8"></script>
> +    <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/skin/readinglist/sidebar.css"/>

Should this HTML document have a <title> element? I wonder if it's used for eg. accessibility. It would likely also be visible in DOM Inspector.

::: browser/components/readinglist/test/ReadingListTestUtils.jsm
@@ +12,5 @@
> +Cu.import("resource://testing-common/Assert.jsm");
> +Cu.import("resource:///modules/readinglist/ReadingList.jsm");
> +
> +
> +/** Preference name controller whether the ReadingList feature is enabled/disabled. */

controlling

@@ +29,5 @@
> +  /**
> +   * Reference to the RLSidebar object controlling the ReadingList sidebar UI.
> +   * @type {object}
> +   */
> +  get controller() {

Would this be more readable if it was just called 'RLSidebar'?

@@ +42,5 @@
> +    return this.controller.list;
> +  },
> +
> +  /**
> +   * Check that the numnber of element in the list matches the expected count.

spelling: num_ber of element_s_

@@ +47,5 @@
> +   * @param {number} count - Expected number of items.
> +   */
> +  expectNumItems(count) {
> +    this.Assert.equal(this.list.childElementCount, count,
> +                 "Should have expected number of items in the sidebar list");

indent

@@ +75,5 @@
> +                 "Node should have correct title attribute");
> +    this.Assert.equal(node.querySelector(".item-title").textContent, item.title,
> +                 "Node's title element's text should match item title");
> +    this.Assert.equal(node.querySelector(".item-domain").textContent, item.domain,
> +                 "Node's domain element's text should match item title");

The indent is off on all these lines. Looks like you did a s/Assert/this.Assert/g and forgot to re-indent the second line of each call.

::: browser/components/readinglist/test/browser/browser_sidebar_mouse_nav.js
@@ +50,5 @@
> +  RLSidebarUtils.expectActiveId(null);
> +
> +  info("Mouse move over item 1");
> +  yield mouseInteraction("mousemove", "SelectedItemChanged", RLSidebarUtils.list.children[0]);
> +  RLSidebarUtils.expectSelectedId("00bd24c7-3629-40b0-acde-37aa81768735");

hard coding these ids here makes the test difficult to read. Could you instead have a variable with the array given to RLUtils.addItem, and compare with array[0].id?

::: browser/components/readinglist/test/browser/browser_ui_enable_disable.js
@@ +13,5 @@
> +  if (enabled) {
> +    Assert.notEqual(sidebarBroadcaster.getAttribute("hidden"), "true",
> +                    "Sidebar broadcaster should not be hidden");
> +    Assert.notEqual(sidebarMenuitem.getAttribute("hidden"), "true",
> +                    "Sidebar meniitem should be visible");

menuitem

@@ +18,5 @@
> +  } else {
> +    Assert.equal(sidebarBroadcaster.getAttribute("hidden"), "true",
> +                 "Sidebar broadcaster should be hidden");
> +    Assert.equal(sidebarMenuitem.getAttribute("hidden"), "true",
> +                    "Sidebar meniitem should be hidden");

nit: fix indent on this line.
spelling: menuitem

@@ +36,5 @@
> +  RLUtils.enabled = true;
> +  checkRLState();
> +
> +  info("Opening ReadingList sidebar");
> +  ReadingListUI.showSidebar();

is a yield missing here?

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8564986 [details] [diff] [review]
Part 1 - v1 (sidebar refactor)

Changing to r+, Blair confirmed over IRC that the behavior change in the uninit method was a typo.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8564987 [details] [diff] [review]
Part 2 - v1 (readinglist sidebar)

I thought I had changed this f+ to r+ too yesterday. All my comments on this patch are only requesting straight forward cleanups. The patch adds new tests, so I would like to be pushed to try before landing though.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> nit: I don't think the added {} improve readability here, and it's not
> consistent with 2 ifs within 10 lines before/after this that don't have {}.

> nit: trailing comma.

AFAIK, it's generally agreed now days that bracing single-line statements is a good thing due to more obvious code, and trailing commas at the end of object definitions is a good thing due to cleaner history when adding new properties.


(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> You can remove the /* */ around this header, the file will go through the
> preprocessor.

Heh... see the latest thread in firefox-dev, regarding JS tooling & pre-processor usage. Anything preprocessed out like this breaks JS tools - putting it in a comment means the license still gets removed in the build step, but tools cope fine.


> I wondered if that keyboard shortcut was already in use. Turns out it is,
> for "Firefox -> Services -> Search with Google"... and that opens Safari!

*sigh* Ctrl-Alt-R then.


> This seems very similar to promiseWaitForEvent which is already in the scope
> from:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/head.js#113
> 
> Do we really need to have 2 different versions?

It's in scope for tests in browser/content/base/test/general, but not in scope for the ReadingList tests. Which is why we have a dozen or so slightly different implementations of that function, and numerous others, spread around numerous head.js files and individual test files :\

So this is a start of unifying our utility functions related to general browser UI, similar to what we have in PlacesTestUtils.jsm and AddonManagerTesting.jsm - it lets a test anywhere in the tree include the module, rather than re-implementing it. I don't want to go through all the various other implementations and de-duping that code in this bug, but it makes a reasonable good-second-bug (bug 1134139).


> > + * @see https://github.com/mozilla-services/readinglist/wiki/API-Design-proposal#data-model
> 
> Is this documentation expected to move to MDN at some point?

Probably not in the near future.


> Should this use nsIEffectiveTLDService.getBaseDomain(this.resolvedUrl, 1) ?

No, sadly eTLD/getBaseDomain isn't terribly useful for stripping common prefixes. eg:
* "www.mydomain.org" => "www.mydomain.org" (additionalParts=1)
* "www.mydomain.com" => "mydomain.org" (additionaParts=0)
* "something.wordpress.org" => "something.wordpress.org" (additionalParts=1)
* "something.wordpress.org" => "wordpress.org" (additionalParts=0)

Manually checking if it starts with "www." and just removing that via slice() is what all other code does elsewhere for this reason.


> > +  get status() {
> > +    return "ok";
> 
> What's the use case for this?

None at the moment - removed.


> > +EXTRA_JS_MODULES.readinglist += [
> 
> Do we really need ReadingList.jsm to be in its own subfolder?

Oh, I meant to mention this here. Mark brought this up on IRC, as he's planning on putting a few JSMs here (Drew too), and they're very specific to ReadingList - putting them all in the common modules directory (without a subdir) will mean polluting that dir with modules that aren't useful outside of ReadingList.


> Should this HTML document have a <title> element? I wonder if it's used for
> eg. accessibility. It would likely also be visible in DOM Inspector.

It's not used for accessibility - the sidebar label gets used automatically. But I guess it is useful for development.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/f15413bf7ef9
https://hg.mozilla.org/integration/fx-team/rev/0e65e74f315f

Comment 18

[Florian Quèze [:florian]]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #16)
> (In reply to Florian Quèze [:florian] [:flo] from comment #12)
> > nit: I don't think the added {} improve readability here, and it's not
> > consistent with 2 ifs within 10 lines before/after this that don't have {}.
> 
> > nit: trailing comma.
> 
> AFAIK, it's generally agreed now days that bracing single-line statements is
> a good thing due to more obvious code

That's why I didn't flag it in new code. I mentioned it at this specific spot because it was inconsistent with the surrounding code.

> (In reply to Florian Quèze [:florian] [:flo] from comment #13)
> > You can remove the /* */ around this header, the file will go through the
> > preprocessor.
> 
> Heh... see the latest thread in firefox-dev

Whatever that thread says, I flagged it because you did it differently in browser-sidebar.js and browser-readinglist.js.

> > I wondered if that keyboard shortcut was already in use. Turns out it is,
> > for "Firefox -> Services -> Search with Google"... and that opens Safari!
> 
> *sigh* Ctrl-Alt-R then.

I'm not convinced breaking a shortcut that opened Safari was a problem, but oh well... :).

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2017719&repo=fx-team

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Re-landed. Serves me right for trying to save resources by not doing a debug Try run - had a couple of leaks.

https://hg.mozilla.org/integration/fx-team/rev/5dfb417f346e
https://hg.mozilla.org/integration/fx-team/rev/9b07a29dcbdd

Comment 21

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2033098&repo=fx-team

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

*sigh* Intermittent test timeout of browser_sidebar_mouse_nav.js. But only on opt, and I can't reproduce locally. So assuming there's a race condition somewhere - punting that to bug 1135350 so this can land.

https://hg.mozilla.org/integration/fx-team/rev/cdb550540b47
https://hg.mozilla.org/integration/fx-team/rev/f617ca1020a2

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdb550540b47
https://hg.mozilla.org/mozilla-central/rev/f617ca1020a2

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Mass change of ReadingList bugs, moving to their own component. Filter bugspam on the following quote:

“Reading is to the mind what exercise is to the body.”
― Joseph Addison

Comment 25

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

(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #13)
> > +/*
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +*/

I agree that we shouldn't use the preprocessor for this, but we should use the JS-comment-only header, rather than a mixed version of the C++/JS ones (i.e. remove the "#", replace with "* ").

Comment 26

[Josh Aas]

Can we back this out to fix 1136291? It's a pretty bad regression.

Comment 27

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

Easier all around to just fix bug 1136291. In the meantime, just stop closing all of your windows :)

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Josh Aas (Mozilla Corporation) from comment #26)
> Can we back this out to fix 1136291? It's a pretty bad regression.

No. Most of the ReadingList bugs are dependant on this bug, and that project has higher priority. We have a fix for bug 1136291 which should land soon anyway.

Comment 29

[Andrei Vaida [:avaida]]

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

I noticed a few potential issues though:
(1) on Windows, with m-c and m-a: an error is thrown by WindowsPreviewPerTab.jsm:406:0 for toggling the Reading List sidebar. It might be related to Bug 1131428, here's a stacktrace:
> NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate] WindowsPreviewPerTab.jsm:406:0

(2) on Linux, with m-a: the "Reading List" header displayed in the sidebar is white, perhaps because of the dev-edition theme. Screenshot: http://i.imgur.com/HqV2yae.png.

Both of these could be treated in separate bugs if necessary. Blair, let me know what's your take on these.

Comment 30

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

(In reply to Andrei Vaida, QA [:avaida] from comment #29)
> (2) on Linux, with m-a: the "Reading List" header displayed in the sidebar
> is white, perhaps because of the dev-edition theme. Screenshot:
> http://i.imgur.com/HqV2yae.png.

New bug for this sounds like a good idea.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Andrei Vaida, QA [:avaida] from comment #29)
> (1) on Windows, with m-c and m-a: an error is thrown by
> WindowsPreviewPerTab.jsm:406:0 for toggling the Reading List sidebar. It
> might be related to Bug 1131428, here's a stacktrace:
> > NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate] WindowsPreviewPerTab.jsm:406:0

This is unrelated, it's bug 1127577.

Comment 32

[Ian Nartowicz]

The listed documentation page says nothing at all about this.  It doesn't mention SidebarUI at all and it still documents using toggleSidebar() to open a sidebar, with no mention of deprecation.

Comment 33

[Florian Quèze [:florian]]

dev-doc-needed per comment 32.

Comment 34

[Jean-Yves Perrier [:teoli]]

Removing dev-doc-needed, because I think this project is dead.

Comment 35

[Ian Nartowicz]

(In reply to Jean-Yves Perrier [:teoli] from comment #34)
> Removing dev-doc-needed, because I think this project is dead.

The console in release and nightly versions still reports that toggleSidebar is deprecated and refers the user to a page that says nothing about it.  Either the code needs backing out or the page needs updating.

Comment 36

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

(In reply to Ian Nartowicz from comment #32)
> The listed documentation page says nothing at all about this.  It doesn't
> mention SidebarUI at all and it still documents using toggleSidebar() to
> open a sidebar, with no mention of deprecation.

I can't see what documentation you are talking about in the comments above.

(In reply to Ian Nartowicz from comment #35)
> The console in release and nightly versions still reports that toggleSidebar
> is deprecated and refers the user to a page that says nothing about it. 
> Either the code needs backing out or the page needs updating.

I assume you are talking about the warning generated at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sidebar.js?offset=0#324? I'm not sure what code you are suggesting needs backing out (I believe the code added in this bug mostly already has been) nor what documentation needs updating, but maybe a different bug is appropriate?

Comment 37

[Ian Nartowicz]

I'm not sure how to explain it any better.  The piece of code that throws console messages about toggleSidebar being deprecated is still on trunk, and the documentation page it points to says nothing about SidebarUI or toggleSidebar being deprecated.  One of them should change.  Your comment 34 suggests that the code should change and stop generating this message.

Comment 38

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

Right, I understand, thanks. I opened bug 1209809