Closed Bug 1354590 Opened 7 years ago Closed 7 years ago

Add `temporary` flag to runtime.onInstalled details

Categories

(WebExtensions :: Developer Tools, enhancement, P5)

Product:
WebExtensions
Component:
Developer Tools
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zombie, Assigned: shatur, Mentored)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: triaged)

Attachments

(1 file)

Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.
59 bytes, text/x-review-board-request
zombie
: review+
aswan
: review+
Details 
Useful for turning on debugging features, like console.logs.
Mentor: tomica
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: triaged
Hey Tomislav!

I would like to work on this. Please, can you elaborate a little more on what needs to be done?

Thanks!!
Flags: needinfo?(tomica)
Hi Tushar, I've assigned the bug to you.

This would require a few things:
 - modify the AddonInstall.startInstall() method somewhere around [1], 
 - so that extraProperties includes a property named `temporarilyInstalled`, similar to [2],
 - which will be forwarded through `addonData` to the `onInstalled` implementation [3],
 - where you can then pass the flag as a property named `temporary`,
 - and finally, add a test to [4].

Let me know if you have any more questions.


1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5912
2) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7342
3) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-runtime.js#44\
4) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js
Assignee: nobody → tushar.saini1285
Status: NEW → ASSIGNED
Flags: needinfo?(tomica)
Actually, the above might not be 100% right, I'll post details after I investigate some more.
(In reply to Tomislav Jovanovic :zombie from comment #2)
>  - modify the AddonInstall.startInstall() method somewhere around [1], 
>
> 1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5912

This should be:
 - modify installAddonFromLocation() method somewhere around [1].

1) http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4111
Hey!

Sorry for late response, was a little bit busy.

I am not able to understand, what we want to accomplish in this bug. Please can you elobrate?

>  - where you can then pass the flag as a property named `temporary`,

Also, when we talk about "temporary" flag, is it similar to "reason"?

On basis of your instruction, done some changes, but not sure if I am in right direction. Please have a look.

Thanks!!
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review138530

Thanks, this is in the right direction, but not quite there.

::: toolkit/components/extensions/ext-runtime.js:44
(Diff revision 1)
>                  if (AddonManagerPrivate.browserUpdated) {
>                    fire.sync({reason: "browser_update"});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: extension.addonData.temporarilyInstalled});

The `temporarilyInstalled` flag will only be present when it's true, so you should ensure `temporary` is boolean:
```
    temporary: !!extension.addonData.temporarilyInstalled
```

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4138
(Diff revision 1)
>          }
>          let uninstallReason = installReason;
>  
>          extraParams.newVersion = newVersion;
>          extraParams.oldVersion = oldVersion;
> +        extraParams.temporarilyInstalled = this.temporarilyInstalled;

I don't think this will work.  Did you try it?

You probably need to compare the `aInstallLocation` argument to `TemporaryInstallLocation`.  Also, this likely shouldn't be inside the two if statements, as they are about calling the `shutdown` method.  The `install` is called a dozen lines below.
Attachment #8863418 - Flags: review?(tomica) → review-
> I am not able to understand, what we want to accomplish in this bug. Please
> can you elobrate?
We want the extension to be able to know if it was being installed as a temporary extension through about:debugging.


> Also, when we talk about "temporary" flag, is it similar to "reason"?
Yes, somewhat similar.  Extension can use `reason` to see why it was installed/updated, and `temporary` to see if that install was, well, temporary.
Hey!

I have made required changes. Please have a look and let me know if any further modifications is required.

I do not know for sure, but from my past experience, I think we need to update the schema of runtime [1], as I think temporary is a parameter of onInstalled event. Am I wrong?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/runtime.json#464

Regards
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review139806

This is close, but you are missing a test for when the flag is true.  To do that, you need to pass `useAddonManager: "permanent"` when loading the extension.  And yeah, you should add the flag to the json schema, and mark it optional.

