Handle app:// protocol for DEBUG=1 mode

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: yurenju, Assigned: chens)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

46 bytes, text/x-github-pull-request
yurenju
: review+
Details | Review
46 bytes, text/x-github-pull-request
yurenju
: review+
Details | Review
currently we use http:// for DEBUG=1 and app:// for device and b2g-desktop and we need to handle those two protocol shceme on build system like bug 1020167

we should register app:// protocol for DEBUG=1 mode to simplify build process.
I was wrong since bug 1020167 does not handle two protocol on build script, but we still use a lot of GAIA_SECHME (app:// or http:// for this variable) and GAIA_DOMAIN in $GAIA_DIR/build/*.
Posted file WIP (obsolete) —
This WIP will register app protocol for DEBUG=1 mode and redirect to http protocol, which serves app data by httpd.js.

But one problem comes into when I try to change scheme from |SCHEME=http://| to |SCHEME=app://|, it will get errors like: 

JavaScript error: http://system.gaiamobile.org:8080/shared/js/dump.js, line 24: settings is null
JavaScript error: http://system.gaiamobile.org:8080/js/applications.js, line 85: apps.mgmt is null
Assignee: nobody → shchen
I imagine the channel.redirectTo() call isn't transparent enough.
You may find some helpful tricks from the original app: protocol implementation, which was in JS in early releases
(and now is in C++):
http://mxr.mozilla.org/mozilla-b2g18/source/netwerk/protocol/app/AppProtocolHandler.js#53
Posted file WIP (obsolete) —
Rewrite app: protocol handler as comment 3 suggested, and also change browser-helper to avoid errors in comment 2. 

This patch works well on original homescreen, but if change to vertical homescreen , it will not have any icons on homescreen.
Attachment #8441241 - Attachment is obsolete: true
May be that because of GAIA_PORT=8080?
Or something unexected now happens in post-manifest.js:manifestInterAppHostnames (which should be useless thanks to your patch)?
Tried on gaia master, still no icon on vertical homescreen,
gaia commit hash is: 646efa07944adfe96c28d6075f95be1037eb657c
Comment on attachment 8441898 [details] [review]
WIP

Hi Yuren, how do you think about this patch? should we also take care GAIA_PORT in this one or file another bug to change it? 

As for vertical homescreen not showing any icon in DEBUG=1 mode, I've filed bug 1015865 to keep tracking it.
Attachment #8441898 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 8441898 [details] [review]
WIP

offline discussed with chens, we will keep GAIA_PORT for changing default http port and redirect app protocol to http server with GAIA_PORT.
Attachment #8441898 - Flags: feedback?(yurenju.mozilla) → feedback+
Posted file Pull Request (obsolete) —
Support app protocol and keep GAIA_PORT
Attachment #8441898 - Attachment is obsolete: true
Attachment #8443197 - Flags: review?(yurenju.mozilla)
(In reply to Sherman Chen [:chens] from comment #7)
> As for vertical homescreen not showing any icon in DEBUG=1 mode, I've filed
> bug 1015865 to keep tracking it.

Correction, it should be bug 1027555.
Comment on attachment 8443197 [details] [review]
Pull Request

I would like to give f+ for this pr and redirect review request to Alex since I'm new to nsIProtocolHandler.

and please file follow up bugs to remove domain or scheme from functions in utils such as getWebapp(), makeWebappsObject(), gaiaOriginURL(), gaiaManifestURL(), etc.
Attachment #8443197 - Flags: review?(yurenju.mozilla)
Attachment #8443197 - Flags: review?(poirot.alex)
Attachment #8443197 - Flags: feedback+
thanks chens correct me, I mean port or scheme, not domain.
(In reply to Yuren [:yurenju] from comment #11)
> and please file follow up bugs to remove domain or scheme from functions in
> utils such as getWebapp(), makeWebappsObject(), gaiaOriginURL(),
> gaiaManifestURL(), etc.

File bug 1028069 for follow up.
Comment on attachment 8443197 [details] [review]
Pull Request

Looks good, thanks for this patch and the followup cleanup!

I dropped a note about how easily map this new app protocol to build_stage:
https://github.com/mozilla-b2g/gaia/pull/20654/files#discussion_r14067900
Attachment #8443197 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #14)
> Comment on attachment 8443197 [details] [review]
> Pull Request
> 
> Looks good, thanks for this patch and the followup cleanup!
> 
> I dropped a note about how easily map this new app protocol to build_stage:
> https://github.com/mozilla-b2g/gaia/pull/20654/files#discussion_r14067900

Getting rid of httpd sounds awesome! But we also need to take care of test-agent while mapping app protocol to build_stage. I would like to file another bug for that work, how do you think?
(In reply to Sherman Chen [:chens] from comment #15)
> (In reply to Alexandre Poirot (:ochameau) from comment #14)
> > I dropped a note about how easily map this new app protocol to build_stage:
> > https://github.com/mozilla-b2g/gaia/pull/20654/files#discussion_r14067900
> 
> Getting rid of httpd sounds awesome! But we also need to take care of
> test-agent while mapping app protocol to build_stage. I would like to file
> another bug for that work, how do you think?

Sure, I didn't expected that it would work as-is.
We also need a filewatcher or something that would automagically refresh stage.
But we should start testing to see if test agent is the only thing left to fix.
(In reply to Alexandre Poirot (:ochameau) from comment #16)
> Sure, I didn't expected that it would work as-is.
> We also need a filewatcher or something that would automagically refresh
> stage.
> But we should start testing to see if test agent is the only thing left to
> fix.

File follow up bug for replace httpd: bug 1029423
Merged to master
https://github.com/mozilla-b2g/gaia/commit/7de7d836ac32a9803d503c2a776d4b08baf02e3f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Reopened because of regression in b2g-inbound.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
revert commit
decffedef4c0c79f8b1faf6feceb9d51e3493269
Posted file Pull Request
Attachment #8443197 - Attachment is obsolete: true
Comment on attachment 8446321 [details] [review]
Pull Request

Change some manifest urls in app unit test, includes comms, settings, keyboard, and l10n in sharedtest.
Attachment #8446321 - Flags: review?(yurenju.mozilla)
could you also paste your tbpl link for gecko try server?
(In reply to Yuren [:yurenju] from comment #24)
> could you also paste your tbpl link for gecko try server?

Gaia-try:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=a2c027cf1c05040eb7fb51d70d799df96fb14fe0

Try: 
https://tbpl.mozilla.org/?tree=Try&rev=608980268a87
Comment on attachment 8446321 [details] [review]
Pull Request

good to me, r=yurenju
Attachment #8446321 - Flags: review?(yurenju.mozilla) → review+
and please separate to two commits next time to help us review more effectively.
Attachment #8446952 - Flags: review?(yurenju.mozilla) → review+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1032784
You need to log in before you can comment on or make changes to this bug.