1029942 - allow activation from about:home

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: abouthome activation

Engagement needs to promote a provider from about:home, and wants to be able to activate directly.  Some of these changes will also work towards supporting activation from the share panel (bug 1014332).

Attached is the patch with exploratory changes in abouthome to illustrate how about home would need to be modified by the snippet system (if possible).  

@cmore, are these changes possible through your system?  Take a look at the patch and lets chat.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8445623 [details]
abouthome activation

@felipe, ignore the changes in aboutHome.xhtml, that will not be a part of the final patch, it just illustrates how activation can work from about urls.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2, internal activation without panel

This removes the "enable" panel when activating from within firefox

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2, internal activation without panel

variable name changes

Comment 4

[Chris More [:cmore]]

osmose: can you review attachments above and see if they would have any unforeseen impact to snippets on about:home?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Chris More [:cmore] from comment #4)
> osmose: can you review attachments above and see if they would have any
> unforeseen impact to snippets on about:home?

To clarify the question I have, can the snippet service inject the type of modifications to about:home illustrated in the changes to aboutHome.xhtml in the first patch.

Comment 6

[Osmose [:osmose, :mkelly]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> To clarify the question I have, can the snippet service inject the type of
> modifications to about:home illustrated in the changes to aboutHome.xhtml in
> the first patch.

So the rest goes through the release process and then we can activate stuff from snippets? Coolio!

You look to be doing two things:

1. Running  script tag that creates a custom event and then triggers it on an element.
2. Injecting a div with an img inside within the #topSection div.

Both of those are reasonable and possible with a snippet. Moving DOM nodes around ain't no thang. :D

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #6)

> 1. Running  script tag that creates a custom event and then triggers it on
> an element.
> 2. Injecting a div with an img inside within the #topSection div.

Just to be clear, you can inject the script tag via snippets as well?  If so, I can move this forward.
Thanks!

Comment 8

[Osmose [:osmose, :mkelly]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> (In reply to Michael Kelly [:mkelly,:Osmose] from comment #6)
> Just to be clear, you can inject the script tag via snippets as well?  If
> so, I can move this forward.
> Thanks!

Yeah. Since the location of the script tag doesn't particularly matter, you don't have to bother with _injecting_ it, just include the script tag as part of the snippet code and it gets executed. See https://github.com/mozilla/snippets/blob/master/templates/fxos-logo-animation.html for an example of a snippet template[1] that includes a script tag.

[1] The snippets service supports using Jinja2 templates for reusable snippet templates that just have a few blanks to be filled in, hence the {{ blah }} stuff everywhere.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: example use in about:home

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: about:home activation

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: about:home activation

tests added.  try at

https://tbpl.mozilla.org/?tree=Try&rev=714e002182b1

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8449681 [details] [diff] [review]
about:home activation

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

::: browser/base/content/browser-social.js
@@ +196,5 @@
>      }
>      if (!(targetDoc instanceof HTMLDocument))
>        return;
>  
> +    if (!aBypassUserEnable && targetDoc.defaultView != content)

is this mean to bypass even for iframes? I think if not necessary we should avoid it. But I'm guessing it's to be accessed through snippets?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #12)
> Comment on attachment 8449681 [details] [diff] [review]
> about:home activation
> 
> Review of attachment 8449681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +196,5 @@
> >      }
> >      if (!(targetDoc instanceof HTMLDocument))
> >        return;
> >  
> > +    if (!aBypassUserEnable && targetDoc.defaultView != content)
> 
> is this mean to bypass even for iframes? I think if not necessary we should
> avoid it. But I'm guessing it's to be accessed through snippets?

Yes, and share will use this as well (bug 1014332)

Comment 14

[Shane Caraveo (:mixedpuppy)]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8449681 [details] [diff] [review]
about:home activation

Approval Request Comment
[Feature/regressing bug #]: activation from about: urls
[User impact if declined]: requesting uplift to support important fall partner promotions, contact me or Chris More for details.
[Describe test coverage new/current, TBPL]: new tests covering activation from about:home
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/24bdb64d85cb for bc1 orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=43376527&tree=Fx-Team

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: about:home activation

bug 1023292 changed the name of an event my test was waiting on, this patch fixes the event name. re-landed

https://hg.mozilla.org/integration/fx-team/rev/6598d8f28ded

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6598d8f28ded

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8452764 [details] [diff] [review]
about:home activation

Approval Request Comment
[Feature/regressing bug #]: activation from about: urls
[User impact if declined]: requesting uplift to support important fall partner promotions, contact me or Chris More for details.
[Describe test coverage new/current, TBPL]: new tests covering activation from about:home
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8452764 [details] [diff] [review]
about:home activation

I reviewed the patch with Shane. There is minimal risk of breakage due to this change. 

Chad confirmed that GMC is good with this change from a product perspective. 

Taking this feature fast track late on Aurora to support the fall window that Shane mentioned. Aurora+

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Adding qawanted to get this on the QA radar. I would like to test this before we attempt to use it with a partner. Shane is going to file a separate bug for a test snippet to facilitate testing.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/83c99da37fe3

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora for mochitest-bc perma-fail.
https://hg.mozilla.org/releases/mozilla-aurora/rev/72514ae1ef74

https://tbpl.mozilla.org/php/getParsedLog.php?id=43650801&tree=Mozilla-Aurora

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: about:home activation for aurora

This is the version for aurora. The difference is an event name that was changed in 33 in bug 1023292 (ie. this undoes the change I needed earlier to fix the breakage on 33).  I tested the patch pushed to aurora then fixed and retested locally.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb0c8492ff29

Comment 26

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

Comment on attachment 8454848 [details] [diff] [review]
about:home activation for aurora

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>+  // in this window.  If this activation happens from within Firefox, such as
>+  // about:home or the share panel, we bypass the enable prompt. Any website
>+  // activation, such as from the activations directory or a providers website
>+  // will still get the prompt.
>+  _activationEventHandler: function SocialUI_activationHandler(e, aBypassUserEnable=false) {

This is an event listener - how can aBypassUserEnable ever be specified as true? Looks like it can't and this is just plumbing to be used by bug 1014332. Bug comment to explain would have been useful!

>-    // Ignore events fired in background tabs or iframes
>-    if (targetDoc.defaultView != content)
>+    if (!aBypassUserEnable && targetDoc.defaultView != content)
>       return;

It's rather confusing that aBypassUserEnable implies two separate things - "allow activations from subframes" and "don't show installation panel". Those should really be split into two separate flags, since I don't think this is relevant to the share case. And an "options" argument with named properties is preferred over boolean flag parameters.

>diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm

>     let message = browserBundle.formatStringFromName("service.install.description",
>-                                                     [requestingURI.host, productName], 2);
>+                                                     [aAddonInstaller.addon.manifest.name, productName], 2);

What is "manifest.name"? Presumably the service's name, rather than its origin URI? This kind of change should have gotten a specific UI/security review.

(BTW, _installProvider should really have explicit "fall through" comments to indicate intended omission of "break;" statements in switch() cases.)

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #26)
> Comment on attachment 8454848 [details] [diff] [review]
> about:home activation for aurora
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
> 
> >+  // in this window.  If this activation happens from within Firefox, such as
> >+  // about:home or the share panel, we bypass the enable prompt. Any website
> >+  // activation, such as from the activations directory or a providers website
> >+  // will still get the prompt.
> >+  _activationEventHandler: function SocialUI_activationHandler(e, aBypassUserEnable=false) {
> 
> This is an event listener - how can aBypassUserEnable ever be specified as
> true? Looks like it can't and this is just plumbing to be used by bug
> 1014332. Bug comment to explain would have been useful!

Yes, there is plumbing in this bug for 1014332, I can add the comment in the patch for that bug.

> >-    // Ignore events fired in background tabs or iframes
> >-    if (targetDoc.defaultView != content)
> >+    if (!aBypassUserEnable && targetDoc.defaultView != content)
> >       return;
> 
> It's rather confusing that aBypassUserEnable implies two separate things -
> "allow activations from subframes" and "don't show installation panel".
> Those should really be split into two separate flags, since I don't think
> this is relevant to the share case. And an "options" argument with named
> properties is preferred over boolean flag parameters.

hm, yeah, that needs a different name.  This particular line is not so much about subframes as it is about any non-current tab browser such as the share panel.  Basically, the fn param and this line exist for share activations in bug 1014332.  I can fixup the name in the share activation patch.

> >diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm
> 
> >     let message = browserBundle.formatStringFromName("service.install.description",
> >-                                                     [requestingURI.host, productName], 2);
> >+                                                     [aAddonInstaller.addon.manifest.name, productName], 2);
> 
> What is "manifest.name"? Presumably the service's name, rather than its
> origin URI? This kind of change should have gotten a specific UI/security
> review.

manifest.name is used in a number of places already, such as menu's and about:addons.  IIRC we did cover the manifest in an early security review, but I'm happy to do a followup security review on it.

Using the name here still fits with the existing text, and actually works better than the domain since the domain would basically be the directory website rather than the service being activated.

> (BTW, _installProvider should really have explicit "fall through" comments
> to indicate intended omission of "break;" statements in switch() cases.)

I have a follow up (bug 1032432) for refactoring a bunch of that and removing two cases we don't need any longer.  That code should become simpler from that.