George Wright (:gw280) (needinfo me!) Assignee
	Description • 10 years ago

We currently embed XUL inside the HTML document that's generated when viewing feeds in order to show the subscribe UI. With e10s this will no longer be possible as we are not going to support XUL popups from the content process.

	Sevaan Franks [:sevaan]
	Comment 1 • 10 years ago

Screenshot attached showing current subscribe ui. The yellow area is what will need to be moved into a chrome notification (infobar maybe).

	Chris Peterson [:cpeterson]
	Comment 2 • 10 years ago

UX is working to a new UI design that doesn't use XUL in content.

	Matthew N. [:MattN]
	Comment 7 • 9 years ago

The conclusion in the bug summary doesn't seem to be required by the driving force in comment 0. We should be able to just implement (most, if not all of) the current dropdown using a <select>. We would port the existing style and not need new UX.

	Jim Mathies [:jimm]
	Comment 9 • 9 years ago

retriage for m6, also, we could use some clarification on what the actual fix is going to be. It sounds like ux was planning some big change, but we won't be implementing that here to fix this bug.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 10 • 9 years ago

so, i have an easy fix from previously that can land (providing it gets an r+) without too much effort. I feel we have two (three in the long run) options here:

1) Remove the XUL menulist stuff from feed subscriber and switch to using HTML select.
2) Add very simple support for XUL menulists to Select{Content,Parent}Helper.jsm whose support is limited in scope to fixing the feed subscriber.

(secret 3rd option, which we should do in the long term - redesign the UX for the feed subscriber to remove all XUL from content)

I think that as options 1 or 2 are strictly interim options to unblock feed subscribing until 3) is done, we should opt for the option that takes the least effort, which is 1. We should probably just land a variation of the first patch on bug 1068626.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 11 • 9 years ago

Sorry, I meant option 2 is the one that requires the least effort.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 12 • 9 years ago

Here's the patch I dusted off from an old git branch I had, rebased on current m-c (it had bitrotted quite significantly). It doesn't quite work but it's most of the way there to getting the RSS subscriber UI to be functional as is.

	Jim Mathies [:jimm]
	Comment 13 • 9 years ago

Comment on attachment 8584775 [details] [diff] [review]
0001-First-attempt-at-networking-a-XUL-menu-across-to-the.patch

Looks good as a work around until we remove xul from the ux.

	Matthew N. [:MattN]
	Comment 15 • 9 years ago

> Assignee: gwright@mozilla.com → philipp@mozilla.com

I don't think we need UX in this bug anymore as we can already make XUL dropdowns work or switch to a similar looking HTML <select>.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 16 • 9 years ago

