851336 - fix our relative url story

Status: RESOLVED WONTFIX

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

bug 840873 points out that we are inconsistent with our handling of relative urls.  The primary culprit is WorkerAPI.jsm in social.request-chat, social.user-recommend-prompt-response (calls SocialService.recommendInfo setter) and social.notification-create.  These should be resolved against the baseurl of the worker.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: resolve relative urls against baseurls

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 725186 [details] [diff] [review]
resolve relative urls against baseurls

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

I think this isn't the correct approach, so it's probably best to get review from someone who does :)

Comment 3

[Shane Caraveo (:mixedpuppy)]

The issue here is one of expectations of web developers and consistency with standard APIs.  Standard APIs, link tags, css, workers (e.g. importScripts), etc. all allow for URIs to be relative from the document.  Our openPanel and openChatWindow APIs have also allowed this, however our worker APIs (e.g for opening chat windows from the worker) required full URIs or URIs relative to origin.  The direction of this patch is to make our APIs consistent with the way URIs work on the web.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

FWIW - The way I see it is that we are offering an API to open a "provider URL", and "provider URLs" should be resolved relative to the provider origin rather than relative to the page.

In the same way, if a manifest is sent with an "activate" event, relative URLs in that manifest are resolved against the origin, *not* against the page sending the event.  Even though these chat URLs are not supplied in the manifest, IMO they should be treated as if they were.

Ultimately it's probably not a big deal either way...

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> The direction of this patch is to make our APIs consistent with the
> way URIs work on the web.

Why APIs exactly? I could see us having different behavior for mozSocial vs. the worker message APIs. Worker APIs aren't really very "webby" to begin with (contrast with mozSocial). The URLs in the manifest are yet another class that might make sense to have resolved against a different base.

I'm inclined to agree with Mark that URLs sent by the worker should be relative to the provider origin, if we need to support relative URLs at all. Using the worker URL as the base URL for anything doesn't really make much sense to me - the worker URL isn't likely to be a useful base in the common case (and the passed-in relative URLs might not even originate from the worker itself, in some cases).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 725186 [details] [diff] [review]
resolve relative urls against baseurls

I think that means r- on this, but I'd love a summary of what the current behavior is vs. the new proposed behavior, for all the cases that are changing.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > The direction of this patch is to make our APIs consistent with the
> > way URIs work on the web.
> 
> Why APIs exactly? I could see us having different behavior for mozSocial vs.
> the worker message APIs. Worker APIs aren't really very "webby" to begin
> with (contrast with mozSocial). The URLs in the manifest are yet another
> class that might make sense to have resolved against a different base.

You can create a worker from a web page, so I do think it is webby.  While it only provides a subset of standard APIs, those that it does have do support relative urls on the base url of the worker (e.g. importScripts, XHR).

In regards to the manifest, given our install mechanism, it makes perfect sense to have the manifest urls be relative to the activation page (resolved to full urls before use).  That would allow the avoidance of bs like this:

https://github.com/mixedpuppy/socialapi-demo/blob/gh-pages/index.html#L14

That code does run from different paths depending on how you set it up.

> I'm inclined to agree with Mark that URLs sent by the worker should be
> relative to the provider origin, if we need to support relative URLs at all.
> Using the worker URL as the base URL for anything doesn't really make much
> sense to me - the worker URL isn't likely to be a useful base in the common
> case (and the passed-in relative URLs might not even originate from the
> worker itself, in some cases).

Even importScripts, a worker specific api, supports urls relative to the base url of the worker script.  Having a relative URL NOT originate from a baseURL in any sense is a bug, it wouldn't work in a script tag either.

I cannot think of one single api I've ever ran across that resolves a relative url on origin.

> I think that means r- on this, but I'd love a summary of what the current behavior is > vs. the new proposed behavior, for all the cases that are changing.

Current behavior is that we resolve some things on base url, some things on origin.  My proposed behavior is we resolve on base url always.  Marks proposal is to resolve on origin always, including mozSocial.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

importScripts() in a worker resolving URLs against the worker URL makes sense - that is a worker-specific API that is implemented "natively" by the worker environment. 

Having our worker-specific APIs (the stuff in WorkerAPI.jsm) resolving URL arguments against the worker URL makes less sense to me, because it's not fundamental to the worker itself and so the worker URL as a base seems arbitrary. It seems more likely to me that the provider origin is a useful base in the common case. Do we need to support relative URLs here at all? I assume we currently use the provider origin as a base for these?

MozSocialAPI methods resolving their URLs against the document URL whose mozSocial is being invoked make sense.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)

> Do we need to support relative URLs here at all? I
> assume we currently use the provider origin as a base for these?

We probably do not.  Facebook uses absolute URLs when opening a chat window (which IIRC is the primary thing we are discussing here) although what the new providers do isn't clear to me.  The current state is that chats opened by the sidebar are resolved against the sidebar address, while chats opened by the worker are resolved against the provider origin.