Closed
Bug 1081093
Opened 10 years ago
Closed 10 years ago
Auto-install Tools Adapter add-on in WebIDE
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
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)
25.39 KB,
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
24.31 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Dave would like the Tools Adapter to auto-install, similar to ADB Helper today.
Reporter | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
(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)
Reporter | ||
Comment 3•10 years ago
|
||
(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/
Reporter | ||
Updated•10 years ago
|
Blocks: fx-dev-edition
Assignee | ||
Comment 4•10 years ago
|
||
Can we re-host the addon source code to github.com/mozilla and make it so the homepage links to that page?
Assignee | ||
Comment 5•10 years ago
|
||
I need a review: https://github.com/campd/fxdt-adapters/pull/76
Assignee | ||
Comment 6•10 years ago
|
||
Assignee: nobody → paul
Attachment #8504640 -
Flags: review?(jryans)
Assignee | ||
Comment 7•10 years ago
|
||
I had to add a string. But I guess we can fallback to a builtin string if it's not present.
Reporter | ||
Comment 8•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
(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
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8504640 -
Attachment is obsolete: true
Attachment #8505439 -
Flags: review?(jryans)
Reporter | ||
Comment 13•10 years ago
|
||
(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.
Reporter | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ae28ca6e5f5d
Attachment #8505585 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8505585 -
Flags: review?(jryans)
Assignee | ||
Comment 17•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8505601 -
Flags: review?(jryans) → review+
Reporter | ||
Comment 18•10 years ago
|
||
Paul, is this ready to land? Looks like some orange, but can't tell if it's your patch.
Flags: needinfo?(paul)
Assignee | ||
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ecae8c8c598b
Attachment #8505601 -
Attachment is obsolete: true
Attachment #8506781 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/369cc491a650
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/369cc491a650
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Reporter | ||
Comment 23•10 years ago
|
||
Paul, don't forget to uplift this to Aurora for Dev Edition.
Flags: needinfo?(paul)
Assignee | ||
Comment 24•10 years ago
|
||
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?
Reporter | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8510058 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d7a6ba48e98
Flags: in-testsuite+
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•