1215694 - Move Pocket to a built-in add-on

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Shane Caraveo (:mixedpuppy)]

We're moving pocket to a built-in addon.  This will facilitate user choice (I rip the pockets off everything I own), alternate firefox distributions that do not want to include the feature, etc.  As well it will help to identify any potential issues with using add-ons for feature implementation.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1: move pocket files into a feature add-on directory

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2: remove most pocket code

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon, make it work

Comment 4

[Shane Caraveo (:mixedpuppy)]

The above patches are for early feedback.  Some issues and remaining items (aside from general cleanup etc etc):

- add-on signing (fx thinks we've sideloaded)
- reader mode support
- ui-tour support
- toolbar images are not moved
- telemetry integration

due to some of the above, there are still smatterings of pocket code in toolkit and browser but the bulk of pocket code is moved into the addon.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8675142 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

looking for early feedback

Comment 7

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

Comment on attachment 8675142 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

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

::: browser/features/[e-mail]/bootstrap.js
@@ +5,5 @@
> +// the default Firefox behaviour.
> +//
> +// This addon is used in specific partner builds where two default preferences,
> +// extensions.toshibadefaults.checktoshibadefaults and extensions.toshibadefaults.firstRun,
> +// would be added via the distribution.ini file for the repack.

What are these two paragraphs referencing? The first one is talking about delaying the default browser prompt, and the second one is about Toshiba preferences. Are these just carryover from a different add-on?

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment on attachment 8675142 [details] [diff] [review]

> What are these two paragraphs referencing? The first one is talking about

oops.  I grabbed an earlier experiment because it has some boilerplate I wanted.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8675142 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

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

This seems pretty rough still. There's still a lot of debugging code, trailing whitespace, "XXX todo" comments and such, but I gave it a run.

The main issues seem to be the prefs (which will require more care now), the name of the package, and some of the architecture / cleanup.

::: browser/components/uitour/test/browser_UITour_availableTargets.js
@@ +9,5 @@
>  
>  var hasWebIDE = Services.prefs.getBoolPref("devtools.webide.widget.enabled");
>  
>  var hasPocket = false;
> +if (Services.prefs.getBoolPref("extensions.pocket.enabled")) {

This (and everything like it) is going to throw if the pref doesn't exist, which would be the case if the prefs were part of the add-on and the add-on was not installed or disabled.

::: browser/features/[e-mail]/bootstrap.js
@@ +41,5 @@
> +  enabledLocales: "cs de en-GB en-US en-ZA es-ES es-MX fr hu it ja ja-JP-mac ko nl pl pt-BR pt-PT ru zh-CN zh-TW",
> +};
> +
> +function setDefaultPrefs() {
> +  let branch = Services.prefs.getDefaultBranch(PREF_BRANCH);

Gahhhh. Why haven't we made the default prefs.js file work for bootstrapped add-ons? I'd rather fix that. :-\

@@ +101,5 @@
> +  };
> +}
> +
> +function setPocketWindow(window) {
> +  Cu.reportError("setPocketWindow "+window);

Debugging code?

@@ +104,5 @@
> +function setPocketWindow(window) {
> +  Cu.reportError("setPocketWindow "+window);
> +  try {
> +  XPCOMUtils.defineLazyModuleGetter(window, "Pocket",
> +                                    "chrome://getpocket/content/Pocket.jsm");

Seems like "pocket" rather than "getpocket" would be a more sensible name for this.

@@ +120,5 @@
> +    enumerable: true
> +  });
> +
> +  Object.defineProperty(window, "pktUI", pktUIGetter("pktUI", window));
> +  Object.defineProperty(window, "pktUIMessaging", pktUIGetter("pktUIMessaging", window));

These are never removed when the add-on is uninstalled, and you're loading the main.js file several times because of the helper. Both of those seem wrong.

@@ +139,5 @@
> +}
> +
> +
> +
> +let AboutSaved = {

FWIW, I think a lot of this would be simpler if these weren't about: pages. I don't really see any need for them to be about: pages at all...

@@ +230,5 @@
> +    onViewHiding: function() {
> +      return Pocket.onPanelViewHiding.apply(this, arguments);
> +    },
> +    onBeforeCreated: function(doc) {
> +      // XXX wondering why I have to create a view, seems like createwidget should do that for me.

If you have ideas about how that API would look - the XUL is different for each panel, so how would you create it "for" the add-on - then please file a bug.

@@ +256,5 @@
> +  if (reason == ADDON_ENABLE) {
> +    // place the button
> +    // XXX this should let us define by id not an integer position, need to
> +    // properly calculate the child position
> +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR, 3 /*"bookmarks-menu-button"*/);

TBH I see no reason why this can't just append to the end. That's what other add-ons do...

@@ +295,5 @@
> +      let d = win.document;
> +      for (let eid in ["context-pocket", "context-savelinktopocket"]) {
> +        let e = d.getElementById(eid);
> +        if (e)
> +          e.parentNode.removeChild(e);

e.remove()

@@ +312,5 @@
> +                                        subject.isContentSelected || subject.onImage ||
> +                                        subject.onCanvas || subject.onVideo || subject.onAudio);
> +    let targetUrl = subject.onLink ? subject.linkUrl : subject.pageUrl;
> +    let targetURI = Services.io.newURI(targetUrl, null, null);
> +    let canPocket = CustomizableUI.getPlacementOfWidget("pocket-button") &&

Seems like if the pocket button isn't placed right now, we shouldn't even be listening for this notification.

@@ +356,5 @@
> +}
> +
> +function createElementWithAttrs(d, type, attrs) {
> +  let e = d.createElement(type);
> +  for (let a in attrs) {

Use for of, please.

@@ +459,5 @@
> +    CustomizableUI.removeListener(this);
> +    eachBrowserWindow(function(window) {
> +      let d = window.document;
> +      for (let prefix of ["panelMenu_", "menu_", "BMB_"]) {
> +        let e = d.getElementById(prefix + "pocket");

Could just have "pocket" in the names you're iterating over...

@@ +462,5 @@
> +      for (let prefix of ["panelMenu_", "menu_", "BMB_"]) {
> +        let e = d.getElementById(prefix + "pocket");
> +        if (e) {
> +          e.parentNode.removeChild(e);
> +          let s = d.getElementById(prefix + "pocketSeparator");

... and add separator here

::: browser/features/[e-mail]/jar.mn
@@ +11,5 @@
> +  pktApi.js  (pocket/pktApi.js)
> +  locale/en-US/pocket.properties       (locale/en-US/pocket.properties)
> +* themes/osx/pocket.css (themes/osx/pocket.css)
> +* themes/linux/pocket.css (themes/linux/pocket.css)
> +* themes/windows/pocket.css (themes/windows/pocket.css)
\ No newline at end of file

These look like they should be in the skin directory?

::: browser/features/[e-mail]/themes/shared/pocket.css
@@ +1,3 @@
> +/* Bug 1164419 - increase Pocket panel size to accomidate wider Russian text. */
> +panelmultiview[mainViewId=PanelUI-pocketView] > .panel-viewcontainer > .panel-viewstack > .panel-mainview:not([panelid="PanelUI-popup"]) {
> +  max-width: 33em; /* standaloneSubviewWidth + 3 */

File a bug to make this a CSS variable, please.

@@ +9,5 @@
> +}
> +
> +#PanelUI-pocketView > .panel-subview-body,
> +#PanelUI-pocketView {
> +  overflow: visible;

Is this actually necessary?

@@ +13,5 @@
> +  overflow: visible;
> +}
> +
> +#pocket-button {
> +  list-style-image: url("chrome://browser/skin/Toolbar.png");

It kind of feels like we should move the image out into its own file as well, but I don't know if that's going to be worth the effort.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8675142 [details] [diff] [review]
> part 3: move code/functionality from part 2 into the addon, make it work
> 
> Review of attachment 8675142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems pretty rough still. There's still a lot of debugging code,
> trailing whitespace, "XXX todo" comments and such, but I gave it a run.

yes, I did specify early feedback.  I'm looking first for problems with/objections to any direction being taken here.  The detailed feedback is awesome.

> 
> The main issues seem to be the prefs (which will require more care now), the
> name of the package, and some of the architecture / cleanup.
> 
> ::: browser/components/uitour/test/browser_UITour_availableTargets.js

That will need to be removed, the goal is that there will be no pocket code outside of the addon.  I haven't looked into how to add to the uitour from a system addon yet.

> ::: browser/features/[e-mail]/bootstrap.js
> @@ +41,5 @@
> > +  enabledLocales: "cs de en-GB en-US en-ZA es-ES es-MX fr hu it ja ja-JP-mac ko nl pl pt-BR pt-PT ru zh-CN zh-TW",
> > +};
> > +
> > +function setDefaultPrefs() {
> > +  let branch = Services.prefs.getDefaultBranch(PREF_BRANCH);
> 
> Gahhhh. Why haven't we made the default prefs.js file work for bootstrapped
> add-ons? I'd rather fix that. :-\

Bug 564675.  Seems like the code that calls the boostrap startup/shutdown could also look for a default prefs file and load/unload those prefs.

> > +let AboutSaved = {
> 
> FWIW, I think a lot of this would be simpler if these weren't about: pages.
> I don't really see any need for them to be about: pages at all...

Yeah, that could simplify a number of things here.

> @@ +230,5 @@
> > +    onViewHiding: function() {
> > +      return Pocket.onPanelViewHiding.apply(this, arguments);
> > +    },
> > +    onBeforeCreated: function(doc) {
> > +      // XXX wondering why I have to create a view, seems like createwidget should do that for me.
> 
> If you have ideas about how that API would look - the XUL is different for
> each panel, so how would you create it "for" the add-on - then please file a
> bug.

I haven't thought it through, but it seems the creation of an empty panelview node would be enough, pass it into onbeforecreated.  That would ensure addons have a node in which they can "do whatever" without worry of changes further up the chain.

> @@ +256,5 @@
> > +  if (reason == ADDON_ENABLE) {
> > +    // place the button
> > +    // XXX this should let us define by id not an integer position, need to
> > +    // properly calculate the child position
> > +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR, 3 /*"bookmarks-menu-button"*/);
> 
> TBH I see no reason why this can't just append to the end. That's what other
> add-ons do...

At present I'm trying to fully replicate the existing pocket code, which does explicitly place the pocket button just to the right of the bookmarks button.  Using addons for feature implementation may still require an ability to be explicit about how we want things to appear.


> @@ +312,5 @@
> > +                                        subject.isContentSelected || subject.onImage ||
> > +                                        subject.onCanvas || subject.onVideo || subject.onAudio);
> > +    let targetUrl = subject.onLink ? subject.linkUrl : subject.pageUrl;
> > +    let targetURI = Services.io.newURI(targetUrl, null, null);
> > +    let canPocket = CustomizableUI.getPlacementOfWidget("pocket-button") &&
> 
> Seems like if the pocket button isn't placed right now, we shouldn't even be
> listening for this notification.

it might be a bit more complicated than that, but worth thinking about.

> > +#pocket-button {
> > +  list-style-image: url("chrome://browser/skin/Toolbar.png");
> 
> It kind of feels like we should move the image out into its own file as
> well, but I don't know if that's going to be worth the effort.

Yeah, that is in the todo, but is not important at this stage of feedback.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8675938 [details] [diff] [review]
part 4: make reader extendable, move reader support to addon

Looking for feedback on the approach of using message manager to allow an addon (e.g. pocket) to add a button to the reader toolbar.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #9)

> ::: browser/features/[e-mail]/jar.mn
> @@ +11,5 @@
> > +  pktApi.js  (pocket/pktApi.js)
> > +  locale/en-US/pocket.properties       (locale/en-US/pocket.properties)
> > +* themes/osx/pocket.css (themes/osx/pocket.css)
> > +* themes/linux/pocket.css (themes/linux/pocket.css)
> > +* themes/windows/pocket.css (themes/windows/pocket.css)
> \ No newline at end of file
> 
> These look like they should be in the skin directory?

I've been thinking about this.  With locale, there is a variable substitution that can happen.  Not so with css/platform (that I can find).  We don't want "skin" in the jar since this is not a full theme (yeah, the "theme" directory should probably change to "style").  Since the addon may need to be signed, we need to include files for all platforms.  I don't see any easy way to automatically pick up the platform dir, it would probably have to be handled run-time, am I wrong?

Comment 13

[Dave Townsend [:mossop]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > ::: browser/features/[e-mail]/bootstrap.js
> > @@ +41,5 @@
> > > +  enabledLocales: "cs de en-GB en-US en-ZA es-ES es-MX fr hu it ja ja-JP-mac ko nl pl pt-BR pt-PT ru zh-CN zh-TW",
> > > +};
> > > +
> > > +function setDefaultPrefs() {
> > > +  let branch = Services.prefs.getDefaultBranch(PREF_BRANCH);
> > 
> > Gahhhh. Why haven't we made the default prefs.js file work for bootstrapped
> > add-ons? I'd rather fix that. :-\
> 
> Bug 564675.  Seems like the code that calls the boostrap startup/shutdown
> could also look for a default prefs file and load/unload those prefs.

It's a little more complicated than that, we'd need to support reloading default prefs at runtime which is a nasty kettle of fish. I wouldn't recommend blocking this bug on that.

Comment 14

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> (In reply to :Gijs Kruitbosch from comment #9)
> 
> > ::: browser/features/[e-mail]/jar.mn
> > @@ +11,5 @@
> > > +  pktApi.js  (pocket/pktApi.js)
> > > +  locale/en-US/pocket.properties       (locale/en-US/pocket.properties)
> > > +* themes/osx/pocket.css (themes/osx/pocket.css)
> > > +* themes/linux/pocket.css (themes/linux/pocket.css)
> > > +* themes/windows/pocket.css (themes/windows/pocket.css)
> > \ No newline at end of file
> > 
> > These look like they should be in the skin directory?
> 
> I've been thinking about this.  With locale, there is a variable
> substitution that can happen.  Not so with css/platform (that I can find). 
> We don't want "skin" in the jar since this is not a full theme (yeah, the
> "theme" directory should probably change to "style").

I don't understand. Skin packages are not limited to themes. Add-ons normally include things in a skin directory that styles them correctly for the default theme (classic/1.0). In some cases add-ons could choose to also include skin information for alternative themes ('modern' on SeaMonkey, or third-party ones). Likewise, third-party themes sometimes add skin packages for popular add-ons, so they don't look out of place.

It would make perfect sense for this add-on to have a skin package which styled its toolbar button and other XUL elements.

>  Since the addon may
> need to be signed, we need to include files for all platforms.  I don't see
> any easy way to automatically pick up the platform dir, it would probably
> have to be handled run-time, am I wrong?

There is an "os" flag you can use: https://developer.mozilla.org/en/docs/Chrome_Registration#os .

Comment 15

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > @@ +230,5 @@
> > > +    onViewHiding: function() {
> > > +      return Pocket.onPanelViewHiding.apply(this, arguments);
> > > +    },
> > > +    onBeforeCreated: function(doc) {
> > > +      // XXX wondering why I have to create a view, seems like createwidget should do that for me.
> > 
> > If you have ideas about how that API would look - the XUL is different for
> > each panel, so how would you create it "for" the add-on - then please file a
> > bug.
> 
> I haven't thought it through, but it seems the creation of an empty
> panelview node would be enough, pass it into onbeforecreated.  That would
> ensure addons have a node in which they can "do whatever" without worry of
> changes further up the chain.

OK. That could certainly be implemented. It might be future-compat-safer, and cleaner, if we made it part of a different function - we could actually let people create the panelview lazily... Could you file a dep bug on that? I can promise reasonably fast reviews. :-)

> > @@ +256,5 @@
> > > +  if (reason == ADDON_ENABLE) {
> > > +    // place the button
> > > +    // XXX this should let us define by id not an integer position, need to
> > > +    // properly calculate the child position
> > > +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR, 3 /*"bookmarks-menu-button"*/);
> > 
> > TBH I see no reason why this can't just append to the end. That's what other
> > add-ons do...
> 
> At present I'm trying to fully replicate the existing pocket code, which
> does explicitly place the pocket button just to the right of the bookmarks
> button.  Using addons for feature implementation may still require an
> ability to be explicit about how we want things to appear.

OK. In that case,  CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR) gets you an array of items where you can find the index of the bookmarks-menu-button, and insert afterwards.

Comment 16

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

Comment on attachment 8675142 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

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

Gijs' feedback pass should be sufficient for now.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 8675142 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

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

Some general comments:

* This would probably be a lot easier to review in mozreview!

* I suspect this is likely to cause perf regressions, because we ran into some but worked around them. So you'll want to start testing so there are no post-landing surprises. (In particular, IIRC, conditionally adding a toolbar button causes a reflow and regresses window opening speed.)

* I don't know if it's worth carrying over the .enabledLocales code. I think the intent, which we never quite got to, was to just turn it on everywhere. (This was originally a hack because we were moving faster than L10N, and then had some uncertainty about what we should do if the product was in a locale Pocket's website didn't support.)

* Is there any migration code in here? You need to handle existing users who have either removed the Pocket button, or have disabled the pref.

* What's the experience in the addons manger for addons like this? Presumably they can just be disabled, but not uninstalled? (Because then we would need some capability to allow a user to re-install them?)

::: browser/features/[e-mail]/install.rdf.in
@@ +26,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Pocket</em:name>
> +    <em:description>Pocket.</em:description>

This should have a meaningful (and localized) description. Particularly because I expect users will want to know why there's suddenly a new addon listed, and won't necessarily know it's part of Firefox. (Some of this hit we already too shipping it the first time, but now there's a very obvious place to explain it.)

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Justin Dolske [:Dolske] from comment #17)
> Comment on attachment 8675142 [details] [diff] [review]
> part 3: move code/functionality from part 2 into the addon, make it work
> 
> Review of attachment 8675142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some general comments:
> 
> * This would probably be a lot easier to review in mozreview!
> 
> * I suspect this is likely to cause perf regressions, because we ran into
> some but worked around them. So you'll want to start testing so there are no
> post-landing surprises. (In particular, IIRC, conditionally adding a toolbar
> button causes a reflow and regresses window opening speed.)

In normal circumstances (ie. addon is enabled during startup) the CUI widget is added and given a place prior to any windows opening, so there is no conditional addition and shouldn't be any repaints.  menuitem elements are added during the opening of each window, but those are not visible so shouldn't cause any repaint.  

> * I don't know if it's worth carrying over the .enabledLocales code. I think
> the intent, which we never quite got to, was to just turn it on everywhere.
> (This was originally a hack because we were moving faster than L10N, and
> then had some uncertainty about what we should do if the product was in a
> locale Pocket's website didn't support.)

I was wondering about that.  It's easy to keep for now.

> * Is there any migration code in here? You need to handle existing users who
> have either removed the Pocket button, or have disabled the pref.

Not yet, good point, I could copy over any user-set pocket prefs and verify the last position of the button.

> 
> * What's the experience in the addons manger for addons like this?
> Presumably they can just be disabled, but not uninstalled? (Because then we
> would need some capability to allow a user to re-install them?)

Correct, only enable/disable.  I'm not yet certain we want it listed.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8675938 [details] [diff] [review]
part 4: make reader extendable, move reader support to addon

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

Sorry for the slow feedback. Overall this approach looks fine to me. I like seeing us remove hard-coded partner things in favor of extension APIs!

::: toolkit/components/reader/AboutReader.jsm
@@ +224,5 @@
> +            let btn = aEvent.target;
> +            this._mm.sendAsyncMessage("Reader:" + btn.getAttribute("message"), { article: this._article });
> +            UITelemetry.addEvent(btn.getAttribute("id") + ".1", "button", null, "reader");
> +
> +          }, true);

I think it could be better to use _setupButton here, and refactor it as necessary. It would actually be nice to get that UI telemetry for all the buttons we have, not just these dynamically added ones. I also wonder if we should file a follow-up for mobile to use this to add a reading list button on the fly, rather than how we include it in the HTML.

Comment 20

[Robert Helmer [:rhelmer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> (In reply to Justin Dolske [:Dolske] from comment #17)
> > 
> > * What's the experience in the addons manger for addons like this?
> > Presumably they can just be disabled, but not uninstalled? (Because then we
> > would need some capability to allow a user to re-install them?)
> 
> Correct, only enable/disable.  I'm not yet certain we want it listed.

System add-ons are completely hidden from the UI right now. Allowing enable/disable is doable (maybe behind a pref and/or a new "features" section in the add-ons manager?) but uninstall is not as the XPI is in the application directory.

Comment 21

[Mike Connor [:mconnor]]

We should (In reply to Robert Helmer [:rhelmer] from comment #20)
> System add-ons are completely hidden from the UI right now. Allowing
> enable/disable is doable (maybe behind a pref and/or a new "features"
> section in the add-ons manager?) but uninstall is not as the XPI is in the
> application directory.

Is there a bug of any sort tracking this question? We'll want some way for system add-ons to be disabled/replaced.

Comment 22

[Dave Townsend [:mossop]]

(In reply to Mike Connor [:mconnor] from comment #21)
> We should (In reply to Robert Helmer [:rhelmer] from comment #20)
> > System add-ons are completely hidden from the UI right now. Allowing
> > enable/disable is doable (maybe behind a pref and/or a new "features"
> > section in the add-ons manager?) but uninstall is not as the XPI is in the
> > application directory.
> 
> Is there a bug of any sort tracking this question? We'll want some way for
> system add-ons to be disabled/replaced.

We can already disable/replace them server-side, is that what you're asking about? We just don't have any way exposed for the user to control these add-ons right now.

Comment 23

[Mike Connor [:mconnor]]

No, I mean for users who want to disable it entirely, or replace it with a similar add-on.

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Connor [:mconnor] from comment #23)
> No, I mean for users who want to disable it entirely, or replace it with a
> similar add-on.

IMO moving the button into the customization pallet should be enough from a user perspective to remove pocket.  Additional addons can add similar functionality from other services.  For 3rd party distributions, a configure flag could be used to remove built-in addons.

Comment 25

[Dave Townsend [:mossop]]

(In reply to Mike Connor [:mconnor] from comment #23)
> No, I mean for users who want to disable it entirely, or replace it with a
> similar add-on.

This isn't what system add-ons are designed for. If you want users to be able to disable something themselves then you don't want it to be a system add-on.

Comment 26

[Robert Helmer [:rhelmer]]

(In reply to Mike Connor [:mconnor] from comment #21)
> We should (In reply to Robert Helmer [:rhelmer] from comment #20)
> > System add-ons are completely hidden from the UI right now. Allowing
> > enable/disable is doable (maybe behind a pref and/or a new "features"
> > section in the add-ons manager?) but uninstall is not as the XPI is in the
> > application directory.
> 
> Is there a bug of any sort tracking this question? We'll want some way for
> system add-ons to be disabled/replaced.

(In reply to Dave Townsend [:mossop] from comment #25)
> (In reply to Mike Connor [:mconnor] from comment #23)
> > No, I mean for users who want to disable it entirely, or replace it with a
> > similar add-on.
> 
> This isn't what system add-ons are designed for. If you want users to be
> able to disable something themselves then you don't want it to be a system
> add-on.

I've just emailed the go-faster mailing list to discuss, this is an issue we discussed early on but I think warrants more now that we have a concrete use case: https://mail.mozilla.org/pipermail/gofaster/2015-November/000191.html

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 8675140 [details] [diff] [review]
part 1: move pocket files into a feature add-on directory

diff --git a/browser/locales/en-US/chrome/browser/browser-pocket.properties b/browser/features/[e-mail]/locale/en-US/pocket.properties
rename from browser/locales/en-US/chrome/browser/browser-pocket.properties
rename to browser/features/[e-mail]/locale/en-US/pocket.properties

We'll need locales/en-US in the path.

Comment 28

[Axel Hecht [:Pike]]

We'll also need a fix in browser/locales/l10n.ini. Not sure if we should have a parent l10n.ini file for browser/features, or include them one by one in browser/locales/l10n.ini

Comment 29

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1: move pocket files into a feature add-on directory

Comment 30

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2: remove most pocket code

Comment 31

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon, make it work

Comment 32

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3.2: removing dead code from pocket addon

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

Still have to register a resource for the addon here since the reader is an about: url and cannot load the pocket icon via chrome:

Comment 34

[Shane Caraveo (:mixedpuppy)]

Patches should handle all the feedback, though I may have some more on the reader patch.  Does not address META-INF or system addon related stuff yet.  Does not move the toolbar icon from browser to addon yet.

Comment 35

[:Gijs (he/him)]

Comment on attachment 8682658 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon, make it work

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

I see you moved the l10n bits - probably a good idea to get feedback from Axel on those. I don't know enough about the l10n.ini thing to comment on that.

::: browser/components/uitour/test/browser_UITour_availableTargets.js
@@ +14,2 @@
>    let isEnabledForLocale = true;
> +  if (Services.prefs.getBoolPref("extensions.pocket.useLocaleList")) {

I concur with dolske that we should just rip out the locale list support.

::: browser/features/[e-mail]/bootstrap.js
@@ +93,5 @@
> +    onViewHiding: function() {
> +      return Pocket.onPanelViewHiding.apply(this, arguments);
> +    },
> +    onBeforeCreated: function(doc) {
> +      // XXX wondering why I have to create a view, seems like createwidget should do that for me.

Did you file a bug for this yet? :-)

@@ +112,5 @@
> +      });
> +    }),
> +  };
> +
> +  //CustomizableWidgets.push(pocketButton);

Nit: no commented out code please. :-)

@@ +119,5 @@
> +  if (reason == ADDON_ENABLE) {
> +    // initially place the button after the bookmarks button
> +    let widgets = CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR);
> +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR,
> +                                   widgets.indexOf("bookmarks-menu-button") + 1);

So, fwiw, this shouldn't be happening every time you enable the add-on, if it can be successive disabled and re-enabled (which it seems is currently not the case but we might be changing that, from reading the mailing list).

Normally people keep track of this with a pref - or just rely on defaultArea -- but if we're picky about where in the navbar this appears, I guess we should go the pref and manual addition route if that becomes a thing. I also suppose that could be a followup if it's not a current reality, though we should keep it in mind if we start allowing people to do that.

@@ +155,5 @@
> +    // iterate through all windows and add pocket to them
> +    eachBrowserWindow(function(win) {
> +      let d = win.document;
> +      for (let eid in ["context-pocket", "context-savelinktopocket"]) {
> +        let e = d.getElementById(eid);

Nit: no particular reason for all the brevity here, or the |d| temp. :-)

I guess more generally, I'd personally prefer longer, more descriptive variable names (than "s", "d", "m", "e", etc.).

@@ +175,5 @@
> +                                        subject.onCanvas || subject.onVideo || subject.onAudio);
> +    let targetUrl = subject.onLink ? subject.linkUrl : subject.pageUrl;
> +    let targetURI = Services.io.newURI(targetUrl, null, null);
> +    let canPocket = pocketEnabled && (targetURI.schemeIs("http") || targetURI.schemeIs("https") ||
> +                              (targetURI.schemeIs("about") && ReaderMode.getOriginalUrl(targetUrl)));

The indenting looks a bit off here...

@@ +367,5 @@
> +    let styleSheetService = Components.classes["@mozilla.org/content/style-sheet-service;1"]
> +                                      .getService(Components.interfaces.nsIStyleSheetService);
> +    let styleSheetURI = Services.io.newURI("chrome://pocket/skin/pocket.css", null, null);
> +    if (styleSheetService.sheetRegistered(styleSheetURI, styleSheetService.AUTHOR_SHEET)) {
> +        styleSheetService.unregisterSheet(styleSheetURI, styleSheetService.AUTHOR_SHEET);

Nit: 2-space indent like the rest of the file.

::: browser/features/[e-mail]/install.rdf.in
@@ +15,5 @@
> +    <em:type>2</em:type>
> +    <em:bootstrap>true</em:bootstrap>
> +
> +    <!-- Target Application this theme can install into, 
> +        with minimum and maximum supported versions. --> 

Trailing whitespace here and below.

@@ +26,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Pocket</em:name>
> +    <em:description>Pocket.</em:description>

Should probably make all of this localizable? If it never shows up in the UI, is <em:description> even required?

::: browser/features/[e-mail]/jar.mn
@@ +14,5 @@
> +  pktApi.jsm  (pocket/pktApi.jsm)
> +  locale/en-US/pocket.properties       (locales/en-US/pocket.properties)
> +* skin/osx/pocket.css (themes/osx/pocket.css)
> +* skin/linux/pocket.css (themes/linux/pocket.css)
> +* skin/windows/pocket.css (themes/windows/pocket.css)
\ No newline at end of file

Nit: line up all the bits in ()

Are all those css files really preprocessed? That's sad.

::: browser/features/[e-mail]/pocket/main.js
@@ +350,5 @@
>          if (inOverflowMenu) {
>          	startheight = overflowMenuHeight;
>          }
>  
> +    	var panelId = showPanel("chrome://pocket/content/panels/saved.html?pockethost=" + Services.prefs.getCharPref("extensions.pocket.site") + "&premiumStatus=" + (pktApi.isPremiumUser() ? '1' : '0') + '&inoverflowmenu='+inOverflowMenu + "&locale=" + getUILocale(), {

If you're feeling awesome, template strings here and earlier would make this a lot more readable.

Also, r=me directly after this lands to reindent the whole lot of these files. So messy. :-(

::: browser/features/moz.build
@@ +8,5 @@
> +
> +# HAS_MISC_RULE = True
> +
> +DIRS += [
> +    '[e-mail]'

Shouldn't this be the other way around, as in, shouldn't we be using [e-mail] for add-on identifiers here?

Comment 36

[Shane Caraveo (:mixedpuppy)]

note to self: will need to address tooltip similar to bug 1166651

Comment 37

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8682662 [details] [diff] [review]
part 4: make reader extendable, move reader support to addon

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

::: browser/features/[e-mail]/bootstrap.js
@@ +256,5 @@
> +        let url = browser.currentURI.spec;
> +        if (url.startsWith("about:reader")) {
> +          browser.messageManager.sendAsyncMessage("Reader:AddButton",
> +                                                  { id: "pocket-button",
> +                                                    image: "resource://pocket/chrome/pocket/images/pocket.svg#pocket-mark"});

We should add a `title` property here to fix bug 1166651.  The title string can be "Save to Pocket" to match the toolbar button.

Comment 39

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #35)

> @@ +119,5 @@
> > +  if (reason == ADDON_ENABLE) {
> > +    // initially place the button after the bookmarks button
> > +    let widgets = CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR);
> > +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR,
> > +                                   widgets.indexOf("bookmarks-menu-button") + 1);
> 
> So, fwiw, this shouldn't be happening every time you enable the add-on, if
> it can be successive disabled and re-enabled (which it seems is currently
> not the case but we might be changing that, from reading the mailing list).
> 
> Normally people keep track of this with a pref - or just rely on defaultArea
> -- but if we're picky about where in the navbar this appears, I guess we
> should go the pref and manual addition route if that becomes a thing. I also
> suppose that could be a followup if it's not a current reality, though we
> should keep it in mind if we start allowing people to do that.

I've thought about this a bit, even implemented and played with it, yet I disagree.  I don't see any circumstance where resetting location when an addon is enabled is a bad thing, whereas I can easily see cases where leaving it in some hidden location can be confusing.  e.g. I move to palette, disable, a month later re-enable.  Is there any normal situation, in normal use cases, where a user would disable/enable the addon in a short time period where the location should be preserved?

Comment 40

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> (In reply to :Gijs Kruitbosch from comment #35)
> 
> > @@ +119,5 @@
> > > +  if (reason == ADDON_ENABLE) {
> > > +    // initially place the button after the bookmarks button
> > > +    let widgets = CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR);
> > > +    CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR,
> > > +                                   widgets.indexOf("bookmarks-menu-button") + 1);

Unrelated note: I just realized this will insert at the start of the navbar when the bookmarks menu button isn't in it. Please don't do that and special-case that so we insert at the end.

> > 
> > So, fwiw, this shouldn't be happening every time you enable the add-on, if
> > it can be successive disabled and re-enabled (which it seems is currently
> > not the case but we might be changing that, from reading the mailing list).
> > 
> > Normally people keep track of this with a pref - or just rely on defaultArea
> > -- but if we're picky about where in the navbar this appears, I guess we
> > should go the pref and manual addition route if that becomes a thing. I also
> > suppose that could be a followup if it's not a current reality, though we
> > should keep it in mind if we start allowing people to do that.
> 
> I've thought about this a bit, even implemented and played with it, yet I
> disagree.  I don't see any circumstance where resetting location when an
> addon is enabled is a bad thing, whereas I can easily see cases where
> leaving it in some hidden location can be confusing.  e.g. I move to
> palette, disable, a month later re-enable.

This code always moves the button. Even if I'd previously moved it somewhere else in the toolbar, or if I moved it to a different toolbar, or to the menu panel. That's definitely wrong.

>  Is there any normal situation,
> in normal use cases, where a user would disable/enable the addon in a short
> time period where the location should be preserved?

Safe mode? Trying to binary search for "which add-on is breaking X" ? Initial misclick-disable in the add-ons manager when trying to do something else, then immediately re-enable, now stuck with the button again?

More generally, you're assuming that it was some kind of accident that the user (re)moved the button. If they were deliberate about it then, I don't see why we would override their choice. If they miss the button, it's easy to restore to defaults within customize mode.

Comment 41

[:Gijs (he/him)]

Oh, and finally, there are a lot of users who have removed the pocket button already because they don't need it. This was our recommended course of action when the feature was implemented because it could not be disabled some other way.

They won't take kindly to our moving it back again automatically when we move pocket to an add-on.

Comment 42

[Shane Caraveo (:mixedpuppy)]

> > > 
> > > So, fwiw, this shouldn't be happening every time you enable the add-on, if
> > > it can be successive disabled and re-enabled (which it seems is currently
> > > not the case but we might be changing that, from reading the mailing list).
> > > 
> > > Normally people keep track of this with a pref - or just rely on defaultArea
> > > -- but if we're picky about where in the navbar this appears, I guess we
> > > should go the pref and manual addition route if that becomes a thing. I also
> > > suppose that could be a followup if it's not a current reality, though we
> > > should keep it in mind if we start allowing people to do that.
> > 
> > I've thought about this a bit, even implemented and played with it, yet I
> > disagree.  I don't see any circumstance where resetting location when an
> > addon is enabled is a bad thing, whereas I can easily see cases where
> > leaving it in some hidden location can be confusing.  e.g. I move to
> > palette, disable, a month later re-enable.
> 
> This code always moves the button. Even if I'd previously moved it somewhere
> else in the toolbar, or if I moved it to a different toolbar, or to the menu
> panel. That's definitely wrong.

No, it only moves it when the addon is explicitly enabled, this code is not called on normal app startup.

> >  Is there any normal situation,
> > in normal use cases, where a user would disable/enable the addon in a short
> > time period where the location should be preserved?
> 
> Safe mode? 

Nope.

> Trying to binary search for "which add-on is breaking X" ?
> Initial misclick-disable in the add-ons manager when trying to do something
> else, then immediately re-enable, now stuck with the button again?

Not a normal use case.  And why would you re-enable if you don't want it anyway?  And how are you stuck with the button?  You now have more control by disabling the addon.

> More generally, you're assuming that it was some kind of accident that the
> user (re)moved the button. If they were deliberate about it then, I don't
> see why we would override their choice. If they miss the button, it's easy
> to restore to defaults within customize mode.

Nope, if the user moved the button, it is not moved back *unless* you ADDON_ENABLE.

> Oh, and finally, there are a lot of users who have removed the pocket button 
> already because they don't need it. This was our recommended course of action 
> when the feature was implemented because it could not be disabled some other way.

> They won't take kindly to our moving it back again automatically when we move pocket to an add-on.

Again, only if ADDON_ENABLE is done, in any case, I cannot rely on a pref for this since the button already existed and the pref did not.  And that gets to the crux of the problem here...Does CUI have some way to query the location of a widget if the widget has not yet been created?

Comment 43

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #42)
> > > > 
> > > > So, fwiw, this shouldn't be happening every time you enable the add-on, if
> > > > it can be successive disabled and re-enabled (which it seems is currently
> > > > not the case but we might be changing that, from reading the mailing list).
> > > > 
> > > > Normally people keep track of this with a pref - or just rely on defaultArea
> > > > -- but if we're picky about where in the navbar this appears, I guess we
> > > > should go the pref and manual addition route if that becomes a thing. I also
> > > > suppose that could be a followup if it's not a current reality, though we
> > > > should keep it in mind if we start allowing people to do that.
> > > 
> > > I've thought about this a bit, even implemented and played with it, yet I
> > > disagree.  I don't see any circumstance where resetting location when an
> > > addon is enabled is a bad thing, whereas I can easily see cases where
> > > leaving it in some hidden location can be confusing.  e.g. I move to
> > > palette, disable, a month later re-enable.
> > 
> > This code always moves the button. Even if I'd previously moved it somewhere
> > else in the toolbar, or if I moved it to a different toolbar, or to the menu
> > panel. That's definitely wrong.
> 
> No, it only moves it when the addon is explicitly enabled, this code is not
> called on normal app startup.

Sure, but the fact remains, it will change the location from where the user put it. It is unexpected that an add-on disable/enable cycle does this.

> > Safe mode? 
> 
> Nope.

Err, can you elaborate on this? Does addon_enable not happen when you restart in normal mode after running in safe mode? That seems like a bug.

>  And why would you re-enable if you don't want it
> anyway?  And how are you stuck with the button?  You now have more control
> by disabling the addon.

TBH, this is what we ask of add-ons generally, and how we treat defaultArea for non-builtin add-ons. They get introduced *once*, no more. I don't think our own add-ons should behave differently.

> > More generally, you're assuming that it was some kind of accident that the
> > user (re)moved the button. If they were deliberate about it then, I don't
> > see why we would override their choice. If they miss the button, it's easy
> > to restore to defaults within customize mode.
> 
> Nope, if the user moved the button, it is not moved back *unless* you
> ADDON_ENABLE.

I'm aware of this happening only on ADDON_ENABLE. I don't see why that makes it OK to move the button around the UI...

> > Oh, and finally, there are a lot of users who have removed the pocket button 
> > already because they don't need it. This was our recommended course of action 
> > when the feature was implemented because it could not be disabled some other way.
> 
> > They won't take kindly to our moving it back again automatically when we move pocket to an add-on.
> 
> Again, only if ADDON_ENABLE is done,

Which presumably happens the first time you run Firefox with this add-on installed, right?

> in any case, I cannot rely on a pref
> for this since the button already existed and the pref did not.  And that
> gets to the crux of the problem here...Does CUI have some way to query the
> location of a widget if the widget has not yet been created?

Yes, CustomizableUI.getPlacementOfWidget() . All of this is documented on https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm .

Comment 44

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #43)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #42)

> > No, it only moves it when the addon is explicitly enabled, this code is not
> > called on normal app startup.
> 
> Sure, but the fact remains, it will change the location from where the user
> put it. It is unexpected that an add-on disable/enable cycle does this.

A disable/enable cycle could be a second or a year.  I think the issue is, when it is a longer cycle, the user may not know where the button is.  I'm not sure that's a bad thing, but it seems like a confusing item.

> > > Safe mode? 
> > 
> > Nope.
> 
> Err, can you elaborate on this? Does addon_enable not happen when you
> restart in normal mode after running in safe mode? That seems like a bug.

Safe mode doesn't disable addons, it bypasses addons, thus enabling is not necessary on normal restart.  If you start in safe mode and look at about:addons, you'll see they are still "enabled" in the UI.

> >  And why would you re-enable if you don't want it
> > anyway?  And how are you stuck with the button?  You now have more control
> > by disabling the addon.
> 
> TBH, this is what we ask of add-ons generally, and how we treat defaultArea
> for non-builtin add-ons. They get introduced *once*, no more. I don't think
> our own add-ons should behave differently.

> > > Oh, and finally, there are a lot of users who have removed the pocket button 
> > > already because they don't need it. This was our recommended course of action 
> > > when the feature was implemented because it could not be disabled some other way.
> > 
> > > They won't take kindly to our moving it back again automatically when we move pocket to an add-on.
> > 
> > Again, only if ADDON_ENABLE is done,
> 
> Which presumably happens the first time you run Firefox with this add-on
> installed, right?

Yes, but if this is managed by a pref the addon uses, it will still move back to the toolbar.

> > in any case, I cannot rely on a pref
> > for this since the button already existed and the pref did not.  And that
> > gets to the crux of the problem here...Does CUI have some way to query the
> > location of a widget if the widget has not yet been created?
> 
> Yes, CustomizableUI.getPlacementOfWidget() . All of this is documented on
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> CustomizableUI.jsm .


That returns null prior to calling createWidget.  Calling after createWidget, I can know the previous placement is fine if it is not the nav bar, however, if it is the nav bar, I do not know if it was previously moved by the user or if it is a first time addition to the nav bar.  

If I could query for a location based on the id prior to createWidget, then I would know for certain whether the user previously had the button in a specific location or if it is a clean new install.  It seems like gFuturePlacements may have that data, but no way to look at it, should I/can I fall back to currentset?

Comment 45

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> (In reply to :Gijs Kruitbosch from comment #43)
> > Yes, CustomizableUI.getPlacementOfWidget() . All of this is documented on
> > https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> > CustomizableUI.jsm .
> 
> 
> That returns null prior to calling createWidget. 

That shouldn't be the case per https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#1685 unless the widget is not placed anywhere in the UI.

I just checked with a random add-on (SQLite Manager) and I can't reproduce - if the button was previously in the bookmarks toolbar, and I disable the add-on, restart, and check the result of calling CustomizableUI.getPlacementOfWidget() it still returns the right value.

Can you debug why you're seeing this?

Comment 46

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> (In reply to :Gijs Kruitbosch from comment #43)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #42)
> > > > Safe mode? 
> > > 
> > > Nope.
> > 
> > Err, can you elaborate on this? Does addon_enable not happen when you
> > restart in normal mode after running in safe mode? That seems like a bug.
> 
> Safe mode doesn't disable addons, it bypasses addons, thus enabling is not
> necessary on normal restart.  If you start in safe mode and look at
> about:addons, you'll see they are still "enabled" in the UI.

Sure, but that's in part because that way you can actually disable them outside of safe mode and use safe mode to recover from broken add-ons. For that UI to work, they need to "look enabled" and be disable-able.

What I'm confused about is that add-ons don't get told we're going to run Firefox without them. For add-ons that keep state, that is important information.

Comment 47

[Filipus Klutiero]

Backlash over Pocket integration has made it to Slashdot again: http://news.slashdot.org/story/15/11/13/2141212/mozilla-has-no-plans-to-offer-firefox-without-pocket

Comment 48

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #45)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> > (In reply to :Gijs Kruitbosch from comment #43)
> > > Yes, CustomizableUI.getPlacementOfWidget() . All of this is documented on
> > > https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> > > CustomizableUI.jsm .
> > 
> > 
> > That returns null prior to calling createWidget. 
> 
> That shouldn't be the case per
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizableUI.jsm#1685 unless the widget is not placed
> anywhere in the UI.
> 
> I just checked with a random add-on (SQLite Manager) and I can't reproduce -
> if the button was previously in the bookmarks toolbar, and I disable the
> add-on, restart, and check the result of calling
> CustomizableUI.getPlacementOfWidget() it still returns the right value.
> 
> Can you debug why you're seeing this?

CustomizableUI.getPlacementOfWidget is here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#3439

Which calls the internal api setting aOnlyRegistered=true.  Since the widget doesn't exist yet, null is returned (see first line of the internal api you linked to above).

Comment 49

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #48)
> Which calls the internal api setting aOnlyRegistered=true.  Since the widget
> doesn't exist yet, null is returned (see first line of the internal api you
> linked to above).

Then we should probably expose the extra parameter on getPlacementOfWidget instead.

Comment 50

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #35)

> @@ +26,5 @@
> > +    </em:targetApplication>
> > +
> > +    <!-- Front End MetaData -->
> > +    <em:name>Pocket</em:name>
> > +    <em:description>Pocket.</em:description>
> 
> Should probably make all of this localizable? If it never shows up in the
> UI, is <em:description> even required?

How would that be done for install.rdf?  We don't localize the other install.rdf.in files in the tree.

Comment 51

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1: move pocket files into a feature add-on directory

moving to browser/extensions/pocket for better tree integration
addon will ultimately be installed to browser/features/addonname, we'll deal  with enable/disable issues later.  This will also allow us to land now as a system addon without addon signing.

Comment 52

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2: remove most pocket code

Comment 53

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon

Comment 54

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3.2: removing dead code from pocket addon

Comment 55

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

Comment 56

[:Gijs (he/him)]

Comment on attachment 8693899 [details] [diff] [review]
part 2: remove most pocket code

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

::: browser/base/content/browser.js
@@ -8301,5 @@
>      }
>      return browser;
>    },
>  };
> -

Don't mess with the end of the file please.

::: browser/themes/shared/browser.inc
@@ +1,5 @@
>  %filter substitution
>  
>  % Note that zoom-reset-button is a bit different since it doesn't use an image and thus has the image with display: none.
>  %define nestedButtons #zoom-out-button, #zoom-reset-button, #zoom-in-button, #cut-button, #copy-button, #paste-button
> +%define primaryToolbarButtons #back-button, #forward-button, #home-button, #print-button, #downloads-button, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #sync-button, #feed-button, #tabview-button, #social-share-button, #open-file-button, #find-button, #developer-button, #preferences-button, #privatebrowsing-button, #save-page-button, #add-ons-button, #history-panelmenu, #nav-bar-overflow-button, #PanelUI-menu-button, #characterencoding-button, #email-link-button, #sidebar-button, @nestedButtons@, #e10s-button, #panic-button, #web-apps-button, #webide-button

I bitrotted this assuming the panorama removal sticks. Sorry.

Comment 57

[:Gijs (he/him)]

Comment on attachment 8693900 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon

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

Why is this removing a browser/extensions/pocket/pocket/jar.mn ? Is this just an artifact of the move of the code in part 1? Can we either squash the commits before landing or fix commit 1? :-)

There's two many things that I'm still not clear on, so this isn't r+ -- sorry!

::: browser/components/uitour/test/browser_UITour_availableTargets.js
@@ +17,5 @@
>                             .getService(Ci.nsIXULChromeRegistry);
>      let browserLocale = chromeRegistry.getSelectedLocale("browser");
>      let enabledLocales = [];
>      try {
> +      enabledLocales = Services.prefs.getCharPref("extensions.pocket.enabledLocales").split(' ');

The locale-checking code here should be removed, just like the code originally from CUI.jsm.

::: browser/extensions/pocket/bootstrap.js
@@ +1,1 @@
> +

This should have a license header.

@@ +26,5 @@
> +const PREF_BRANCH = "extensions.pocket.";
> +const PREFS = {
> +  api: "api.getpocket.com",
> +  site: "getpocket.com",
> +  oAuthConsumerKey: "40249-e88c401e1b1f2242d9e441c4"

Lots of things check extensions.pocket.enabled, which isn't in here, and so that code (Services.prefs.getBoolPref) will now throw. We should add that pref or update callsites. There might be other prefs that I forgot about, too.

I also don't see any migration of the existing prefs from users, so if my profile had browser.pocket.enabled then that preference will now be "lost". Seems like that shouldn't happen either.

@@ +46,5 @@
> +    }
> +  }
> +}
> +
> +function eachBrowserWindow(callback) {

Nit: in our continuing quest for nicer-to-read-code, may I suggest:

function* allBrowserWindows() {
  var winEnum = Services.wm.getEnumerator("navigator:browser");
  while (winEnum.hasMoreElements()) {
    let win = winEnum.getNext();
    // skip closed windows
    if (win.closed)
      continue;
    yield win;
  }
}

to be used like:

for (let win of allBrowserWindows()) {
  // do stuff
}

@@ +104,5 @@
> +      doc.getElementById("PanelUI-multiView").appendChild(view);
> +    },
> +    // If the user has the "classic" Pocket add-on installed, use that instead
> +    // and destroy the widget.
> +    conditionalDestroyPromise: new Promise(resolve => {

It seems like we should do this check before bothering with calling createWidget... Is there a reason we can't do that?

@@ +114,5 @@
> +
> +  CustomizableUI.createWidget(pocketButton);
> +  CustomizableUI.addListener(pocketButton);
> +  // placed is null if location is palette
> +  let placed = CustomizableUI.getPlacementOfWidget("pocket-button", false, true);

The defaultArea is in the nav-bar (not an area that might not exist), and the button will definitely exist because you just called createWidget, so you don't need the two extra arguments here.

@@ +124,5 @@
> +    // initially place the button after the bookmarks button if it is in the UI
> +    let widgets = CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR);
> +    let bmbtn = widgets.indexOf("bookmarks-menu-button");
> +    if (bmbtn > -1) {
> +      CustomizableUI.addWidgetToArea("pocket-button", CustomizableUI.AREA_NAVBAR, bmbtn + 1);

moveWidgetWithinArea("pocket-button", bmbtn + 1);

AIUI this code will only run for first-time install, and so if the button has been placed it will be in the nav-bar anyway.

@@ +180,5 @@
> +                                        subject.isContentSelected || subject.onImage ||
> +                                        subject.onCanvas || subject.onVideo || subject.onAudio);
> +    let targetUrl = subject.onLink ? subject.linkUrl : subject.pageUrl;
> +    let targetURI = Services.io.newURI(targetUrl, null, null);
> +    let canPocket = pocketEnabled &&

You early returned so no need to check this.

@@ +202,5 @@
> +      } else {
> +        sibling.parentNode.appendChild(menu);
> +      }
> +    }
> +    menu.hidden = !(canPocket && showSaveCurrentPageToPocket);

Note that now it will take a restart for these menus to be hidden again if you "disable" pocket by removing the toolbar button. That's a regression compared to the builtin state of things.

@@ +258,5 @@
> +  shutdown: function(reason) {
> +    CustomizableUI.removeListener(this);
> +    eachBrowserWindow(function(window) {
> +      for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
> +                          "panelMenu_pocketSeparator", "menu_pocketSeparator", "BMB_pocketSeparator"]) {

Nit: indenting. Could use the prefixes instead like you did in updatePocketItemVisibility ?

@@ +259,5 @@
> +    CustomizableUI.removeListener(this);
> +    eachBrowserWindow(function(window) {
> +      for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
> +                          "panelMenu_pocketSeparator", "menu_pocketSeparator", "BMB_pocketSeparator"]) {
> +        let e = window.document.getElementById(id);

Nit: please use a longer variable name.

@@ +273,5 @@
> +    PocketContextMenu.shutdown();
> +    this.unregisterStylesheet();
> +  },
> +  observe: function(aSubject, aTopic, aData) {
> +    // if this is a browser window we want to init our overlay

Why would browser-delayed-startup-finished ever fire for something that isn't a browser window?

@@ +310,5 @@
> +      sib.parentNode.insertBefore(sep, sib);
> +    }
> +
> +    // add to bookmarks-menu-button
> +    // XXX todo watch for customization changes and update

This code now always adds all the things. There's an onCustomizeEnd handler, but that calls this instead of just calling updatePocketItemVisibility for each window, which AFAICT is the only thing it needs to do. I think this comment can be removed. Did I miss something?

@@ +319,5 @@
> +        "label": gPocketBundle.GetStringFromName("pocketMenuitem.label"),
> +        "class": "menuitem-iconic bookmark-item subviewbutton",
> +        "oncommand": "openUILink(Pocket.listURL, event);"
> +      });
> +      sep = createElementWithAttrs(document, "menuseparator", {

let is block-scoped now so sep is not defined here, and neither is menu.

@@ +349,5 @@
> +    for (let prefix of ["panelMenu_", "menu_", "BMB_"]) {
> +      this.updatePocketItemVisibility(document, prefix);
> +    }
> +  },
> +  onCustomizeEnd: function(aWindow) {

use onWidgetRemoved and onWidgetAdded instead and check that the widget in question is pocket. onCustomizeEnd doesn't get called if you use the context menu to move a button.

@@ +375,5 @@
> +    let styleSheetService = Components.classes["@mozilla.org/content/style-sheet-service;1"]
> +                                      .getService(Components.interfaces.nsIStyleSheetService);
> +    let styleSheetURI = Services.io.newURI("chrome://pocket/skin/pocket.css", null, null);
> +    if (styleSheetService.sheetRegistered(styleSheetURI, styleSheetService.AUTHOR_SHEET)) {
> +        styleSheetService.unregisterSheet(styleSheetURI, styleSheetService.AUTHOR_SHEET);

nit: 2-space indent please.

@@ +389,5 @@
> +
> +function shutdown(data, reason) {
> +  // XXX for speed sake, we should only do a shutdown if we're being disabled.
> +  // On an app shutdown, just let it fade away...
> +  PocketOverlay.shutdown(reason);

Who's actually checking the disabled reason thing then? Neither this nor .shutdown seems to do so.

::: browser/extensions/pocket/install.rdf.in
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<!-- 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/. -->
> +

Nit: 1 line of whitespace is enough

@@ +26,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Pocket</em:name>
> +    <em:description>When you find something you want to view later, put it in Pocket.</em:description>

If this is going to be user-visible, IMO we should make it localizable. Let's file a followup bug for this.

@@ +27,5 @@
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Pocket</em:name>
> +    <em:description>When you find something you want to view later, put it in Pocket.</em:description>
> +

Don't need this blank line either.

::: browser/extensions/pocket/locales/en-US/pocket.properties
@@ +30,5 @@
>  signupfirefox = Sign up with Firefox
>  viewlist = View List
> +
> +# LOCALIZATION NOTE(pocket-button.label, pocket-button.tooltiptext, saveToPocketCmd.label, saveLinkToPocketCmd.label, pocketMenuitem.label):
> +# "Pocket" is a brand name.

Where are we adding this to l10n.ini as suggested by Pike in his earlier review comments?

::: browser/extensions/pocket/pocket/pktApi.jsm
@@ +46,5 @@
> +this.EXPORTED_SYMBOLS = ["pktApi"];
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Nit: not super useful to make this lazy - someone else will have used it by now.

::: browser/extensions/pocket/themes/shared/pocket.css
@@ +30,5 @@
> +
> +  #pocket-button[cui-areatype="menu-panel"][panel-multiview-anchor=true] {
> +    -moz-image-region: rect(32px, 992px, 64px, 960px);
> +  }
> +

Nit: no need for this empty line.

::: browser/extensions/pocket/themes/windows/pocket.css
@@ +1,4 @@
> +%include ../shared/pocket.css
> +
> +@media (min-resolution: 1.1dppx) {
> +

Nit: no need for the blank line.

Comment 58

[:Gijs (he/him)]

Comment on attachment 8693901 [details] [diff] [review]
part 3.2: removing dead code from pocket addon

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

::: browser/extensions/pocket/pocket/main.js
@@ -72,5 @@
>  
> -	/**
> -     * Initalizes Pocket UI and panels
> -     */
> -	function onLoad() {

Did we never call this?

@@ +577,5 @@
>      	pocketPanelDidShow: pocketPanelDidShow,
>      	pocketPanelDidHide: pocketPanelDidHide,
>  
> +    	tryToSaveUrl: tryToSaveUrl,
> +		tryToSaveCurrentPage: tryToSaveCurrentPage

Nit: please fix the indenting.

Comment 59

[:Gijs (he/him)]

Comment on attachment 8693898 [details] [diff] [review]
part 1: move pocket files into a feature add-on directory

Clearing review because of the jar.mn thing. Maybe I'm missing something?

Also, why is this going to browser/extensions and is Hello using browser/features ? All this is pretty confusing... are there docs on how this is supposed to be working?

Comment 60

[Axel Hecht [:Pike]]

This needs additional touches to work on the l10n side of things.

At least stuff around browser/locales/l10n.ini and filter.py. Can we catch up on this in mozlando? I think I can be of better help with full background.

I'd also prefer this to land in the next cycle, with some additional learnings from the devtools code move.

Comment 61

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #59)
> Comment on attachment 8693898 [details] [diff] [review]
> part 1: move pocket files into a feature add-on directory
> 
> Clearing review because of the jar.mn thing. Maybe I'm missing something?
> 
> Also, why is this going to browser/extensions and is Hello using
> browser/features ? All this is pretty confusing... are there docs on how
> this is supposed to be working?

loop code is in browser/extensions, the final build step places it in dist/bin/browser/features (as does the pocket addon build) which is the system addons location.

Comment 62

[:Gijs (he/him)]

Comment on attachment 8693903 [details] [diff] [review]
part 4: make reader extendable, move reader support to addon

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

::: browser/extensions/pocket/bootstrap.js
@@ +51,5 @@
> +  let resource = Services.io.getProtocolHandler("resource")
> +                 .QueryInterface(Ci.nsIResProtocolHandler);
> +  let alias = Services.io.newFileURI(installPath);
> +  if (!installPath.isDirectory())
> +      alias = Services.io.newURI("jar:" + alias.spec + "!/", null, null);

No, please don't do this. Sync IO and some other hardcoded assumptions. I believe you can just alias the chrome URI as-is, and if you want the chrome URI resolved to something else, you can ask the chrome registry to do that.

A simpler solution for now would be to make the pocket content package contentaccessible, like browser is.

@@ +77,5 @@
> +  eachBrowserWindow((win) => {
> +    if (!win.gBrowser)
> +      return;
> +    var browsers = win.gBrowser.browsers;
> +    for (let browser of browsers) {

You don't need to and shouldn't do this manually. Just use the global frame message manager:

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Message_Manager/Message_manager_overview#Frame_message_managers

@@ +300,5 @@
> +        let placement = CustomizableUI.getPlacementOfWidget("pocket-button");
> +        if (placement) {
> +          if (placement.area == CustomizableUI.AREA_PANEL) {
> +            doc.defaultView.PanelUI.show().then(function() {
> +              // The DOM node might not exist yet if the panel wasn't opened before.

*not have existed

::: toolkit/components/reader/AboutReader.jsm
@@ +229,5 @@
> +          if (message.data.text)
> +            btn.textContent = message.data.text;
> +          let tb = this._doc.getElementById("reader-toolbar");
> +          tb.appendChild(btn);
> +          this._setupButton(message.data.id, (button) => {

Nit: No need for () around a single arg arrow function

@@ +230,5 @@
> +            btn.textContent = message.data.text;
> +          let tb = this._doc.getElementById("reader-toolbar");
> +          tb.appendChild(btn);
> +          this._setupButton(message.data.id, (button) => {
> +            this._mm.sendAsyncMessage("Reader:" + button.getAttribute("id"), { article: this._article });

I think it would be better if this had a prefix, like "ButtonClicked" or whatever. Now I can add a button called "Added" or whatever and it'll conflict with other messages.

@@ +238,5 @@
> +      }
> +      case "Reader:RemoveButton": {
> +        if (message.data.id) {
> +          let btn = this._doc.getElementById(message.data.id);
> +          btn.parentNode.removeChild(btn);

btn.remove();

@@ +918,5 @@
>  
>        aEvent.stopPropagation();
> +      let btn = aEvent.target;
> +      callback(btn);
> +      UITelemetry.addEvent(btn.getAttribute("id") + ".1", "button", null, "reader");

This is opting everyone into UITelemetry events from this button. That isn't a good idea, IMO. I think Pocket should take care of this itself if this is still valuable. This would also be an excellent excuse to just remove it.

Comment 63

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #58)
> Comment on attachment 8693901 [details] [diff] [review]
> part 3.2: removing dead code from pocket addon
> 
> Review of attachment 8693901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/pocket/pocket/main.js
> @@ -72,5 @@
> >  
> > -	/**
> > -     * Initalizes Pocket UI and panels
> > -     */
> > -	function onLoad() {
> 
> Did we never call this?

No.  My assumption is that this is code from some library or the older pocket addon.

Comment 64

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #57)
> Comment on attachment 8693900 [details] [diff] [review]
> part 3: move code/functionality from part 2 into the addon
> 
> Review of attachment 8693900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is this removing a browser/extensions/pocket/pocket/jar.mn ? 

moved that to part 1.

> @@ +26,5 @@
> > +const PREF_BRANCH = "extensions.pocket.";
> > +const PREFS = {
> > +  api: "api.getpocket.com",
> > +  site: "getpocket.com",
> > +  oAuthConsumerKey: "40249-e88c401e1b1f2242d9e441c4"
> 
> Lots of things check extensions.pocket.enabled, 

Only tour tests check that pref.  I added it back to avoid breaking those tests, but nothing else uses that.  Given this is an addon, and ideally will have ui to disable, as well just moving the button to palette also essentially disables it, it's not necessary.  It is also not necessary to migrate any prefs as the only important ones (above) shouldn't have been changed by anyone anyway.

> ::: browser/extensions/pocket/locales/en-US/pocket.properties
> @@ +30,5 @@
> >  signupfirefox = Sign up with Firefox
> >  viewlist = View List
> > +
> > +# LOCALIZATION NOTE(pocket-button.label, pocket-button.tooltiptext, saveToPocketCmd.label, saveLinkToPocketCmd.label, pocketMenuitem.label):
> > +# "Pocket" is a brand name.
> 
> Where are we adding this to l10n.ini as suggested by Pike in his earlier
> review comments?

Per his last message there are additional "things" that need to be done, I'm not clear what that is.  I'm leaning towards leaving the properties where they are in browser, land the addon using those, and have a followup bug to move those properties.

Comment 65

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1: move pocket files into a feature add-on directory

Comment 66

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2: remove most pocket code

comments addressed, carry forward r+

Comment 67

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon

Comment 68

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3.2: removing dead code from pocket addon

updated, carry forward r+

Comment 69

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

Comment 70

[Shane Caraveo (:mixedpuppy)]

Attachment: part 5: l10n

I think this is what needs to be done...not 100% sure.

Comment 71

[:Gijs (he/him)]

Comment on attachment 8695585 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon

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

Almost there, but the "not working!" comment is a bit scary...

You also seem to have changed the destination directory to extensions rather than features, was that intentional?

::: browser/components/customizableui/CustomizableUI.jsm
@@ +61,5 @@
>   * version the button is removed in as the value.  e.g. "pocket-button": 5
>   */
>  var ObsoleteBuiltinButtons = {
> +  "loop-button": 5,
> +  "pocket-button": 5

This should be 6 and we should increment kVersion, right?

::: browser/extensions/pocket/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +FINAL_TARGET = 'dist/bin/browser/extensions/[e-mail]'
> +
> +# not working!

This is a bit of a disconcerting comment... :-)

Can you expand on what's going on here? Is this the final patch or not?

Comment 72

[:Gijs (he/him)]

Comment on attachment 8695587 [details] [diff] [review]
part 4: make reader extendable, move reader support to addon

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

r+ but I'm a little surprised that the message topic sent doesn't match the message topic that the parent listens for. Easy to fix of course, but I worry whether this is the right patch and/or if you tested this. :-)

::: browser/extensions/pocket/bootstrap.js
@@ +222,5 @@
> +// PocketReader
> +// Listen for reader mode setup and add our button to the reader toolbar
> +var PocketReader = {
> +  startup: function(reason) {
> +    let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);

Services.mm

Same below.

@@ +248,5 @@
> +        // we can use the resoure url.
> +        message.target.messageManager.
> +          sendAsyncMessage("Reader:AddButton", { id: "pocket-button",
> +                                                 title: gPocketBundle.GetStringFromName("pocket-button.tooltiptext"),
> +                                                 image: "chrome://pocket/content/images/pocket.svg#pocket-mark"})

Nit: missing semicolon.

@@ +251,5 @@
> +                                                 title: gPocketBundle.GetStringFromName("pocket-button.tooltiptext"),
> +                                                 image: "chrome://pocket/content/images/pocket.svg#pocket-mark"})
> +        break;
> +      }
> +      case "Reader:pocket-button": {

This is no longer the right message topic. Did you test this change and/or is this patch outdated?

Comment 73

[Axel Hecht [:Pike]]

Comment on attachment 8695589 [details] [diff] [review]
part 5: l10n

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

Interesting commit message.

This should work on the build side of things.

I want to chat with some of my peeps in Orlando about how we can be more effective in helping localizers with refactorings like this.

Landing in the next cycle gives us some time for that, in particular before it hits the majority of locales in aurora.

Comment 74

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #71)
> Comment on attachment 8695585 [details] [diff] [review]
> part 3: move code/functionality from part 2 into the addon
> 
> Review of attachment 8695585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there, but the "not working!" comment is a bit scary...
> 
> You also seem to have changed the destination directory to extensions rather
> than features, was that intentional?

heh. after a recent update that changed the build system, I couldn't figure out why bootstrap.js was not being added to the directory after changing locations (thus the comment) with 'mach build browser'. It requires a full build to do so now. The location was changed to test restartless stuff, since when located in 'features' it is a system addon and hidden. anyhow, I'll fix the patch.

> ::: browser/components/customizableui/CustomizableUI.jsm
> @@ +61,5 @@
>>    * version the button is removed in as the value.  e.g. "pocket-button": 5
>>    */
>>   var ObsoleteBuiltinButtons = {
>> +  "loop-button": 5,
>> +  "pocket-button": 5
> 
> This should be 6 and we should increment kVersion, right?

depends on when we land this. if we land it now, it could be the same

Comment 75

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #74)

> > ::: browser/components/customizableui/CustomizableUI.jsm
> > @@ +61,5 @@
> >>    * version the button is removed in as the value.  e.g. "pocket-button": 5
> >>    */
> >>   var ObsoleteBuiltinButtons = {
> >> +  "loop-button": 5,
> >> +  "pocket-button": 5
> > 
> > This should be 6 and we should increment kVersion, right?
> 
> depends on when we land this. if we land it now, it could be the same

or maybe not, since some nightly users will already have 5

Comment 76

[Shane Caraveo (:mixedpuppy)]

(In reply to Axel Hecht [:Pike] from comment #73)
> Comment on attachment 8695589 [details] [diff] [review]
> part 5: l10n
> 
> Review of attachment 8695589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Interesting commit message.

that is so strange...I remember typing 'l10n support', but only once :)

> This should work on the build side of things.
> 
> I want to chat with some of my peeps in Orlando about how we can be more
> effective in helping localizers with refactorings like this.
> 
> Landing in the next cycle gives us some time for that, in particular before
> it hits the majority of locales in aurora.

I think that will be fine.

Comment 77

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon

last comments addressed

Comment 78

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

last comments addressed.  also fixed appearance of the pocket reader button if the pocket toolbar button is moved into/out of palette.

Comment 79

[Axel Hecht [:Pike]]

Could you share an hg bundle or a repo where I could pull the patches from? I'd like to try a thing or two locally, and that'd be easier than applying all the patches individually.

Comment 80

[:Gijs (he/him)]

Comment on attachment 8696037 [details] [diff] [review]
part 3: move code/functionality from part 2 into the addon

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

Alright, let's do it!

Sorry for the delays... weekend + mozlando travel and yesterday I lost this a bit... for future reference, if I'd realized earlier that the interdiff was so small ( https://bugzilla.mozilla.org/attachment.cgi?oldid=8695585&action=interdiff&newid=8696037&headers=1 ) it would have been quicker. :-)

Comment 81

[Shane Caraveo (:mixedpuppy)]

Attachment: combined patch

A single patch comprising all the previous individual patches.

Comment 82

[Shane Caraveo (:mixedpuppy)]

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

Comment 83

[Axel Hecht [:Pike]]

The treeherder builds complain in l10n-check, that's a sign that we need something on the build side. If that test passes, there might still be bugs, but if it fails, we're surely in trouble. The error message is in a code area where I'd look at glandium:


 11:19:09     INFO -  Error: Locale doesn't contain browser/features/[e-mail]/
 11:19:11     INFO -  Traceback (most recent call last):
 11:19:11     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 60, in <module>
 11:19:11     INFO -      main()
 11:19:11     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/toolkit/mozapps/installer/l10n-repack.py", line 56, in main
 11:19:11     INFO -      non_resources=args.non_resource, non_chrome=NON_CHROME)
 11:19:11     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/python/mozbuild/mozpack/packager/l10n.py", line 247, in repack
 11:19:11     INFO -      _repack(app_finder, l10n_finder, copier, formatter, non_chrome)
 11:19:11     INFO -    File "/tools/python27/lib/python2.7/contextlib.py", line 24, in __exit__
 11:19:11     INFO -      self.gen.next()
 11:19:11     INFO -    File "/builds/slave/try-lx-00000000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate
 11:19:11     INFO -      raise AccumulatedErrors()

Comment 84

[Shane Caraveo (:mixedpuppy)]

(In reply to Axel Hecht [:Pike] from comment #83)
> The treeherder builds complain in l10n-check, that's a sign that we need
> something on the build side. If that test passes, there might still be bugs,
> but if it fails, we're surely in trouble. The error message is in a code
> area where I'd look at glandium:

That try didn't actually include the l10n patch in this bug, I'll rerun it with that, but there are a number of other issues as well.

Comment 85

[Mike Hommey [:glandium]]

The l10n patch is probably not enough, you probably need to add something to browser/locales/Makefile.in.

Comment 86

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Hommey [:glandium] from comment #85)
> The l10n patch is probably not enough, you probably need to add something to
> browser/locales/Makefile.in.

any help in understanding what is necessary would be appreciated.

Comment 87

[Mike Hommey [:glandium]]

You need to add a $(MAKE) -C ... command in the libs-%: recipe in that file so that the pocket directory is traversed. I'm actually surprised we didn't do that for loop, btw, don't we have l10n for loop?

Comment 88

[Shane Caraveo (:mixedpuppy)]

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

Comment 89

[Shane Caraveo (:mixedpuppy)]

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

Comment 90

[Shane Caraveo (:mixedpuppy)]

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

Comment 91

[Shane Caraveo (:mixedpuppy)]

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

Comment 92

[Shane Caraveo (:mixedpuppy)]

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

Comment 93

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1: move pocket files into a feature add-on directory

slight directory structure changes/cleanup, carry forward r+

Comment 94

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3.2: removing dead code from pocket addon

bitrot refresh, carry forward r+

Comment 95

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3: move code/functionality from part 2 into the addon

directory structure and build changes for l10n.  added enabled pref support to enable/disable addon for test breakage (bug 1235627 for follow up).  carry forward r+

Comment 96

[Shane Caraveo (:mixedpuppy)]

Attachment: part 4: make reader extendable, move reader support to addon

directory structure update

Comment 97

[Shane Caraveo (:mixedpuppy)]

Attachment: part 5: l10n

l10n build update

Comment 98

[Shane Caraveo (:mixedpuppy)]

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

Comment 99

[Shane Caraveo (:mixedpuppy)]

Attachment: part 5: l10n

Comment 100

[Mike Hommey [:glandium]]

Comment on attachment 8702731 [details] [diff] [review]
part 5: l10n

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

Please fold before landing.

Comment 101

[Shane Caraveo (:mixedpuppy)]

Attachment: PocketSystemAddon (combined patches for landing)

Comment 102

[Shane Caraveo (:mixedpuppy)]

Attachment: minor test fixes, disabling pocket addon during mochitest, bug 1235627 for followup

Comment 103

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d
Bug 1215694 move pocket to a system addon, r=Gijs, r=glandium

Comment 104

[Shane Caraveo (:mixedpuppy)]

Happy New Year!

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ba655f6bc67

Comment 106

[Axel Hecht [:Pike]]

I'm pretty sure this breaks in language packs.

Seems there's nothing hooking up browser/features/[e-mail]/chrome.manifest.

Fuchsia:locale-fr axelhecht$ find . -name \*manif\*
./browser/chrome/fr.manifest
./browser/chrome.manifest
./browser/features/chrome.manifest
./browser/features/[e-mail]/chrome.manifest
./browser/features/[e-mail]/fr.manifest
./chrome/fr.manifest
./chrome.manifest
./webapprt/chrome/fr.manifest
./webapprt/chrome.manifest
Fuchsia:locale-fr axelhecht$ find . -name \*manif\* -exec grep -H getpocket {} \;
./browser/features/chrome.manifest:manifest [e-mail]/chrome.manifest application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

I haven't run a localized nightly yet that actually has their locale files moved, but the chrome.manifest in /Applications/FirefoxNightly.app/Contents/Resources/browser/features/[e-mail] looks OK to me.

Comment 107

[Francesco Lodolo [:flod]]

I moved them around for Italian but it didn't get into today's build (I have Pocket in English right now).
http://hg.mozilla.org/l10n-central/it/rev/6000f0fa38a4

Side note: if you put Pocket into Australis menu, instead of a toolbar, it's really broken.

Comment 108

[Shane Caraveo (:mixedpuppy)]

(In reply to Axel Hecht [:Pike] from comment #106)
> I'm pretty sure this breaks in language packs.
> 
> Seems there's nothing hooking up
> browser/features/[e-mail]/chrome.manifest.

Could you explain further?  As well, that in combination with the following comment that the manifest looks ok is a touch confusing.

Comment 109

[Axel Hecht [:Pike]]

It only half-way makes sense to me, TBH, I hope glandium knows if my assumption on what the files do are accurate.

Comment 110

[Mike Hommey [:glandium]]

Ah, that's because jarmaker puts the extra manifest in browser/features... and that one is not referenced anywhere. Please file a followup bug. A quick way around is to forcefully add a "manifest features/chrome.manifest" in "browser/chrome.manifest", using buildlist.py, when doing a repack.

Comment 111

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 44.0a1 (2015-10-16) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Firefox Developer Edition 46.0a2!

Build ID: 20160216004009
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 112

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Not yet Pocket has moved into the list of built in addon.

Component: 
Name 			Firefox
Version 		46.0b9
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

Expected Results: 
As per reporter, pocket should be in built in addon list.

Actual Results: 
Due to user privacy issues and customizations, it has seem to be cancelled.
I will go with firefox on this issue.