Closed Bug 1081093 Opened 5 years ago Closed 5 years ago

Auto-install Tools Adapter add-on in WebIDE

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(firefox34 unaffected, firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: jryans, Assigned: paul)

References

Details

Attachments

(2 files, 5 obsolete files)

Dave would like the Tools Adapter to auto-install, similar to ADB Helper today.
Paul, you added the ADB Helper auto-install, could you work on this?

I think we should should have a pref to control when the auto-install is done.
Flags: needinfo?(paul)
(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> Paul, you added the ADB Helper auto-install, could you work on this?
> 
> I think we should should have a pref to control when the auto-install is
> done.

Yes.

Can you add the addon to the ftp?
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #2)
> Can you add the addon to the ftp?

ftp://ftp.mozilla.org/pub/mozilla.org/labs/fxdt-adapters/
Can we re-host the addon source code to github.com/mozilla and make it so the homepage links to that page?
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Attachment #8504640 - Flags: review?(jryans)
I had to add a string. But I guess we can fallback to a builtin string if it's not present.
Comment on attachment 8504640 [details] [diff] [review]
v1

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

::: browser/devtools/webide/content/addons.js
@@ +80,5 @@
> +  switch (type) {
> +    case "adb":
> +    case "adapters":
> +      li.setAttribute("addon", type);
> +      name.textContent = Strings.GetStringFromName("addons_" + type + "_label");

What if we just used the "title" field of the add-on, instead of making up these special strings?

::: browser/devtools/webide/content/webide.js
@@ +78,5 @@
>          addons.adb.install();
>        }, console.error);
>      }
> +    if (autoinstallFxdtAdapters) {
> +      console.log("To auto install adapters");

Are you leaving these in, or just for debugging?

::: browser/devtools/webide/modules/addons.js
@@ +32,5 @@
>    }
>  }
>  
> +let addonsListener = {};
> +addonsListener.onEnabled =

This is a pretty unusual formatting choice... but I suppose it's clear enough.

@@ +46,3 @@
>    });
>  }
> +AddonManager.addAddonListener(addonsListener);

I guess doing this will help our add-on panel be consistent with changes in the regular add-on manager?  Sounds good!

@@ +105,5 @@
> +      if (addon && !addon.userDisabled) {
> +        this.status = "installed";
> +        return;
> +      }
> +      this.status = "preparing";

It looks like the |status| would now be stuck at "preparing" if the add-on is |userDisabled|.  Does that matter?

@@ +185,4 @@
>  
> +function AdaptersAddon() {
> +  EventEmitter.decorate(this);
> +  this.xpiLink = ADAPTERS_LINK.replace(/#OS#/g, OS);

So, blame me if you wish... but there is one naming difference with the adapters add-on.

For 32-bit Linux, the adapters add-on uses "linux32" instead of "linux" like the other add-ons.  It's probably enough to fix up the value of OS used in this constructor to support this case.

::: browser/devtools/webide/webide-prefs.js
@@ +5,5 @@
>  
>  pref("devtools.webide.showProjectEditor", true);
>  pref("devtools.webide.templatesURL", "https://code.cdn.mozilla.net/templates/list.json");
>  pref("devtools.webide.autoinstallADBHelper", true);
> +pref("devtools.webide.autoinstallFxdtAdapters", true);

I think we should keep it off for now, and then we can enable when the add-on is in a better state.  I filed bug 1082632 for this.

I'd like to finish my runtime API work and Dave would likely want to see the proxy work make more progress first.
Attachment #8504640 - Flags: review?(jryans)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #5)
> I need a review: https://github.com/campd/fxdt-adapters/pull/76

I reviewed this as well...  Wasn't sure if you could merge, so I merged it for you.
(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> ::: browser/devtools/webide/content/addons.js
> @@ +80,5 @@
> > +  switch (type) {
> > +    case "adb":
> > +    case "adapters":
> > +      li.setAttribute("addon", type);
> > +      name.textContent = Strings.GetStringFromName("addons_" + type + "_label");
> 
> What if we just used the "title" field of the add-on, instead of making up
> these special strings?

The addon might not be installed at that time, so you don't have the string.

> ::: browser/devtools/webide/content/webide.js
> @@ +78,5 @@
> >          addons.adb.install();
> >        }, console.error);
> >      }
> > +    if (autoinstallFxdtAdapters) {
> > +      console.log("To auto install adapters");
> 
> Are you leaving these in, or just for debugging?

Oops.

> @@ +46,3 @@
> >    });
> >  }
> > +AddonManager.addAddonListener(addonsListener);
> 
> I guess doing this will help our add-on panel be consistent with changes in
> the regular add-on manager?  Sounds good!

It was already the case, but I was using Devices and Simulator APIs to know their status.

> @@ +105,5 @@
> > +      if (addon && !addon.userDisabled) {
> > +        this.status = "installed";
> > +        return;
> > +      }
> > +      this.status = "preparing";
> 
> It looks like the |status| would now be stuck at "preparing" if the add-on
> is |userDisabled|.  Does that matter?

Calling `addon.userDisabled = false` will trigger `addonsListener.onEnabled`.

> @@ +185,4 @@
> >  
> > +function AdaptersAddon() {
> > +  EventEmitter.decorate(this);
> > +  this.xpiLink = ADAPTERS_LINK.replace(/#OS#/g, OS);
> 
> So, blame me if you wish... but there is one naming difference with the
> adapters add-on.
> 
> For 32-bit Linux, the adapters add-on uses "linux32" instead of "linux" like
> the other add-ons.  It's probably enough to fix up the value of OS used in
> this constructor to support this case.

Like the other add-ons? adb-helper use just linux. Can we be consistent with adb-helper?

> ::: browser/devtools/webide/webide-prefs.js
> @@ +5,5 @@
> >  
> >  pref("devtools.webide.showProjectEditor", true);
> >  pref("devtools.webide.templatesURL", "https://code.cdn.mozilla.net/templates/list.json");
> >  pref("devtools.webide.autoinstallADBHelper", true);
> > +pref("devtools.webide.autoinstallFxdtAdapters", true);
> 
> I think we should keep it off for now, and then we can enable when the
> add-on is in a better state.  I filed bug 1082632 for this.
> 
> I'd like to finish my runtime API work and Dave would likely want to see the
> proxy work make more progress first.

ok
Attached patch interdiff (obsolete) — Splinter Review
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #8504640 - Attachment is obsolete: true
Attachment #8505439 - Flags: review?(jryans)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #10)
> > @@ +185,4 @@
> > >  
> > > +function AdaptersAddon() {
> > > +  EventEmitter.decorate(this);
> > > +  this.xpiLink = ADAPTERS_LINK.replace(/#OS#/g, OS);
> > 
> > So, blame me if you wish... but there is one naming difference with the
> > adapters add-on.
> > 
> > For 32-bit Linux, the adapters add-on uses "linux32" instead of "linux" like
> > the other add-ons.  It's probably enough to fix up the value of OS used in
> > this constructor to support this case.
> 
> Like the other add-ons? adb-helper use just linux. Can we be consistent with
> adb-helper?

Hmm, changing that now would require publishing a bunch of symlinks or duplicate copies in case anyone's already installed a 32-bit Linux version of the add-on (or else they'd lose future updates).

Also, ADB Helper's approach is pretty strange since every other platform ends in the bit-value, even another 32-bit one (win32).

I think the better choice with less maintenance headaches is to fix up "linux" to "linux32" here for this add-on alone.
Comment on attachment 8505439 [details] [diff] [review]
v1.1

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

Looks great, but please do the linux -> linux32 fix up.
Attachment #8505439 - Flags: review?(jryans)
Comment on attachment 8505585 [details] [diff] [review]
v1.2 with s/linux/linux32

Didn't get the r+ actually.
Attachment #8505585 - Attachment description: v1.2 (r=jryans) with s/linux/linux32 → v1.2 with s/linux/linux32
Attachment #8505585 - Flags: review+ → review?(jryans)
Attachment #8505585 - Flags: review?(jryans)
Attached patch v1.2 with s/linux/linux32 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=1c6733bfe499
Attachment #8505438 - Attachment is obsolete: true
Attachment #8505439 - Attachment is obsolete: true
Attachment #8505585 - Attachment is obsolete: true
Attachment #8505601 - Flags: review?(jryans)
Attachment #8505601 - Flags: review?(jryans) → review+
Paul, is this ready to land?  Looks like some orange, but can't tell if it's your patch.
Flags: needinfo?(paul)
(In reply to J. Ryan Stinnett [:jryans] from comment #18)
> Paul, is this ready to land?  Looks like some orange, but can't tell if it's
> your patch.

It's my patch. Found the issue.
Flags: needinfo?(paul)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/369cc491a650
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Paul, don't forget to uplift this to Aurora for Dev Edition.
Flags: needinfo?(paul)
Approval Request Comment
[Feature/regressing bug #]: required for dev-edition
[User impact if declined]: missing feature
[Describe test coverage new/current, TBPL]: in m-c
[Risks and why]: new feature. Always a bit risky.
[String/UUID change made/needed]: none
Flags: needinfo?(paul)
Attachment #8510058 - Flags: approval-mozilla-aurora?
Attachment #8510058 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.