1134315 - directory activation uses directory origin in panel

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

When activating from the activations directory, the panel says "would you like to activate services from activations.cdn.mozilla.net".  If you're activating (for example) pocket, it should say "would you like to activate services from getpocket.com".

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: 1134315

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

So what happens to "internal" then?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566138 [details] [diff] [review]
1134315

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

What are the possibilities for data.installType? If it is just "internal" or "foreign" then this is no behavior change.

Comment 4

[Shane Caraveo (:mixedpuppy)]

There are 3 possible results, directory, internal and foreign.  directory and internal should use the origin in the manifest, foreign should always use origin of the activation.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566138 [details] [diff] [review]
1134315

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

::: toolkit/components/social/SocialService.jsm
@@ +550,5 @@
>      let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
>  
> +    // foreign activation uses the activation url for origin, otherwise use the manifest origin
> +    let requestingURI =  Services.io.newURI(data.installType == "foreign" ?
> +                                            data.url : data.manifest.origin, null, null);

Can you please refactor this to be more clear (at the cost of being extra verbose)?

let url = data.installType == "foreign" ?
            data.url :
            data.installType == "directory" ||
            data.installType == "internal" ?
              data.manifest.origin;
let requestingURI = Services.io.newURI(url, null, null);

Also, it would be nice if you updated the code at line 163 and 165 to use double-quotes instead of single-quotes.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: 1134315.txt

carry forward r+

https://hg.mozilla.org/integration/fx-team/rev/1fc897448cad

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fc897448cad

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> let url = data.installType == "foreign" ?
>             data.url :
>             data.installType == "directory" ||
>             data.installType == "internal" ?
>               data.manifest.origin;

I didn't realize until Gavin mentioned this change that the second case of the second ternary operator was missing from my recommendation. I see that the code that landed added in an `undefined` result for this case, which probably isn't something that we want passed to newURI.

Shane, would you be OK with changing this to an if/else-if/else and in the else calling Cu.reportError() or some other type of logging and throwing?

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: 1134315

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8566642 [details] [diff] [review]
1134315

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

This is better. Thanks!

Comment 11

[Shane Caraveo (:mixedpuppy)]

backout  :   https://hg.mozilla.org/integration/fx-team/rev/3b28be32ebc7
new patch:   https://hg.mozilla.org/integration/fx-team/rev/cee52e6c3662

Comment 12

[Guilherme Lima]

AFAICT this landed on MC
https://hg.mozilla.org/mozilla-central/rev/3b28be32ebc7
https://hg.mozilla.org/mozilla-central/rev/cee52e6c3662

Comment 13

[David Lawrence [:dkl]]

Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl