Closed Bug 1161435 Opened 4 years ago Closed 4 years ago

netErrorURL should be resolved from start-manifest url

Categories

(Firefox OS Graveyard :: Runtime, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Tracking Status
firefox41 --- fixed

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch version 1 (obsolete) — Splinter Review
Attachment #8601329 - Flags: review?(fabrice)
Attached patch version 2 (obsolete) — Splinter Review
Attachment #8601330 - Flags: review?(fabrice)
Fabrice, we want to be able to define the network error URL as relative to the start-manifest URL.

The start-manifest URL is only accessible in the parent process.

Here are 2 versions of the same idea. One where the netErrorURL is resolved in the parent process, one where only the startManifestURL is communicated to the child process, where the full URL is resolved.

The 2nd approach might be a tiny bit better because later we might want to do something similar again (querying the startManifestURL from the parent process).

Maybe we could also store the manifest URL in a pref or a setting.
Apparently, <script src="foo.js"> is not found. I'm suspecting gecko to resolve foo.js from the original URI, not the new one. I don't know how to work around that.
Attachment #8601329 - Flags: review?(fabrice)
Attachment #8601330 - Flags: review?(fabrice)
The start manifest url is in a pref, it's b2g.system_manifest_url. You should be able to just use that instead of remoting to the parent.

I faced the same url resolution issue when I did a quick hack for that. It works with app:// urls but not http(s) ones probably because of the way we treat the originalUri in the app:// protocol handler.
(In reply to Fabrice Desré [:fabrice] from comment #5)
> The start manifest url is in a pref, it's b2g.system_manifest_url. You
> should be able to just use that instead of remoting to the parent.

It is not when you use --start-manifest.
(In reply to Paul Rouget [:paul] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #5)
> > The start manifest url is in a pref, it's b2g.system_manifest_url. You
> > should be able to just use that instead of remoting to the parent.
> 
> It is not when you use --start-manifest.

Then we should fix that.
(In reply to Fabrice Desré [:fabrice] from comment #7)
> (In reply to Paul Rouget [:paul] from comment #6)
> > (In reply to Fabrice Desré [:fabrice] from comment #5)
> > > The start manifest url is in a pref, it's b2g.system_manifest_url. You
> > > should be able to just use that instead of remoting to the parent.
> > 
> > It is not when you use --start-manifest.
> 
> Then we should fix that.

That's what I'm saying in comment 3. But we can't just override b2g.system_manifest_url, so should we use a new pref name? Like b2g.effective_system_manifest_url or something…
Flags: needinfo?(fabrice)
Wait, I didn't realize there was 2 prefs. system_manifest_url and start_manifest_url.
Flags: needinfo?(fabrice)
Attached patch v2 (obsolete) — Splinter Review
That's much better :)
Assignee: nobody → paul
Attachment #8601329 - Attachment is obsolete: true
Attachment #8601330 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8602578 - Flags: review?(fabrice)
Comment on attachment 8602578 [details] [diff] [review]
v2

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

::: b2g/components/B2GAboutRedirector.js
@@ +20,5 @@
> +  // new URL("error.html", "http://foo.com/index.html") -> http://foo.com/error.html
> +  // new URL("http://bar.com/error.html", "http://foo.com/index.html") -> http://bar.com/error.html
> +  // new URL("http://bar.com/error.html", undefined) -> http://bar.com/error.html
> +  // new URL("error.html", undefined) -> throw "error.html is not a valid URL."
> +  return new URL(netErrorURL, systemManifestURL).href;

nit: since you import Services.jsm, you could use Services.io.newURI(netErrorURL, null, systemManifestURL).spec and not have to importGlobalProperties();
Attachment #8602578 - Flags: review?(fabrice) → review+
Attached patch v3 (obsolete) — Splinter Review
Addressed Fabrice's comment.
Attachment #8602578 - Attachment is obsolete: true
Attachment #8603206 - Flags: review+
I'd like to land this in m-c, and land the graphene part in larch from bug 1134106.
Attachment #8603206 - Attachment is obsolete: true
Attachment #8604448 - Flags: review+
Keywords: checkin-needed
Summary: Graphene: netErrorURL should be resolved from start-manifest url → netErrorURL should be resolved from start-manifest url
Blocks: 1163905
https://hg.mozilla.org/mozilla-central/rev/60854c327e1c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.