allow activation from about:home

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

({qawanted})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 unaffected, firefox31 unaffected, firefox32+ fixed, firefox33 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Posted file abouthome activation (obsolete) —
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.
Attachment #8445623 - Flags: feedback?(chrismore.bugzilla)
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.
Attachment #8445623 - Flags: feedback?(felipc)
This removes the "enable" panel when activating from within firefox
Assignee: nobody → mixedpuppy
Attachment #8446160 - Flags: feedback?(felipc)
variable name changes
Attachment #8446263 - Flags: feedback?(felipc)
Attachment #8446160 - Attachment is obsolete: true
Attachment #8446160 - Flags: feedback?(felipc)
osmose: can you review attachments above and see if they would have any unforeseen impact to snippets on about:home?
Flags: needinfo?(mkelly)
(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.
(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
Flags: needinfo?(mkelly)
(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!
Flags: needinfo?(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.
Flags: needinfo?(mkelly)
Attachment #8445623 - Attachment is obsolete: true
Attachment #8445623 - Flags: feedback?(felipc)
Attachment #8445623 - Flags: feedback?(chrismore.bugzilla)
Posted patch about:home activation (obsolete) — Splinter Review
Attachment #8446263 - Attachment is obsolete: true
Attachment #8446263 - Flags: feedback?(felipc)
Attachment #8448452 - Flags: feedback?(felipc)
Posted patch about:home activation (obsolete) — Splinter Review
tests added.  try at

https://tbpl.mozilla.org/?tree=Try&rev=714e002182b1
Attachment #8448452 - Attachment is obsolete: true
Attachment #8448452 - Flags: feedback?(felipc)
Attachment #8449681 - Flags: review?(felipc)
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?
Attachment #8449681 - Flags: review?(felipc) → review+
(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 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
Attachment #8449681 - Flags: approval-mozilla-aurora?
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
Attachment #8449681 - Attachment is obsolete: true
Attachment #8449681 - Flags: approval-mozilla-aurora?
Attachment #8452764 - Flags: review+
Flags: needinfo?(mixedpuppy)
https://hg.mozilla.org/mozilla-central/rev/6598d8f28ded
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Flags: in-testsuite+
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
Attachment #8452764 - Flags: approval-mozilla-aurora?
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+
Attachment #8452764 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Keywords: qawanted
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.
Attachment #8454848 - Flags: review+
Flags: needinfo?(ryanvm)
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.)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Depends on: 1073863
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.