Last Comment Bug 711157 - about:home Visual Refresh & Launcher
: about:home Visual Refresh & Launcher
Status: RESOLVED FIXED
[about-home][qa-]
: user-doc-needed
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 14 Branch
: All All
: -- normal with 2 votes (vote)
: Firefox 14
Assigned To: Frank Yan (:fryn)
:
:
Mentors:
https://people.mozilla.com/~shorlande...
Depends on: 731350 739725 733853 735135 735879 736279 736444 736845 736846 740235 740581 746466 750056
Blocks: 658512 731546 733304
  Show dependency treegraph
 
Reported: 2011-12-15 10:32 PST by Chris Lee [:clee]
Modified: 2013-12-27 14:26 PST (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v. 1 (53.74 KB, patch)
2012-02-09 16:30 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
screenshot (235.10 KB, image/png)
2012-02-09 16:31 PST, Frank Yan (:fryn)
no flags Details
screenshot w/ session restore button (277.74 KB, image/png)
2012-02-09 16:32 PST, Frank Yan (:fryn)
no flags Details
patch v.2 (53.80 KB, patch)
2012-02-09 16:56 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch (56.98 KB, patch)
2012-02-09 17:43 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v2 (50.83 KB, patch)
2012-02-16 15:08 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v2 w/ incremented snippet ver. (48.94 KB, patch)
2012-02-16 16:14 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v3 (62.60 KB, patch)
2012-02-27 18:23 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
screenshot of patch v3 (280.52 KB, image/png)
2012-02-27 18:24 PST, Frank Yan (:fryn)
no flags Details
patch v4 (62.98 KB, patch)
2012-02-27 19:34 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
screenshot of patch v4 (304.51 KB, image/png)
2012-02-27 19:39 PST, Frank Yan (:fryn)
no flags Details
patch v4.1 (62.85 KB, patch)
2012-02-27 19:47 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v5 (66.26 KB, patch)
2012-02-28 02:10 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
screenshot of patch v5 (309.55 KB, image/png)
2012-02-28 02:12 PST, Frank Yan (:fryn)
no flags Details
patch v5.1 (79.78 KB, patch)
2012-02-28 03:37 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v5.2 (79.78 KB, patch)
2012-02-28 03:42 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v6 (80.57 KB, patch)
2012-03-01 17:49 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
screenshot of patch v6 w/ wide viewport (301.49 KB, image/png)
2012-03-01 17:50 PST, Frank Yan (:fryn)
shorlander: ui‑review+
Details
screenshot of patch v6 w/ narrow viewport (297.06 KB, image/png)
2012-03-01 17:50 PST, Frank Yan (:fryn)
shorlander: ui‑review+
Details
patch v6.1 (83.32 KB, patch)
2012-03-01 18:27 PST, Frank Yan (:fryn)
mak77: review+
Details | Diff | Splinter Review
screenshot of patch v6.1 w/ focus (297.85 KB, image/png)
2012-03-01 18:29 PST, Frank Yan (:fryn)
shorlander: ui‑review+
Details
coalesced patch for aurora (bugs 733651, 711157, 736279) [r=mak] [ui-r=shorlander] (87.18 KB, patch)
2012-03-15 16:08 PDT, Frank Yan (:fryn)
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Lee [:clee] 2011-12-15 10:32:35 PST
The work for Home Tab part 1 is to move the design of the about:home to an updated style by adding launchers to the most common areas in Firefox.  

The details/requirements are being tracked on the feature page -- https://wiki.mozilla.org/Features/Desktop/Firefox_Home_Tab

The changes here will primarily (but minimally) impact those users who have not setup a custom home page.
Comment 1 Justin Dolske [:Dolske] 2011-12-25 01:03:42 PST
Interesting proposal from a Reddit user here:

http://saming.free.fr/fhome/en/

(via http://www.reddit.com/r/firefox/comments/nphup/what_do_you_think_about_my_firefox_homepage/)

Worth considering if we want to be more bold with the redesign? (This can happen in a separate bug, doesn't need to block adding launchers if that's all we want to do here.)
Comment 2 account 2011-12-25 04:09:39 PST
Thanks Justin, I'm also trying to do a design from here:

http://people.mozilla.com/~jboriss/specs/home_tab_first_iteration_spec1.png

The concept of quick icons is simple to make, however they are not at my disposal, so I don't know where I could use them. Here:

http://saming.free.fr/fhome/test/
Comment 3 Sean Newman 2012-01-13 10:27:14 PST
How about changing the contents of the home tab to be more dynamic and user-interactive? I'd like to be able to add multiple search engines to that page, I want to be able to change them (I hate that google is a hardcoded search engine on that page for en-us localization).
I also think that this page should have some kind of sockets, so users could drag'n'drop some urls (maybe from bookmarks) to these sockets to occupy them.
It would be very useful for users who have about:home set as the home page, which gets opened in every new tab (for NewTabURL/NewTabHomepage extensions users).
Comment 4 Chris Lee [:clee] 2012-01-13 13:37:34 PST
(In reply to Sean Newman from comment #3)
> How about changing the contents of the home tab to be more dynamic and
> user-interactive? I'd like to be able to add multiple search engines to that
> page, I want to be able to change them (I hate that google is a hardcoded
> search engine on that page for en-us localization).
> I also think that this page should have some kind of sockets, so users could
> drag'n'drop some urls (maybe from bookmarks) to these sockets to occupy them.
> It would be very useful for users who have about:home set as the home page,
> which gets opened in every new tab (for NewTabURL/NewTabHomepage extensions
> users).

Conceptually, this is the direction we are moving to for Phase 2.  The bug here is to track Phase 1 work which is outlined in the spec in the feature page above.  

I will open a Phase 2 bug to track this work very shortly.  Thanks!
Comment 5 Siddhartha Dugar [:sdrocking] 2012-02-02 04:30:22 PST
In my opinion Home Tab should integrate the ideas of about:home and the new New Tab page. We can have a search box, a session restore button, thumbnails for top sites and buttons for Bookmarks, History, etc. The last part can wait till we have in-content pages for Library.
Comment 6 Frank Yan (:fryn) 2012-02-09 16:30:23 PST
Created attachment 595904 [details] [diff] [review]
patch v. 1

Implementation done.

Jennifer, we need new icons, please. :)
Comment 7 Frank Yan (:fryn) 2012-02-09 16:31:08 PST
Created attachment 595905 [details]
screenshot

Here's a screenshot of about:home with the patch.
Comment 8 Frank Yan (:fryn) 2012-02-09 16:32:43 PST
Created attachment 595906 [details]
screenshot w/ session restore button

Here's a screenshot with the session restore button.
Comment 9 Frank Yan (:fryn) 2012-02-09 16:56:40 PST
Created attachment 595915 [details] [diff] [review]
patch v.2

small fix for RTL mode.
Comment 10 Frank Yan (:fryn) 2012-02-09 17:43:54 PST
Created attachment 595924 [details] [diff] [review]
patch

Fixed typo & comments.
Comment 11 Marco Bonardo [::mak] 2012-02-13 14:50:41 PST
Comment on attachment 595924 [details] [diff] [review]
patch

Review of attachment 595924 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I have quite some comments on this, but as a general feedback it looks great!

The main thing is, this changes lots of the dom structure of the page, some styles and especially the button-like style from the snippets.
That may be fine, depending on UX review, though I think marketing remote snippets rely on that styling and use the activateSnippetsButtonClick() function too.
So this change should be really planned with care involving marketing staff that manages snippets on the server, we may have to rev the snippets version in the requesting url and have the servers send a different versions based on that version.
This should be clarified and planned before we actually review and land this.

Are the new icons correctly optimized?
I think may be worth to make a single launchers.png and use -moz-image-region rather than spreading these many icons in base/content, that folder is already quite crowded.

I wonder if we should just simplify the thing and make Pair a device a button like the other ones, maybe just a Sync button that brings to Sync preferences?

I couldn't test this deeply regarding the fluid layout cause I keep hitting an assertion on this page, not sure why :(
###!!! ASSERTION: We should have a margin here! (out of memory?): 'hasMargin', file c:/mozilla/mozilla-inbound/layout/generic/nsFrame.cpp, line 835
Did you hit this while testing? Though from the few experiments I could do dismissing the assertion dialog, it was looking fine.

::: browser/base/content/aboutHome.css
@@ +41,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  %endif
>  
>  html {
> +  font-family: Segoe, "Lucida Grande", sans-serif;

what is this choice based on?
I don't see other places in the UI where we use Segoe, we use Segoe UI somewhere, the in-content UI uses message-box. newTab is just sans-serif, as this was. I suppose there should be some consistency across the pages we show to the user.

@@ +70,1 @@
>  }

why do you force a non-native color?

@@ +88,1 @@
>  }

we should ensure this fixed size works fine with current remote snippets, as well.
If we remove the button border is there anything that prevents them from growing horizontally?

@@ +95,1 @@
>  }

