Closed Bug 1124326 Opened 8 years ago Closed 8 years ago

Support Cordova projects in WebIDE

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

With pre-push / build step support (bug 1000993), we are very close to support Cordova projects.

We just need a few tweaks due to the manifest only being present in build output dir, instead of the root dir.
Comment on attachment 8552791 [details] [diff] [review]
Build more often to support Cordova. r=ochameau

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

Alex, I am now building more often to support Cordova.

How does this feel with a Gaia app?  Is it too slow now?  I could investigate building in the background if so.
Attachment #8552791 - Flags: feedback?(poirot.alex)
Comment on attachment 8552791 [details] [diff] [review]
Build more often to support Cordova. r=ochameau

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

Please fix env issue.
Also, that would be great if we can live without calling build step on project change.

Otherwise it looks good to build when adding/importing an app.

::: browser/devtools/webide/modules/app-manager.js
@@ -304,5 @@
> -      }
> -
> -      this.update("project");
> -
> -      this.checkIfProjectIsRunning();

The two new calls to build step in import/new shouldn't be painful,
but I have some doubts about this one.
When you have a gaia app, the build step is quite CPU consuming and takes quite some time and it seems to delay UI switching to the selected gaia app. I imagine it allows to update more frequently data coming from build step, but if the build step is slow, we have to be careful about when we call it and limit the calls!

In a very naive workflow, we restart firefox, [auto-]select the app, rebuild it, want to update it to the phone, rebuild it a second time for nothing, push it to the phone.

::: browser/devtools/webide/modules/build.js
@@ +58,5 @@
>  
>        args = manifest.prepackage.args || [];
>        args = args.map(a => a.replace(/%project%/g, project.location));
>  
>        env = manifest.prepackage.env || [];

Please ensure mergin `let env = []` set couple of lines before and manifest.prepackage.env ! Otherwise it is breaking gaia support.
Attachment #8552791 - Flags: feedback?(poirot.alex) → feedback+
This version no longer builds on project switch.  It turns out all we needed was to know where the packageDir lives, so that's now separate from actually building.
Attachment #8552791 - Attachment is obsolete: true
Attachment #8565699 - Flags: review?(poirot.alex)
For Cordova, let's fake out the package.json file so you can build any Cordova project as-is.

If people want to use a more complex command, then they can still add a package.json to do so.
Attachment #8565700 - Flags: review?(poirot.alex)
Attachment #8565699 - Attachment is obsolete: true
Attachment #8565699 - Flags: review?(poirot.alex)
Attachment #8566062 - Flags: review?(poirot.alex)
Attachment #8565700 - Flags: review?(poirot.alex) → review+
Comment on attachment 8566062 [details] [diff] [review]
0001-Bug-1124326-Improve-packageDir-support-for-Cordova.-.patch

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

Thanks, I just had two nits.

::: browser/devtools/webide/modules/app-manager.js
@@ +293,5 @@
>  
>        // Clear out tab store's selected state, if any
>        this.tabStore.selectedTab = null;
>  
> +      this._switchProjects(this._selectedProject).then(() => {

Shouldn't we use this.selectedProject?

@@ +324,5 @@
> +    }
> +    if (project.type == "tab") {
> +      this.tabStore.selectedTab = project.app;
> +    }
> +  }),

nit: I don't see a big value in pulling that off from selectedProject getter, except that we do wait for validateProject completion, was that important?
Attachment #8566062 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> ::: browser/devtools/webide/modules/app-manager.js
> @@ +293,5 @@
> >  
> >        // Clear out tab store's selected state, if any
> >        this.tabStore.selectedTab = null;
> >  
> > +      this._switchProjects(this._selectedProject).then(() => {
> 
> Shouldn't we use this.selectedProject?

Since we are inside the setter for |selectedProject|, I think it's fine to use the raw value, since you're at the point where the raw value is set.

> @@ +324,5 @@
> > +    }
> > +    if (project.type == "tab") {
> > +      this.tabStore.selectedTab = project.app;
> > +    }
> > +  }),
> 
> nit: I don't see a big value in pulling that off from selectedProject
> getter, except that we do wait for validateProject completion, was that
> important?

Fair enough, I'll merge it back in.  I think the yield was needing in a previous version, but it seems okay without it now.
https://hg.mozilla.org/mozilla-central/rev/7d7b31036bb7
https://hg.mozilla.org/mozilla-central/rev/48d76f7edcfe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Here's what I've done for this:

* new page: https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Working_with_Cordova_apps_in_WebIDE

* new section in the Apps docs for Cordova support, linking to the new page: https://developer.mozilla.org/en-US/Apps/Tools_and_frameworks/Cordova_support_for_Firefox_OS#Testing_and_debugging_using_WebIDE

* added a note in "Custom build step" https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Running_and_debugging_apps#Running_a_custom_build_step saying that the Cordova package.json is no auto-generated

Let me know what you think.
Flags: needinfo?(jryans)
(In reply to Will Bamberg [:wbamberg] from comment #13)
> Let me know what you think.

Cool, this looks great to me!
Flags: needinfo?(jryans)
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.