Alexandre Poirot [:ochameau] Assignee
	Description • 10 years ago

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.

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 10 years ago

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.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 10 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 3 • 10 years ago

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, "application.zip");
> +    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

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 10 years ago

Updated according to feedback, still miss some unit tests.

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8544654 [details] [diff] [review]
patch v2

Landed on cypress:
  https://hg.mozilla.org/projects/cypress/rev/ba489cd69419

	Doug Sherk (:drs) (inactive)
	Comment 6 • 9 years ago

Alexandre, can we land this on m-c?

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 9 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 8 • 9 years ago

Do we want to land that on m-c or not?