wants a comment on what this solves

@@ +102,5 @@
>  
> +@media all and (max-width: 448px) {
> +  #searchForm {
> +    width: 98%;
> +  }

this rule is repeated twice

@@ +110,5 @@
> +  -moz-box-flex: 1;
> +  padding: 6px 8px;
> +  border-radius: 3px 0 0 3px;
> +  border: 1px solid #c1c6cd;
> +  background-image: -moz-linear-gradient(rgba(98,136,185,.15), rgba(98,136,185,0));

are these values constistent with any other search field in the product? if so wants a comment to keep them in sync.

@@ +122,5 @@
>  }
>  
> +#searchText:-moz-placeholder {
> +  font-style: italic;
> +  color: #bbb;

does not placeholder text already have a native color?

@@ +269,5 @@
>  
> +.launchButton {
> +  margin: 25px 4px;
> +  border: 1px solid transparent;
> +  border-radius: 5px;

we should probably try to keep radius more consistent with the platforms, I know this is not per platform, though we are going towards sharper edges.
So first of all we should use the same radius everywhere in this page and that may probably be 3px, that is the most common one in the product (well aero uses 3.5 and gnome sometimes 2.5, though should not be much noticeable)

::: browser/base/content/aboutHome.js
@@ +41,5 @@
>  // If a definition requires additional params, check that the final search url
>  // is handled correctly by the engine.
>  const SEARCH_ENGINES = {
>    "Google": {
> +    params: "source=hp&channel=np"

should figure out if there's any problem gettind rid of the logos with kev. there should not be though better being on the safe side.

::: browser/base/content/browser.js
@@ +2765,5 @@
>    if (/^about:home$/i.test(document.documentURI)) {
>      let ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
>               getService(Components.interfaces.nsISessionStore);
>      if (!ss.canRestoreLastSession)
> +      document.getElementById("bottomSection").className = "";

why a class rather than an an attribute?

@@ +2785,4 @@
>        return;
>  
>      var ot = event.originalTarget;
>      var errorDoc = ot.ownerDocument;

errorDoc naming here is really misleading... sounds like a good time to find a better name, even just ownerDoc would be better

@@ +2923,5 @@
> +      else if (ot == errorDoc.getElementById("addons")) {
> +        BrowserOpenAddonsMgr();
> +      }
> +      else if (ot == errorDoc.getElementById("apps")) {
> +        openLinkIn("https://apps.mozillalabs.com/appdir/", "tab", {inBackground: false});

"inBackground: false" is the default, there should be no reason to pass it

::: browser/base/jar.mn
@@ +23,1 @@
>          content/browser/aboutHome-snippet1.png        (content/aboutHome-snippet1.png)

IIRC aboutHome-restore-icon-small.png existed cause the rescaled version of aboutHome-restore-icon.png was looking blurry, what did change about that? Do we have a better rescaling algorithm or you measured the difference and it's unnoticeable? Did you just make the image smaller?

::: browser/locales/en-US/chrome/browser/aboutHome.dtd
@@ +5,5 @@
>  <!-- These strings are used in the about:home page -->
>  
>  <!ENTITY abouthome.pageTitle "&brandFullName; Start Page">
>  
> +<!ENTITY abouthome.searchText.placeholder "Search">

Search sounds a bit generic (web, history, bookmarks, other?), maybe Search the Web? Even if based on the screenshot I assume you wanted to make it like "search [Engine]" where the latter part is the button. In such a case you need a localization note specifying this is used as a verb, not a noun, since it's completely unclear and using a noun would break this association.
I also wonder if just saying "search" would help someone using a screen reader, you may get some feedback from MarcoZ if just "search" may be fine.
Comment 12 Frank Yan (:fryn) 2012-02-13 17:09:30 PST
(In reply to Marco Bonardo [:mak] from comment #11)
> The main thing is, this changes lots of the dom structure of the page, some
> styles and especially the button-like style from the snippets.
> That may be fine, depending on UX review, though I think marketing remote
> snippets rely on that styling and use the activateSnippetsButtonClick()
> function too.

I don't think the snippet has to look like a massive button for a user to be convinced to click the link. IIRC, Jennifer has talked to marketing regarding these about:home changes.

> So this change should be really planned with care involving marketing staff
> that manages snippets on the server, we may have to rev the snippets version
> in the requesting url and have the servers send a different versions based
> on that version.
> This should be clarified and planned before we actually review and land this.

I could leave the activateSnippetsButtonClick() in there, but it seems really risky that we execute JS in the snippets' HTML.

> Are the new icons correctly optimized?
> I think may be worth to make a single launchers.png and use
> -moz-image-region rather than spreading these many icons in base/content,
> that folder is already quite crowded.

I agree. I'll do that when we get the finished icons. Even with an r+, I'm not going to land this until we have those.

> I wonder if we should just simplify the thing and make Pair a device a
> button like the other ones, maybe just a Sync button that brings to Sync
> preferences?

Actually, we want to remove that sync link altogether. Adding a Sync button might make sense though. Jennifer?

> I couldn't test this deeply regarding the fluid layout cause I keep hitting
> an assertion on this page, not sure why :(
> ###!!! ASSERTION: We should have a margin here! (out of memory?):
> 'hasMargin', file c:/mozilla/mozilla-inbound/layout/generic/nsFrame.cpp,
> line 835
> Did you hit this while testing? Though from the few experiments I could do
> dismissing the assertion dialog, it was looking fine.

I haven't tested it in a debug build.

> > +  font-family: Segoe, "Lucida Grande", sans-serif;
> 
> what is this choice based on?
> I don't see other places in the UI where we use Segoe, we use Segoe UI
> somewhere, the in-content UI uses message-box. newTab is just sans-serif, as
> this was. I suppose there should be some consistency across the pages we
> show to the user.

Looking at the changeset for bug 630777, it does indeed look like I should be using message-box. I'll fix that. Thanks.

> why do you force a non-native color?

Jennifer picked that color for the links. It looks prettier and matches the rest of the page better. Jennifer, is there another reason?

> we should ensure this fixed size works fine with current remote snippets, as
> well.
> If we remove the button border is there anything that prevents them from
> growing horizontally?

The snippet's width and horizontal position should line up with the search box above it. Perhaps, we could do something like: `width: 33%; min-width: 440px;`, so it starts to expand if the window is really wide? Jennifer, what do you think?

> wants a comment on what this solves

It keeps the document from overflowing horizontally unnecessarily, but, yes, I can write that comment.

> @@ +110,5 @@
> > +  -moz-box-flex: 1;
> > +  padding: 6px 8px;
> > +  border-radius: 3px 0 0 3px;
> > +  border: 1px solid #c1c6cd;
> > +  background-image: -moz-linear-gradient(rgba(98,136,185,.15), rgba(98,136,185,0));
> 
> are these values constistent with any other search field in the product? if
> so wants a comment to keep them in sync.

I just followed Jennifer's spec. Jennifer, are there plans for consistency across other pages?

> 
> @@ +122,5 @@
> >  }
> >  
> > +#searchText:-moz-placeholder {
> > +  font-style: italic;
> > +  color: #bbb;
> 
> does not placeholder text already have a native color?

Yes, but it was darker (on white backgrounds) compared to Jennifer's spec. I suppose I could just leave it or apply opacity to it. Jennifer, thoughts?

> > +.launchButton {
> > +  margin: 25px 4px;
> > +  border: 1px solid transparent;
> > +  border-radius: 5px;
> 
> we should probably try to keep radius more consistent with the platforms, I
> know this is not per platform, though we are going towards sharper edges.
> So first of all we should use the same radius everywhere in this page and
> that may probably be 3px, that is the most common one in the product (well
> aero uses 3.5 and gnome sometimes 2.5, though should not be much noticeable)

Yes, this is true. I personally prefer sharper edges. It looks crisper and less toy-like. Jennifer, how does Marco's suggestion sound?

> ::: browser/base/content/aboutHome.js
> @@ +41,5 @@
> >  // If a definition requires additional params, check that the final search url
> >  // is handled correctly by the engine.
> >  const SEARCH_ENGINES = {
> >    "Google": {
> > +    params: "source=hp&channel=np"
> 
> should figure out if there's any problem gettind rid of the logos with kev.
> there should not be though better being on the safe side.

I pinged kev, and we're going to revert the changes to avoid the trouble. Thanks for the catch.

> ::: browser/base/content/browser.js
> >      if (!ss.canRestoreLastSession)
> > +      document.getElementById("bottomSection").className = "";
> 
> why a class rather than an an attribute?

Good point. The document is XHTML, I simply wasn't used to setting arbitrary attributes in HTML. :P

> >      var ot = event.originalTarget;
> >      var errorDoc = ot.ownerDocument;
> 
> errorDoc naming here is really misleading... sounds like a good time to find
> a better name, even just ownerDoc would be better

Okay! :)

> @@ +2923,5 @@
> > +      else if (ot == errorDoc.getElementById("addons")) {
> > +        BrowserOpenAddonsMgr();
> > +      }
> > +      else if (ot == errorDoc.getElementById("apps")) {
> > +        openLinkIn("https://apps.mozillalabs.com/appdir/", "tab", {inBackground: false});
> 
> "inBackground: false" is the default, there should be no reason to pass it

Ah, I see now. https://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#258

> >          content/browser/aboutHome-snippet1.png        (content/aboutHome-snippet1.png)
> 
> IIRC aboutHome-restore-icon-small.png existed cause the rescaled version of
> aboutHome-restore-icon.png was looking blurry, what did change about that?
> Do we have a better rescaling algorithm or you measured the difference and
> it's unnoticeable? Did you just make the image smaller?

I just removed aboutHome-restore-icon.png and then renamed aboutHome-restore-icon-small.png to aboutHome-restore-icon.png, since we aren't going to use the larger icon anymore.

Thanks for the feedback. An updated patch will be coming soon!
Comment 13 Marco Bonardo [::mak] 2012-02-13 17:48:18 PST
(In reply to Frank Yan (:fryn) from comment #12)
> I don't think the snippet has to look like a massive button for a user to be
> convinced to click the link. IIRC, Jennifer has talked to marketing
> regarding these about:home changes.

Sure, my point was not about what is better, rather that we can't change it without properly handling both cases, or we'll end up serving broken contents to older versions.

> I could leave the activateSnippetsButtonClick() in there, but it seems
> really risky that we execute JS in the snippets' HTML.

What do you mean by risky? this runs with content privileges. Surely a wrong script may break the page, but it's something we control and snippets are tested before going live. Having js in snippets was a clear request in the design, and all the remote ones have it.
Btw, no point in keeping the method if you remove the button style, we should rev the snippets version and have the server send different content to newer versions, that doesn't rely on the button styling and presence.
Comment 14 Caspy7 2012-02-15 14:05:15 PST
I don't know how finalized this design is, but if we are going to keep the sync link I'm concerned that many users will not know what the text 'Pair a Device' means exactly.
Can we consider an alternate text or approach?
Comment 15 Frank Yan (:fryn) 2012-02-16 15:08:15 PST
Created attachment 598022 [details] [diff] [review]
patch v2

Updated patch incorporating feedback from mak and shorlander.
The border-radii are now all 2.5px, as recommended by shorlander to match his designs for the rest of the browser UI.

(In reply to Marco Bonardo [:mak] from comment #13)
> Btw, no point in keeping the method if you remove the button style, we
> should rev the snippets version and have the server send different content
> to newer versions, that doesn't rely on the button styling and presence.

How do I rev the snippets version?
Comment 16 Marco Bonardo [::mak] 2012-02-16 15:11:42 PST
(In reply to Frank Yan (:fryn) from comment #15)
> How do I rev the snippets version?

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#894
Comment 17 Frank Yan (:fryn) 2012-02-16 16:14:37 PST
Created attachment 598049 [details] [diff] [review]
patch v2 w/ incremented snippet ver.

Cool, thanks.
Comment 18 Marco Bonardo [::mak] 2012-02-21 03:23:55 PST
Comment on attachment 598049 [details] [diff] [review]
patch v2 w/ incremented snippet ver.

Review of attachment 598049 [details] [diff] [review]:
-----------------------------------------------------------------

not a lot of comments left, mostly synchronizations tasks with UX, marketing, Sync.

I'll just do a final pass once these are finalized, I don't have anything else to point out until then.

::: browser/base/content/aboutHome.css
@@ +42,5 @@
>  %endif
>  
>  html {
> +  font: message-box;
> +  font-size: 1em;

would this hardcoded font size somehow hurt the user's preferences?

@@ +68,4 @@
>  }
>  
> +a {
> +  color: #08c;

this is still non-native on any platform

@@ +109,5 @@
> +  -moz-box-flex: 1;
> +  padding: 6px 8px;
> +  border-radius: 2.5px 0 0 2.5px;
> +  border: 1px solid #c1c6cd;
> +  background-image: -moz-linear-gradient(rgba(98,136,185,.15), rgba(98,136,185,0));

the background still needs to be verified for coherence with the other product pages

@@ +120,5 @@
>  }
>  
>  #searchSubmit {
> +  -moz-margin-start: -4px;
> +  background: -moz-linear-gradient(#efefef, #d8d8d8);

as well as this one

@@ +138,5 @@
>                0 -1px 0 #d7d7d7 inset;
>  }
>  
>  #searchSubmit:active {
> +  background: -moz-linear-gradient(#d8d8d8, #efefef);

and so on :)

@@ +153,2 @@
>    color: rgb(60,60,60);
> +  font-size: .75em;

why this reduction in the snippets text size? is it related to an increase in the global text size?

@@ +210,5 @@
>    -moz-transition-property: background-color, box-shadow;
>    -moz-transition-duration: 0.25s;
>    -moz-transition-timing-function: ease-out;
> +  color: rgb(70,70,70);
> +  font-size: 1.3em;

why is this so big?

@@ +211,5 @@
>    -moz-transition-duration: 0.25s;
>    -moz-transition-timing-function: ease-out;
> +  color: rgb(70,70,70);
> +  font-size: 1.3em;
> +  line-height: 0;

what does this fix? worth a comment

@@ +294,5 @@
> +}
> +
> +#downloads {
> +  background-image: -moz-image-rect(url("chrome://browser/content/aboutHome-launcher.png"),0,160,32,128);
> +}

could you rather define the background-image in .launchButton and just the regions here?

@@ +301,5 @@
> +  position: absolute;
> +  right: 6px;
> +  bottom: 6px;
> +  text-align: right;
> +}

I wonder if these should invert in rtl mode, be on the left side with text-align left.

::: browser/base/content/aboutHome.xhtml
@@ +104,5 @@
>  
> +        <div id="links">
> +          <a class="aboutHomeLink" href="javascript:" id="sync">&abouthome.firefoxSync;</a>
> +          <a class="aboutHomeLink" href="http://www.mozilla.com/about/">&abouthome.aboutMozilla;</a>
> +        </div>

We have to ping someone from the Sync team, or someone involved in adding the pair a device link in the previous implementation, to be sure we are not missing something. That said I think this makes more sense than what we had before.
I'd still provide a launcher rather than a link though, it's a functionality like downloads or history.

::: browser/base/content/test/browser_aboutHome.js
@@ +114,5 @@
>         "A default snippet is visible.");
>  
>      executeSoon(runNextTest);
>    }
>  },

is any of the new functionality in need of some basic testing? for example we may check launchers correctly appear and do something?

::: browser/components/nsBrowserContentHandler.js
@@ +890,5 @@
>    },
>  
>    loadSnippetsURL: function AHU_loadSnippetsURL()
>    {
> +    const STARTPAGE_VERSION = 2;

need to be sure the server side script is already handling versioning correctly (maybe you may ping lorchard and ask to be forwarded to the right person, or file a dependency bug that has to be verified fixed before this can land)

::: browser/locales/en-US/chrome/browser/aboutHome.dtd
@@ +7,5 @@
>  <!ENTITY abouthome.pageTitle "&brandFullName; Start Page">
>  
>  <!ENTITY abouthome.searchEngineButton.label "Search">
>  
> +<!ENTITY abouthome.firefoxSync  "Firefox Sync">

while it's true the page is not perfect from a branding point of view, we should not make it worse. you likely want syncBrand.fullName.label from syncBrand.dtd (note bug 658512, in case you have spare time for it).
Comment 19 Frank Yan (:fryn) 2012-02-27 18:23:43 PST
Created attachment 601146 [details] [diff] [review]
patch v3

(In reply to Marco Bonardo [:mak] from comment #18)
> > +  font-size: 1em;
> 
> would this hardcoded font size somehow hurt the user's preferences?

"em" isn't hardcoding. It will adjust itself based on the zoom level.
I changed all the em values to percentages to be less confusing, but it does the same thing.

> > +a {
> > +  color: #08c;
> 
> this is still non-native on any platform

Changed to -moz-nativehyperlinktext. Thanks for catching this.

> the background still needs to be verified for coherence with the other
> product pages

I've discussed this with shorlander, and we re-designed the whole page to match his designs for the rest of the browser, including in-content UI.

> @@ +153,2 @@
> > +  font-size: .75em;
> 
> why this reduction in the snippets text size? is it related to an increase
> in the global text size?

100% is simply too big. Again, this will scale with the user's preferences and zoom level, accordingly.

> > +  line-height: 0;
> 
> what does this fix? worth a comment

Comment added.

> > +  background-image: -moz-image-rect(url("chrome://browser/content/aboutHome-launcher.png"),0,160,32,128);
> 
> could you rather define the background-image in .launchButton and just the
> regions here?

No, we discussed the differences between -moz-image-rect and -moz-image-region on IRC.
Also, I switched from using background-image to using ::before and content, but -moz-image-rect doesn't work for the content property, so I split up the images again and created a directory for them.

> > +  right: 6px;
> 
> I wonder if these should invert in rtl mode

The Mozilla link's text is now a wordmark, so it doesn't need to be inverted.

> I'd still provide a launcher rather than a link though, it's a functionality
> like downloads or history.

Launcher for Sync added.

> is any of the new functionality in need of some basic testing? for example
> we may check launchers correctly appear and do something?

I don't think this is necessary for now. If and when we fold the home page content into the new tab page, tests should absolutely be written.

> > +    const STARTPAGE_VERSION = 2;
> 
> need to be sure the server side script is already handling versioning
> correctly (maybe you may ping lorchard and ask to be forwarded to the right
> person, or file a dependency bug that has to be verified fixed before this
> can land)

I just emailed Les about this.

> while it's true the page is not perfect from a branding point of view, we
> should not make it worse. you likely want syncBrand.fullName.label from
> syncBrand.dtd (note bug 658512, in case you have spare time for it).

I'd like to get this landed first, since it seems to be top-priority for many people. I'll assign myself to bug 658512 and fix it as a follow-up.
Comment 20 Frank Yan (:fryn) 2012-02-27 18:24:49 PST
Created attachment 601147 [details]
screenshot of patch v3
Comment 21 Frank Yan (:fryn) 2012-02-27 18:31:27 PST
(In reply to Frank Yan (:fryn) from comment #19)
> > while it's true the page is not perfect from a branding point of view, we
> > should not make it worse. you likely want syncBrand.fullName.label from
> > syncBrand.dtd (note bug 658512, in case you have spare time for it).
> 
> I'd like to get this landed first, since it seems to be top-priority for
> many people. I'll assign myself to bug 658512 and fix it as a follow-up.

Er, never mind. I was not thinking straight. I'll fix this and upload a new patch.
Comment 22 Frank Yan (:fryn) 2012-02-27 19:34:25 PST
Created attachment 601157 [details] [diff] [review]
patch v4

Fixed sync branding/localization.
Pushed a few pixels.
Comment 23 Frank Yan (:fryn) 2012-02-27 19:39:07 PST
Created attachment 601158 [details]
screenshot of patch v4

This matches the prototype exactly pixel-for-pixel, except for the restore session button, which is correctly vertically centered with my patch.
https://people.mozilla.com/~shorlander/files/aboutHome-prototype-i02/aboutHome-prototype-i02.html
Comment 24 Frank Yan (:fryn) 2012-02-27 19:47:23 PST
Created attachment 601159 [details] [diff] [review]
patch v4.1

Minor implementation detail. Looks and works the same as v4.
Comment 25 Dão Gottwald [:dao] 2012-02-28 01:55:35 PST
Comment on attachment 601159 [details] [diff] [review]
patch v4.1

> *       content/browser/aboutHome.xhtml               (content/aboutHome.xhtml)
> *       content/browser/aboutHome.js                  (content/aboutHome.js)
> *       content/browser/aboutHome.css                 (content/aboutHome.css)

>+        content/browser/backgroundNoise.png           (content/backgroundNoise.png)

Please move these to abouthome/ as well.
Comment 26 Frank Yan (:fryn) 2012-02-28 02:10:50 PST
Created attachment 601213 [details] [diff] [review]
patch v5

Updated with new history and settings icons from Stephen.

I also added media queries for narrow viewports.
The min/max-width values were tested using the Greek localization, which has the longest words (longer than German and Russian actually).

(In reply to Dão Gottwald [:dao] from comment #25)
> > *       content/browser/aboutHome.xhtml               (content/aboutHome.xhtml)
> > *       content/browser/aboutHome.js                  (content/aboutHome.js)
> > *       content/browser/aboutHome.css                 (content/aboutHome.css)
> 
> >+        content/browser/backgroundNoise.png           (content/backgroundNoise.png)
> 
> Please move these to abouthome/ as well.

I moved aboutHome.(xhtml|css|js) to abouthome/.
backgroundNoise.png will be shared by other pages, e.g. about:newtab, so I didn't move that one.
Comment 27 Frank Yan (:fryn) 2012-02-28 02:12:34 PST
Created attachment 601216 [details]
screenshot of patch v5
Comment 28 Dão Gottwald [:dao] 2012-02-28 02:24:42 PST
(In reply to Frank Yan (:fryn) from comment #26)
> backgroundNoise.png will be shared by other pages, e.g. about:newtab, so I
> didn't move that one.

No, about:newtab styling lives in browser/themes/.
Comment 29 Frank Yan (:fryn) 2012-02-28 03:37:19 PST
Created attachment 601230 [details] [diff] [review]
patch v5.1

(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to Frank Yan (:fryn) from comment #26)
> > backgroundNoise.png will be shared by other pages, e.g. about:newtab, so I
> > didn't move that one.
> 
> No, about:newtab styling lives in browser/themes/.

I'm aware of that.
We'll likely end up having two copies of the same file then, but if we fold about:home into the new tab page, as according to the current plan, it won't be a problem anymore.
Comment 30 Frank Yan (:fryn) 2012-02-28 03:42:57 PST
Created attachment 601233 [details] [diff] [review]
patch v5.2

Forgot to update aboutHome.css
Comment 31 Robert Kaiser 2012-02-28 05:12:41 PST
(In reply to Frank Yan (:fryn) from comment #19)
> Created attachment 601146 [details] [diff] [review]
> patch v3
> 
> (In reply to Marco Bonardo [:mak] from comment #18)
> > > +  font-size: 1em;
> > 
> > would this hardcoded font size somehow hurt the user's preferences?
> 
> "em" isn't hardcoding. It will adjust itself based on the zoom level.
> I changed all the em values to percentages to be less confusing, but it does
> the same thing.

Right. Still, isn't "1em" the same as not specifying it anyhow? IIRC, 1em is bascially defined as "the font-size of the parent or the default font-size if no parent does exist".
Comment 32 Marco Bonardo [::mak] 2012-02-28 12:01:00 PST
Comment on attachment 601233 [details] [diff] [review]
patch v5.2

Review of attachment 601233 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutHome.css
@@ +43,5 @@
>  
>  html {
> +  font: message-box;
> +  font-size: 100%;
> +  background-color: hsl(0,0%,90%);

nit: does it flicker if we keep the original -moz-Field color here? (to mostly have a matching background for -moz-FieldText;) (just figured inContentUI.css uses -moz-dialog and -moz-dialogText, so we could even switch to those if they look more consistent)

@@ +46,5 @@
> +  font-size: 100%;
> +  background-color: hsl(0,0%,90%);
> +  background-image: url(chrome://browser/content/abouthome/noise.png),
> +                    -moz-linear-gradient(hsla(0,0%,100%,.7), hsla(0,0%,100%,.4));
> +  background-attachment: fixed;

It's a pity we can't use chrome://global/skin/inContentUI/background-texture.png as all the other inContentUI, though I tried it and doesn't look really nice :(

@@ +142,3 @@
>    cursor: pointer;
> +  -moz-transition-property: background-color, border-color, box-shadow;
> +  -moz-transition-duration: .15s;

please, may you use milliseconds when the durations are so small? (there are others below)

is this time consistent with the new navbar buttons timing?

@@ +177,5 @@
> +  display: block;
> +  float: left;
> +  margin-top: -6px;
> +  -moz-padding-start: 30px;
> +  -moz-padding-end: 9px;

how does this behave with long text snippets? (3 or more lines) and with short text snippets (1 line)? is it vertically centered in any case?
I tried making 4 lines of text, and it wraps around the icon, that is not the wanted behavior. could you restore the previous one?

@@ +202,1 @@
>  }

where is this snippet class used, and why do we assume it has an icon?
snippets can be anything, from a single line of text to an html5 video or mini-game.

@@ +261,4 @@
>  }
>  
>  #restorePreviousSession {
> +  margin: 0 1px;

what is this margin for? just consistency with the other buttons?

@@ +307,2 @@
>    #restorePreviousSession::before {
> +    display: inline-block; /* display on same line as text label */

going to a new line happens for a brief while (exactly 921 980) and looks jumpy, couldn't we enforce them on the same line for all sizes to make it less jumpy?

::: browser/base/content/browser.js
@@ +2757,5 @@
>  /**
>   * Handle command events bubbling up from error page content
>   */
>  function BrowserOnClick(event) {
> +    if (!event.isTrusted /* don't trust synthetic events */ ||

nit: I prefer if you move the comment after || and make it a line comment (makes easier to comment out pieces of code for testing purposes)
Comment 33 Marco Bonardo [::mak] 2012-02-28 12:10:10 PST
(In reply to Marco Bonardo [:mak] from comment #32)
> @@ +307,2 @@
> >    #restorePreviousSession::before {
> > +    display: inline-block; /* display on same line as text label */
> 
> going to a new line happens for a brief while (exactly 921 980) and looks
> jumpy, couldn't we enforce them on the same line for all sizes to make it
> less jumpy?

Actually I just thought that in some locale "Restore previous session" can be much longer than in English.  So the pixel calculation here could be too optimistic.
Comment 34 Marco Bonardo [::mak] 2012-02-28 12:56:08 PST
just to add notes on things discussed on irc, Add-ons is "Componenti aggiuntivi", it enlarges the button quite a lot, breaking rhythm.
Comment 35 Frank Yan (:fryn) 2012-02-28 13:07:50 PST
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #31)
> (In reply to Frank Yan (:fryn) from comment #19)
> > (In reply to Marco Bonardo [:mak] from comment #18)
> > > > +  font-size: 1em;
> 
> Right. Still, isn't "1em" the same as not specifying it anyhow? IIRC, 1em is
> bascially defined as "the font-size of the parent or the default font-size
> if no parent does exist".

No, it matters, because `font: message-box;` in the previous line actually sets the font-size, so I need to reset it to 100%.

(In reply to Marco Bonardo [:mak] from comment #32)
> (just figured
> inContentUI.css uses -moz-dialog and -moz-dialogText, so we could even
> switch to those if they look more consistent)

This page's design is an improvement over the current in-content UI, so we should switch inContentUI.css in the future instead. Plus, we want the background color to be consistent across platforms.

> > +  background-image: url(chrome://browser/content/abouthome/noise.png),
> 
> It's a pity we can't use
> chrome://global/skin/inContentUI/background-texture.png as all the other
> inContentUI, though I tried it and doesn't look really nice :(

Yeah, noises aren't all created equal. I wish we had a fast, memoizing noise image generating service. :P

> > +  -moz-transition-duration: .15s;
> 
> please, may you use milliseconds when the durations are so small? (there are
> others below)

Will do.

> is this time consistent with the new navbar buttons timing?

The new navbar buttons' transition-duration is 250ms in the current patch, but it should be 150ms, since that's what Stephen has in his mockup.

> > +  display: block;
> > +  float: left;
> > +  margin-top: -6px;
> > +  -moz-padding-start: 30px;
> > +  -moz-padding-end: 9px;
> 
> how does this behave with long text snippets? (3 or more lines) and with
> short text snippets (1 line)? is it vertically centered in any case?
> I tried making 4 lines of text, and it wraps around the icon, that is not
> the wanted behavior. could you restore the previous one?

To be clear, this is only for the default snippets. I'll fix that.

> where is this snippet class used, and why do we assume it has an icon?
> snippets can be anything, from a single line of text to an html5 video or
> mini-game.

This class is used in the current set of snippets. It makes the icon and text line up with the search engine's horizontal rhythm above it. The right way to do this would probably be to find the people who create the snippets and ask them to fix the snippets accordingly for STARTPAGE_VERSION == 2. I'll remove the styling here and coordinate that.

> >  #restorePreviousSession {
> > +  margin: 0 1px;
> what is this margin for? just consistency with the other buttons?

This was just used for a media query that we're going to remove. I'll remove it.
Comment 36 Marco Bonardo [::mak] 2012-02-28 13:18:56 PST
(In reply to Frank Yan (:fryn) from comment #35)
> This class is used in the current set of snippets. It makes the icon and
> text line up with the search engine's horizontal rhythm above it. The right
> way to do this would probably be to find the people who create the snippets
> and ask them to fix the snippets accordingly for STARTPAGE_VERSION == 2.
> I'll remove the styling here and coordinate that.

yes, tha's better, they can provide their own css fwiw.
Comment 37 Stephen Horlander [:shorlander] 2012-02-29 07:41:35 PST
Comment on attachment 601216 [details]
screenshot of patch v5

Clearing this for now until we can work out the tweaks to accommodate scaling and localization.
Comment 38 Jennifer Morrow [:Boriss] (UX) 2012-02-29 14:14:53 PST
Thanks so much for the icon work, Stephen!  Frank, if you need any graphics or additional icons, let me know: I have the Australis styles locally now.
Comment 39 Frank Yan (:fryn) 2012-03-01 17:49:14 PST
Created attachment 602217 [details] [diff] [review]
patch v6

Chris Lee and I are not okay with removing the labels from the buttons, since we don't think that the icons are not recognizable enough on their own, and requiring the user to hover over them to read tooltips to identify them is an undesired inconvenience. The cost of retaining the labels has been mitigated by allowing the labels to wrap when they reach a very large width.

If the restore button causes the bottom section not to fit on one line, then the button is rendered in a smaller size and placed on a separate line. Marco raised concerns that this looks "jumpy" when resizing the window, but we are not and should not optimize for the rare case that a user decides to resize a window, while staring at about:home. Instead, we are optimizing the page to look and work great in both the narrow and wide viewport cases.

I discussed this with Stephen, and he's okay with it. I will upload screenshots for ui-review.
Comment 40 Frank Yan (:fryn) 2012-03-01 17:50:05 PST
Created attachment 602219 [details]
screenshot of patch v6 w/ wide viewport
Comment 41 Frank Yan (:fryn) 2012-03-01 17:50:36 PST
Created attachment 602220 [details]
screenshot of patch v6 w/ narrow viewport
Comment 42 Frank Yan (:fryn) 2012-03-01 18:27:47 PST
Created attachment 602232 [details] [diff] [review]
patch v6.1

Fixed alignment issue of restore icon.
Added more shorlander magic to search submit button.
Comment 43 Frank Yan (:fryn) 2012-03-01 18:29:01 PST
Created attachment 602233 [details]
screenshot of patch v6.1 w/ focus

Blue and bold!
Comment 44 Dão Gottwald [:dao] 2012-03-02 03:16:30 PST
Please test this with high-contrast themes on Windows.
Comment 45 Chris Lee [:clee] 2012-03-02 13:01:39 PST
(In reply to Frank Yan (:fryn) from comment #43)
> Created attachment 602233 [details]
> screenshot of patch v6.1 w/ focus
> 
> Blue and bold!

Per comment 39, I'm good with edge cases here and overall looks great.  Thanks shorlander and others for feedback and UI tweaks.
Comment 46 Girish Sharma [:Optimizer] 2012-03-02 23:51:00 PST
IMHO, the restore previous session button should be above the menu items list as when restore previous button is visible, it might be more important than the buttons for add-ons, settings etc. and should be given more visual importance (may be not make it grey?)
Comment 47 Marco Bonardo [::mak] 2012-03-05 08:13:26 PST
(In reply to Dão Gottwald [:dao] from comment #44)
> Please test this with high-contrast themes on Windows.

I don't think the current solution with an hardcoded background color may work fine with high-contrast, though using the default background would make the page appear quite badly.  Maybe we should also hardcode the text color, and just consider this as common content.
Comment 48 Girish Sharma [:Optimizer] 2012-03-05 08:20:11 PST
(In reply to Marco Bonardo [:mak] from comment #47)
> I don't think the current solution with an hardcoded background color may
> work fine with high-contrast, though using the default background would make
> the page appear quite badly.  Maybe we should also hardcode the text color,
> and just consider this as common content.

Can't everything be done without using hardcoded style elements ?
Comment 49 Marco Bonardo [::mak] 2012-03-05 08:33:21 PST
yes, breaking most of the visual design. So project management will have to make a call for either of these: don't use hardcoded background color and have it changing across platforms/themes, or hardcode both background and font color.
Comment 50 Marco Bonardo [::mak] 2012-03-05 09:15:34 PST
Comment on attachment 602232 [details] [diff] [review]
patch v6.1

Review of attachment 602232 [details] [diff] [review]:
-----------------------------------------------------------------

I have to trust Stephen on colors choice, so not going through those specifically.
I assume project management already signed-off the Sync change from links to button.

Sticking points before landing this:
- get go from marketing team on the snippets version bump
- get go from project management on the high contrast issue

I have no further comments, apart the following ones

::: browser/base/content/aboutHome.css
@@ +51,1 @@
>    color: -moz-FieldText;

as said in the previous comment about high-contrast theme, either we hardcode both color and background-color, or neither. I'm not sure the level of accessibility support we want to give to this page, but doing half won't work either cases.

::: browser/base/content/aboutHome.js
@@ +157,5 @@
>  
>    loadSnippets();
> +
> +  fitToWidth();
> +  window.addEventListener("resize", fitToWidth);

please remove the listener on unload

@@ +281,5 @@
>  
> +function fitToWidth() {
> +  if (window.scrollMaxX)
> +    document.body.setAttribute("narrow", "true");
> +  else if (document.body.hasAttribute("narrow") && !window.scrollMaxX) {

please when bracing one side of an if/else, brace all sides.

I don't understand what's the point of checking !window.scrollMaxX since the check in the if already excludes that
Comment 51 Stephen Horlander [:shorlander] 2012-03-05 10:35:19 PST
(In reply to Marco Bonardo [:mak] from comment #49)
> yes, breaking most of the visual design. So project management will have to
> make a call for either of these: don't use hardcoded background color and
> have it changing across platforms/themes, or hardcode both background and
> font color.

Yeah we don't want to break the design for this. 

If high contrast themes are something we want to support without breaking the design then we should probably have a media query for them. I actually thought that was something we could currently detect?
Comment 52 Asa Dotzler [:asa] 2012-03-05 10:55:40 PST
(In reply to Stephen Horlander from comment #51)
> (In reply to Marco Bonardo [:mak] from comment #49)
> > yes, breaking most of the visual design. So project management will have to
> > make a call for either of these: don't use hardcoded background color and
> > have it changing across platforms/themes, or hardcode both background and
> > font color.
> 
> Yeah we don't want to break the design for this. 
> 
> If high contrast themes are something we want to support without breaking
> the design then we should probably have a media query for them. I actually
> thought that was something we could currently detect?

This is not something we need to address before shipping this feature. Someone please file a follow-up bug to make this work better with high-contrast themes.
Comment 53 Marco Bonardo [::mak] 2012-03-05 11:21:00 PST
(In reply to Asa Dotzler [:asa] from comment #52)
> This is not something we need to address before shipping this feature.
> Someone please file a follow-up bug to make this work better with
> high-contrast themes.

Sure, we should at least hardcode the color though, for now.
Comment 54 Frank Yan (:fryn) 2012-03-06 00:02:46 PST
Addressed review comments and pushed:
https://hg.mozilla.org/integration/fx-team/rev/b5a3e22944f9
Comment 55 Jesper Hansen 2012-03-06 02:40:08 PST
Will this remove the current speeddial?
Comment 56 Stephen Horlander [:shorlander] 2012-03-06 06:03:34 PST
(In reply to Jesper Hansen from comment #55)
> Will this remove the current speeddial?

No. about:home and about:newtab are, currently, different things.
Comment 57 Guillaume C. [:ge3k0s] 2012-03-06 07:03:18 PST
Shouldn't the favicon be change to the current "home" button instead of firefox logo ?
Comment 58 Tim Taubert [:ttaubert] 2012-03-07 01:34:10 PST
Since its push https://tbpl.mozilla.org/?tree=Fx-Team&rev=b5a3e22944f9 there seem to be some weird oranges (almost perma-orange). I first thought these were only happening on my try pushes but then looking at the fx-team pushlog they first occured when this was pushed and are not existent on other branches.

Win XP debug shows:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | observe1: 2 windows in data being writted to disk - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | observe1: 1 closed window according to API - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | observe2: 1 closed window in data being writted to disk - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | observe2: 1 closed window according to API - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | 1 closed window according to API - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625016.js | Test timed out

Other sessionstore tests seem to time out randomly (Win XP dbg only), preceded by this error message:

###!!! ASSERTION: We should have a margin here! (out of memory?): 'hasMargin', file c:/mozilla/mozilla-inbound/layout/generic/nsFrame.cpp, line 835

Mak already noticed this in comment #11. I think we should maybe back this out but I know this is important to a lot of people so I wanted to give you the chance to maybe push a quick follow-up. I have no clue what could be causing this.
Comment 59 Frank Yan (:fryn) 2012-03-07 02:41:41 PST
Pushed a followup to hard-code the color (since that got accidentally left out) and to try to work around the above issue:
https://hg.mozilla.org/integration/fx-team/rev/3bb5b7d3a6f1
Comment 60 Tim Taubert [:ttaubert] 2012-03-07 23:41:11 PST
I wasn't able to figure out those oranges so we had to unfortunately back out:

https://hg.mozilla.org/integration/fx-team/rev/7e93dce2eb28
https://hg.mozilla.org/integration/fx-team/rev/af56647a2715

I think the only oranges we saw were these both but I still have no clue why they're affected. The test for bug 625016 loads about:rights. The second test loads about:home so there could be a reason here.

browser/components/sessionstore/test/browser_625016.js
browser/components/sessionstore/test/browser_707862.js
Comment 61 Rob Campbell [:rc] (:robcee) 2012-03-08 06:34:39 PST
https://hg.mozilla.org/mozilla-central/rev/b5a3e22944f9
Comment 63 Justin Dolske [:Dolske] 2012-03-12 17:53:04 PDT
Just a quick update on status. There are two main issues we've been pursuing since the landing/backout and over the weekend...

1) M-oth test failures. These happen in a variety of the sessionstore browser chrome test. We're still not sure of the exact cause -- mak has patches to change some potential issues in the tests themselves, but failures then occur elsewhere. Even when the guts of about:home are eviscerated (empty page, no JS) the test failures still occur. O_o Frank and I are looking into "bisecting" within the about:Home patch to try and figure out what the change is which causes the problem. Marco is also still looking into the tests.

2) Reproducible crash on Windows XP talos box. We got access to a test box (bug 734523, bug 734520) for better hands-on testing, since we've been otherwise unable to reproduce locally. Frank found that resizing the window in a certain way with about:home loaded generated a bunch of asserts and a crash. We know how to make a small change to avoid the asserts/crash, but the M-oth tests still fail. :( Will try to reproduce with a simpler testcase, perhaps with Jesse's Lithium.

Focusing on #1 right now, since we have a workaround for #2.
Comment 64 Frank Yan (:fryn) 2012-03-14 16:56:00 PDT
Pushed to fx-team (again):
https://hg.mozilla.org/integration/fx-team/rev/039e03f6d382
Comment 65 Richard Newman [:rnewman] 2012-03-14 16:58:26 PDT
This will need work on SuMo, at least for Sync setup.
Comment 66 Francesco Lodolo [:flod] 2012-03-15 10:24:50 PDT
Layout doubts from a l10n point of view:

1) What happens if a long localized string must be displayed on two lines (e.g. "add-ons" in Italian is "Componenti aggiuntivi"?

2) What happens with a very long single word?

3) Do we really want to reuse historyRestoreLastSession as a button's label? We already reused a menu label for a link, now we're moving it to a button. Considering the layout, it could be useful having a separate entity (e.g. localizers may want to use a shortened version of that string).
Comment 67 Marco Bonardo [::mak] 2012-03-15 10:39:39 PDT
(In reply to Francesco Lodolo [:flod] from comment #66)
> Layout doubts from a l10n point of view:
> 
> 1) What happens if a long localized string must be displayed on two lines
> (e.g. "add-ons" in Italian is "Componenti aggiuntivi"?
> 
> 2) What happens with a very long single word?

Hi Francesco, we discussed long localization details and also used italian as an example for the implementation, the buttons labels will just wrap. We tested some long words, though it's hard to figure out a long enough real-life example.
See https://bug711157.bugzilla.mozilla.org/attachment.cgi?id=602220

> 3) Do we really want to reuse historyRestoreLastSession as a button's label?

afaict, it was already used as a button in the previous implementation.

+        <button class="launchButton" id="restorePreviousSession">&historyRestoreLastSession.label;</button>
       </div>
 
-      <div id="sessionRestoreContainer">
-        <button id="restorePreviousSession">&historyRestoreLastSession.label;</button>
-      </div>

though a follow-up bug to split it may be useful if that's hurting localization.
Comment 68 Frank Yan (:fryn) 2012-03-15 16:08:19 PDT
Created attachment 606392 [details] [diff] [review]
coalesced patch for aurora (bugs 733651, 711157, 736279) [r=mak] [ui-r=shorlander]
Comment 69 Alex Keybl [:akeybl] 2012-03-15 16:54:10 PDT
Comment on attachment 606392 [details] [diff] [review]
coalesced patch for aurora (bugs 733651, 711157, 736279) [r=mak] [ui-r=shorlander]

[Triage Comment]
Per agreement with l10n folks, and given how early we are in the cycle, we're going to be making a string change exception here. Approved for Aurora 13. Please land today (3/15) to make it into our first pushed Aurora 13 update. Thanks!
Comment 70 Justin Dolske [:Dolske] 2012-03-15 23:18:02 PDT
Gavin merged fx-team over to central this evening...

http://hg.mozilla.org/integration/fx-team/rev/039e03f6d382
http://hg.mozilla.org/mozilla-central/rev/039e03f6d382

Despite this having 5 green runs in a row after landing, now about half of the Linux64 PGO runs are orange, with timeouts in browser_625016.js. WTF, sigh.

I'm holding off on the Aurora landing to see what mak wants to do here. More test tweaks? Can we just disable it? Last we talked it sounded like you were pretty sure these test failures were just a result of some subtle timing shifts exposing existing problems with the test -- if that's still the case, then we should land away on Aurora ASAP.
Comment 71 Francesco Lodolo [:flod] 2012-03-15 23:29:06 PDT
(In reply to Marco Bonardo [:mak] from comment #67)
> Hi Francesco, we discussed long localization details and also used italian
> as an example for the implementation, the buttons labels will just wrap. We
> tested some long words, though it's hard to figure out a long enough
> real-life example.

The longest single word I found with a quick search is this one (16 characters), but I see that the session restore button can wrap on a new line, so it shouldn't be a problem
/ga-IE/browser/chrome/browser/browser.dtd (View Hg log or Hg annotations)
 line 99 -- <!ENTITY bookmarksMenu.label "Leabharmharcanna">

> afaict, it was already used as a button in the previous implementation.
My bad, I remembered it as a link below the search box.
Comment 72 Marco Bonardo [::mak] 2012-03-16 02:45:29 PDT
(In reply to Justin Dolske [:Dolske] from comment #70)
> Despite this having 5 green runs in a row after landing, now about half of
> the Linux64 PGO runs are orange, with timeouts in browser_625016.js. WTF,
> sigh.

I don't think this is anything we should block on, the failure is slightly different from what I used to see, it's intermittent (I was hitting a permanent one) and on PGO of a single platform, and shows some tabview asserts before (?!). May even be some of the recent changes, we are backporting only the single patch here. Surely it's not nice, but we have far worse random failures in the tree.
I filed Bug 736416 to track it, we may need some assistance from session restore peers. But as I said I'd not surely block on this, we blocked before cause we had sort of a permaorange on WinXP.

I'll proceed and land.
Comment 74 Marco Bonardo [::mak] 2012-03-16 02:54:37 PDT
ehr, this happened before, I didn't notice that gavin forgot to mark the merge to central, though I saw it and checked tests status :/
https://hg.mozilla.org/mozilla-central/rev/039e03f6d382
Comment 75 Siddhartha Dugar [:sdrocking] 2012-03-16 07:33:46 PDT
In the new about:home the Nightly logo and the search box do not adjust to the window size. Will this no longer happen?
Comment 76 Marco Bonardo [::mak] 2012-03-16 08:05:50 PDT
(In reply to Siddhartha Dugar [:sdrocking] from comment #75)
> In the new about:home the Nightly logo and the search box do not adjust to
> the window size. Will this no longer happen?

correct
Comment 77 Alfred Kayser 2012-03-19 01:35:25 PDT
Interesting is that the home page says
"It's easy to customize your Firefox exactly the way you want it. Choose from thousands of add-ons."
But actually the home page itself is NOT customisable/themeable?
Is this on purpose?
What about the open web and allowing every one to customize everything?
Comment 78 Marco Bonardo [::mak] 2012-03-19 04:21:40 PDT
(In reply to Alfred Kayser from comment #77)
> Interesting is that the home page says
> "It's easy to customize your Firefox exactly the way you want it. Choose
> from thousands of add-ons."
> But actually the home page itself is NOT customisable/themeable?

you're mixing up contexts here, the snippet is clearly about add-ons and the page is not customizable on purpose (may change in future, no doubt, but for now that's by design). No idea what the the open Web has to do here.

Note You need to log in before you can comment on or make changes to this bug.