Allow reloading a temporary add-on

VERIFIED FIXED in Firefox 48

Status

P2
normal
VERIFIED FIXED
3 years ago
10 months ago

People

(Reporter: andy+bugzilla, Assigned: kumar)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

43 Branch
Firefox 48
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified, relnote-firefox 48+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
So you've loaded a temporary add-on, supposing you make changes? It's really awkward to close Firefox and reload the add-on (after all its only temporary). What would be really nice is a re-install (or reload of the add-on). If we land the API in bug 1225944 then we can give a nice button or link to that.
Depends on: 1225944
Alex, is this something you'd be interested in taking on / mentoring?
Flags: needinfo?(poirot.alex)
I don't have the bandwith to take this bug but I can review/mentor it.

I think we should not do much thing specific to temporary addon. At least regarding the UI.
about:debugging miss the enable/disable + remove buttons about:addons has.
I imagine we should start with those and may be come up with a reload button which would be handy for developers.
Flags: needinfo?(poirot.alex)
I'll try it out
Assignee: nobody → kumar.mcmillan
Priority: -- → P2
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

https://reviewboard.mozilla.org/r/43557/#review40193

I have a bunch of simplications here and there but it looks good overall!
Could you split this patch in two:
 - /devtools/ r? again me once comments are addressed
 - /toolkit/ r? mossop for this part
