563738 - Add current Mozilla "snippets" (links and special events) in the about:home page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

we should not lose possibility to add current special links in the first incarnation of the home page

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This is the basic stuff.

TODO:
- This requires a dom storage hack, should discuss with DOM guys to see if I can help fixing it properly or get a quick path for about: pages

- localStorage is main-thread only, I've filed a bug to investigate a new db schema to do writes in a separate thread, but for this specific bug I think we can go as it is and let that as a future target.

- The chrome json file is just an example file I used to test, it has a simple snippet in English with the mozilla icon, if we want to provide a chrome json to all users we will have to make one per locale and make it part of the localizations package (asking some localizer to localize a json could be hard though). I think we should put this as a placeholder (maybe we can add more english snippets to it if we want to have better testing), activate a moz page to serve those, then push a patch to remove the placeholder. The moz_page will receive browser's locale so return values can be in the correct language.

- This is pretty simplicistic and built around snippets, since "widgets" design has not started and can be easily done later with the page in place.

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 447402 [details] [diff] [review]
patch v1.0

A few comments:

I'd suggest to use the URLFormatter proper to mangle the update URL. We likely want to use version, platform etc at some point.

I'm not sure if we want to update the snippet only on load, now that we have the power.

We very likely want to have html as snippets, not just text.

Other random toughts: Should the snippets have priorities?
A pref for the idle time? 5 minutes might be too long.
Snippets might want to carry the RTL information themselves. We may want to push an important but not yet localized snippets to RTL languages.

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #2)
> (From update of attachment 447402 [details] [diff] [review])
> I'd suggest to use the URLFormatter proper to mangle the update URL. We likely
> want to use version, platform etc at some point.

Yes, I was guessing how much informations we were needing

> I'm not sure if we want to update the snippet only on load, now that we have
> the power.

Not sure what you mean here, is it a sugestion for a snippet that changes on a timer while the page is loaded? These things can be easily done later

> We very likely want to have html as snippets, not just text.

the json string can contain everything, when you say html you mean that we want to be able to have anything there? like being a ble to put a larger banner that can increase in size, show animations/movies etc?

> Other random toughts: Should the snippets have priorities?

Not sure why, changing the server file to have important ones would be enough

> A pref for the idle time? 5 minutes might be too long.

This does not need to run absolutely every 7 days, even if we update after 8 or 15 days is fine compared to what we have today, I can easily imagine a user having 5 minutes of idle in 15 days

> Snippets might want to carry the RTL information themselves. We may want to
> push an important but not yet localized snippets to RTL languages.

So we show them LTR even if the locale is RTL? Not sure what kind of information could have that high priority, currently updating snippets can take a long time, so they were never thought as a way to bring urgent news.
Sounds like a sensible suggestion.

Thanks for feedback.

Comment 4

[Axel Hecht [:Pike]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 447402 [details] [diff] [review] [details])
> > We very likely want to have html as snippets, not just text.
> 
> the json string can contain everything, when you say html you mean that we want
> to be able to have anything there? like being a ble to put a larger banner that
> can increase in size, show animations/movies etc?

Well, right now the snippets contain links, so that much at least.

If I read the patch correctly, you're using .textContent to fill in the snippet text, and I'd suggest to use innerHTML.

Re RTL, we currently have rather active teams for our RTL locales, but there are some long-tail not-so-active locales lingering out there. Speaking as pessimistically as possible, the message could be "all our RTL-language localizers got hit by a truck, we need new volunteers".

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 447402 [details] [diff] [review]
patch v1.0

Is there a related WebDev bug for this yet?

Is there a reason to have the "Mozilla" and "moz" prefixes everywhere?

>+    // Get a random snippet.
>+    let index = Math.round(Math.random() * snippets.length);

If Math.random() returns a number that is close to 1, you'll end up trying to get an invalid array index.

>+    snippetElt.style.backgroundImage = cssUrlEscape(snippets[index].icon);

I like that this can this be a data: URI - means it would work while offline too. Is there always going to be an image? And is there a reason to not use an <img> element and set its src attribute?

