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

RESOLVED FIXED in Firefox 4.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings][about-home], )

Attachments

(3 attachments, 11 obsolete attachments)

9.07 KB, patch
Details | Diff | Splinter Review
2.69 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.92 KB, patch
Details | Diff | Splinter Review
we should not lose possibility to add current special links in the first incarnation of the home page
Summary: Add current Mozilla badges in the about:home page → Add current Mozilla "snippets" (links and special events) in the about:home page
Assignee: nobody → mak77
Depends on: 568091
Depends on: 568095
Posted patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #447402 - Flags: feedback?(bmcbride)
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.
(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.
(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 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).
Attachment #447402 - Flags: feedback?(bmcbride) → feedback+
(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.
Posted patch patch v1.1 (obsolete) — Splinter Review
small update, fixes most of the comments
Attachment #447402 - Attachment is obsolete: true
Posted patch patch v1.2 (obsolete) — Splinter Review
updated on latest about:home patch.
Attachment #449449 - Attachment is obsolete: true
Whiteboard: [needs bug 563723][needs feedback/service from marketing/webdev]
Whiteboard: [needs bug 563723][needs feedback/service from marketing/webdev] → [needs feedback/testService from marketing/webdev]
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.
blocking2.0: --- → beta6+
Whiteboard: [needs feedback/testService from marketing/webdev]
(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.
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.
(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.
(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?
(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.
(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?
(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.
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/.
(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.
... 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?
(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.
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.
(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.
(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...
Posted patch patch v2.0 (obsolete) — Splinter Review
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.
Attachment #451679 - Attachment is obsolete: true
Attachment #472752 - Flags: review?(dietrich)
Posted patch patch v2.1 (obsolete) — Splinter Review
And I should attach the correct patch, too.
Attachment #472752 - Attachment is obsolete: true
Attachment #472752 - Flags: review?(dietrich)
Attachment #472753 - Flags: review?(dietrich)
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)
Attachment #472753 - Flags: review?(dietrich) → review?(l10n)
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?
Attachment #472753 - Flags: review?(l10n) → review?(dietrich)
Mossop pointed out bug 430235 that was trying to achieve similar results regarding urlformatter.
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.
Attachment #472753 - Flags: review?(dietrich)
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.
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.
Attachment #472753 - Attachment is obsolete: true
Attachment #472801 - Flags: review?(dietrich)
Posted patch patch v1.3 (obsolete) — Splinter Review
remaining part of the patch. let's start with a feedback request since I'm missing the default snippet text.
Attachment #472807 - Flags: feedback?(gavin.sharp)
Depends on: 593366
Whiteboard: [urlformatter changes need review][patch waiting for feedback and bug 593366]
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.
(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.
Posted patch patch v1.4 (obsolete) — Splinter Review
Since I've moved bug 592112 down in my queue, this is unbitrotted so it can apply before that one.
Attachment #472807 - Attachment is obsolete: true
Attachment #472834 - Flags: feedback?(gavin.sharp)
Attachment #472807 - Flags: feedback?(gavin.sharp)
FWIW, start.mozilla.com still resolves to google. Do we still have users out there that use that?
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
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.
Attachment #472801 - Flags: review?(dietrich) → review+
Whiteboard: [urlformatter changes need review][patch waiting for feedback and bug 593366] → [patch waiting for feedback and bug 593366]
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!
Attachment #472834 - Flags: feedback?(gavin.sharp) → feedback+
(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.
PS: I used a random address and a random default phrase, because I'm missing both.
pushed the urlformatter changes, with fixed test (comment 33)
http://hg.mozilla.org/mozilla-central/rev/a2107d95a1fd
Attachment #472801 - Attachment description: urlformatter patch v1.0 → urlformatter patch v1.0 [checkin: comment 42]
Whiteboard: [patch waiting for feedback and bug 593366]
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).
Posted patch urlformatter patch v1.1 (obsolete) — Splinter Review
This should clear the test failures, will ensure on try once I get another patch fixed.
Attachment #472801 - Attachment is obsolete: true
Posted patch patch v1.5 (obsolete) — Splinter Review
- 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.)
Attachment #472834 - Attachment is obsolete: true
Attachment #473545 - Flags: review?(gavin.sharp)
Attachment #473545 - Flags: review?(l10n)
Whiteboard: [strings]
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?
Attachment #473545 - Flags: review?(l10n) → feedback+
Whiteboard: [strings] → [strings][has patch][needs review gavin]
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
(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.
(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.
(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.
PS: Yes, that redirect is also tangential to the actual implementation here.
fwiw, I think the snippets injection is not something that blocks beta6 once we have default snippets in place, at least.
(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.
(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!
still a glitch in the test, passes locally.
Attachment #473484 - Attachment is obsolete: true
Posted patch patch v1.6 (obsolete) — Splinter Review
should fix Pike's comments.
Attachment #473545 - Attachment is obsolete: true
Attachment #473786 - Flags: review?(gavin.sharp)
Attachment #473786 - Flags: feedback?(l10n)
Attachment #473545 - Flags: review?(gavin.sharp)
Blocks: 594989
Comment on attachment 473786 [details] [diff] [review]
patch v1.6

joy :-)
Attachment #473786 - Flags: feedback?(l10n) → feedback+
pushed again the urlformatter changes:
http://hg.mozilla.org/mozilla-central/rev/5e7e8e3404c7
Attachment #473784 - Attachment description: urlformatter patch v1.2 → urlformatter patch v1.2 [checkin: comment 59]
(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"
(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 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).
Attachment #473786 - Flags: review?(gavin.sharp) → review+
(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.
Attachment #474228 - Flags: review?(gavin.sharp)
added mozBackgroundRequest, ready to land.
Attachment #473786 - Attachment is obsolete: true
Attachment #474228 - Flags: review?(gavin.sharp) → review+
Whiteboard: [strings][has patch][needs review gavin] → [strings][has patch][can land]
urlformatter typo fix:
http://hg.mozilla.org/mozilla-central/rev/662d1650cc5c
snippets:
http://hg.mozilla.org/mozilla-central/rev/3e24e4a0c2b5
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [strings][has patch][can land] → [strings]
Target Milestone: --- → Firefox 4.0b6
Attachment #474228 - Attachment description: fix urlformatter typo → fix urlformatter typo [checkin: comment 66]
Attachment #474230 - Attachment description: patch v1.7 → patch v1.7 [checkin: comment 66]
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 :(
Depends on: 595394
Oh, you can't set mozBackgroundRequest without privilege. Just remove it?
(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.
I've updated the docs.
This bug is being tracked as part of this set: http://areweprettyyet.com/4/syncPromotion/
Whiteboard: [strings] → [strings][about-home]
Depends on: 676552
Depends on: 682944
No longer depends on: 682944
Depends on: 682944
You need to log in before you can comment on or make changes to this bug.