Bonus point if you also split the /devtools/ part in two:
 - using listAddons and the actors (addons-tab.js modification + actors/addon.js (only isRestartless and iconURL) + I imagine some tests fixes
 - the rest about the reload button
 But again, just a bonus, your patch isn't terribly complex/big.

::: devtools/client/aboutdebugging/components/addons-tab.js:85
(Diff revision 1)
>  
>      this.setState({ debugDisabled });
>    },
>  
>    updateAddonsList() {
> -    AddonManager.getAllAddons(addons => {
> +    this.props.client.listAddons(({ addons, error }) => {

We recently enable promises on all client APIs.

this.props.client.listAddons()
    .then(({ addons }) => { ... onSuccess ... },
          error => { ... onError .... });

or

updateAddonsList: Task.async(function *() {
  let { addons } = yield this.props.client.listAddons();

(we will someday deprecate and remove the old callback style)
(the promise is rejected in case of error)

::: devtools/client/aboutdebugging/components/addons-tab.js:93
(Diff revision 1)
> +      }
> +      let extensions = addons.filter(addon => addon.debuggable).map(addon => {
>          return {
>            name: addon.name,
>            icon: addon.iconURL || ExtensionIcon,
> -          addonID: addon.id
> +          type: addon.type,

You pass addon.type but are not using it anywhere.
I prefer to wait for a first usage before introducing any code.
If you remove it, please also remove it from the actor.

::: devtools/client/aboutdebugging/test/browser_addons_install.js:20
(Diff revision 1)
> -    "The addon name appears in the list of addons: " + names);
> -
>    // Now uninstall this addon
> +  const addonsContainer = document.getElementById("addons");
> +  const onAddonRemoved = waitForMutation(addonsContainer,
> +    { subtree: true, childList: true });

Same comment than waitForAddonToAppear and my colliding patch.
You could only listen for { childList: true } by targeting "#addon .targets"

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:27
(Diff revision 1)
> +    addonsFound.push(name);
> +    if (name === ADDON_NAME) {
> +      addon = candidate;
> +      break;
> +    }
> +  }

We already do such thing in browser_service_workers_push.js, for consistency, you could do that:
let names = [...document.querySelectorAll("#addons .target-name")];
  let name = names.filter(element => element.textContent === ADDON_NAME)[0];
  ok(name, "Found the addon in the list");
  let targetElement = name.parentNode.parentNode;
  let reloadBtn = targetElement.querySelector(".reload-button");

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:31
(Diff revision 1)
> +    }
> +  }
> +  ok(addon, "Located " + ADDON_NAME + " add-on in list: " + addonsFound);
> +
> +  const onDisabled = promiseAddonEvent("onDisabled");
> +  const onEnabled = promiseAddonEvent("onEnabled");

Ideally you wouldn't use AddonManager at all except in the actor. So that we could run this test against a remote target. Like against fennec!
But we are not there yet so I wouldn't r- for this. This would just be a nice to have.
BrowserAddonActor already register an AddonListener. It implements onDisabled/onUninstalled, but doesn't do much when it receives onDisabled.

I can mentor you about that, but in a followup as it isn't that simple.
The followup would be to correctly handle addon enable/disable state in about:debugging, which we currently doesn't support at all.

::: devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js:36
(Diff revision 1)
>    ok(debugButtons.every(b => b.disabled), "Debug buttons should be disabled");
>  
>    info("Click on 'Enable addons debugging' checkbox.");
>    let addonsContainer = document.getElementById("addons");
>    let onAddonsMutation = waitForMutation(addonsContainer,
> -    { subtree: true, attributes: true });
> +    { subtree: true, attributes: true, childList: true });

Is this really necessary?
Do you know why?

::: devtools/client/aboutdebugging/test/head.js:147
(Diff revision 1)
> + * @return {Promise}
> + */
> +function waitForAddonToAppear(document, addonName) {
> +  return waitForMutation(
> +    document.getElementById("addons"),
> +    { subtree: true, attributes: true, childList: true },

you should just need { childList: true }
if you target document.querySelector("#addons .targets")

::: devtools/client/aboutdebugging/test/head.js:149
(Diff revision 1)
> +function waitForAddonToAppear(document, addonName) {
> +  return waitForMutation(
> +    document.getElementById("addons"),
> +    { subtree: true, attributes: true, childList: true },
> +    () => {
> +      let names = [...document.querySelectorAll("#addons .target-name")];

Please do not introduce this test function argument to waitForMutation, there shouldn't be multiple addons in process of being installed.
You can yield on the waitForMutation call and immediately after that assert. Like what we used to do:
 let names = [...document.querySelectorAll("#addons .target-name")];
  names = names.map(element => element.textContent);
  ok(names.includes(ADDON_NAME),
    "The addon name appears in the list of addons: " + names);

At the end, we were just missing the right waitForMutation before this assert.

I do have a patch in bug 1261055 (the second) to refactor exactly that. I can pull it off in a dedicated bug to land it first if that helps. Or I can rebase after yours, tell me.

::: devtools/client/aboutdebugging/test/head.js:171
(Diff revision 1)
>   * @param {Object} mutationOptions
> + * @param {Function} test - Callback that should return true when the mutation
> + *                          changed what you wanted.
>   * @return {Promise}
>   */
> -function waitForMutation(target, mutationOptions) {
> +function waitForMutation(target, mutationOptions, test=() => true) {

So please remove this test function.
Attachment #8736790 - Flags: review?(poirot.alex)
About that:

> I do have a patch in bug 1261055 (the second) to refactor exactly that. I
> can pull it off in a dedicated bug to land it first if that helps. Or I can
> rebase after yours, tell me.

I got my patch r+, I can land it if that helps, attachment 8737187 [details] [diff] [review].
So feel free to rebase on top of it.
https://reviewboard.mozilla.org/r/43557/#review40193

> Please do not introduce this test function argument to waitForMutation, there shouldn't be multiple addons in process of being installed.
> You can yield on the waitForMutation call and immediately after that assert. Like what we used to do:
>  let names = [...document.querySelectorAll("#addons .target-name")];
>   names = names.map(element => element.textContent);
>   ok(names.includes(ADDON_NAME),
>     "The addon name appears in the list of addons: " + names);
> 
> At the end, we were just missing the right waitForMutation before this assert.
> 
> I do have a patch in bug 1261055 (the second) to refactor exactly that. I can pull it off in a dedicated bug to land it first if that helps. Or I can rebase after yours, tell me.

ok, let me try and get the test working without it. I think using the actor introduced some latency (or something) where the initial mutation did not include the new add-on but the second mutation did. Maybe I can fix that with a more direct mutation selector as you suggested.
https://reviewboard.mozilla.org/r/43557/#review40193

> You pass addon.type but are not using it anywhere.
> I prefer to wait for a first usage before introducing any code.
> If you remove it, please also remove it from the actor.

Good catch. It was left over from before addons-tab.js had its own component.
https://reviewboard.mozilla.org/r/43557/#review40193

> Is this really necessary?
> Do you know why?

I figured out a better way. This along with the other tests need to start by waiting for the initial list of add-ons because using the add-on actor introduces some latency. I'll post the next revision for review when I fix everything else up.
https://reviewboard.mozilla.org/r/43557/#review40193

> Ideally you wouldn't use AddonManager at all except in the actor. So that we could run this test against a remote target. Like against fennec!
> But we are not there yet so I wouldn't r- for this. This would just be a nice to have.
> BrowserAddonActor already register an AddonListener. It implements onDisabled/onUninstalled, but doesn't do much when it receives onDisabled.
> 
> I can mentor you about that, but in a followup as it isn't that simple.
> The followup would be to correctly handle addon enable/disable state in about:debugging, which we currently doesn't support at all.

ok, I'll try to do it in a follow up just so it doesn't hold up this patch too long.

> ok, let me try and get the test working without it. I think using the actor introduced some latency (or something) where the initial mutation did not include the new add-on but the second mutation did. Maybe I can fix that with a more direct mutation selector as you suggested.

I looked at the patch in bug 1261055 but I don't think it will work as is once I introduce the switch to using the add-on actor. In that case, I think it would be easier if you rebased on top of mine. Let me know what you think.
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/1-2/
Attachment #8736790 - Flags: review?(poirot.alex)
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

https://reviewboard.mozilla.org/r/43557/#review40605

Looks good. Did you got a green try?

::: devtools/client/aboutdebugging/components/addon-target.js:46
(Diff revision 2)
> +       dom.button({
> +          className: "reload-button",
> +          onClick: this.reloadAddon
> +        }, Strings.GetStringFromName("reload")) :
> +       null
> +      )

Please execute ./mach eslint devtools/client/aboutdebugging/

There is indentation issues here.
Note that there is a bug of warnings in about:debugging that are not related to your patch.
You should only fix errors.

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:10
(Diff revision 2)
> +const ADDON_ID = "test-devtools@mozilla.org";
> +const ADDON_NAME = "test-devtools";
> +
> +add_task(function* () {
> +  const { tab, document } = yield openAboutDebugging("addons");
> +  yield waitForInitialAddonList(document)

Missing `;`

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:30
(Diff revision 2)
> +  const reloadButton = targetElement.querySelector(".reload-button");
> +  ok(reloadButton, "Found its reload button");
> +
> +  const onDisabled = promiseAddonEvent("onDisabled");
> +  const onEnabled = promiseAddonEvent("onEnabled");
> +

You may also ensure that bootstrap.js `install` function is being called by listening for the event being sent from it:
let onBootstrapInstallCalled = new Promise(done => {
  Services.obs.addObserver(function listener() {
    Services.obs.removeObserver(listener, "test-devtools", false);
    done();
  }, "test-devtools", false);
});

::: devtools/client/aboutdebugging/test/browser_addons_reload.js:37
(Diff revision 2)
> +
> +  const [disabledAddon] = yield onDisabled;
> +  ok(disabledAddon.name === ADDON_NAME, "Add-on was disabled: " + disabledAddon.name);
> +
> +  const [enabledAddon] = yield onEnabled;
> +  ok(enabledAddon.name === ADDON_NAME, "Add-on was re-enabled: " + enabledAddon.name);

Lines 34 and 37 are longer than 80.

::: devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js:22
(Diff revision 2)
>      ]};
>      SpecialPowers.pushPrefEnv(options, resolve);
>    });
>  
>    let { tab, document } = yield openAboutDebugging("addons");
> +  yield waitForInitialAddonList(document)

Missing `;`

::: devtools/client/aboutdebugging/test/head.js:7
(Diff revision 2)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  /* eslint-env browser */
>  /* eslint-disable mozilla/no-cpows-in-tests */
>  /* exported openAboutDebugging, closeAboutDebugging, installAddon,
> -   uninstallAddon, waitForMutation, assertHasTarget,
> +   promiseAddonEvent, uninstallAddon, waitForMutation, assertHasTarget,

Then do not forget to remove this.

::: devtools/client/aboutdebugging/test/head.js:251
(Diff revision 2)
> + * resolved value is an array of arguments passed for the event.
> + */
> +function promiseAddonEvent(event) {
> +  return new Promise(resolve => {
> +    let listener = {
> +      [event]: function(...args) {

Whaa I didn't knew this `[event]` notation.
JS is going crazy :-o

::: devtools/client/aboutdebugging/test/head.js:255
(Diff revision 2)
> +    let listener = {
> +      [event]: function(...args) {
> +        AddonManager.removeAddonListener(listener);
> +        resolve(args);
> +      }
> +    }

Missing `;`

::: devtools/client/aboutdebugging/test/head.js:259
(Diff revision 2)
> +      }
> +    }
> +
> +    AddonManager.addAddonListener(listener);
> +  });
> +}

This is only used by reload test and I would like to avoid using this. Could you move it to reload test?
Attachment #8736790 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/43557/#review40675

::: devtools/client/aboutdebugging/test/head.js:148
(Diff revision 2)
> + */
> +function waitForInitialAddonList(document) {
> +  return new Promise(resolve => {
> +    // The actor can be slow to return the initial list of add-ons
> +    // so this waits for it when necessary.
> +    if ([...document.querySelectorAll("#addons .targets .target")].length) {

Does that ever happen? (targets elements already created)

When launching tests locally, I always go through the mutation observer.
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Does that ever happen? (targets elements already created)
> 
> When launching tests locally, I always go through the mutation observer.

I always go through the mutation observer as well but because it's a timing issue it seemed safer to account for faster machines. Theoretically a faster machine could beat the race. I can remove that check if you think it's too unlikely. 

I just finished revisions on the toolkit side so I'll post another review request shortly.
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/2-3/
Attachment #8736790 - Flags: review?(poirot.alex)
There are also some minor changes related to bug 1261522
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/3-4/
Attachment #8736790 - Flags: review?(poirot.alex) → review+
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

https://reviewboard.mozilla.org/r/43557/#review41243

Be careful, there is now eslint error in your /toolkit/ patch:

TEST-UNEXPECTED-ERROR | toolkit/mozapps/extensions/test/xpcshell/test_isReloadable.js:26:8 | "addon" is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | toolkit/mozapps/extensions/test/xpcshell/test_isReloadable.js:27:5 | "addon" is not defined. (no-undef)

::: devtools/server/actors/addon.js:168
(Diff revision 4)
> +        this.conn.send({ from: this.actorID, type: "addonReloaded" });
> +      })
> +      .catch(error => {
> +        DevToolsUtils.reportException("BrowserAddonActor.prototype.onReload",
> +                                      error);
> +      });

addonReloaded is never used. Please get rid of it.
If you want the reload request to fail if `_addon.reload` fails, just return `this._addon.reload()`. It will pipe the error back to the client. It will also hold the request until the promise is finished and only resolve the client request promise when the request is done.

The client can then do:
  this.client.request(...).then(success, error);
https://reviewboard.mozilla.org/r/43557/#review40675

> Does that ever happen? (targets elements already created)
> 
> When launching tests locally, I always go through the mutation observer.

If try is green, yes, drop it.

About:debugging tests try to always be confident about the mutation we observe.
If we have a doubt, we modify the test to be sure of what and when something is modified.
So that we track unecessary reflow/DOM modifications!
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/4-5/
ochameau, could you take another look? This simplifies the patch after the latest bug 1261522 review. The biggest change is I'm no longer checking if the add-on can be reloaded or not. When trying to write a test, I realized there is currently no way to inject an add-on into about:debugging that cannot be reloaded because of the isDebuggable check ;)
Flags: needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/43557/#review41599

::: devtools/client/aboutdebugging/components/addon-target.js:57
(Diff revision 5)
> +  reloadAddon() {
> +    let { client, target } = this.props;
> +    client.request({
> +      to: target.addonActor,
> +      type: "reload"
> +    }).then(() => {}, (error) => {

s/.then(() => {}, (error) =>/.catch(error =>/

- `catch` is helpful when you care only about rejection
- if you have only one argument, parentheses are optional for arrow functions.
https://reviewboard.mozilla.org/r/43557/#review41599

> s/.then(() => {}, (error) =>/.catch(error =>/
> 
> - `catch` is helpful when you care only about rejection
> - if you have only one argument, parentheses are optional for arrow functions.

Oh neat, that works. I tried it on another object but I got 'catch is undefined'.
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/5-6/
Flags: needinfo?(poirot.alex)
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/6-7/
Attachment #8736790 - Attachment description: MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r?ochameau → MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Oops. I was convinced you finally landed that patch...
Looks like you will have to rebase on top of bug 1261055 which I landed at about the same time Ryan tried to land yours.
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/7-8/
Fixing conflicts was straight forward. It's rebased now and all tests pass on Try.
Keywords: checkin-needed
Hi, hate to ask but this might need rebasing again :(

apply changeset? [ynmpcq?]: y
applying 376761595a46
patching file devtools/client/aboutdebugging/components/addon-target.js
Hunk #1 FAILED at 30
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/aboutdebugging/components/addon-target.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

can you take a look ?
Flags: needinfo?(kumar.mcmillan)
Keywords: checkin-needed
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/8-9/
Fixed up and ready to go! Try build is passing
Flags: needinfo?(kumar.mcmillan)
Keywords: checkin-needed
sorry had to back this out for timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=8711939&repo=fx-team
Flags: needinfo?(kumar.mcmillan)
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/9-10/
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

https://reviewboard.mozilla.org/r/43557/#review43361

This seems like a resonable plan to me.
Comment on attachment 8736790 [details]
MozReview Request: Bug 1246030 - Allow reloading an add-on in about:debugging. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43557/diff/10-11/
OK, this should account for timing issues better in waitForInitialAddonList(). Passed a Try run.
Flags: needinfo?(kumar.mcmillan)
Keywords: checkin-needed

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5b30ad336bd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Keywords: dev-doc-needed
Release Note Request (optional, but appreciated)
[Why is this notable]: Allows add-on developers to easily reload their work to apply changes during development without restarting Firefox.
[Suggested wording]: Reload add-ons from about:debugging for easy add-on development
[Links (documentation, blog post, etc)]: None yet.
relnote-firefox: --- → ?
Added to Fx48 Aurora release notes
relnote-firefox: ? → 48+
Updated:
https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Updating_a_temporary_add-on
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Packaging_and_installation#Updating_a_temporary_add-on

I would like to talk only about the "Reload" button here, and not mention the restarting-Firefox hack. But I think this ought to wait until Firefox 48 is in the release channel.

Let me know if this covers it, please!
Flags: needinfo?(kumar.mcmillan)
Looks good! I just have one question about this:

> Note that loading the add-on again by pressing "Load Temporary Add-on" without restarting Firefox does not work.

In what way does it not work? It should work. This action will replace the old add-on and fully reload it. However, if you noticed that the name does not get updated, that bug will be fixed as part of bug 1267870. More specifically, the about:debugging page has a bug where it does not empty the cache on re-installation (it only empties it when the add-on is uninstalled).
Flags: needinfo?(kumar.mcmillan) → needinfo?(wbamberg)
(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #45)
> Looks good! I just have one question about this:
> 
> > Note that loading the add-on again by pressing "Load Temporary Add-on" without restarting Firefox does not work.
> 
> In what way does it not work? It should work. This action will replace the
> old add-on and fully reload it.

I'm sorry, you're right, this does work from Firefox 48. I still think this is worth noting, though, until Firefox 48 is in the release channel, since it's an obvious thing to try.

I've reworked this to be more explicit about versions though. Does that look all right now?
Flags: needinfo?(wbamberg) → needinfo?(kumar.mcmillan)
ok. I personally don't think it's so crucial to mention because the effect is only cosmetic -- that is, it's only the display name in about:debugging. The manifest permissions are still applied, which is a more crucial issue. In my experience, when there are a lot of notes in documentation about caveats, I tend to get overwhelmed.
Flags: needinfo?(kumar.mcmillan)
(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #47)
> ok. I personally don't think it's so crucial to mention because the effect
> is only cosmetic -- that is, it's only the display name in about:debugging.
> The manifest permissions are still applied, which is a more crucial issue.

It sounds like there are two things here, which are getting mixed up.

** thing 1 **

Before Fx48, if you try to update a temporary add-on by clicking "Load Temporary Add-on" again, and selecting the add-on again, this does not work. "Does not work" here means that the add-on is not updated (a warning is logged to the Browser Console). It's not a cosmetic problem.

I think this is definitely worth mentioning, as it's a very obvious thing to try. We can remove this when Fx48 is in release channel.

** thing 2 **

After Fx48, "reload" does not update the name and description fields in about:debugging and about:addons.

This is arguably a cosmetic problem (although name and description are also functional elements).

Still, I think it is worth mentioning, because the first thing someone will try if they are trying out "Reload" will be to change the name of the add-on and see it update. This is precisely *because* it's a cosmetic thing, giving immediate feedback that the reload worked. If people try this, the feature will appear broken. Telling people that it isn't broken, despite this appearance, seems worth doing.

> In my experience, when there are a lot of notes in documentation about
> caveats, I tend to get overwhelmed.

I agree! It's a judgement call about when such caveats should be included, and when they should not. My judgement in this case is to include them.
Keywords: dev-doc-needed → dev-doc-complete
> Before Fx48, if you try to update a temporary add-on by clicking "Load
> Temporary Add-on" again, and selecting the add-on again, this does not work.
> "Does not work" here means that the add-on is not updated (a warning is
> logged to the Browser Console). It's not a cosmetic problem.

oh, right! That bug was fixed so long ago I forgot :) I agree that it's worth mentioning.
Verified fixed on Firefox 48.0.2 (20160823121617),Firefox 49 (20160912134115),Firefox 50.0a2 (2016) and Firefox 51.0a1 (2016-09-14) under Windows 10 64-bit. The “Reload” button is successfully displayed and the webextensions are updated without restarting Firefox.
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.