As Axel said, would be nice to allow HTML here (although the security group may or may not have anything to say about that). That could also potentially negate the need for special handling of any image (as a HTML snippet could include all that itself).

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)

> >+    snippetElt.style.backgroundImage = cssUrlEscape(snippets[index].icon);
> 
> I like that this can this be a data: URI - means it would work while offline
> too. Is there always going to be an image? And is there a reason to not use an
> <img> element and set its src attribute?

It was easier at first glance to make it ltr/rtl, and have long text aligned and not wrapping the image. There are most likely better ways.
Usually each snippet has an icon, that is used to catch user's eyes. So I guess we can suppose icon always exists.

> As Axel said, would be nice to allow HTML here (although the security group may
> or may not have anything to say about that).

the page does not have chrome privileges, so it should not be a problem, I'll put innerHTML, not sure why i used text now that i see it.

That could also potentially negate
> the need for special handling of any image (as a HTML snippet could include all
> that itself).

I wanted to make this easy to maintain for marketing, so they don't have to create html and especially ensure how it fits into the page and that is valid xhtml and so on. This clearly limits the layout but requires only icon and text.
Requirements are not set in stone so far, so i'm going the small and simple way for now.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

small update, fixes most of the comments

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.2

updated on latest about:home patch.

Comment 9

[Johnathan Nightingale [:johnath]]

Okay, let's wake this bug back up now that Engagement, IT, Webdev, Product and Engineering have all had a talk about it. The current spec is here:

https://wiki.mozilla.org/Firefox/Projects/Firefox_Start#Dynamic_Content_Delivery

In brief: Firefox will background-load a chunk of HTML from a URL, dump it in localstorage, and inject that into the start page.

As for an update URL, Justin suggests:

http://start.mozilla.com/%STARTPAGE_VERSION%/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/content.json

The only modification I'd make is to the suffix - this isn't json, it's an HTML blob, so .html or even .blob would work fine for me.

There is follow-on work to do in other bugs around pre-populating localStorage with info that content might want to use to tailor messages, but which we would rather not send across the wire. There's also a question around whether the blobs should be delivered over https, but for now, let's go with this as-is.

This bug is purely to track implementation of the update ping in about:home, fetching and storing the resulting content, and displaying it on page load, when available.

I'll file a separate bug around presenting some default content when the dynamic content is unavailable.

blocking:beta6+ this is needed for feature freeze.

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
...
> In brief: Firefox will background-load a chunk of HTML from a URL, dump it in
> localstorage, and inject that into the start page.
> 
> As for an update URL, Justin suggests:
> 
> http://start.mozilla.com/%STARTPAGE_VERSION%/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/content.json
...
> There's also a question around whether the
> blobs should be delivered over https, but for now, let's go with this as-is.

Without any checksumming or other validation, what stops an attacker from MITM'ing start.mozilla.com and injecting whatever HTML he/she wants, including JavaScript? What privileges does about:home run with? Could I get such JS to run as chrome?

This isn't just loading a random web page here. A blob of <whatever> is being grabbed from an HTTP URL and stored for some period of time, leading to persistent ownage.

Either this update should go over SSL, or there should be some other type of validation that the blob is actually what is expected.

Comment 11

[Johnathan Nightingale [:johnath]]

I'm guessing that you're just thinking out loud, and not assuming that I would be okay with injecting randomly obtained code into privileged content? about:home is unprivileged, because of course it would be.

The https question is still open, but comes down entirely to whether there is anything substantively different that injected content could do with this page, vs any other page. Bug 593379 is the only thing I can think of readily, and I think the contents of that set of data will be the dominant mover in terms of http/https. If the page has no sensitive information, I don't see a great reason to insist on https - a would be attacker could inject their malicious content into any other page just as easily.

Comment 12

[Reed Loden [:reed]]

(In reply to comment #11)
> I'm guessing that you're just thinking out loud, and not assuming that I would
> be okay with injecting randomly obtained code into privileged content?

Sure. This is why my comments were mostly in the form of questions.

> about:home is unprivileged, because of course it would be.

We've had cases before where about: pages *were* privileged incorrectly, giving direct access to chrome://, so I'm just checking. :)