::: toolkit/components/extensions/ext-runtime.js:40
(Diff revision 2)
>          onInstalled: new SingletonEventManager(context, "runtime.onInstalled", fire => {
>            let listener = () => {
>              switch (extension.startupReason) {
>                case "APP_STARTUP":
>                  if (AddonManagerPrivate.browserUpdated) {
> -                  fire.sync({reason: "browser_update"});
> +                  fire.sync({reason: "browser_update", temporary: !!extension.addonData.temporarilyInstalled});

I don't think an addon can be temporarily installed on APP_STARTUP.

::: toolkit/components/extensions/ext-runtime.js:44
(Diff revision 2)
>                  if (AddonManagerPrivate.browserUpdated) {
> -                  fire.sync({reason: "browser_update"});
> +                  fire.sync({reason: "browser_update", temporary: !!extension.addonData.temporarilyInstalled});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: !!extension.addonData.temporarilyInstalled});

nit: you can calculate the local value `temporary` once, and use it in both places.

::: toolkit/components/extensions/ext-runtime.js:47
(Diff revision 2)
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: !!extension.addonData.temporarilyInstalled});
>                  break;
>                case "ADDON_UPGRADE":
> -                fire.sync({reason: "update", previousVersion: extension.addonData.oldVersion});
> +                fire.sync({reason: "update", temporary: !!extension.addonData.temporarilyInstalled,

Why are you removing previousVersion here?  Don't do that.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 2)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(details.temporary, onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

details.temporary is optional, so we should make onInstalledTemporary optional as well, and default it to false.  In other words, just convert both to boolean before comparing them.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:142
(Diff revision 2)
>  
>    yield expectEvents(extension, {
>      onStartupFired: false,
>      onInstalledFired: true,
>      onInstalledReason: "install",
> +    onInstalledTemporary: false,

And given that we made it optional above, we don't actually need to add it when it is false, here or below.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4190
(Diff revision 2)
>      }
>  
>      let file = addon._sourceBundle;
>  
>      XPIProvider._addURIMapping(addon.id, file);
> +    extraParams.temporarilyInstalled = (aInstallLocation == TemporaryInstallLocation);

nits: please use ===, no need for parentheses, and I still think this would be better around line #4145, so that it can be picked up in the shutdown/uninstall case too.
Attachment #8863418 - Flags: review?(tomica) → review-
> you need to pass `useAddonManager: "permanent"` 

oops, I meant "temporary".
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review139862

Nice work Tushar!  

I'm asking Andrew to double-check in case I missed something, but r=me.

::: toolkit/components/extensions/ext-runtime.js:46
(Diff revision 3)
>                  if (AddonManagerPrivate.browserUpdated) {
>                    fire.sync({reason: "browser_update"});
>                  }
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary: temporary});

nit: you can just use property value shorthand [1], here and below, like so:

    fire.sync({reason: "install", temporary});

1) https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015
Attachment #8863418 - Flags: review?(tomica) → review+
Attachment #8863418 - Flags: review?(aswan)
Attachment #8863418 - Flags: review?(aswan)
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

Looking good but there should also be a test case that for regular installation, the temporary flag is false (this can probably just be added to an existing test)

::: toolkit/components/extensions/ext-runtime.js:49
(Diff revision 4)
>                  break;
>                case "ADDON_INSTALL":
> -                fire.sync({reason: "install"});
> +                fire.sync({reason: "install", temporary});
>                  break;
>                case "ADDON_UPGRADE":
> -                fire.sync({reason: "update", previousVersion: extension.addonData.oldVersion});
> +                fire.sync({reason: "update", temporary, previousVersion: extension.addonData.oldVersion});

this is getting big enough, please split this up with each property on its own line

::: toolkit/components/extensions/schemas/runtime.json:480
(Diff revision 4)
>                  "optional": true,
>                  "description": "Indicates the previous version of the extension, which has just been updated. This is present only if 'reason' is 'update'."
>                },
> +              "temporary": {
> +                "type": "boolean",
> +                "optional": true,

why is this marked optional?  you're always sending it...

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 4)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(!!details.temporary, !!onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

just give onInstalledTemporary a default value instead of using `!!` please
Attachment #8863418 - Flags: review?(aswan) → review-
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

The updated `expectEvents` always checks the value of `details.temporary`, and expects false by default, so I thought this was covered by existing tests.  Or did you have something else in mind?
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140350

::: toolkit/components/extensions/schemas/runtime.json:480
(Diff revision 4)
>                  "optional": true,
>                  "description": "Indicates the previous version of the extension, which has just been updated. This is present only if 'reason' is 'update'."
>                },
> +              "temporary": {
> +                "type": "boolean",
> +                "optional": true,

It's not sent on APP_STARTUP.  I guess we could just send `false`, but I didn't like it since I don't think it could ever be true in that case.
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review140344

Duh, of course.  That's kind of non-obvious though, how about forgetting the line-item comment about giving it a default value and just passing it in explicitly, its like a dozen call sites.
Comment on attachment 8863418 [details]
Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details.

https://reviewboard.mozilla.org/r/135184/#review141246

Nice, thanks!

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:65
(Diff revision 5)
> -function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledPrevious}) {
> +function* expectEvents(extension, {onStartupFired, onInstalledFired, onInstalledReason, onInstalledTemporary, onInstalledPrevious}) {
>    extension.sendMessage("get-on-installed-details");
>    let details = yield extension.awaitMessage("on-installed-details");
>    if (onInstalledFired) {
>      equal(details.reason, onInstalledReason, "runtime.onInstalled fired with the correct reason");
> +    equal(!!details.temporary, !!onInstalledTemporary, "runtime.onInstalled fired with the correct temporary flag");

nit: the !! isn't necessary on onInstalledTemporary

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js:328
(Diff revision 5)
>  
>    yield extension.markUnloaded();
>    yield promiseShutdownManager();
>  });
> +
> +add_task(function* test_when_addon_installed_temporarily() {

nit: this name could be shortened...
Attachment #8863418 - Flags: review?(aswan) → review+
Hey Tomislav!

I have made requested changes. I think this will be landable. Please have a look & do tell me if further modification is needed. Thanks for mentoring!!
Good job Tushar.  

The only thing left is to ask for landing by adding the checkin-needed keyword.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 666f6453f70f -d c9732a51552b: rebasing 395638:666f6453f70f "Bug 1354590 - Add 'temporary' flag to runtime.onInstalled details. r=aswan,zombie" (tip)
merging toolkit/components/extensions/ext-runtime.js
merging toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/test_ext_runtime_onInstalled_and_onStartup.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Hey Tomislav!

Test have been modified to use async/await from Task.jsm/yield, so I've modified the test. Can you please have a look before flagging checkin flag??

Thanks!!
Keywords: checkin-needed
Flags: needinfo?(tomica)
Looks good.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/830cc1aae18f
Add 'temporary' flag to runtime.onInstalled details. r=aswan,zombie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/830cc1aae18f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you, Tushar! Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition#May_2017. Your Mozillians profile has also been vouched. :) 

Welcome onboard! I look forward to seeing you around!
Thanks for the vouching!!
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onInstalled

Marking dev-doc-complete, but let me know if we need anything else.
Keywords: dev-doc-needed → dev-doc-complete
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: