Closed Bug 563723 Opened 10 years ago Closed 9 years ago

Add an about:home page that mimics current start page

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: mak, Assigned: mak)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Whiteboard: [in-litmus-bug-week][about-home])

Attachments

(2 files, 12 obsolete files)

We should create a local copy of the start page that we can provide through an "about:home" tab.
This special tab could then become an home-tab (no label, sticky) similar to an app-tab.
In future the page will be expandable and easily changeable without having to go through the usual approval and deployment tasks.
Blocks: 563734
this is a bit different, Bug 563734 is more similar (same)
Summary: Create a simple "home" tab that mimics current start page → Add an about:home page that mimics current start page
Clarified the title a bit.
Does this newly created bugs mean that you are starting working on it? Or someone else?
Blocks: 544819
No longer blocks: 563734
Blocks: 563738
We are still in taking-off stage, you'll know once assignee will be set.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
This is current search page code/images, epurated from external scripts and whistles and reindented.
Attachment #445962 - Attachment is patch: false
Attachment #445962 - Attachment mime type: text/plain → application/zip
Attached patch patch v1.0 (obsolete) — Splinter Review
Problems to solve still:

- I've solved Google localization askin localizers to provide the Google UI locale, as defined in Use the GoogleInterface in Your Language http://www.google.com.au/language_tools. So for example is "en" for english, "it" for italian. We have some search url in region.properties, but I did not give this page chrome privileges and those urls are for site searches. I asked gandalf if there was a way to get "it", or "en" from the locale, but he told me that not always the fallback xx-XX => xx is correct.

- Mozilla snippets are somehow in this svn: http://viewvc.svn.mozilla.org/vc/projects/google_snippets/. I need a mozilla.org hosted script that gives me a random ready snippet or a json with {icon , description}, I could pass the google UI locale to this script to get the localized string.

- The default engine as defined in region.properties could not be Google, but the page currently assumes Google and I also added a Google logo (since we wanted the page to be exactly like the old one). If we want to use default search engine based on locale I'll need chrome privileges and clearly we can't add all of the logos (we could ask to put a data uri in the localization file though).

apart the above things, this seems to work fine, tested ltr and rtl, with various font sizes and zoom.
Attachment #446219 - Flags: feedback?(johnath)
The start page on google infrastructure has probably tens or even hundreds of millions of page views per day, so I guess we need wery heavy server requirements there and if we want random promotional snippets displayed, it probably means no static caching of the page (unless we do it all javascript maybe). That's just FYI.
I think we could have a single page (or json file) with all associations in it, and have js code extract a random entry from it (So caching issue should be solved). But there could still be some high server requirement, not sure about this point since our network infrastructure is far out of my sight.
An hacky alternative could be to parse and extract things from the current page, but looks really really really hackish.
Johnath, we should probably figure out a plan about this thing before starting working on the above page/json.
Wouldn't it just be better to have the snippets locally and update them if necessary with new firefox releases ?
we would loose much of the marketing interest in doing so (the possibility to update them at any time to promote our activities). This is how we did it for Firefox 1.0 and snippets became very rapidly outdated.
There is a Firefox release nearly every month. From the svn link above, it doesn't look like the snippets are updated so often that updating once a month would make them outdated. If updating through Firefox releases isn't considered frequent enough, how about the browser checks for updates once in a while. This would be much less demanding than performing requests each time the about:home page is opened.
(In reply to comment #12)
> There is a Firefox release nearly every month.

Many users lag to update, for various reasons, also because major upgrades are suggested months after they are available. Thus I think that even if we have a release every month, in case of major releases we have many users that won't be on that release per months.
The current frequency of updates of snippets depends on our agreement with Google, we can't update them more than once a month, it doesn't mean that we don't want to update them whenever we want.
We talked about this briefly on the l10n call today:

IMHO, we should ship at least one default snippet, and update more snippets on the fly "every now and then". I don't mind whether daily, weekly, monthly. Pascal suggested that we could possibly use localstorage for the snippets, which probably means that we can use privilegded chrome code to update the snippets, and have non-priv'ed code in about:home to read it out. Hopefully, that is.

We should get a product decision on whether we tie ourselves to google as search provider. The obvious candidates for alternatives would be Russia (using yandex as default search), and the Kurdish locale, which is suffering from being Kurdish in latin script in the UI, while the Kurdish locale on google is only arabic script.
We'll want a solution for snippets here that can be more fluid than blocking on software updates, absolutely. We'll need to work with webdev/IT to figure out how to serve it, but I imagine it will end up looking something like:

1) Browser fetches a localized JSON file "once in a while"
2) Browser keeps that locally "somewhere"
3) Browser "randomly" chooses one of the supplied snippets each about:home page load 

If that ends up being how things play out, we could probably build out the logic like this:

1) When about:home is loaded, we pull the JSON blob out of the page's localStorage, pick a snippet, and display it
2) In the background on about:home, after page load, we check whether our blob is more than $expiry_time old, and if so, we XHR ourselves a new one from a pref-controlled URL
3) For now, that pref points to a chrome/resource URL and a bundled json file

If this is crazy, say so. If it isn't, though, it lets us have a page which is 100% locally hosted, gives us an ability to manage load by controlling expiry time, and lets us nail down the structure of this file before/while talking with IT about how to host it.

I'm guessing, in this world, that the JSON blob itself would mostly be a set of icon+text pairings (localized), but may also contain some metadata like a TTL, or "don't update until" timestamp.

Crazy?
Well, this workload, that looks working, still has a requirement, that the page has chrome privileges. If we split the work as explained in comment 15, we can have a piece of chrome code updating contents and the page picking it up.
Regardless these 2 pieces, I think we should take decision whether we need this page being content or chrome privileged, this pretty much changes the way we implement on it.
discussing on IRC, the best option would be the separation, chrome background component takes care of updating data in the page's localStorage, and the page is notified when data is changed/available (or just keeps polling).

We should have some sort of "widgets" that can understand data and a data provider for each widget that knows how to feed it.