> The https question is still open, but comes down entirely to whether there is
> anything substantively different that injected content could do with this page,
> vs any other page. Bug 593379 is the only thing I can think of readily, and I
> think the contents of that set of data will be the dominant mover in terms of
> http/https. If the page has no sensitive information, I don't see a great
> reason to insist on https - a would be attacker could inject their malicious
> content into any other page just as easily.

I'm mostly thinking that this would a great URL for attackers to MITM for the purposes of stealing passwords for sites such as bank sites, etc. What makes this a larger problem than just a normal MITM is that these snippets are cached for some period of time, so an attacker just has to MITM the user once for persistence to take over. That's really my main concern.

If we could disallow JS for the blob, I'd feel much better about this.

Comment 13

[Johnathan Nightingale [:johnath]]

(In reply to comment #12)
> We've had cases before where about: pages *were* privileged incorrectly, giving
> direct access to chrome://, so I'm just checking. :)

Fair enough - sorry if I sounded defensive - I could have been clearer, and there's nothing wrong with double checking.

> I'm mostly thinking that this would a great URL for attackers to MITM for the
> purposes of stealing passwords for sites such as bank sites, etc. What makes
> this a larger problem than just a normal MITM is that these snippets are cached
> for some period of time, so an attacker just has to MITM the user once for
> persistence to take over. That's really my main concern.
> 
> If we could disallow JS for the blob, I'd feel much better about this.

Can't really kill JS since engagement wants the flexibility to have smarts in these snippets, but I'm also not clear on your attack. Given that the page is unprivileged, how would MitM code steal passwords (or anything else beyond google search terms) in this scenario?

I'll certainly agree that MitM could steal search terms entered on that page, but someone in a position to MitM our snippets can also just MitM the search engine (also http) and steal the same data regardless, no?

Comment 14

[Reed Loden [:reed]]

(In reply to comment #13)
> Given that the page is unprivileged, how would MitM code steal passwords
> (or anything else beyond google search terms) in this scenario?

Phishing scenario... Display a Google login page instead (rather than the search page)... or display a bank login page. Users aren't smart, sadly, and this is going to be their default homepage, so they are likely to see this page quite often.

> I'll certainly agree that MitM could steal search terms entered on that page,
> but someone in a position to MitM our snippets can also just MitM the search
> engine (also http) and steal the same data regardless, no?

Yes, they could, but with JS (and really CSS), can rewrite the page to look like anything else, then either use <form> or JS to send data that the user enters back to some nefarious collector.

Comment 15

[Johnathan Nightingale [:johnath]]

(In reply to comment #14)
> (In reply to comment #13)
> > Given that the page is unprivileged, how would MitM code steal passwords
> > (or anything else beyond google search terms) in this scenario?
> 
> Phishing scenario... Display a Google login page instead (rather than the
> search page)... or display a bank login page. Users aren't smart, sadly, and
> this is going to be their default homepage, so they are likely to see this page
> quite often.

Right, so I understand that scenario but that was true beforehand anyhow, right? We used to use an http-based page, meaning that MitM was a guaranteed possibility every load, whereas now it's only a possibility once a day when we pull from localStorage, right? Both still quite attackable, but no more so, ya?

Comment 16

[Justin Fitzhugh]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Given that the page is unprivileged, how would MitM code steal passwords
> > > (or anything else beyond google search terms) in this scenario?
> > 
> > Phishing scenario... Display a Google login page instead (rather than the
> > search page)... or display a bank login page. Users aren't smart, sadly, and
> > this is going to be their default homepage, so they are likely to see this page
> > quite often.
> 
> Right, so I understand that scenario but that was true beforehand anyhow,
> right? We used to use an http-based page, meaning that MitM was a guaranteed
> possibility every load, whereas now it's only a possibility once a day when we
> pull from localStorage, right? Both still quite attackable, but no more so, ya?

Agreed - this is no increase in attack surface and we aren't inserting anything into localstore currently that's worrisome, so until we do (which may or may not ever happen) lets keep it http.

Comment 17

[Axel Hecht [:Pike]]

Do we still want to allow multiple snippets in parallel as we're doing for the current start page? Like, right now we're serving some 5 snippets in parallel, AFAICT, http://viewvc.svn.mozilla.org/vc/projects/google_snippets/trunk/static/en/.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> Do we still want to allow multiple snippets in parallel as we're doing for the
> current start page? Like, right now we're serving some 5 snippets in parallel.

Only one is shown at a time, we will probably update all of them at once and show a random one between the available ones.

Comment 19

[Axel Hecht [:Pike]]

... which may change the snippet sync format back to JSON, passing a list of html fragments? Also, should we have an option to make different snippets have different probability?

Comment 20

[Johnathan Nightingale [:johnath]]

(In reply to comment #19)
> ... which may change the snippet sync format back to JSON, passing a list of
> html fragments? Also, should we have an option to make different snippets have
> different probability?

GOTO Comment 9. We get a blob of html and inject it into the doc - full stop. If that blob performs magic having to do with multiple strings and random probability distributions and the like, I'm sure engagement will want to work closely with l10n on the subject, but that discussion is out of scope for this bug.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Updated the URL to point at the updated design spec, as I think it was confusing Axel.

This bug is about creating the JS code that we will ship in about:home which will:

 - look in localstorage to see if there's content there
 - display the content; if none exists, display default content (to be provided in bug 593366)
 - fetch new content from the server, save it in localstorage for next time

That content will be responsible for its own randomization, probability of display, etc. The packaging format is, I think, HTML, but that depends largely on what IT wants to deliver.

Comment 22

[Axel Hecht [:Pike]]

(In reply to comment #20)
> I'm sure engagement will want to work closely with l10n on the subject, but 
> that discussion is out of scope for this bug.

To be precise, I don't expect "randomize 5 snippets" to pop up on pascalc's lap 100 times. If engagement wants that and can do that without pascalc seeing it, that's fine. l10n though doesn't commit to make that happen whichever way.

Comment 23

[Justin Fitzhugh]

(In reply to comment #21)
> The packaging format is, I think, HTML, but that depends largely
> on what IT wants to deliver.

Makes no difference to us - html/json/xml/whatever, just a file to host.  Whatever works best for you all...

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v2.0

This is a first implementation for feedback.

Looking at this I've discovered that we have various versions of the urlformatter around, that is really uncool. I've updated the urlformatter to report the new used entities (I've taken most of them from nsBlocklistService.js, that indeed should use the urlFormatter service instead of going by itself, nsUpdateService.js looks the same). Looks like I'm being asked to get those values now, since probably there is some reusable code server-side, so no reasons to not make urlformatter more complete.

So, based on the fact urlformatter is Dietrich's secret nephew, asking a first review from him, I'll file bugs to convert other services to use urlFormatter once I get confirmed there is no special reason for them to not use it.
And then I'll ask for an additional review from a browser peer, but feel free to give feedback and comment on this to speed up the process.

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch v2.1

And I should attach the correct patch, too.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 472753 [details] [diff] [review]
patch v2.1

oops, looks like I wrongly interpreted original author, moving to Pike (will revert that original author change too)

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 472753 [details] [diff] [review]
patch v2.1

It's kinda cute that dietrich kept me as original author, but attachment 232935 [details] from bug 347944 comment 1 doesn't really make me an authority on this code.

WRT using this code elsewhere, at least the sent locale code differs from use case to use case. I'm actually not sure which we want to send here, or if at all. update.locale, currently selected global locale, or should we go by accept-lang header?

Comment 28

[Marco Bonardo [:mak]]

Mossop pointed out bug 430235 that was trying to achieve similar results regarding urlformatter.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 472753 [details] [diff] [review]
patch v2.1

After a brief talk in #fx-team, I'll split the patch and handle urlformatter apart, trying to avoid duplicates in urlformatter, and fixing services that are wrongly not using it.
Mossop also asked me to fix bug 469806 to ensure the url does not change, and said we can't use straight urlformatter in extensions manager (but we could maybe partially use it and add additional replaces).
Will also have to avoid breaking extensions using urlformatter as it is today.

Comment 30

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

Most of that urlformatter stuff sounds like post-4.0 re-factoring work. We should be minimizing the changes we make to the urlformatter at this stage.

Comment 31

[Marco Bonardo [:mak]]

Attachment: urlformatter patch v1.0 [checkin: comment 42]

As Gavin said, this only adds missing pieces to the urlformatter.
These are:
+    BUILD_TARGET:
+    OS_VERSION:
+    CHANNEL:
+    DISTRIBUTION:
+    DISTRIBUTION_VERSION:

I preserved names currently used by other service to minimize future changes to them.

Comment 32

[Marco Bonardo [:mak]]

Attachment: patch v1.3

remaining part of the patch. let's start with a feedback request since I'm missing the default snippet text.

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 472801 [details] [diff] [review]
urlformatter patch v1.0 [checkin: comment 42]

fwiw, I prefer tests that do not hard-code to be run on an en-US build.

Comment 34

[Marco Bonardo [:mak]]

(In reply to comment #33)
> Comment on attachment 472801 [details] [diff] [review]
> urlformatter patch v1.0
> 
> fwiw, I prefer tests that do not hard-code to be run on an en-US build.

ok, will fetch the value dynamically along with other comments.

Comment 35

[Marco Bonardo [:mak]]

Attachment: patch v1.4

Since I've moved bug 592112 down in my queue, this is unbitrotted so it can apply before that one.

Comment 36

[Axel Hecht [:Pike]]

FWIW, start.mozilla.com still resolves to google. Do we still have users out there that use that?

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

re: comment 36 - this change will only affect Firefox 4 users, no plan to back-port. start.mozilla.com will continue to resolve to google for those users. Firefox 4 will ship with about:home as the default homepage URI, not start.mozilla.com

Comment 38

[Axel Hecht [:Pike]]

Right now, start.mozilla.com is a DNS alias to google, 

host start.mozilla.com
start.mozilla.com is an alias for www.google.com.
www.google.com is an alias for www.l.google.com.

As such, we couldn't use that domain as the one to get our snippets updated from, which is the proposed domain from comment 9.

Comment 39

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

Comment on attachment 472834 [details] [diff] [review]
patch v1.4

Yeah, we can't use start.mozilla.com, but the name doesn't really matter, so that's OK! snippets.mozilla.com? Need to make sure it's set up to allow cross-site XHR, too.

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

>-function onSearchSubmit(aEvent) {
>+function onSearchSubmit(aEvent)
>+{

I would rather you omit these changes!

Comment 40

[Marco Bonardo [:mak]]

(In reply to comment #39)
> >-function onSearchSubmit(aEvent) {
> >+function onSearchSubmit(aEvent)
> >+{
> 
> I would rather you omit these changes!

well, the code style is messed up (helf one way half the other way), the correct one based on the guide should be braces on new line.

Comment 41

[Marco Bonardo [:mak]]

PS: I used a random address and a random default phrase, because I'm missing both.

Comment 42

[Marco Bonardo [:mak]]

pushed the urlformatter changes, with fixed test (comment 33)
http://hg.mozilla.org/mozilla-central/rev/a2107d95a1fd

Comment 43

[Marco Bonardo [:mak]]

had to backout because the test failed on linux and mac universal builds due to some different string (Universal-gcc3 on mac, Linux%202.6.31.5-127.fc12.i686.PAE%20(GTK%202.18.9) vs Linux%202.6.31.5-127.fc12.i686.PAE on linux).

Comment 44

[Marco Bonardo [:mak]]

Attachment: urlformatter patch v1.1

This should clear the test failures, will ensure on try once I get another patch fixed.

Comment 45

[Marco Bonardo [:mak]]

Attachment: patch v1.5

- Use proper strings from bug 593366
- Inject urls later for localization (we don't want the urls to sneak on the dtd)
- Use snippets.mozilla.org (guessed url based on previous comments.)

Comment 46

[Axel Hecht [:Pike]]

Comment on attachment 473545 [details] [diff] [review]
patch v1.5

feedback+ from me, with nits:

I'd make the link insertion code fallback gracefully if there's not link tag in the span. Just defense in depth here.

I'd like to get a localization note per snippet, with the full entity name, mentioning the url that will get linked to. That might not help for the first link right now, but it'll do in the end. Not sure if it's worth adding that that link is a 404 for now by design. Or if we should file a bug on mozilla.com to make it be a redirect to http://www.mozilla.com/en-US/firefox/beta/features/ as long as the actual 4.0 feature page isn't live yet? Probably a good idea anyway?

Comment 47

[Axel Hecht [:Pike]]

mak, did you test that we can actually get <script> tags in as part of the snippet?

<Pike> hsivonen: innerHTML = "<script>foo='bar'</script>" does not work?
<hsivonen> Pike: the script doesn't run in that case

Comment 48

[Marco Bonardo [:mak]]

(In reply to comment #46)
> I'd like to get a localization note per snippet, with the full entity name,
> mentioning the url that will get linked to.

I didn't want to put the link in more than one place to avoid them going out of sync, and forcing updates to sync multiple files. do you think that would really help more than the link description?

> That might not help for the first
> link right now, but it'll do in the end. Not sure if it's worth adding that
> that link is a 404 for now by design. Or if we should file a bug on mozilla.com
> to make it be a redirect to http://www.mozilla.com/en-US/firefox/beta/features/
> as long as the actual 4.0 feature page isn't live yet? Probably a good idea
> anyway?

in such a case we don't get status 200 and the default snippets are shown instead, so I think it doesn't matter.

Comment 49

[Marco Bonardo [:mak]]

(In reply to comment #47)
> mak, did you test that we can actually get <script> tags in as part of the
> snippet?

yeah, I just got a mail, that's an interesting problem, but it does not stop the [strings] changes for default snippets.

Comment 50

[Axel Hecht [:Pike]]

(In reply to comment #48)
> (In reply to comment #46)
> > I'd like to get a localization note per snippet, with the full entity name,
> > mentioning the url that will get linked to.
> 
> I didn't want to put the link in more than one place to avoid them going out of
> sync, and forcing updates to sync multiple files. do you think that would
> really help more than the link description?

Yeah, in particular if localizers want to use consistent wording on the link and the page. Also, there are tools that use the (entity name) piece to map comments to entities in non-source l10n editors (purely form based), and they'll likely not find the mapping right now. So we'd had to enumerate all entities one by one anyway, so duplicating the note isn't all that bad. I'm OK with that being a description like "the Firefox features page on mozilla.com" so you're not stuck on the version number, though.

> > That might not help for the first
> > link right now, but it'll do in the end. Not sure if it's worth adding that
> > that link is a 404 for now by design. Or if we should file a bug on mozilla.com
> > to make it be a redirect to http://www.mozilla.com/en-US/firefox/beta/features/
> > as long as the actual 4.0 feature page isn't live yet? Probably a good idea
> > anyway?
> 
> in such a case we don't get status 200 and the default snippets are shown
> instead, so I think it doesn't matter.

The 404 page is when you go from the displayed snippet to the page on mozilla.com from the default snippet. The URL we have is invalid right now, and I think a redirect on mozilla.com to serve the beta features page would make sense at this point.

Comment 51

[Axel Hecht [:Pike]]

PS: Yes, that redirect is also tangential to the actual implementation here.

Comment 52

[Marco Bonardo [:mak]]

fwiw, I think the snippets injection is not something that blocks beta6 once we have default snippets in place, at least.

Comment 53

[Marco Bonardo [:mak]]

(In reply to comment #47)
> mak, did you test that we can actually get <script> tags in as part of the
> snippet?

actually I've found a couple possibilities to do so, including simply searching for all <script>s in the injected code and relocating them creating new dom elements (that will be run once inserted)... so provided a snippet has a single <script> containing all the needed code, would be easy to have relocating code in aboutHome.js to run that.

Comment 54

[Marco Bonardo [:mak]]

(In reply to comment #50)
> The 404 page is when you go from the displayed snippet to the page on
> mozilla.com from the default snippet. The URL we have is invalid right now, and
> I think a redirect on mozilla.com to serve the beta features page would make
> sense at this point.

I misread this, yes having snippets linking to nonexistent pages is bad, worth filing a bug, I must admin I did not try the links thinking they were valid already!

Comment 55

[Axel Hecht [:Pike]]

Filed bug 594978.

Comment 56

[Marco Bonardo [:mak]]

Attachment: urlformatter patch v1.2 [checkin: comment 59]

still a glitch in the test, passes locally.

Comment 57

[Marco Bonardo [:mak]]

Attachment: patch v1.6

should fix Pike's comments.

Comment 58

[Axel Hecht [:Pike]]

Comment on attachment 473786 [details] [diff] [review]
patch v1.6

joy :-)

Comment 59

[Marco Bonardo [:mak]]

pushed again the urlformatter changes:
http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7

Comment 60

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

(In reply to comment #59)
> pushed again the urlformatter changes:
> http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7

spotted a typo in these:
http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7#l1.88
"distibution" -> "distribution"

Comment 61

[Marco Bonardo [:mak]]

(In reply to comment #60)
> spotted a typo in these:
> http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7#l1.88
> "distibution" -> "distribution"

good catch, will push a typo + test fix (setting the prefs in advance)

Comment 62

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

Comment on attachment 473786 [details] [diff] [review]
patch v1.6

>diff --git a/browser/base/content/aboutHome.css b/browser/base/content/aboutHome.css

>+#defaultSnippets {
>+  text-align: center;
>+}

Why isn't this needed on #snippets too?

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

>+function loadSnippets()

>+    // Try to update from network.
>+    let xhr = new XMLHttpRequest();

We should probably set mozBackgroundRequest = true (won't make much of a difference in the common case because the URL won't be https, but still doesn't hurt).

Comment 63

[Marco Bonardo [:mak]]

(In reply to comment #62)
> >+#defaultSnippets {
> >+  text-align: center;
> >+}
> 
> Why isn't this needed on #snippets too?

as discussed on IRC, snippets can do this by themselves, and it's too early to figure out their needs.

Comment 64

[Marco Bonardo [:mak]]

Attachment: fix urlformatter typo [checkin: comment 66]

Comment 65

[Marco Bonardo [:mak]]

Attachment: patch v1.7 [checkin: comment 66]

added mozBackgroundRequest, ready to land.

Comment 66

[Marco Bonardo [:mak]]

urlformatter typo fix:
http://hg.mozilla.org/mozilla-central/rev/662d1650cc5c
snippets:
http://hg.mozilla.org/mozilla-central/rev/3e24e4a0c2b5

Comment 67

[Marco Bonardo [:mak]]

looks like this needs a try finally still, so that any exception is handled with defaultSnippets. I see a DOM_SECURITY_ERROR exception coming from setting mozBackgroundRequest :(

Comment 68

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

Oh, you can't set mozBackgroundRequest without privilege. Just remove it?

Comment 69

[Marco Bonardo [:mak]]

(In reply to comment #68)
> Oh, you can't set mozBackgroundRequest without privilege. Just remove it?

Checking the code at (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2857) this is true, but actually MDC does not say that. See https://developer.mozilla.org/en/XMLHttpRequest the channel property is clearly stated as being chrome-only but there is no note on mozBackgroundRequest. I'll add such a note to MDC and remove that line of code.

Comment 70

[Marco Bonardo [:mak]]

I've updated the docs.

Comment 71

[Marco Bonardo [:mak]]

pushed the property removal
http://hg.mozilla.org/mozilla-central/rev/84ee6bc0484d

Comment 72

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug is being tracked as part of this set: http://areweprettyyet.com/4/syncPromotion/