563723 - Add an about:home page that mimics current start page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Peter Henkel [:Terepin]]

Isn't this duplicate of this bug?
https://bugzilla.mozilla.org/show_bug.cgi?id=544819

Comment 2

[Marco Bonardo [:mak]]

this is a bit different, Bug 563734 is more similar (same)

Comment 3

[Marco Bonardo [:mak]]

Clarified the title a bit.

Comment 4

[Peter Henkel [:Terepin]]

Does this newly created bugs mean that you are starting working on it? Or someone else?

Comment 5

[Marco Bonardo [:mak]]

We are still in taking-off stage, you'll know once assignee will be set.

Comment 6

[Marco Bonardo [:mak]]

Attachment: current search page, epurated.

This is current search page code/images, epurated from external scripts and whistles and reindented.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 8

[Pascal Chevrel:pascalc]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Mike Hommey [:glandium]]

Wouldn't it just be better to have the snippets locally and update them if necessary with new firefox releases ?

Comment 11

[Pascal Chevrel:pascalc]

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.

Comment 12

[Mike Hommey [:glandium]]

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.

Comment 13

[Marco Bonardo [:mak]]

(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.

Comment 14

[Pascal Chevrel:pascalc]

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.

Comment 15

[Axel Hecht [:Pike]]

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.

Comment 16

[Johnathan Nightingale [:johnath]]

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?

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Mike Hommey [:glandium]]

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.

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Marco Bonardo [:mak]]

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.

Comment 22

[Johnathan Nightingale [:johnath]]

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.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 24

[Marco Bonardo [:mak]]

Anybody interested in the "Mozilla snippets" part should move to bug 563738.

Comment 25

[Reed Loden [:reed]]

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...

Comment 26

[Peter Henkel [:Terepin]]

So, if I understand this correctly, after it lands, Home Tab can be created?

Comment 27

[Marco Bonardo [:mak]]

(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.

Comment 28

[Axel Hecht [:Pike]]

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?

Comment 29

[Marco Bonardo [:mak]]

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.

Comment 30

[Axel Hecht [:Pike]]

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

Comment 31

[Marco Bonardo [:mak]]

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.

Comment 32

[(no longer active)]

Are there try server builds available with this patch?

Comment 33

[Marco Bonardo [:mak]]

Not yet, I will soon update DOM storage patch to something that makes sense, fix first comments I got and produce a build.

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch v1.2

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.

Comment 35

[Marco Bonardo [:mak]]

Attachment: patch 1.3

minor fixes

Comment 36

[Marco Bonardo [:mak]]

first trybuilds available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-90a62915efd1/

notice the page is not linked anywhere, you will have to navigate to it manually

Comment 37

[Marco Bonardo [:mak]]

ps: snippets will appear after 5 minutes of idle

Comment 38

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

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.

Comment 39

[Marco Bonardo [:mak]]

(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).

Comment 40

[Marco Bonardo [:mak]]

oh, and the SS definitions don't allow for additional links (like advanced search, preferences, or whatever engines could ask us to add)

Comment 41

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

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.

Comment 42

[Marco Bonardo [:mak]]

(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.

Comment 43

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

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.

Comment 44

[Marco Bonardo [:mak]]

Attachment: patch v1.4

inject defaultEngine values from search service, remove the localization hacks that are now not needed anymore.

Comment 45

[Marco Bonardo [:mak]]

Attachment: patch v1.5

minor update

Comment 46

[Dão Gottwald [:dao]]

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));.

Comment 47

[Dão Gottwald [:dao]]

Is AboutHomeUtils.jsm going to be used in different places? If not, I'd say it shouldn't be a module.

Comment 48

[Marco Bonardo [:mak]]