(In reply to Matthew N. [:MattN] from comment #15)
> > Assignee: gwright@mozilla.com → philipp@mozilla.com
> 
> I don't think we need UX in this bug anymore as we can already make XUL
> dropdowns work or switch to a similar looking HTML <select>.

This is very much a hack though and I think that long term we should investigate moving the feed subscriber out of the content area and into the chrome.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 18 • 9 years ago

Here's an idea. Stephen, what do you think?
Not really super-happy with the style...

	Mike Conley (:mconley) (:⚙️)
	Comment 20 • 9 years ago

Stealing because gw280 is on PTO.

	Mike Conley (:mconley) (:⚙️)
	Comment 21 • 9 years ago

I agree that the feed reader stuff should probably get moved into the parent chrome or something, but for now, the workaround to just make the dropdown work is probably more than acceptable.

I figure we can try to make the feed reader great (or dead) in the coming months.

	Mike Conley (:mconley) (:⚙️)
	Comment 22 • 9 years ago

Bug 1109714 - Remove XUL from about:feeds. r?mak

	Mike Conley (:mconley) (:⚙️)
	Comment 23 • 9 years ago

Aaaaaand un-stealing, because now _I'm_ going on PTO.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 25 • 9 years ago

It's a game of hot potato.

(we reckon it's better for me to keep this for now, and he gets another fun bug)

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 26 • 9 years ago

My WIP patch. It's not quite functional yet, but it's close.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 29 • 9 years ago

So this mostly works now. You can subscribe to the feed using the built-in subscriber, and web handlers work fine. There are a few inefficiencies with the way I've done it right now (like getContentHandlers() just rebuilds the entire handler DB every time we call it).

I'm not sure about the correct way to handle setComplexValue in the child process for the purposes of FeedWriter.js. At the moment I've grabbed the string from the object and sent that over as the object from which the complex value is derived can't be sent cross process. 

I'm just putting this up for review to get some feedback on the direction this is heading in.

	Mike Conley (:mconley) (:⚙️)
	Comment 30 • 9 years ago

Comment on attachment 8678828 [details] [diff] [review]
feedwriter.patch

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

Hey George,

I think this is the right direction. I can't say I'm super familiar with the feed reader code - Mano or mak probably know it best. Anyhow, I've poked at it and found a few things. Let me know if you have any questions!

-Mike

::: browser/components/feeds/FeedConverter.js
@@ +420,5 @@
>        LOG("unexpected handler: " + handler);
>        // fall through
>      case "bookmarks":
> +      Services.cpmm.sendAsyncMessage("FeedConverter:addLiveBookmark",
> +                                     { spec: spec,

You can actually avoid the repetition here - ES6 allows us to shortcut when the key and value share the same name, like so:

Services.cpmm.sendAsyncMessage("FeedConverter:addLiveBookmark",
                               { spec, title, subtitle });

::: browser/components/feeds/FeedWriter.js
@@ +243,2 @@
>  
>    /* Magic helper methods to be used instead of xbl properties */

I guess this comment will need to be updated.

@@ +243,3 @@
>  
>    /* Magic helper methods to be used instead of xbl properties */
>    _getSelectedItemFromMenulist: function FW__getSelectedItemFromList(aList) {

It's not a menulist anymore. Maybe should just be _getSelectedOption or something.

@@ +243,4 @@
>  
>    /* Magic helper methods to be used instead of xbl properties */
>    _getSelectedItemFromMenulist: function FW__getSelectedItemFromList(aList) {
> +    var options = aList.options;

let, not var.

@@ +637,3 @@
>     *          The menuitem's associated file
>     */
>    _initMenuItemWithFile: function(aMenuItem, aFile) {

Probably fine - though I'll bet we'll want to update the documentation and variable names, since this isn't a menuitem anymore.

@@ +669,3 @@
>  
>                // Show and select the selected application menuitem
>                this._selectedAppMenuItem.hidden = false;

Is hidden even paid attention to right now?

@@ +751,4 @@
>        return;
>      }
>  
> +    if (event.type == "click") {

Can we add a switch for event.type here? That's a pretty common pattern when we're reacting to more than 1 type of event in a handler.

@@ +756,4 @@
>            this.subscribe();
> +      }
> +    } else if (event.type = "change") {
> +      switch (this._getSelectedItemFromMenulist(event.target).id) {

There's only one item in this switch statement - might as well just use a standard conditional.

@@ +772,5 @@
>      }
>    },
>  
> +  _getElementsByAttribute: function FW__getElementsByAttribute(aAttributeName, aAttributeValue) {
> +    var menuitems = this._document.getElementsByClassName("menuitem-iconic");

There's an easier way to do this:

let menu = this._document.getElementById("handlersMenuList");
return menu.querySelectorAll(`[${aAttributeName}={$aAttributeValue}]`);

However, there's only one call to this as far as I can see, using webhandlerurl as the attribute name. Might as well specialize this unless you intend on using it elsewhere, so something like:

_getWebHandlerURLElements: function(url) {
  let menu = this._document.getElementById("handlersMenuList");
  return menu.querySelectorAll(`[webhandlerurl={$aAttributeValue}]`);
}

@@ +848,5 @@
>      }
>    },
>  
>    _initSubscriptionUI: function FW__initSubscriptionUI() {
> +    let handlersMenuPopup = this._document.getElementById("handlersMenuList");

Probably best to just refer to this as the handlersMenuList instead of handlersMenuPopup. Let's try to eliminate all of the old "menulist", "menupopup" terminology being used in here.

@@ +902,4 @@
>                                    defaultFeedReader;
>        menuItem = liveBookmarksMenuItem.cloneNode(false);
>        menuItem.removeAttribute("selected");
> +      menuItem.setAttribute("id", "defaultHandlerMenuItem");

So wait, multiple items can have this id?

In HTML, we want ids to be unique. If multiple things are going to have this value, it should probably be a separate attribute on the node.

@@ +944,4 @@
>          }
> +        menuItem = liveBookmarksMenuItem.cloneNode(false);
> +        menuItem.removeAttribute("selected");
> +        menuItem.className = "menuitem-iconic";

This class doesn't really make sense on a <option> item. Also, menuItem is probably not the right variable name. Should be option or something.

@@ +964,4 @@
>  
>      // We update the "Always use.." checkbox label whenever the selected item
>      // in the list is changed
> +    handlersMenuPopup.addEventListener("change", this, false);

The third argument for capturing defaults to false if omitted. Might as well just omit it.

@@ +968,4 @@
>  
>      // Set up the "Subscribe Now" button
> +    this._document.getElementById("subscribeButton")
> +        .addEventListener("click", this, false);

The third argument for capturing defaults to false if omitted. Might as well just omit it.

@@ +1053,4 @@
>      this._window = window;
>      this._document = window.document;
>      this._document.getElementById("feedSubscribeLine").offsetTop;
> +    this._handlersMenuList = this._document.getElementById("handlersMenuList");

Again, this should probably be something like this._handlersList.

@@ +1099,5 @@
>    },
>  
>    close: function FW_close() {
> +    this._document.getElementById("subscribeButton")
> +        .removeEventListener("click", this, false);

The third argument for capturing defaults to false if omitted. Might as well just omit it.

@@ +1101,5 @@
>    close: function FW_close() {
> +    this._document.getElementById("subscribeButton")
> +        .removeEventListener("click", this, false);
> +    this._document.getElementById("handlersMenuList")
> +        .removeEventListener("change", this, false);

The third argument for capturing defaults to false if omitted. Might as well just omit it.

::: browser/components/feeds/WebContentConverter.js
@@ +909,5 @@
> +   */
> +  _init: function WCCRC__init() {
> +    var ps = Services.prefs;
> +
> +    var kids = ps.getBranch(PREF_CONTENTHANDLERS_BRANCH)

This looks like it's been copied over from FeedHandler.js. Is there any way of de-duplicating the code?

@@ +941,5 @@
> +        var type = childPrefs[i];
> +        var uri = autoBranch.getCharPref(type);
> +        if (uri) {
> +          var handler = this.getWebContentHandlerByURI(type, uri);
> +          this._setAutoHandler(type, handler);

_setAutoHandler isn't defined... setAutoHandler is though.

@@ +964,5 @@
> +    }
> +    return false;
> +  },
> +  _registerContentHandlerWithBranch: function(branch) {
> +    /**

I see that a lot of this is copied from WebContentConverterRegistrar. Is there a way we can de-deduplicate the code by inheriting or mixing in some common logic?

@@ +1023,5 @@
> +  },
> +
> +  setAutoHandler(aContentType, aHandler) {
> +    let cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"].
> +                 getService(Ci.nsIMessageSender);

Services.cpmm

::: browser/modules/Feeds.jsm
@@ +18,5 @@
>    init() {
>      let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
>      mm.addMessageListener("WCCR:registerProtocolHandler", this);
> +
> +    let ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIMessageListenerManager);

I think we can get this via Services.ppmm.

@@ +19,5 @@
>      let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
>      mm.addMessageListener("WCCR:registerProtocolHandler", this);
> +
> +    let ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
> +    ppmm.addMessageListener("WCCR:setAutoHandler", this);

What is WCCR? Should we be using that for the message for addLiveBookmark instead of FeedConverter?

@@ +53,5 @@
> +      }
> +
> +      case "FeedConverter:addLiveBookmark": {
> +        let data = aMessage.data;
> +        var wm =

Services.wm should get you the nsIWindowMediator.

Alternatively, you can use RecentWindow.jsm, which exposes getMostRecentBrowserWindow, which should do the right thing.

In any case, avoid using var in here - I think we prefer let now for most cases.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 33 • 9 years ago

So here's a first attempt at something that can maybe be landable. All the functionality (to the best of my knowledge?) works. There's still some code duplication in WebContentConverter.js, though.

	Marco Bonardo [:mak]
	Comment 35 • 9 years ago

I may look at this tomorrow.
I assume the patch to look at is the one attached here and flagged, not the one on MozReview? I'm a little bit confused by seeing both...

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 36 • 9 years ago

yes, the one on mozreview is an old one from :mconley. I haven't jumped to using mozreview yet for my patches :)

	Marco Bonardo [:mak]
	Comment 38 • 9 years ago

Comment on attachment 8689194 [details] [diff] [review]
0001-Make-the-feed-subscriber-UI-work-in-e10s.patch

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

There are some trailing spaces around, please clean them up.
For the next version it would be nice to have an a11y review, by moving from XUL to xhtml we might have to fix some a11y details or add aria labels.

Not the easiest review, sorry if it's a bit long... Globally it look ok, it's a bit complex, but this is feeds code, that is horrible to begin with, so not easy to modify it in a clean way. I will need to have a second look at this :/

::: browser/base/content/browser-feeds.js
@@ +169,5 @@
>      }
>    },
>  
> +  setFeedCharPref: function(aPrefName, aPrefValue) {
> +    Services.prefs.setCharPref(aPrefName, aPrefValue);

what's the point of this indirection? it doesn't sound useful

@@ +186,5 @@
> +   * @param   file
> +   *          A nsIFile to look up the name of
> +   * @returns The display name of the application represented by the file.
> +   */
> +  _getFileDisplayName: function FW__getFileDisplayName(file) {

please use shorthand method names, and avoid having both a name and a label.

@@ +187,5 @@
> +   *          A nsIFile to look up the name of
> +   * @returns The display name of the application represented by the file.
> +   */
> +  _getFileDisplayName: function FW__getFileDisplayName(file) {
> +#ifdef XP_WIN

we are in the process of removing preprocessing from most browser files. This should use AppConstants instead of ifdef

@@ +204,5 @@
> +#endif
> +    return file.name;
> +  },
> +
> +  _selectedApp: null,

this looks unused

@@ +206,5 @@
> +  },
> +
> +  _selectedApp: null,
> +
> +  chooseClientApp: function(aTitle, aPrefName, aMessageManager) {

nit: it seems strange that this takes a messagemanager, rather than a browser or a window (and then use its messagemanager internally)

@@ +208,5 @@
> +  _selectedApp: null,
> +
> +  chooseClientApp: function(aTitle, aPrefName, aMessageManager) {
> +    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> +    let fpCallback = function fpCallback_done(aResult) {

please just put this inline in the fp.open() call as an arrow function.

@@ +210,5 @@
> +  chooseClientApp: function(aTitle, aPrefName, aMessageManager) {
> +    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> +    let fpCallback = function fpCallback_done(aResult) {
> +      if (aResult == Ci.nsIFilePicker.returnOK) {
> +        selectedApp = fp.file;

selectedapp is undeclared

@@ +221,5 @@
> +#else
> +#ifdef XP_MACOSX
> +#expand             if (fp.file.leafName != "__MOZ_MACBUNDLE_NAME__") {
> +#else
> +#expand             if (fp.file.leafName != "__MOZ_APP_NAME__-bin") {

I hope we can do these expansions with AppConstants... otherwise just let them there and we can manage them apart.

@@ +262,5 @@
> +    // https://foo.com/index.rdf -> feed:https://foo.com/index.rdf
> +    var ios =
> +        Cc["@mozilla.org/network/io-service;1"].
> +        getService(Ci.nsIIOService);
> +    var feedURI = ios.newURI(aSpec, null, null);

NetUtil.newURI or makeURI

@@ +268,5 @@
> +      feedURI.scheme = "feed";
> +      aSpec = feedURI.spec;
> +    }
> +    else
> +      aSpec = "feed:" + aSpec;

please brace

@@ +275,5 @@
> +    // notably systems where GNOME is not installed.
> +    try {
> +      var ss =
> +          Cc["@mozilla.org/browser/shell-service;1"].
> +          getService(Ci.nsIShellService);

no reason for a newline after = and generally please align on dots (and [)

@@ +282,5 @@
> +      // If we couldn't use the shell service, fallback to using a
> +      // nsIProcess instance
> +      var p =
> +          Cc["@mozilla.org/process/util;1"].
> +          createInstance(Ci.nsIProcess);

no reason for a newline here as well

@@ +309,5 @@
> +        let defaultClientApp = Cc["@mozilla.org/browser/shell-service;1"].
> +                               getService(Ci.nsIShellService).
> +                               defaultFeedReader;
> +
> +        if (selectedClientApp.exists()) {

nit: it may be nice to use OS.File here, since this is already async... using the nsIFile.path

@@ +318,5 @@
> +                                                         type: "DefaultAppMenuItem" });
> +          }
> +          msg.target.messageManager.sendAsyncMessage("FeedWriter:SetApplicationLauncherMenuItem",
> +                                                     { name: this._getFileDisplayName(selectedClientApp),
> +                                                       type: "SelectedAppMenuItem" });

these could get better indentation to stay closer to 80 chars limit...

::: browser/components/feeds/FeedConverter.js
@@ +377,5 @@
>      case "client":
> +      Services.cpmm.sendAsyncMessage("FeedConverter:ExecuteClientApp",
> +                                     { spec: spec,
> +                                       title: title,
> +                                       subtitle: subtitle,

nit: can use shorthand properties

@@ +385,5 @@
> +      // Default system feed reader
> +      Services.cpmm.sendAsyncMessage("FeedConverter:ExecuteClientApp",
> +                                     { spec: spec,
> +                                       title: title,
> +                                       subtitle: subtitle,

ditto

::: browser/components/feeds/FeedWriter.js
@@ +242,5 @@
>      return this._bundle.GetStringFromName(key);
>    },
>  
> +  /* Get the selected option in the select dropdown */
> +  _getSelectedOption: function FW__getSelectedOption(aList) {

please use shorthand method name

@@ +244,5 @@
>  
> +  /* Get the selected option in the select dropdown */
> +  _getSelectedOption: function FW__getSelectedOption(aList) {
> +    let options = aList.options;
> +    return options[aList.selectedIndex];

I think aList.selectedOptions[0] just works... that makes me wonder if this helper method is still useful at all

@@ +699,4 @@
>      }
>    },
>  
> +  _getWebHandlerElementsForURL: function FW__getWebHandlerElementsForURL(aURL) {

nit: shorthand

@@ +777,4 @@
>      menuItem.setAttribute("handlerType", "client");
> +
> +    this._mm.sendAsyncMessage("FeedWriter:RequestClientAppName",
> +                             { feedTypePref: getPrefAppForType(feedType) });

may this in any case call us back before we have created the menuitem? would it be safer to send the message after we have appended the menuitem?

@@ +813,5 @@
> +    var wccr = Cc["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"].
> +               getService(Ci.nsIWebContentConverterService);
> +    var handlers = wccr.getContentHandlers(this._getMimeTypeForFeedType(feedType));
> +    if (handlers.length != 0) {
> +      for (var i = 0; i < handlers.length; ++i) {

for (let handler of handlers) (and the if is not needed)

@@ +830,1 @@
>        }

nit: pointless newline

@@ +930,3 @@
>      this._window = window;
>      this._document = window.document;
>      this._document.getElementById("feedSubscribeLine").offsetTop;

uh, what's the scope of this, being assigned to nothing?
could you please check if we can remove it?

@@ +968,5 @@
> +            break;
> +          case "SelectedAppMenuItem":
> +            menuItem = this._selectedAppMenuItem;
> +            break;
> +        }

I don't like switch inside switch... what about a simple if/elseif

@@ +970,5 @@
> +            menuItem = this._selectedAppMenuItem;
> +            break;
> +        }
> +
> +        menuItem.textContent = msg.data.name;

based on the code, menuItem may be null

@@ +975,5 @@
> +        menuItem.style.display = "";
> +        menuItem.selected = true;
> +
> +        // Potentially a bit racy, but I don't think we can get into a state where this callback is set and
> +        // we're not coming back from ChooseClientApp in browser-feeds.js

A possible alternative (not sure if it would work) may be to pass a DOM promise in the message... provided that's possible at all.

@@ +1050,5 @@
> +      this._mm.sendAsyncMessage("FeedWriter:SetFeedCharPref",
> +                                { pref: aPrefName,
> +                                  value: aPrefValue });
> +  },
> +  setFeedComplexString: function FW_setFeedComplexString(aPrefName, aPrefValue) {

please add newline before method, and use shorthand

::: browser/components/feeds/WebContentConverter.js
@@ +898,4 @@
>  };
>  
>  function WebContentConverterRegistrarContent() {
> +  this._contentTypes = { };

nit: no space between braces (I guess we need to start thinking to eslint rules)

But actually, what about using a Map?

@@ +907,5 @@
> +   * Load the auto handler, content handler and protocol tables from 
> +   * preferences.
> +   */
> +  _init: function WCCRC__init() {
> +    var ps = Services.prefs;

nit: please use let

@@ +909,5 @@
> +   */
> +  _init: function WCCRC__init() {
> +    var ps = Services.prefs;
> +
> +    var kids = ps.getBranch(PREF_CONTENTHANDLERS_BRANCH)

nit: children is more common

@@ +920,5 @@
> +      if (!match)
> +        continue;
> +      else
> +        nums.push(match[1]);
> +    }

looks like this may just be rewritten using .map().filter() where map on non matching entries will insert an empty string and filter can just filter out empty strings.

@@ +923,5 @@
> +        nums.push(match[1]);
> +    }
> +
> +    // sort them, to get them back in order
> +    nums.sort(function(a, b) {return a - b;});

it is not the same as jut invoking sort() without arguments? could just be concatenated to the previous filter() I suggested...

@@ +926,5 @@
> +    // sort them, to get them back in order
> +    nums.sort(function(a, b) {return a - b;});
> +
> +    // now register them
> +    for (var i = 0; i < nums.length; i++) {

for...of

@@ +928,5 @@
> +
> +    // now register them
> +    for (var i = 0; i < nums.length; i++) {
> +      var branch = ps.getBranch(PREF_CONTENTHANDLERS_BRANCH + nums[i] + ".");
> +      this._registerContentHandlerWithBranch(branch);

s/With/Having/

@@ +934,5 @@
> +
> +    // We need to do this _after_ registering all of the available handlers, 
> +    // so that getWebContentHandlerByURI can return successfully.
> +    try {
> +      var autoBranch = ps.getBranch(PREF_CONTENTHANDLERS_AUTO);

this Try may end up covering actual code bugs, looks like it should only catch cases where autoBranch does not exist and thus it should be limited to that and an if should handle the following code.

@@ +937,5 @@
> +    try {
> +      var autoBranch = ps.getBranch(PREF_CONTENTHANDLERS_AUTO);
> +      var childPrefs = autoBranch.getChildList("");
> +      for (var i = 0; i < childPrefs.length; ++i) {
> +        var type = childPrefs[i];

for (let type of autoBranch.getChildList("")) {

@@ +941,5 @@
> +        var type = childPrefs[i];
> +        var uri = autoBranch.getCharPref(type);
> +        if (uri) {
> +          var handler = this.getWebContentHandlerByURI(type, uri);
> +          this._setAutoHandler(type, handler);

handler may be null

@@ +961,5 @@
> +      // This uri has already been registered
> +      if (services[i].uri == uri)
> +        return true;
> +    }
> +    return false;

all of this can be replaces with a
return this._contentTypes[contentType]
           .some(e => e.uri == uri);

@@ +963,5 @@
> +        return true;
> +    }
> +    return false;
> +  },
> +  _registerContentHandlerWithBranch: function(branch) {

add newline before the method

@@ +971,5 @@
> +     * 
> +     * How we deal with that is to check to see if there's no prefs in the 
> +     * branch and stop cycling once that's true.  This doesn't fix the case
> +     * where a user manually removes a reader, but that's not supported yet!
> +     */

please move this to a proper javadoc above the method.

@@ +985,5 @@
> +      this._updateContentTypeHandlerMap(type, uri, title);
> +    }
> +    catch(ex) {
> +      // do nothing, the next branch might have values
> +    }

Imo, the try/catch is something that should be done by the caller, not here, indeed the comment does not make much sense if not inside a loop.

@@ +989,5 @@
> +    }
> +  },
> +
> +  _updateContentTypeHandlerMap: 
> +  function WCCR__updateContentTypeHandlerMap(contentType, uri, title) {

please avoid naming functions twice, it's no more needed, this will suffice:
_updateContentTypeHandlerMap(contentType, uri, title) {

@@ +1004,5 @@
> +      var converterContractID = this._getConverterContractID(contentType);
> +      var cr = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> +      cr.registerFactory(WCC_CLASSID, WCC_CLASSNAME, converterContractID, 
> +                         WebContentConverterFactory);
> +    }

So, this method is called _updateContentTypeHandlerMap but it's doing the actual registration...
it would be cleaner if it would be named _addToContentTypeHandlerMap, raturn true if it was added, false otherwise, and if it returns true we do the registration in _registerContentHandlerHavingBranch

@@ +1011,5 @@
> +  /**
> +   * See nsIWebContentConverterService
> +   */
> +  getContentHandlers(contentType, countRef) {
> +    this._init();

may getContentHandlers be invoked multiple times and thus _init should rather be an ensureInitialized (checking a local _initialized property?). As it is now it looks like getContentHandlers will try to rebuild the map every time and just skip over due to duplicates.

@@ +1012,5 @@
> +   * See nsIWebContentConverterService
> +   */
> +  getContentHandlers(contentType, countRef) {
> +    this._init();
> +    countRef.value = 0;

countRef is optional, so you should null check it before assigning to .value

@@ +1024,5 @@
> +
> +  setAutoHandler(aContentType, aHandler) {
> +    Services.cpmm.sendAsyncMessage("WCCR:setAutoHandler",
> +                                   { contentType: aContentType,
> +                                     handler: aHandler });

nit: naming arguments as (contentType, handler) would allow to use the shorthand version { contentType, handler } here, and would be more consistent with the other arguments naming here

@@ +1028,5 @@
> +                                     handler: aHandler });
> +  },
> +
> +  getWebContentHandlerByURI(contentType, uri) {
> +    var handlers = this.getContentHandlers(contentType, { });

the count argument is optional.
This could just be
return this.getContentHandlers(contentType)
           .find(e => e.uri == uri) || null;

::: browser/components/feeds/content/subscribe.xhtml
@@ +37,5 @@
>            <p id="feedSubscriptionInfo1" />
>            <p id="feedSubscriptionInfo2" />
>          </div>
> +        <div id="feedSubscribeLine">
> +          <span id="subscribeUsingDescription"></span>

I guess we may try to be nicer and use <label>s

::: browser/components/feeds/nsIFeedResultService.idl
@@ +32,5 @@
>     *          The subtitle of the feed to add.
>     * @param   feedType
>     *          The nsIFeed type of the feed.  See nsIFeed.idl
> +   * @param   feedReader
> +   *          The type of feed reader we're using (client, bookmarks, default)

What about we make it optional, and if it's not provided it will default to the default feed reader. That will also preserve some compatibility.

::: browser/components/places/content/bookmarkProperties.js
@@ +437,4 @@
>  
>    onDialogAccept() {
>      // We must blur current focused element to save its changes correctly
> +    if (document.commandDispatcher.focusedElement) {

when is this undefined? I wonder if this was caused by bug 1208063?
It looks like hiding some bug...

::: browser/modules/Feeds.jsm
@@ +43,5 @@
>          break;
>        }
> +
> +      case "WCCR:setAutoHandler": {
> +        let data = aMessage.data;

since all cases are doing "let data = aMessage.data;" what about moving it to the method scope?

@@ +52,5 @@
> +      }
> +
> +      case "FeedConverter:addLiveBookmark": {
> +        let data = aMessage.data;
> +        let topWindow = Services.wm.getMostRecentWindow("navigator:browser");

Please use Recentwindow.getMostRecentBrowserWindow() instead

::: browser/themes/osx/feeds/subscribe-ui.css
@@ -6,5 @@
>    padding: 3px;
>  }
>  
> -.chooseApplicationMenuItem {
> -  list-style-image: url("chrome://browser/skin/preferences/application.png");

why did you remove this rule from OSX but not from Windows?
http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/feeds/subscribe-ui.css#23

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 40 • 9 years ago

OK, so I've addressed most of your feedback. 

I've mostly left out the larger refactorings you've asked for, mainly because I think that such refactorings are outside the scope of this bug. The code is the existing stuff which I didn't write or understand particularly well, so I'm loathe to go too deeply into changing it.

Regarding the bookmarkProperties change, I thought it was because the focusedElement is in the other process, so it's null when we check it there. I'll run a check to see if it was due to bug 1208063.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 41 • 9 years ago

Yes, the focusedElement hunk is no longer needed, so you can disregard that from the latest patch :)

	Marco Bonardo [:mak]
	Comment 42 • 9 years ago

Comment on attachment 8695076 [details] [diff] [review]
0001-Make-the-feed-subscriber-UI-work-in-e10s.patch

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

Sorry for the many coding style comments in the previous review, we are moving towards an eslint world and soon we won't be permissive anymore about coding style.
I see the timeline here, so let's land this, then we can cleanup in follow-ups.

I'm going to do some testing before the final r+, in the meanwhile there's still some things to check.

::: browser/base/content/browser-feeds.js
@@ +180,5 @@
> +   * Get the human-readable display name of a file. This could be the
> +   * application name.
> +   * @param   file
> +   *          A nsIFile to look up the name of
> +   * @returns The display name of the application represented by the file.

@return (no s)

@@ +199,5 @@
> +          } catch (e) {}
> +        }
> +        break;
> +      default:
> +        return file.leafName;

let's move this return to the main function body and remove default, otherwise if one of the 2 case throws an exception we end up returning undefined...
Even if this was in the original code, it's clearly a bug we should not copy.

@@ +220,5 @@
> +          // XXXmano TBD: can probably add this to nsIShellService
> +          let appName = "";
> +          switch (AppConstants.platform) {
> +            case "win":
> +              appName = "__MOZ_APP_NAME__.exe";

Did you check that this works?
I'm asking cause before this was using #expand and I don't know if __MOZ_APP_NAME__ (and __MOZ_MACBUNDLE_NAME__) are properly replaced by the preprocessor without that. (I think not)

here you could use AppConstants.MOZ_APP_NAME

@@ +223,5 @@
> +            case "win":
> +              appName = "__MOZ_APP_NAME__.exe";
> +              break;
> +            case "macosx":
> +              appName = "__MOZ_MACBUNDLE_NAME__";

you should add this to AppConstants and add it to http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/moz.build#103
Provided __MOZ_MACBUNDLE_NAME__ is still different from __MOZ_APP_NAME__ (I assume the latter points to the executable, while the former to the app bundle, but things could have changed in years)

Then try to dump from here and see if it works as expected.

@@ +226,5 @@
> +            case "macosx":
> +              appName = "__MOZ_MACBUNDLE_NAME__";
> +              break;
> +            default:
> +              appName = "__MOZ_APP_NAME__-bin";

AppConstants.MOZ_APP_NAME

@@ +250,5 @@
> +    let clientApp = null;
> +    if (aFeedHandler == "default") {
> +      clientApp = Cc["@mozilla.org/browser/shell-service;1"]
> +                    .getService(Ci.nsIShellService).
> +                  defaultFeedReader;

nit: fancy indentation

::: browser/components/feeds/FeedWriter.js
@@ +644,2 @@
>      if (checkbox) {
> +      if (this._handlersList) {

nit: these 2 ifs could be merged

::: browser/components/feeds/WebContentConverter.js
@@ +903,5 @@
>  
>  WebContentConverterRegistrarContent.prototype = {
> +
> +  /**
> +   * Load the auto handler, content handler and protocol tables from 

trailing space

@@ +906,5 @@
> +  /**
> +   * Load the auto handler, content handler and protocol tables from 
> +   * preferences.
> +   */
> +  _init: function WCCRC__init() {

use shorthands in newly added code please

@@ +919,5 @@
> +      if (!match) {
> +        return "";
> +      } else {
> +        return match[1];
> +      }

return match ? match[1] : "";

@@ +925,5 @@
> +      if (child == "") {
> +        return false;
> +      }
> +
> +      return true;

filter(child => !!child)

@@ +926,5 @@
> +        return false;
> +      }
> +
> +      return true;
> +    }).sort();

So in the end this is just:

let nums = children.map(child => {
    let match = /^(\d+)\.uri$/.exec(child);
    return match ? match[1] : "";
  }).filter(child => !!child)
    .sort();

@@ +938,5 @@
> +        // do nothing, the next branch might have values
> +      }
> +    }
> +
> +    // We need to do this _after_ registering all of the available handlers, 

trailing space

@@ +941,5 @@
> +
> +    // We need to do this _after_ registering all of the available handlers, 
> +    // so that getWebContentHandlerByURI can return successfully.
> +    try {
> +      var autoBranch = ps.getBranch(PREF_CONTENTHANDLERS_AUTO);

I'd prefer (since it's less error-prone):
let autoBranch;
try {
  autoBranch = ...

@@ +969,5 @@
> +  /**
> +   * Since we support up to six predefined readers, we need to handle gaps 
> +   * better, since the first branch with user-added values will be .6
> +   * 
> +   * How we deal with that is to check to see if there's no prefs in the 

various trailing spaces

@@ +978,5 @@
> +   *          The pref branch to register the content handler under
> +   *
> +   */
> +  _registerContentHandlerHavingBranch: function(branch) {
> +    var vals = branch.getChildList("");

nit: please use let in newly added methods

@@ +998,5 @@
> +    if (this._typeIsRegistered(contentType, uri)) 
> +      return;
> +    
> +    this._contentTypes[contentType].push(new ServiceInfo(contentType, uri, title));
> +    

various trailing  spaces

@@ +1019,5 @@
> +    }
> +
> +    if (!(contentType in this._contentTypes))
> +      return [];
> +    

trailing space
(and very-nit: I feel like there are far too many newlines in this simple method, causing too sparse code)

::: browser/components/feeds/nsIFeedResultService.idl
@@ +38,5 @@
>    void addToClientReader(in AUTF8String uri, 
>                           in AString title,
>                           in AString subtitle,
> +                         in unsigned long feedType,
> +                         in AString feedReader);

why didn't you make feedReader optional (and default to "default"?)

::: browser/components/places/content/bookmarkProperties.js
@@ +438,5 @@
>    onDialogAccept() {
>      // We must blur current focused element to save its changes correctly
> +    if (document.commandDispatcher.focusedElement) {
> +      document.commandDispatcher.focusedElement.blur();
> +    }

as previously said, remember to remove this workaround.

	Marco Bonardo [:mak]
	Comment 43 • 9 years ago

> browser/components/feeds/content/subscribe.xhtml
>           <p id="feedSubscriptionInfo1" />
>           <p id="feedSubscriptionInfo2" />
>         </div>
>         <div id="feedSubscribeLine">
>           <span id="subscribeUsingDescription"></span>
	
this should also be a label, for the select

>             <option disabled="true">&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;</option>
>           </select>
>           <br />
>           <label id="checkboxText" for="alwaysUse"></label>
>           <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
	
the label should be after the checkbox, note you can put the input directly inside the label at the right position and avoid the "for" attribute, if you wish

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 44 • 9 years ago

OK, I've addressed your latest round of review comments. To answer your previous question about init() being called every time, this was a deliberate decision as I figured that adding caching would be adding complexity without much benefit as I can't see the performance hit being something that we should be too worried about.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 45 • 9 years ago

Comment on attachment 8697339 [details] [diff] [review]
0001-Bug-1109714-Make-the-feed-subscriber-UI-work-in-e10s.patch

It looks like :mak is on PTO, so I'm bumping the review to blake

	Jared Wein [:jaws] (please needinfo? me)
	Comment 46 • 9 years ago

Since gw280 is out now I'll try to finish up the patch based on mak's review comments as well as any from mrbkap.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 47 • 9 years ago

Comment on attachment 8697339 [details] [diff] [review]
0001-Bug-1109714-Make-the-feed-subscriber-UI-work-in-e10s.patch

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

r+ for everything that we didn't talk about on IRC the other day. I'll look at that as a patch on top of this, if possible.

::: browser/components/feeds/FeedWriter.js
@@ +787,3 @@
>  
> +    this._mm.sendAsyncMessage("FeedWriter:RequestClientAppName",
> +                             { feedTypePref: getPrefAppForType(feedType) });

Nit: the { is one space underindented.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 48 • 9 years ago

Comment on attachment 8697339 [details] [diff] [review]
0001-Bug-1109714-Make-the-feed-subscriber-UI-work-in-e10s.patch

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

::: browser/components/feeds/content/subscribe.xhtml
@@ +40,5 @@
> +        <div id="feedSubscribeLine">
> +          <label id="subscribeUsingDescription" for="handlersMenuList"></label>
> +          <select id="handlersMenuList">
> +            <option id="liveBookmarksMenuItem" selected="true">&feedLiveBookmarks;</option>
> +            <option disabled="true">&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;</option>

I don't see these characters being moved here from some other place in this patch. The character you are using here is the "Box Drawings Heavy Horizontal", which seems odd to use here, as it won't make sense to screen readers and I guess is used to show a "missing or other" option?

We should either this option blank or have a localizable string for this option.

@@ +42,5 @@
> +          <select id="handlersMenuList">
> +            <option id="liveBookmarksMenuItem" selected="true">&feedLiveBookmarks;</option>
> +            <option disabled="true">&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;&#x2501;</option>
> +          </select>
> +          <br />

We shouldn't use line breaks in the markup for styling purposes. Can you make #handlersMenuList display:block; in the CSS?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 49 • 9 years ago

Comment on attachment 8697339 [details] [diff] [review]
0001-Bug-1109714-Make-the-feed-subscriber-UI-work-in-e10s.patch

(restoring mrbkap's r+, not mine. sorry for the confusion)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 50 • 9 years ago

I looked in logs for #developers and #e10s and I don't see this conversation. Is it possible to copy/paste a log of that conversation in to this bug?

	Marco Bonardo [:mak]
	Comment 52 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #48)
> I don't see these characters being moved here from some other place in this
> patch. The character you are using here is the "Box Drawings Heavy
> Horizontal", which seems odd to use here, as it won't make sense to screen
> readers and I guess is used to show a "missing or other" option?

I actually thought it was used to separate local handlers from web handlers? I'm not sure what we should do here, I also found it a little bit odd, but didn't want to block on details. Maybe we should just remove it.
Unfortunately I couldn't find the time to apply the patch and actually try it to figure that out.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 53 • 9 years ago

Mostly works now, but there are a few refactorings that will be done still and the UI is missing enough spacing between elements.

I'll see about using optgroups in the select list instead of the awkward disabled option that the patch is using.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 54 • 9 years ago

Those were a hack to give horizontal line separators in the select dropdown. We can't really do it otherwise (we have lineseparator elements in XUL menulists). Mike Conley and I discussed it in the office and we came to the conclusion of "this is terrible, but it probably doesn't matter all that much in the long run".

As for the conversation between me and Blake on IRC, it was done in private. I can provide that patch, and I will make myself available tomorrow to get this sorted if you want.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 55 • 9 years ago

To clarify, I did look into using the optgroup but it didn't really provide the same sort of visual as the lineseparator element did (and I was trying to preserve existing UX as much as possible). I'm happy to go with whatever you suggest though, if you think optgroups are better than the separator hack.

	Marco Bonardo [:mak]
	Comment 56 • 9 years ago

Fwiw, I think we could just remove the separator. This ui is unlikely to be used by newbies, especially since one needs to add the RSS button to the toolbar first (while time ago it was exposed in the bookmarks menu and in the urlbar). Likely for the current target users it won't be a problem.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 57 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 58 • 9 years ago

Try push with my latest patch, https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcba4845dec2

I left in the separators that gw280 and mconley made since without them it looks awkward to have "Choose Application..." between the other options.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 59 • 9 years ago

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

	Brad Lassey [:blassey] (use needinfo?)
	Comment 60 • 9 years ago

Jared, what's the status here? do you need another review? ready to land?

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 61 • 9 years ago

It looks like just the one test failure now - test_bug589543.html?

This should be trivial to fix up; we renamed handlersMenuPopup to handlersMenuList so we should just getElementById() for that id instead.

I don't think this would require another r+, it's small enough to just carry imho.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 62 • 8 years ago

There is also a bunch of memory leaks showing up in those try pushes that I have been looking in to. Yes, that one test failure is easy to fix and I have that fixed locally, I've just been looking at the memory leaks.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 63 • 8 years ago

still need to push it to try server

	Jared Wein [:jaws] (please needinfo? me)
	Comment 64 • 8 years ago

George said he will take it over again and push it to tryserver himself.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 65 • 8 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 66 • 8 years ago

I think I've found the leak, updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=505d6c9b23ff

	Blake Kaplan (:mrbkap) (inactive)
	Comment 67 • 8 years ago

I didn't mean for my comments on IRC to be a secret, sorry about that. I got lazy when typing my review comments. I worried to George that the very generic "set a pref" API could be abused in order to work around the sandbox if a content process got compromised. I originally thought that it could be used to directly run an application but that's mitigated by the fact that the original patch didn't set the right kind of pref. That being said, I'm much happier that the new messageManager API only allows the content process to narrowly set just feedreader prefs (and nothing that could lead to running an application).

	Pulsebot
	Comment 68 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/120a0841a9cc

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 69 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/120a0841a9cc

	Alfred Kayser
	Comment 70 • 8 years ago

This is wrong:
<label id="checkboxText">
  <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
</label>

This should be:
<input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
<label id="checkboxText"></label>

The label is to follow the input not contain it.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 71 • 8 years ago

I did it like that based on mak's comment 43

	Jared Wein [:jaws] (please needinfo? me)
	Comment 72 • 8 years ago

(In reply to Alfred Kayser from comment #70)
> This is wrong:
> <label id="checkboxText">
>   <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> </label>
> 
> This should be:
> <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> <label id="checkboxText"></label>
> 
> The label is to follow the input not contain it.

Can you explain why you think this is wrong? The text of the label is added by setting the textContent of the last textNode child of checkboxText. See https://hg.mozilla.org/mozilla-central/rev/120a0841a9cc#l4.785

	Marco Bonardo [:mak]
	Comment 73 • 8 years ago

a label can contain its input, the only difference is wheter we add text before or after the input... that's mostly a ui decision for which we should probably be consistent with in-content ui.

	Alfred Kayser
	Comment 75 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #72)
> (In reply to Alfred Kayser from comment #70)
> > This is wrong:
> > <label id="checkboxText">
> >   <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> > </label>
> > 
> > This should be:
> > <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> > <label id="checkboxText"></label>
> > 
> > The label is to follow the input not contain it.
> 
> Can you explain why you think this is wrong? The text of the label is added
> by setting the textContent of the last textNode child of checkboxText. See
> https://hg.mozilla.org/mozilla-central/rev/120a0841a9cc#l4.785

When a theme wants to style an input, it needs to have the label following the input.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 76 • 8 years ago

(In reply to Alfred Kayser from comment #75)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #72)
> > (In reply to Alfred Kayser from comment #70)
> > > This is wrong:
> > > <label id="checkboxText">
> > >   <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> > > </label>
> > > 
> > > This should be:
> > > <input type="checkbox" id="alwaysUse" class="alwaysUse" checked="false"/>
> > > <label id="checkboxText"></label>
> > > 
> > > The label is to follow the input not contain it.
> > 
> > Can you explain why you think this is wrong? The text of the label is added
> > by setting the textContent of the last textNode child of checkboxText. See
> > https://hg.mozilla.org/mozilla-central/rev/120a0841a9cc#l4.785
> 
> When a theme wants to style an input, it needs to have the label following
> the input.

The input is still style-able just like it was before. Are you saying that theme authors may have expected labels to *always* follow inputs and that those assumptions will break some themes? Do you know of a theme that is broken by this?

	Alfred Kayser
	Comment 77 • 8 years ago

At least my themes are.
This is because in order to be able to style an input (override the ugly system standard), one has to hide the input itself, and style the label with the nicer checkbox image/styling. 
input{display:none}
input+label::before{background-image(...);...}
etc..

But possibly someone can tell me how to do this for inputs inside a label?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 78 • 8 years ago

I see what you are saying now. I was able to style the text of the label as I pleased, hide the input, and place some generated content before the label, but I'm not able to propagate the :checked psuedo-class to the label so the checkbox will not appear "checked" without some extra JS.

Gijs, do you think this is something we should refactor the code here to support heavyweight themes better? Personally, I don't think we should, as our current direction sounds like we will want to limit what heavyweight themes can do (within reason), and styling the feed-reader page probably isn't super high on the list of requests.

	:Gijs (he/him)
	Comment 79 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #78)
> I see what you are saying now. I was able to style the text of the label as
> I pleased, hide the input, and place some generated content before the
> label, but I'm not able to propagate the :checked psuedo-class to the label
> so the checkbox will not appear "checked" without some extra JS.
> 
> Gijs, do you think this is something we should refactor the code here to
> support heavyweight themes better? Personally, I don't think we should, as
> our current direction sounds like we will want to limit what heavyweight
> themes can do (within reason), and styling the feed-reader page probably
> isn't super high on the list of requests.

I mean, tbh, the following:

<label><input> Label</label>

is one of exactly two ways to correctly use a label with an input, the other being:

<input id="whatever">
and then, wherever in the markup,

<label for="whatever"/>

There is no requirement to have the label immediately after the input, and if anything that would make a11y harder to do because all the inputs would need IDs and the labels would all need "for" attributes.

I'm not really sure why a claim is being made that a complete theme needs to be able to hide the checkbox here - the in-content prefs use custom styling for checkboxes and radio buttons, and they don't use the strategy described by Alfred, either.

I don't think we should muddle up the markup here, especially not because it is a pretty small, isolated point of the UI anyway.

	Shane Caraveo (:mixedpuppy)
	Comment 81 • 8 years ago

[Tracking Requested - why for this release]:
The rss dropdown is still broken in 45, this should be uplifted to fix breakage unless 45 beta will not run e10s.  My understanding is that e10s will be run for some portion of beta users.

	Sylvestre Ledru [:Sylvestre]
	Comment 82 • 8 years ago

George, could you fill the uplift request to aurora? Thanks

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 83 • 8 years ago

Don't think I ever actually put the final patch on bugzilla. This is the one that landed on m-i, carrying r+ from previous patch.

--

Approval Request Comment
[Feature/regressing bug #]: bug 1109714
[User impact if declined]: feed subscriber non-functional in e10s
[Describe test coverage new/current, TreeHerder]: been on nightly for 2 weeks now. full test coverage from m-c.
[Risks and why]: new UI also used for non-e10s. Possible undiscovered regressions there.
[String/UUID change made/needed]: UUID change made to nsIFeedResultService.idl. No string changes, just refactoring where the strings were preserved.

	Sylvestre Ledru [:Sylvestre]
	Comment 84 • 8 years ago

Comment on attachment 8709497 [details] [diff] [review]
feedreader-final.patch

OK, even if I am surprised by the size of the patch, let's try it. I guess it cannot be worst for e10s users.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 85 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/746c9a0348f6

	Mayur Patil
	Comment 86 • 8 years ago

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: 
To test, whether a bookmark button is on BM toolbar.
Icon of Live Bookmarks.


Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Able to subscribe through live bookmark.
As others commented, Live Bookmark supported. So this has also verified.