Since the home page is going to have different kind of contents on it and components willing to use it, managing security of all of them separately could be non-trivial. While security of plain-data is much simpler to ensure.
I think there should be a set of snippets delivered with the software. I'm pretty sure there is a subset that is stable enough to be shipped with the software. That would have several advantages, such as allowing offline installations to display a snippet without requiring network connectivity even the first time, or simply avoid network delays when displaying about:home the first time.
current proposal is a mixture. Notice this is not finalized at all:

- about:home stays content privileged per security reasons
- background components can write jsoned objects to its localStorage
- a widget able to understand the data is added to the page
- read/write should happen in a worker thread, because localStorage activity should not hurt ui responsiveness
- if a widget has no data, it should not appear, if it has, it can appear as soon as data has been parsed
- data can be updated in background, as the component prefers, so for example Weave could write remote tabs when they are synced
- bonus points if the widget can be notified when data is changed and live refresh itself
problems found so far:
- DOM Storage is not thread safe, the specs talk about a storage mutex, but we don't implement any, thus it's not possible to use it across different threads, nor in worker threads, it is more like any other DOM change, main-thread only.
- DOM storage is unable to handle about: URIs because they don't have a host.

Other parts should be done.

Most likely I need to figure out a way around empty host, and to check if we can make DOM storage at least do writes on a separate thread.  Its structure is far more complex than what it does, it is managing 3 separate databases (disk, memory, privatebrowsing) when it could just have one partitioned across disk and memory, with an async flusher. Rewriting it is far from this bug though. Actually could be a P2 since for snippets we would just need to update once a week and that would not hurt, clearly increasing number of widgets it could become an issue.
Let's be careful not to get trapped in an implementation choice we don't like, too. If DOM Storage isn't what we want, we could always use something like chrome-initiated postMessage calls that spin off worker threads. Or, for that matter, we could use (crappy, synchronous, main thread) DOM Storage for now, with a blocker to convert to IndexDB or something more pleasingly thread-friendly.
Attached patch patch v1.1 (obsolete) — Splinter Review
Should work as expected, I did not put magic into it, since this is for initial landing and there is much we could add there.
Snippets are handled in a separate bug (since they have some dom storage dependency)

Johnath, setting review on you, but feel free to forward.
Attachment #446219 - Attachment is obsolete: true
Attachment #447397 - Flags: review?(johnath)
Attachment #446219 - Flags: feedback?(johnath)
Anybody interested in the "Mozilla snippets" part should move to bug 563738.
Comment on attachment 447397 [details] [diff] [review]
patch v1.1

Why does this patch hardcode so much Google stuff into browser/ when Google isn't the default start page or search engine for every locale build we ship? That seems suboptimal and just wrong...
So, if I understand this correctly, after it lands, Home Tab can be created?
(In reply to comment #25)
> (From update of attachment 447397 [details] [diff] [review])
> Why does this patch hardcode so much Google stuff into browser/ when Google
> isn't the default start page or search engine for every locale build we ship?
> That seems suboptimal and just wrong...

Because the requirement was explicitly to reproduce current start page as first step, see my comments above. I'm more than fine to change that if requirements change, and changing that later is not hard, most likely would just require to ask localizers for an engine string, and links to options (if present), logo would be hard (and most likely removed).
Also, this is not a change in behavior I think, since our current start page has google search.
See my comment 15, too.

We should hopefully do something more fool-proof than putting urls into l10n, that's usually just causing grief. In particular the step to keep google should be hard to mess with.

Another side question, do we still need to pass special params to google's search for official builds?
We could have a json file defining all the search engines, and l10n could handle this file internally (from what I got there would be 2 or 3 locales requiring different engines, so it should not be a huge work, but I let to you to state that). The json could contain for each search engine a logo datauri, engine string and engine links. It would be the same for all locales (I'd not mind much the overhead of a couple data uris).

I'm not sure about the second question, since I can't tell what is a "special" param.
Go to the current start page with an official release build, and you'll find yourself searching ala

http://www.google.com/search?client=firefox-a&rls=org.mozilla%3Aen-US%3Aofficial&channel=s&hl=en&source=hp&q=foo+yeah&meta=&btnG=Google+Search
I'm retaining all the params but channel (?), client and rls. The latters are version of the client, not sure what Google use them for, I imagine for the navigation bar in the page, they can easily gather this information from the UA so it does not seem sensible.
Are there try server builds available with this patch?
Not yet, I will soon update DOM storage patch to something that makes sense, fix first comments I got and produce a build.
Flags: in-litmus?
Attached patch patch v1.2 (obsolete) — Splinter Review
Moved all search engines definitions to a separate file, added Google and Yandex so far, added also official/unofficial and distribution id values.

Axel, i added a locale.name to global.dtd, I have no better ideas on how to gather this in a content context, would that be an issue for localizers? I use it to catch a good locale for the search engine UI (and the search engine itself) and to put it into the query string.

Drive-by reviews are welcome as usual.
I'll update the snippets patch and produce a trybuild.
Attachment #447397 - Attachment is obsolete: true
Attachment #447397 - Flags: review?(johnath)
Attached patch patch 1.3 (obsolete) — Splinter Review
minor fixes
Attachment #449433 - Attachment is obsolete: true
first trybuilds available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mak77@bonardo.net-90a62915efd1/

notice the page is not linked anywhere, you will have to navigate to it manually
ps: snippets will appear after 5 minutes of idle
Is there any reason why we can't get the search engine information from the search service, rather than adding another data store? You also don't need to worry about l10n concerns in that case.
(In reply to comment #38)
> Is there any reason why we can't get the search engine information from the
> search service

I thought about reusing that pieces (if you look at my definitions you can find some borrowed part indeed).
The first problem is that this page does not have chrome privileges, because will end up being a mixup of information that could partly come from chrome, partly from the web. Search engine must be immediately available on page opening, cannot be lazy loaded like snippets or user history stats. I'm not sure i can read those xmls from content, most likely xhr would not allow me to access them (indeed i could not even use a json for engine definition, and I went for a script, that i can just include). I cannot even access search service preferences.
The search service definitions don't have a larger logo of the engine, that we have on the page (and I was required to reproduce what we have today).
The definition translates hl (google ui locale) with {moz:locale} but some of our locales code don't have 1:1 fallbacks thus I do a fallbacks handling (try exact match, try partial match, fallback to "en").
The search service definitions are completely up to localizers, while the start page is more mozilla-side (not sure engines will always match across our needs and localized engines, even if today it should be the same).
oh, and the SS definitions don't allow for additional links (like advanced search, preferences, or whatever engines could ask us to add)
It would be relatively easy to inject the search engine information into the page from chrome, right? That info is already all loaded at startup.

The larger logo would need to be handled separately, you're right, but that doesn't sound like a big deal.

We already support non-locale-dependent plugins (Google being one of them), so I don't think our desired start page provider not matching one of the localized engines is something we need to worry about.

Our Google plugin doesn't pass the hl parameter (it passes moz:locale in the "rls" parameter, but that isn't used for locale detection AFAIK). If we want to start mapping our locale codes to Google locale codes and passing them in the hl parameter for this, we should do it for the search bar as well, and I think that has larger implications that are best dealt with in a separate followup bug.
(In reply to comment #41)
> It would be relatively easy to inject the search engine information into the
> page from chrome, right? That info is already all loaded at startup.

If i'd know what to inject yes, I still don't know if we want to allow users to switch engines, if the engine should be the currently selected one or the default one. This also adds some work to the Ts path (localstorage will have to init the persistent DB, unless at each start i use a sessionstorage, still it's additional work at each start).

> The larger logo would need to be handled separately, you're right, but that
> doesn't sound like a big deal.

Each locale can add their own engines, how do we manage logos then? unless you are suggesting to just gather google.xml and yandex.xml and not all search plug-ins. Still if a locale changes the default engine we have to add the logo and it's hard to sync the two things, l10n team will have to notify each time that happens.

> Our Google plugin doesn't pass the hl parameter (it passes moz:locale in the
> "rls" parameter, but that isn't used for locale detection AFAIK).

The suggestion url passes "hl" param (and I guess it could even be wrong if our locale name does not match), not sure why the normal search does not.

If we want to
> start mapping our locale codes to Google locale codes and passing them in the
> hl parameter for this, we should do it for the search bar as well, and I think
> that has larger implications that are best dealt with in a separate followup
> bug.

We are already doing it for suggestions. But as I said i'm not sure how they guess the locale if we don't pass rls (that is the case for unofficial builds) nor hl. Right now they most likely check ip and provide something based on that.
Also, I use hl to provide links to engine localized pages, if I don't ask for a UI locale then I will remove any engine links since putting an italian user onto a english advanced search page would be really bad. That is going to regress functionality of the page.
Yes, the current Google locale detection is some mixture of accept-lang/UA locale/geolocation. That we pass moz:locale in the suggestions URL is a bug (fixed by the patch in bug 557890). Let's not let the hl-issue sidetrack this bug.

We don't need to support arbitrary engines, and there's no need to worry about which engine is selected in the search bar. We only have a few start page providers at the moment (I think Google/Yandex/Baidu is exhaustive, but I'm not sure), and I'm pretty sure they all have associated plugins in the relevant locales whose information we can use, and supplement with hardcoded larger icons or additional URLs, if needed.
Whiteboard: [next: gather search strings from search service xml definitions]
Attached patch patch v1.4 (obsolete) — Splinter Review
inject defaultEngine values from search service, remove the localization hacks that are now not needed anymore.
Attachment #449442 - Attachment is obsolete: true
Whiteboard: [next: gather search strings from search service xml definitions]
Attached patch patch v1.5 (obsolete) — Splinter Review
minor update
Attachment #451653 - Attachment is obsolete: true
Comment on attachment 451678 [details] [diff] [review]
patch v1.5

>+            var isAllowedPage = false;
>+            [
>+              /^about:neterror\?/,
>+              /^about:blocked\?/,
>+              /^about:certerror\?/,
>+              /^about:home$/,
>+            ].some(function(re) {
>+              return isAllowedPage = re.test(targetDoc.documentURI);
>+            });

This is weird, should look like this instead:
var isAllowedPage = [...].some(function (re) re.test(targetDoc.documentURI));.
Is AboutHomeUtils.jsm going to be used in different places? If not, I'd say it shouldn't be a module.
(In reply to comment #47)
> Is AboutHomeUtils.jsm going to be used in different places? If not, I'd say it
> shouldn't be a module.

Ideally any browser component could want to inject things into the page, that's why it's a module. For now it's used only by glue.
Is the jsm bringing perf wins, too?
Only if you use it multiple times, it's just overhead otherwise.
(In reply to comment #50)
> Only if you use it multiple times, it's just overhead otherwise.

well it depends, it's lazyloaded instead of being a browserglue object that should be created on component load, plus it's going through fastload. Btw since the design part is still a bit up in the clouds I did not want to limit future possibilities, if I make it a bg only component than everything willing to interact will have to pass through browserGlue, than we will have a worse overhead.
(In reply to comment #51)
> (In reply to comment #50)
> > Only if you use it multiple times, it's just overhead otherwise.
> 
> well it depends, it's lazyloaded instead of being a browserglue object

We're talking about plain object creation with no code actually running, right?

I've only skimmed the patch, I don't even know what the module exactly does, so I can't tell if preparing this code for being reused in the future makes sense. If you have a clear use case in mind, fine, but otherwise I would keep this simple and plain and only move it to a module when a concrete use case arises.
Discussed this with Marco today; we should unbitrot this patch and get it reviewed so we can land this early next week to bake on nightlies.

We might also want to file follow-up bugs for:

 - updating the design to use a less compressed version of the Firefox logo, and to use a large-text search box as used on www.google.com

 - interacting with a snippet service that's hosted on www.mozilla.com
blocking2.0: --- → beta4+
(In reply to comment #53)
>  - updating the design to use a less compressed version of the Firefox logo

As we're likely to extend the places where we support SVG images with Firefox 4 (see bug 276431), perhaps we could make it an SVG-image (to highlight this)? It will scale to any size, and if done properly, be lighter in file-size. Do we have an SVG-version of the Firefox logo already?
(In reply to comment #54)
> As we're likely to extend the places where we support SVG images with Firefox 4
> (see bug 276431), perhaps we could make it an SVG-image (to highlight this)? It
> will scale to any size, and if done properly, be lighter in file-size. Do we
> have an SVG-version of the Firefox logo already?

no idea where I can get an official one, but would be cool. I am using the 128px png version so far.
ps: notice that the icons I'm using are depending on the build, so on Minefield the icon is the Minefield's one. if we add an svg file, would be cool to have it in the branding folders so it's auto-adapting, thus we need svgs for all brandings (minefield, unofficial, firefox, ...).
Let's handle the SVG question in a separate bug. Not sure we need arbitrary scaling since we can always generate new images from the original artwork as needed. Showcasing the tech is good, but I suspect SVG is also more startup/pageload burden, would like to split it off from the initial landing, so they can be evaluated independently.
Attached patch patch v1.6 (obsolete) — Splinter Review
I'll make a trybuild out of this.
Attachment #451678 - Attachment is obsolete: true
for anyone interested in lookingat this simple page http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mak77@bonardo.net-b3991367e9d0/
the page is not linked yet, so you have to manually navigate to about:home
(In reply to comment #55)
> I am using the 128px png version so far.

Agreed SVG is a separate issue, but if you happen to want a higher res image, the official logos at various sizes are at https://wiki.mozilla.org/Firefox3.5/Logos
FWIW, I glanced over the try server build, nothing obviously bad. Of course, the devil is in the details (literally translated from German, that is).
Screenshot from the build: http://grab.by/5Q9i

(Marco: does the patch also have the Firefox branding for when --official-branding is enabled?)
(In reply to comment #62)
> (Marco: does the patch also have the Firefox branding for when
> --official-branding is enabled?)

it gets stuff from whatever branding folder the product is built with
Comment on attachment 464397 [details] [diff] [review]
patch v1.6

Detail nit,

+<!-- LOCALIZATION NOTE (brandStart):
+     Anything that is not brandShortName must be in a <span/>
+-->
+<!ENTITY abouthome.brandStart "&brandShortName; <span>Start</span>">

I'd prefer that to be the other way around, I guess. I somewhat consider the brandShortName to be the highlight or extra markup, not the rest. In particular if a locale had "Start" to be more than a word.

In an earlier patch, I saw an extra entity 'en-US' flying by. I didn't reverse engineer that completely, that's gone?
(In reply to comment #64)
> Comment on attachment 464397 [details] [diff] [review]
> patch v1.6
> 
> Detail nit,
> 
> +<!-- LOCALIZATION NOTE (brandStart):
> +     Anything that is not brandShortName must be in a <span/>
> +-->
> +<!ENTITY abouthome.brandStart "&brandShortName; <span>Start</span>">
> 
> I'd prefer that to be the other way around, I guess. I somewhat consider the
> brandShortName to be the highlight or extra markup, not the rest. In particular
> if a locale had "Start" to be more than a word.

Yeah, unfortunatly I use css opacity there and so doing the opposite would be hard, could workaround some other way.

> In an earlier patch, I saw an extra entity 'en-US' flying by. I didn't reverse
> engineer that completely, that's gone?

Yeah, killed it, it was hackish
Whiteboard: [needs a reviewer][waiting feedback on trybuild and search params]
There is one concern regarding licensing: the google and yandex logos are certainly not tri-licensed, let alone free (as in free speech).
(In reply to comment #66)
> There is one concern regarding licensing: the google and yandex logos are
> certainly not tri-licensed, let alone free (as in free speech).

hm, yeah, good catch, that's easy to remove luckily.
Comment on attachment 464397 [details] [diff] [review]
patch v1.6

I collected a request from Pike to figure out span and one to remove search engine logos. Apart those it would be really cool to start reviewing and evaluating changes, so starting with review requests.
Notice this does not fix the above 2 requests, I'll fix those along with other comments.
Alos this is blocked on getting external feedback related to the search engine strings.
Attachment #464397 - Flags: review?(mano)
I'm moving this to beta5+ as I think it's a little risky to take this late, but we should make sure to drop it into nightlies ASAP, tomorrow if we can.

The Google team is monitoring this bug and will take an integrated nightly build and put it through their tests to ensure that they're seeing the right things. We can make changes based on that feedback once we've got the main trunk of code landed.
blocking2.0: beta4+ → beta5+
Started reviewing.  Review to be posted early tomorrow.
Comment on attachment 464397 [details] [diff] [review]
patch v1.6

Nothing major.

+const SEARCH_ENGINES = [
+  {
+    name: "Google"

It would be better to build the object in this form:

const SEARCH_ENGINES = [
  "Google": {
   …
   }
}

And then you could use SEARCH_ENGINES[engineName] rather than a loop.

+function onLoad(event)
+{
+  setupSearchEngine();
+  document.searchForm.searchText.focus();

Make that document.getElementById("searchText")

+function onSearchSubmit(event) {
+  let searchTerms = document.searchForm.searchText.value;

ditto.

+  if (gSearchEngine && searchTerms.length > 0) {
…
+  event.preventDefault();

What's this for?

+}
+
+
+function setupSearchEngine() {
+  gSearchEngine = JSON.parse(localStorage["search-engine"]);
+
+  // Look for extended information, like logo and links.
+  for (let i = 0; i < SEARCH_ENGINES.length; i++) {
+    if (gSearchEngine.name == SEARCH_ENGINES[i].name) {

See my previous comment on the structure of this object.

+  if (gSearchEngine.image) {
+    // Add search engine logo.
+    let logoElt = document.getElementById("searchEngineLogo");
+    logoElt.src = gSearchEngine.image;

nit: inline the variable here.

+  }
+
+  if (gSearchEngine.links && gSearchEngine.links.length > 0) {
+    // Add search engine links.
+    let linksElt = document.getElementById("searchEngineLinks");
+    gSearchEngine.links.forEach(function(aLink) {
+      let linkElt = document.createElement('a');
+      linkElt.href = parseSearchEngineUrl(aLink.href);
+      let linkLabel = document.createTextNode(gSearchEngineLinksLabels[aLink.id]);

1) See my comment about the usage of those strings. 2) If you do decide to go with the standardized labels, just hide and unhide static elements.


diff --git a/browser/base/content/aboutHome.xhtml b/browser/base/content/aboutHome.xhtml
new file mode 100644
--- /dev/null
+++ b/browser/base/content/aboutHome.xhtml

+    <script type="text/javascript;version=1.7"><![CDATA[

Why 1.7?

+      let gSearchEngineLinksLabels = {
+        "advanced": "]]>&abouthome.searchEgineLinks.advanced;<![CDATA["
+      , "preferences": "]]>&abouthome.searchEgineLinks.preferences;<![CDATA["
+      };

I don't think it'd be smart to standardize those strings. Those string should go to
a .properties file and be per Engine.

+    ]]></script>
+    <script type="text/javascript;version=1.7"

Ditto.

+            src="chrome://browser/content/aboutHome.js"/>
+  </head>
+
+  <body dir="&locale.dir;" onload="onLoad(event)">
+

AFAICT locale.dir usage is deprecated.  Use -moz-locale-dir instead (as an inline css).

+          <img id="searchEngineLogo" title="&abouthome.searchEngineLogo.title;"/>
+          <form name="searchForm" onsubmit="onSearchSubmit(event)">
+          <input type="text" name="searchText" value="" id="searchText" maxLength="256"/>
+          <br/>
+          <input type="submit" value="&abouthome.searchEngineButton.label;"/>
+          <span id="searchEngineLinks"></span>
+          </form>

nit: please fix the indent here.

diff --git a/browser/branding/nightly/content/jar.mn b/browser/branding/nightly/content/jar.mn
--- a/browser/branding/nightly/content/jar.mn
+++ b/browser/branding/nightly/content/jar.mn
@@ -5,3 +5,5 @@ browser.jar:
   content/branding/aboutFooter.png               (aboutFooter.png)
   content/branding/icon48.png                    (icon48.png)
   content/branding/icon64.png                    (icon64.png)
+  content/browser/icon128.png                    (../mozicon128.png)
+  content/browser/icon16.png                     (../default16.png)

Are those temporary? Shouldn't we have copies within this directory?

diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js

+let AboutHomeUtils = {
+  get _storage() {
+    delete this._storage;
+    let aboutHomeURI = Services.io.newURI("moz-safe-about:home", null, null);
+    let principal = Cc["@mozilla.org/scriptsecuritymanager;1"].
+                    getService(Ci.nsIScriptSecurityManager).
+                    getCodebasePrincipal(aboutHomeURI);
+    let dsm = Cc["@mozilla.org/dom/storagemanager;1"].
+              getService(Ci.nsIDOMStorageManager);
+    return this._storage = dsm.getLocalStorageForPrincipal(principal, "");
+  },
+

defineLazyGetter should be used here

+  loadDefaultSearchEngine: function AHU_loadDefaultSearchEngine()
+  {
+    let defaultEngine = Services.search.defaultEngine;
+    let searchurl = defaultEngine.getSubmission("placeholder").uri.spec;
+    let engine = {
+      name: defaultEngine.name
+    , description: defaultEngine.description
+    , iconUrl: defaultEngine.iconURI.spec
+    , searchUrl: searchurl.replace("placeholder", "{searchTerms}")
+    }
+    this.setItem("search-engine", JSON.stringify(engine));
+  },
+

Gavin, could you double-check the search service usage here?

+  setItem: function AHU_setItem(aName, aValue, aCallback, aThisObject)
+  {
+    if (typeof(aName) != "string" || aName.length == 0 ||
+        typeof(aValue) != "string")
+      throw new Error("setItem arguments should be valid strings.");
+

Isn't |throw| enough (ditto for the other instances)?

+  _callback: function AHU__callback(aCallback, aThisObject)
+  {
+    if (!aCallback || typeof(aCallback) != "function")
+      return;

typeof(aCallback) != "function"… How would that happen?

diff --git a/browser/locales/en-US/chrome/browser/aboutHome.dtd b/browser/locales/en-US/chrome/browser/aboutHome.dtd

+<!ENTITY abouthome.brandStart "&brandShortName; <span>Start</span>">
+

I don't see the value of this span.

diff --git a/browser/themes/gnomestripe/browser/aboutHome.css b/browser/themes/gnomestripe/browser/aboutHome.css
new file mode 100644
--- /dev/null
+++ b/browser/themes/*/browser/aboutHome.css

+body[dir="ltr"] #brandLogo {
...
+body[dir="rtl"] #brandLogo {

-moz-locale-dir should be used instead.
Attachment #464397 - Flags: review?(mano) → review-
1. event.preventDefault - figured later why it's there.
2. -moz-locale-dir doesn't work in non-xul documents :-/
More wrong review comments from me...
1. Components.utils.import is disabled for content
2. throw new Error generates a bit more useful error object.
Attached patch Updated patch (obsolete) — Splinter Review
Conservatively updated patch.
Attachment #464397 - Attachment is obsolete: true
Attachment #467608 - Flags: review?(dietrich)
Attachment #467608 - Attachment is patch: true
Attachment #467608 - Attachment mime type: application/octet-stream → text/plain
Attached patch interdiff (obsolete) — Splinter Review
Comment on attachment 467608 [details] [diff] [review]
Updated patch

changes look fine, r=me. if you think the search service usage needs further review from gavin, we can get post-facto review when he's back from vacation.
Attachment #467608 - Flags: review?(dietrich) → review+
checkin-needed.  Be aware of the new files.
Keywords: checkin-needed
I'd really prefer to get the span magic done the opposite way around for

#brandStart > span {
  opacity: 0.4;
}

Couldn't we do 

#brandStart {
  opacity: 0.4;
}

#brandStart > span {
  opacity: 1;
}

instead?
PS: keyword and whiteboard status look pretty inconsistent, what's the right status here?
(In reply to comment #78)
> I'd really prefer to get the span magic done the opposite way around for
> 
> #brandStart > span {
>   opacity: 0.4;
> }
> 
> Couldn't we do 
> 
> #brandStart {
>   opacity: 0.4;
> }
> 
> #brandStart > span {
>   opacity: 1;
> }
> 
> instead?

I can't be accertive on this, but I believe #brandStart { opacity: 0.4 will mean that #brandStart > span { opacity: 1 will have the same result as if it were #brandStart { opacity: 1 and #brandStart > span { opacity: 0.4.

For #brandStart > span, of course.
Whiteboard: [needs a reviewer][waiting feedback on trybuild and search params]
Comment on attachment 467608 [details] [diff] [review]
Updated patch

>+            <span id="searchEngineLinks">
>+              <a hidden="true" id="searchEngineAdvancedLink">&abouthome.searchEgineLinks.advanced;</a>
>+              <a hidden="true" id="searchEngineAdvancedPreferences">&abouthome.searchEgineLinks.preferences;</a>
>+            </span>

s/searchEgineLinks/searchEngineLinks/

>+<!ENTITY abouthome.searchEgineLinks.advanced "Advanced Search">
>+<!ENTITY abouthome.searchEgineLinks.preferences "Preferences">

Ditto.
Comment on attachment 467608 [details] [diff] [review]
Updated patch

>diff -r 55ef0e0529bc browser/base/content/aboutHome.js

>+const SEARCH_ENGINES = {
>+  "Google": {

>+  , params: "source=hp"
>+  , links: {
>+      advanced: "http://www.google.com/advanced_search"
>+    , preferences: "http://www.google.com/preferences"
>+    }
>+  }

The search service should be able to handle keeping track of these extra params/URLs once bug 587780 is fixed - we should get a followup filed to clean this up.

>+function parseSearchEngineUrl(aUrl, aTerms, aAdditionalParams) {

Seems like this would be clearer if it just used gSearchEngine.searchUrl and .params directly rather than having them be passed in. (can always refactor if we have multiple engines).

>+  const SEARCH_TOKENS = {
>+    "{searchTerms}": typeof(aTerms) == "string" ? encodeURIComponent(aTerms) : ""
>+  }

encodeURIComponent(aTerms || "")?

>diff -r 55ef0e0529bc browser/components/nsBrowserGlue.js

>+    AboutHomeUtils.loadDefaultSearchEngine();

This will result in the search service being initialized earlier than before - need to keep an eye out for possible Ts effects.

>+let AboutHomeUtils = {

>+  loadDefaultSearchEngine: function AHU_loadDefaultSearchEngine()
>+  {
>+    let defaultEngine = Services.search.defaultEngine;

This will need to use the property being added in bug 587691.

>+    let searchurl = defaultEngine.getSubmission("placeholder").uri.spec;

Can't you just use {searchTerms} here directly, rather than manually replacing "placeholder" later?

You should assert that getSubmission().postData is null, because the code in aboutHome.js clearly can't handle POST engines, and it's possible (though unlikely) that someday a default engine uses POST.

>+    let engine = {

>+    , description: defaultEngine.description

Unused, no point in setting it (we also don't expose it anywhere atm, so the values may not necessarily be useful).

>+    , iconUrl: defaultEngine.iconURI.spec

iconURI can be null. But this property seems to be unused? Can't this just instead set "image" so that we don't need to hardcode images in aboutHome.js?

>+  fetchAndSetItem: function AHU_fetchAndSetItem(aURI, aName, aCallback, aThisObject)

What's this for? Looks to be unused.

>diff -r 55ef0e0529bc browser/themes/gnomestripe/browser/jar.mn
>diff -r 55ef0e0529bc browser/themes/pinstripe/browser/jar.mn
>diff -r 55ef0e0529bc browser/themes/winstripe/browser/jar.mn

Are there any platform styling differences? Do we actually want this to be modifiable by themes?
(In reply to comment #83)
> >+function parseSearchEngineUrl(aUrl, aTerms, aAdditionalParams) {
> 
> Seems like this would be clearer if it just used gSearchEngine.searchUrl and
> .params directly

Or even just put this in onSearchSubmit directly, rather than in a separate functino.
(In reply to comment #83)
> >+    let searchurl = defaultEngine.getSubmission("placeholder").uri.spec;
> 
> Can't you just use {searchTerms} here directly, rather than manually replacing
> "placeholder" later?

It should work, I was worried that would have been a bit confusing due to it going to replace {searchTerms} with {searchTerms}, but it's fine.

> >+    , description: defaultEngine.description
> 
> Unused, no point in setting it (we also don't expose it anywhere atm, so the
> values may not necessarily be useful).

yep, I exposed it initially for future use, but we actually don't use it, removable.

> >+    , iconUrl: defaultEngine.iconURI.spec
> 
> iconURI can be null. But this property seems to be unused? Can't this just
> instead set "image" so that we don't need to hardcode images in aboutHome.js?

"image" has an higher res than this icon, it's a full logo rather then a icon, I exposed it because I did not know if we were willing to add the icon to the input field rather than the logo above it. As you said it's currently unused and can be removed and added back in future if needed.

> >+  fetchAndSetItem: function AHU_fetchAndSetItem(aURI, aName, aCallback, aThisObject)
> 
> What's this for? Looks to be unused.

hm, I probably left this in, I used it to read a locally hosted json for the cached Mozilla snippets, should have been moved to the other patch.

> >diff -r 55ef0e0529bc browser/themes/gnomestripe/browser/jar.mn
> >diff -r 55ef0e0529bc browser/themes/pinstripe/browser/jar.mn
> >diff -r 55ef0e0529bc browser/themes/winstripe/browser/jar.mn
> 
> Are there any platform styling differences? Do we actually want this to be
> modifiable by themes?

No differences for now, but since we allow to style other about pages, I suspect we want to allow the same here.
Whiteboard: [needs updated patch]
Duplicate of this bug: 589323
(In reply to comment #85)
> No differences for now, but since we allow to style other about pages, I
> suspect we want to allow the same here.

Well, other about: pages have different purposes. Since the current page isn't modifiable by themes, and to avoid the duplication, I think we should probably just leave the styling in content/ for now. We can always break it out into /themes later.
(In reply to comment #87)
> Well, other about: pages have different purposes. Since the current page isn't
> modifiable by themes, and to avoid the duplication, I think we should probably
> just leave the styling in content/ for now. We can always break it out into
> /themes later.

Thinking to bug 518989, we originally thought nobody wanted to style about:support, then we had to split it out pretty soon, and your comment in that bug sounds like going the direction of "we are not too worried about themes breaking the page". Avoiding duplication is a good point, but having to do the same stuff twice is not funny. Btw, just wanted to point out we already hit a similar discussion.
Marco, do you still need my help updating this patch, or is it yours again?

(And, if you need prompt review for a final revision, please ping over irc(.
(In reply to comment #89)
> Marco, do you still need my help updating this patch, or is it yours again?

I can bring it on, thanks for review and moving it along. I'll nag you for further reviews soon.
(In reply to comment #88)
> Thinking to bug 518989, we originally thought nobody wanted to style
> about:support, then we had to split it out pretty soon, and your comment in
> that bug sounds like going the direction of "we are not too worried about
> themes breaking the page".

I was ambivalent in that bug. Given the differences here (introduces style duplication, current home page is not theme-able, etc.), I still think it would be better to leave the styles in content/ for now.
Blocks: 590068
Attached patch patch v1.8 (obsolete) — Splinter Review
I should have fixed all remaining comments, patch is much smaller due to css compression to a single file.
Attachment #467608 - Attachment is obsolete: true
Attachment #467716 - Attachment is obsolete: true
Attachment #468690 - Flags: review?(mano)
Whiteboard: [needs updated patch] → [needs review]
Comment on attachment 468690 [details] [diff] [review]
patch v1.8

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

>+const SEARCH_ENGINES = {
>+  "Google": {
>+    image: "data:image/png,%89..."

>+, "Яндекс":
>+  {
>+    image: "data:image/png,%89..."

These are shorter if you base64 encode them (at the cost of a bit extra processing overhead, I suppose...). Not sure what the better tradeoff is, probably doesn't matter :)

http://pastebin.mozilla.org/774980

>+function onSearchSubmit(aEvent) {

>+  if (gSearchEngine && searchTerms.length > 0) {
>+    const SEARCH_TOKENS = {
>+      "_searchTerms_": encodeURIComponent(searchTerms || "")

The searchTerms.length check makes the || "" fallback here redundant, actually.

>+    if (gSearchEngine.params)
>+      url += "&" + gSearchEngine.params;

This is a bit fragile, since it's possible for the engine's URL to end with "&", or it may not use a query string at all (e.g. http://answers.com/{searchTerms} ). But I guess we only do this for the hardcoded engines in SEARCH_ENGINES, so not really a problem in practice. Maybe add a comment next to SEARCH_ENGINES definition that if you add a "params" property, you need to make sure the engine handles it correctly? This parameter addition could also move to setupSearchEngine().

>diff --git a/browser/branding/nightly/content/jar.mn b/browser/branding/nightly/content/jar.mn
>diff --git a/browser/branding/unofficial/content/jar.mn b/browser/branding/unofficial/content/jar.mn
>diff --git a/other-licenses/branding/firefox/content/jar.mn b/other-licenses/branding/firefox/content/jar.mn

>   content/branding/aboutFooter.png               (aboutFooter.png)
>   content/branding/icon48.png                    (icon48.png)
>   content/branding/icon64.png                    (icon64.png)
>+  content/browser/icon128.png                    (../mozicon128.png)
>+  content/browser/icon16.png                     (../default16.png)

Shouldn't these be in branding/ like the others?

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+let AboutHomeUtils = {

>+  setItem: function AHU_setItem(aName, aValue, aCallback, aThisObject)

aCallback, aThisObject, and _callback are now unused.

r=me with those addressed.
Attachment #468690 - Flags: review+
(In reply to comment #93)

> >diff --git a/other-licenses/branding/firefox/content/jar.mn b/other-licenses/branding/firefox/content/jar.mn
> 
> >   content/branding/aboutFooter.png               (aboutFooter.png)
> >   content/branding/icon48.png                    (icon48.png)
> >   content/branding/icon64.png                    (icon64.png)
> >+  content/browser/icon128.png                    (../mozicon128.png)
> >+  content/browser/icon16.png                     (../default16.png)
> 
> Shouldn't these be in branding/ like the others?

iirc, when I initially tried to use them from branding I was getting security errors

btw, there is a more compelling problem, I see a nsStringBuffer leak on tryserver, but DOMStorage is a cycle collection partecipant.
also, only debug mochitests are leaking
(In reply to comment #94)
> iirc, when I initially tried to use them from branding I was getting security
> errors

You could fix that easily enough by adding a contentaccessible=yes to the branding jar.mns, I think.

(Having the branding/ manifest put stuff under a directory defined by the content/ manifest is strange, and maybe even fragile - not sure how that works exactly.)
(In reply to comment #96)
> You could fix that easily enough by adding a contentaccessible=yes to the
> branding jar.mns, I think.

ah, that works, thanks!
so, 2 remaining issues:
- initing Search service there causes a nsStringBuffer leak in mochitests, the leak is reproduceable running mochitest plain tests in js/ folder (but also with other mochitests, 2,4,5 are all leaking). This comes out when calling _buildCache, but no idea what is causing it.
- I'm seeing a Ts regression on tryserver and it's not ignorable (30% on Win). We are doing 2 things at startup with this: initing search service and initing domstorage. The default search engine should not change often, so we could update the storage value just once a week or so.
Attachment #468690 - Flags: review?(mano)
this stuff in search service is creating some sort of objects cycle

    function collapseMozParams(aParam)
      this.mozparams[aParam.name] || aParam;
    json.params = this.params.map(collapseMozParams, this);

if i clone this.mozparams[aParam.name] object with a really stupid JSON.parse(JSON.stringify(this.mozparams[aParam.name])) we don't leak.
I tried to make a balanced tree but nsStringBuffer is so much used that Perl goes out of memory during log analysis.
this is quite crazy, if I just change the above code to

    "tag"; // nonsense no-op
    function collapseMozParams(aParam)
      this.mozparams[aParam.name] || aParam;

Using any other string value leaks, using "tag" does not leak, params has a "tag" property that comes from amazon engine. This makes me think we are maybe leaking a "tag" string.
Any idea is appreciated.
and just changing the name of the param in amazondotcom.xml from "tag" to "tags" solves the leak as well. there is a name conflict somewhere.
(In reply to comment #98)
> The default search engine should not change often, so we could
> update the storage value just once a week or so.

It should change never, actually (for a given build). So we should probably only do this once per version - maybe in nsBrowserContentHandler's get defaultArgs(), under "case OVERRIDE_NEW_MSTONE:"? I think that may actually run later than _onProfileStartup (but still before the first window load).
Attached patch patch v2.0 (obsolete) — Splinter Review
This implements various suggestions from Gavin, both from previous comments and from IRC. It should solve all issues including the leak and the Ts hit. But just in case I'm going to double check on try before asking final review on latest changes.
Attachment #468690 - Attachment is obsolete: true
Whiteboard: [needs review] → [needs try run and partial review on latest changes]
Attached patch patch v2.1 (obsolete) — Splinter Review
This did not show any mochitest leak nor Ts regressions on tryserver. the only glitch was a b-c leak but it's easy explainable (and fixed here) due to the fact the object was global and browserContentHandler does not have a way to shutdown (we don't need it atm too)
Attachment #469143 - Attachment is obsolete: true
Whiteboard: [needs try run and partial review on latest changes] → [needs partial review on latest changes]
Attachment #469287 - Flags: review?(gavin.sharp)
Comment on attachment 469287 [details] [diff] [review]
patch v2.1

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

>+// If a definition requires additional params, check that the final search url
>+// is handled corectly by the engine.

"correctly"

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

>+ *  OVERRIDE_NEW_BUILD if this is the first run with a new build of the same
>+ *                     Gecko milestone (i.e. right after a nightly upgrade).

Can you use "build ID" everywhere that this currently uses "build", for clarity? I.e. OVERRIDE_NEW_BUILD_ID, pref/variable be "buildID", and change this comment to something like:

"if this is the first run with a new build ID (e.g. right after a nightly upgrade)"

> function needHomepageOverride(prefb) {

>+  if (savedbuild && build != savedbuild) {

Why null check? If someone resets the pref manually (and leaves the mstone pref set) we should run this code.

>+let AboutHomeUtils = {

>+    this.setItem("search-engine", JSON.stringify(engine));
>+  },
>+
>+  setItem: function AHU_setItem(aName, aValue)
>+  {
>+    if (typeof(aName) != "string" || aName.length == 0 ||
>+        typeof(aValue) != "string")
>+      throw new Error("setItem arguments should be valid strings.");
>+
>+    this._storage.setItem(aName, aValue);

The argument validation and indirection is kind of unnecessary now that there's only one caller - just call _storage.setItem directly and get rid of the setItem function?
Attachment #469287 - Flags: review?(gavin.sharp) → review+
Attached patch patch v2.2Splinter Review
Fixed Gavin's comments. Thanks everybody for feedback. This can land.
Attachment #469287 - Attachment is obsolete: true
Whiteboard: [needs partial review on latest changes]
Blocks: 590877
http://hg.mozilla.org/mozilla-central/rev/291cea9d9fca
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
uargh sorry, I was checking startup and forgot to remove it :(
Am I the only one who finds quite weird that tooltip on Google's logo ("Search engine logo", abouthome.searchEngineLogo.title)? 

I suppose that's done in order to maintain the possibility to easily switch search engine, but is that tooltip/string really needed?
(In reply to comment #110)
> Am I the only one who finds quite weird that tooltip on Google's logo ("Search
> engine logo", abouthome.searchEngineLogo.title)? 

I think accessibility could appreciate knowing what's that image, but we still need an accessibility post review before final, most likely.
(In reply to comment #111)
> (In reply to comment #110)
> > Am I the only one who finds quite weird that tooltip on Google's logo ("Search
> > engine logo", abouthome.searchEngineLogo.title)? 
> 
> I think accessibility could appreciate knowing what's that image, but we still
> need an accessibility post review before final, most likely.

What you really want there is alt="Google" (not title), so that a screen reader announces approximately the same info that a sighted user reads.

The Minefield logo should have neither alt nor title, as it doesn't add any info, being right next to "Minefield Start".
The problem with the search engine title, if i understand the code correctly, is that it depends on the default search engine set in the browser, and it can well be anything, even something that Firefox doesn't provide by default. The only useful thing to do would be to use the search engine short name, but even then, it might be awkward. Probably less than the generic "Search engine logo", though.
Depends on: 592112
Depends on: 592990
Depends on: 595896
Litmus testcase added:
https://litmus.mozilla.org/show_test.cgi?id=12787
Flags: in-litmus? → in-litmus+
Whiteboard: [in-litmus-bug-week]
Depends on: 598264
Blocks: 612453
Will there be support for search engines other than google and yandex?
Not planned, AFAICT. Any specific reason for asking that?
Whiteboard: [in-litmus-bug-week] → [in-litmus-bug-week][about-home]
It says so here: https://wiki.mozilla.org/Firefox/Projects/Firefox_Start
"control/customization of search engine used" under Goals.
that part ended up being delayed to next versions and needs decisions.
Depends on: 800775
No longer depends on: 800775
You need to log in before you can comment on or make changes to this bug.