(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.

Comment 49

[Axel Hecht [:Pike]]

Is the jsm bringing perf wins, too?

Comment 50

[Dão Gottwald [:dao]]

Only if you use it multiple times, it's just overhead otherwise.

Comment 51

[Marco Bonardo [:mak]]

(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.

Comment 52

[Dão Gottwald [:dao]]

(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.

Comment 53

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

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

Comment 54

[d]

(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?

Comment 55

[Marco Bonardo [:mak]]

(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.

Comment 56

[Marco Bonardo [:mak]]

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, ...).

Comment 57

[Johnathan Nightingale [:johnath]]

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.

Comment 58

[Marco Bonardo [:mak]]

Attachment: patch v1.6

I'll make a trybuild out of this.

Comment 59

[Marco Bonardo [:mak]]

for anyone interested in lookingat this simple page http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b3991367e9d0/
the page is not linked yet, so you have to manually navigate to about:home

Comment 60

[Justin Dolske [:Dolske]]

(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

Comment 61

[Axel Hecht [:Pike]]

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).

Comment 62

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

Screenshot from the build: http://grab.by/5Q9i

(Marco: does the patch also have the Firefox branding for when --official-branding is enabled?)

Comment 63

[Marco Bonardo [:mak]]

(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 64

[Axel Hecht [:Pike]]

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?

Comment 65

[Marco Bonardo [:mak]]

(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

Comment 66

[Mike Hommey [:glandium]]

There is one concern regarding licensing: the google and yandex logos are certainly not tri-licensed, let alone free (as in free speech).

Comment 67

[Marco Bonardo [:mak]]

(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 68

[Marco Bonardo [:mak]]

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.

Comment 69

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

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.

Comment 70

[Asaf Romano (gone)]

Started reviewing.  Review to be posted early tomorrow.

Comment 71

[Asaf Romano (gone)]

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.

Comment 72

[Asaf Romano (gone)]

1. event.preventDefault - figured later why it's there.
2. -moz-locale-dir doesn't work in non-xul documents :-/

Comment 73

[Asaf Romano (gone)]

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.

Comment 74

[Asaf Romano (gone)]

Attachment: Updated patch

Conservatively updated patch.

Comment 75

[Dietrich Ayala (:dietrich)]

Attachment: interdiff

Comment 76

[Dietrich Ayala (:dietrich)]

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.

Comment 77

[Asaf Romano (gone)]

checkin-needed.  Be aware of the new files.

Comment 78

[Axel Hecht [:Pike]]

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?

Comment 79

[Axel Hecht [:Pike]]

PS: keyword and whiteboard status look pretty inconsistent, what's the right status here?

Comment 80

[Tiago Sá]

(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.

Comment 81

[Asaf Romano (gone)]

Sure, will do.

Comment 82

[Reed Loden [:reed]]

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 83

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

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?

Comment 84

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

(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.

Comment 85

[Marco Bonardo [:mak]]

(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.

Comment 87

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

(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.

Comment 88

[Marco Bonardo [:mak]]

(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.

Comment 89

[Asaf Romano (gone)]

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(.

Comment 90

[Marco Bonardo [:mak]]

(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.

Comment 91

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

(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.

Comment 92

[Marco Bonardo [:mak]]

Attachment: patch v1.8

I should have fixed all remaining comments, patch is much smaller due to css compression to a single file.

Comment 93

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

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.

Comment 94

[Marco Bonardo [:mak]]

(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.

Comment 95

[Marco Bonardo [:mak]]

also, only debug mochitests are leaking

Comment 96

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

(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.)

Comment 97

[Marco Bonardo [:mak]]

(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!

Comment 98

[Marco Bonardo [:mak]]

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.

Comment 99

[Marco Bonardo [:mak]]

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.

Comment 100

[Marco Bonardo [:mak]]

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.

Comment 101

[Marco Bonardo [:mak]]

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.

Comment 102

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

(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).

Comment 103

[Marco Bonardo [:mak]]

Attachment: patch v2.0

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.

Comment 104

[Marco Bonardo [:mak]]

Attachment: patch v2.1

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)

Comment 105

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

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?

Comment 106

[Marco Bonardo [:mak]]

Attachment: patch v2.2

Fixed Gavin's comments. Thanks everybody for feedback. This can land.

Comment 107

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/291cea9d9fca

Comment 108

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

https://hg.mozilla.org/mozilla-central/rev/fe4057f8386d - noticed this but forgot to mention it!

Comment 109

[Marco Bonardo [:mak]]

uargh sorry, I was checking startup and forgot to remove it :(

Comment 110

[Francesco Lodolo [:flod]]

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?

Comment 111

[Marco Bonardo [:mak]]

(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.

Comment 112

[Dão Gottwald [:dao]]

(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".

Comment 113

[Mike Hommey [:glandium]]

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.

Comment 114

[u279076]

Litmus testcase added:
https://litmus.mozilla.org/show_test.cgi?id=12787

Comment 115

[u310142]

Will there be support for search engines other than google and yandex?

Comment 116

[Axel Hecht [:Pike]]

Not planned, AFAICT. Any specific reason for asking that?

Comment 117

[u310142]

It says so here: https://wiki.mozilla.org/Firefox/Projects/Firefox_Start
"control/customization of search engine used" under Goals.

Comment 118

[Marco Bonardo [:mak]]

that part ended up being delayed to next versions and needs decisions.