Closed Bug 957418 Opened 7 years ago Closed 7 years ago

Enable Inter-App Communication API for non-release builds of Firefox

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

Attachments

(2 files)

FirefoxOS developers would like to be able to develop features which depend on this API in the browser. We can turn this on by default everywhere, but it seems like a safer option, and one that would be easier to get support for, would be to enable it for only non-release builds of Firefox for now.
Attachment #8356914 - Attachment description: 0001-Bug-957418-Enable-Inter-App-Communication-API-for-no.patch → Gecko Patch - Enable Inter-App API for non-release builds
Comment on attachment 8356914 [details] [diff] [review]
Gecko Patch - Enable Inter-App API for non-release builds

Hi Ben - 

Any chance you'd be able to review this one? Jonas is on PTO for a while, and we want to move quickly on these features. Thanks!
Attachment #8356914 - Flags: review?(bent.mozilla)
Comment on attachment 8356916 [details] [review]
Gaia Patch - Enable Inter-App API Pref

Alex, or Yuren - Could one of you guys review the simple build changes here? (Adding a pref). Probably shouldn't land this until the gecko patch does. Thanks!
Attachment #8356916 - Flags: review?(yurenju.mozilla)
Attachment #8356916 - Flags: review?(poirot.alex)
Comment on attachment 8356916 [details] [review]
Gaia Patch - Enable Inter-App API Pref

Hi Kevin, it would be better if generate |rules| on build time to support different domain name and port if 3rd party developer change them.
Attachment #8356916 - Flags: review?(yurenju.mozilla)
(In reply to Yuren Ju [:yurenju] from comment #5)
> Comment on attachment 8356916 [details] [review]
> Gaia Patch - Enable Inter-App API Pref
> 
> Hi Kevin, it would be better if generate |rules| on build time to support
> different domain name and port if 3rd party developer change them.

I disagree that 3rd party developers will be changing anything. I do think that a build script to inject additional hostnames into these manifests may be a better approach for the time being. I'm not sure if I have time to do that, but I'll investigate.
Comment on attachment 8356916 [details] [review]
Gaia Patch - Enable Inter-App API Pref

Hi Yuren - What do you think about something like this?
Attachment #8356916 - Flags: review?(yurenju.mozilla)
Hi Kevin,

I'm still wondering why we write the manifestURLs in manifest.webapp, it should be generated in build time or we will have some problems if GAIA_DOMAIN is changed and hard to find the root cause.

we can have a Makefile in apps/search and generate manifest.webapp by a manifest.template, you can reference Makefile in apps/email to create a build script in apps/search/build/ for generating manifest.webapp.

does it make sense?
(In reply to Yuren Ju [:yurenju] from comment #8)
> Hi Kevin,
> 
> I'm still wondering why we write the manifestURLs in manifest.webapp, it
> should be generated in build time or we will have some problems if
> GAIA_DOMAIN is changed and hard to find the root cause.
> 
> we can have a Makefile in apps/search and generate manifest.webapp by a
> manifest.template, you can reference Makefile in apps/email to create a
> build script in apps/search/build/ for generating manifest.webapp.
> 
> does it make sense?

Do we have a use case where gaia domain is actually changed? I also wanted the ability to have a more generic build script that has the ability to fix multiple apps, instead of special casing this only for the search app. We will run into this problem everywhere we use the interapp API for now.
Comment on attachment 8356914 [details] [diff] [review]
Gecko Patch - Enable Inter-App API for non-release builds

I think this is more of a question for ehsan.
Attachment #8356914 - Flags: review?(bent.mozilla) → review?(ehsan)
Comment on attachment 8356914 [details] [diff] [review]
Gecko Patch - Enable Inter-App API for non-release builds

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

Note that the API itself is hidden behind the dom.inter-app-communication-api.enabled pref.  This patch just adds the missing JS modules to the packaging.  The pref remains disabled on non-b2g environments.
Attachment #8356914 - Flags: review?(ehsan) → review+
Duplicate of this bug: 957698
(In reply to Kevin Grandon :kgrandon from comment #9)
> Do we have a use case where gaia domain is actually changed? I also wanted
> the ability to have a more generic build script that has the ability to fix
> multiple apps, instead of special casing this only for the search app. We
> will run into this problem everywhere we use the interapp API for now.

We certainly don't want "gaiamobile.org" becoming a fixture of the web, just like the value "Mozilla" in |navigator.appCodeName|

http://www.w3.org/html/wg/drafts/html/master/webappapis.html#the-navigator-object

If we deny the capability for venders to customize this value in the future, every FxOS device out there will be using |gaiamobile.org| for eternity.

Actually, IMHO, we should generate a set of UUIDs for our apps instead of keep using these made-up hostnames. But this is not urgent in anyway.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> (In reply to Kevin Grandon :kgrandon from comment #9)
> > Do we have a use case where gaia domain is actually changed? I also wanted
> > the ability to have a more generic build script that has the ability to fix
> > multiple apps, instead of special casing this only for the search app. We
> > will run into this problem everywhere we use the interapp API for now.
> 
> We certainly don't want "gaiamobile.org" becoming a fixture of the web, just
> like the value "Mozilla" in |navigator.appCodeName|
> 
> http://www.w3.org/html/wg/drafts/html/master/webappapis.html#the-navigator-
> object
> 
> If we deny the capability for venders to customize this value in the future,
> every FxOS device out there will be using |gaiamobile.org| for eternity.
> 
> Actually, IMHO, we should generate a set of UUIDs for our apps instead of
> keep using these made-up hostnames. But this is not urgent in anyway.

I totally agree that GAIA_DOMAIN should be customizable, and should work everywhere. This patch actually helps that effort.

Regarding the manifest issue: I am just following the WebAPI implementation, and do not think that checking in a manifest.template for several apps is the proper solution.
Comment on attachment 8356916 [details] [review]
Gaia Patch - Enable Inter-App API Pref

Kevin is right, this pull request can handle different GAIA_DOMAIN. I left some comments on github, please fix nits before land it, thank you.
Attachment #8356916 - Flags: review?(yurenju.mozilla) → review+
Attachment #8356916 - Flags: review?(poirot.alex)
Adding checkin-needed for the gecko patch here: https://bug957418.bugzilla.mozilla.org/attachment.cgi?id=8356914
Keywords: checkin-needed
Thank you very much for the review Yuren. Gaia patch landed: https://github.com/mozilla-b2g/gaia/commit/f08d39eeead2c86712bbaa5ce8cbcadeeb103c7a
https://hg.mozilla.org/mozilla-central/rev/e2abf3e210cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Kevin, do we still need |manifestInterAppHostnames()| in webapp-manifests.js? seems we no longer need it.
Flags: needinfo?(kgrandon)
(In reply to Yuren [:yurenju][:小朱] from comment #21)
> Kevin, do we still need |manifestInterAppHostnames()| in
> webapp-manifests.js? seems we no longer need it.

Yes, we actually still need this to fix the origin case when running in Firefox Nightly. We change the origin from app:// to http:// so the ports will work properly in Nightly, so we still need this unless there is a better way of doing it.
Flags: needinfo?(kgrandon)
You need to log in before you can comment on or make changes to this bug.