Closed Bug 1114681 Opened 5 years ago Closed 3 years ago

Import runtime app as local project


(DevTools :: WebIDE, defect, P2)

Windows 7


(Not tracked)



(Reporter: ochameau, Assigned: ochameau)



(Whiteboard: [spark])


(1 file, 2 obsolete files)

It would help hacking core apps, or you phone, if we could pull any app installed on the phone and start working on it right away in WebIDE.
It is overlapping recent suggestion from kgrandon like bug 1045073 comment 6, but it is more generic. The main drawback of pulling sources from the device is that this isn't the original app sources, so that it will be harder to contribute back to the original project by providing a patch agains the source tree. It is more for hacking than contributing.
Attached patch wip (obsolete) — Splinter Review
Here is a prototype patch that allows importing packaged app from device as a local project.
It misses some polish, tests and also take care of hosted apps.
Attached patch patch v1 (obsolete) — Splinter Review
Added hosted app support.

Ryan, I don't feel confident about calling webide.js from app-manager.js,
but I don't know how to do that differently, any idea?
See app-manager.js: fetchSelectedProject().

I would like to provide some unit test, at least against the actor before proceeding to the review.
Attachment #8540309 - Attachment is obsolete: true
Attachment #8542629 - Flags: feedback?(jryans)
Comment on attachment 8542629 [details] [diff] [review]
patch v1

Review of attachment 8542629 [details] [diff] [review]:

Cool, this seems neat overall!

At a high level, I think we can make this fit existing WebIDE patterns by inverting control so that we go details pane -> webide.js -> app-manager.js, instead of details -> app-manager -> webide.  I've suggested how to do it in the comments.

::: browser/devtools/webide/content/details.js
@@ +132,5 @@
>    AppManager.removeSelectedProject();
>  }
> +
> +function fetchProject() {
> +  AppManager.fetchSelectedProject();

Instead of going to |AppManager|, call out to WebIDE first, so something like |window.parent.UI.fetchProject|.

::: browser/devtools/webide/content/webide.js
@@ +1034,5 @@
>        yield UI.importAndSelectApp(url);
>      }), "importing hosted app");
>    },
> +  fetchPackagedApp: function(packagePath) {

After making the suggested details.js change, we'll come here first before app-manager.  You should change this to call |AppManager| to fetch, and then proceed from there.

I think you should move this to |UI| instead of |Cmds|...  I've only put things in |Cmds| if it's called by a XUL <command>, though admittedly the whole distinction seems a bit silly to me.

::: browser/devtools/webide/modules/app-manager.js
@@ +339,5 @@
>    get selectedRuntime() {
>      return this._selectedRuntime;
>    },
> +  fetchSelectedProject: function () {

This will now be called by webide.js instead of the other way around.  So, you should return whatever webide.js needs from here, instead of calling webide.js methods.

::: toolkit/devtools/server/actors/webapps.js
@@ +1062,5 @@
> +    let info = reg.getAppInfo(id);
> +    let packagePath = OS.Path.join(info.path, "");
> +    let packageFile = FileUtils.File(packagePath);
> +    let fileSize = packageFile.fileSize;
> +    this.conn.startBulkSend({

I am quite happy to see a second use of bulk data finally! :D
Attachment #8542629 - Flags: feedback?(jryans) → feedback+
Attached patch patch v2Splinter Review
Updated according to feedback, still miss some unit tests.
Attachment #8542629 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
Alexandre, can we land this on m-c?
Blocks: cypress
No longer blocks: spark
Flags: needinfo?(poirot.alex)
Priority: -- → P2
Whiteboard: [lightsaber]
This patch was more like a prototype for lightsaber demos, we would need to decide what to do with these protos.
This feature is very similar to the git import one and we would need to coordinate if we want both as it can easily become disturbing.
I wish we would have received some feedback about WebIDE during lightsaber protos,
but I haven't received any from lightsaber group :(
I'm not sure anyone dogfooded this feature except me and this doesn't seem to have received any traction.
Flags: needinfo?(poirot.alex)
Whiteboard: [lightsaber] → [ignite]
Do we want to land that on m-c or not?
Blocks: spark-webide
No longer blocks: cypress
Whiteboard: [ignite] → [spark]
Closed: 3 years ago
Resolution: --- → WONTFIX
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.