Closed Bug 696520 Opened 8 years ago Closed 8 years ago

Enable add-on installation

Categories

(Firefox for Android :: General, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- verified
fennec 11+ ---

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Blocks: 696518
Attached patch wip (obsolete) — Splinter Review
This patch adds an "Add-ons" entry to the menu, that loads about:addons
XPInstallObserver just launch install() for non-trusted sites for now (waiting on bug 694672).

With this patch, and those in bug 696324 and 696187 I'm able to install the sample "phony" add-on located at http://people.mozilla.com/~fdesre/extensions/
Assignee: nobody → fabrice
Priority: -- → P1
Attached patch updated WIP (obsolete) — Splinter Review
Updated wip: I used prompts instead of a notification box - not ideal, but usable.
Attachment #568821 - Attachment is obsolete: true
Attachment #569252 - Flags: feedback?(mark.finkle)
Comment on attachment 569252 [details] [diff] [review]
updated WIP


>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+var XPInstallObserver = {
>+  observe: function xpi_observer(aSubject, aTopic, aData)
>+  {

Put the  { on the line above

>+    switch (aTopic) {
>+      case "addon-install-started":
>+        //var messageString = Strings.browser.GetStringFromName("alertAddonsDownloading");
>+        //ExtensionsView.showAlert(messageString);

At some point, this could be a toaster alert, maybe?

>+      case "addon-install-blocked":
>+        dump("XPInstallObserver addon-install-blocked");
>+        var installInfo = aSubject.QueryInterface(Ci.amIWebInstallInfo);
>+        var host = installInfo.originatingURI.host;
>+        // XXX for some reason Strings.brand is undefined...

Add blank line before comment for strings

>+        catch (e) {}
>+        if (!enabled) {

Add a blank line before the if line

>+          }
>+          else {
>+            messageString = strings.formatStringFromName("xpinstallDisabledMessage2", [brandShortName, host], 2);

} else {

>+        }
>+        else {
>+          notificationName = "xpinstall";

} else {

>+          buttons = [{
>+            label: strings.GetStringFromName("xpinstallPromptAllowButton"),
>+            accessKey: null,
>+            popup: null,

When we switch to DoorHangers, we can drop "accessKey" maybe "popup" too?

Use more "let" and less "var" :)
Attachment #569252 - Flags: feedback?(mark.finkle) → feedback+
JS helper to use the doorhanger notifications independently from the content permissions prompts.
Attachment #569252 - Attachment is obsolete: true
Attachment #569484 - Flags: review?(mark.finkle)
Addressed previous comments.
Attachment #569486 - Flags: review?(mark.finkle)
Comment on attachment 569484 [details] [diff] [review]
part 1/2 : doorhanger helper

>+    // XXX Test, remove me!
>+    NativeWindow.doorhanger.show("Hello World!",
>+      [
>+        { label: "foo", callback: function() {dump("Gecko foo");} },
>+        { label: "bar", callback: function() {dump("Gecko bar");} },
>+      ]);

Remember to remove

>+  doorhanger: {

>+    show: function(aMessage, aButtons) {
>+      // get the tabId of the current tab
>+      let tabID = BrowserApp.selectedTab.id;

Let's allow a tab to be optional passed in as the last param, just in case:

      show: function(aMessage, aButtons, aTab) {
        // Use thecurrent tab, if none is provided
        let tabID = (aTab ? aTab.id : BrowserApp.selectedTab.id);

>+      aButtons.forEach((function(aButton) {
>+        this._callbacks[this._callbacksId] = { cb: aButton.callback, prompt: this._promptId };
>+        aButton.callback = this._callbacksId;
>+        this._callbacksId++;
>+      }).bind(this));
>+      this._promptId++;

Add a blank line before the promptId++

>   observe: function(aSubject, aTopic, aData) {
>+        this.menu._callbacks[aData]();
>+    }
>+    else if (aTopic == "Doorhanger:Reply") {

} else {

Nice!

I suppose it might be possible (and rare) for these doorhangers to conflict with the ContentPersmission.js doorhangers. Since you register a "Doorhanger:Reply" observer all the time, you _will_ catch the ContentPermission notifications. Hopefully, you won't have any active door hangers though. Also, your callbackId's will hopefully not conflict with the ContentPermissions Id's

Since ContentPermission access BrowserApp, we might be able to just change it to use NativeWindow.doorhangers ?
Attachment #569484 - Flags: review?(mark.finkle) → review+
Comment on attachment 569486 [details] [diff] [review]
part 2/2 : enable add-ons

It would be nice to figure out the Strings.brand issue
Attachment #569486 - Flags: review?(mark.finkle) → review+
pushed:
https://hg.mozilla.org/projects/birch/rev/5bd3a03bc11b
https://hg.mozilla.org/projects/birch/rev/04e3f9b03522
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
tracking-fennec: --- → 11+
Verified fixed on:
Nightly 13.0a1 (2012-02-21)
Beta 11.0 (20120221151724)
Device: Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.