Closed Bug 455553 Opened 16 years ago Closed 12 years ago

New Tab Page feature

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 12

People

(Reporter: asaf, Assigned: ttaubert)

References

(Depends on 4 open bugs)

Details

(Keywords: addon-compat, Whiteboard: [a11y-ping=davidb])

Attachments

(6 files, 44 obsolete files)

111.23 KB, image/png
Details
24.53 KB, patch
Details | Diff | Splinter Review
23.70 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
16.54 KB, patch
Details | Diff | Splinter Review
38.73 KB, patch
dao
: review+
Details | Diff | Splinter Review
61.61 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch prototype v0.01 (obsolete) — Splinter Review
* A new chrome page in place, opened when opening a new tab (but not when opening a new window).
 * The url is never shown in the location bar
 * The chrome page has access to the browser window, the previously opened tab and to the selection within that tab.
 * I tried to alter sessionstore service so that it doesn't store that page, still doesn't work.
(In reply to comment #1)
>  * I tried to alter sessionstore service so that it doesn't store that page

Why would we restore a blank tab when the last thing the user saw was a non-blank one? AFAICT this new page isn't in any way special to how SessionStore should handle it - it will leave a trace in a tab's history, won't it (i.e. you'll be able to navigate back to the new tab page)?
You're not restoring blank tabs in sessionstore right now, are you?

I'm going to make sure that you cannot navigate back to the new tab page, it's not supposed to be part of the session history.
Comment on attachment 338928 [details] [diff] [review]
prototype v0.01

(In reply to comment #3)
> You're not restoring blank tabs in sessionstore right now, are you?

No. That's mostly due to the fact that blank tabs aren't in a tab's history, though, not because of treating about:blank in a special way ourselves.

> I'm going to make sure that you cannot navigate back to the new tab page,

Ideally, this could be achieved in our nsISHistory implementation (in which case your SessionStore modifications would work). Should you however choose to hack your way around, you'll want to remove the newTab.xul entry in nsSessionStore.js slightly above the place you've modified:

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>     else if (history && history.count > 0) {
>       for (var j = 0; j < history.count; j++)
>         tabData.entries.push(this._serializeHistoryEntry(history.getEntryAtIndex(j, false),
>                                                          aFullData));
>       tabData.index = history.index + 1;
OS: Mac OS X → All
Hardware: PC → All
What kind of content is going to be shown on this page?  I'm thinking something like the following would be nice:
Speed Dial - https://addons.mozilla.org/en-US/firefox/addon/4810
Fast Dial  - https://addons.mozilla.org/en-US/firefox/addon/5721

Should show recent things like starred and bookmarked pages and closed tabs along with most visited pages...all of them with previews, title and favicon.  The reason why I strongly suggest starred pages is because they most likely often are used because they are so hard to find hidden away in the library.  Those are probably the most useful thing to show on this page.
Version: unspecified → Trunk
Hit commit too fast, 

(In reply to comment #1)
>  * The url is never shown in the location bar

Would the user be able to bookmark this page so if they wanted as a homepage they will be able to use it?  No point in only showing the content when only opening a new tab.
I don't know if comments are welcomed, but I've been using Opera a lot and Chrome too recently, both try to show useful stuff in a new tab and on startup, but both fail in my opinion...

Because what they show is completely useless. Automatically populated Speed Dial in Chrome is polluted with redirect pages like the Google login system (various GMail pages take up the top 6 visited sites). Also Forums' refresh-after-post pages tend to show up. 

Other stuff like search widgets are already part of the UI, moreover, a Speed Dial is useful even when being finished with a page and wanting to navigate away, and there's "Most Visited" in the bookmark bar for this currently (with the same flaws as Chrome's Speed Dial).

An empty page with an input box on (above) it is not intimidating at all! Look at Google's success!
How about a stripped and local version of the normal start page:
http://www.google.nl/firefox?client=firefox-a&rls=org.mozilla:en-US:official
e..g. with 'about:start'.
Possibly combined with some material from:
http://en-us.www.mozilla.com/en-US/firefox/features/
So that users are pointed to (new) features.

The page has be very lean and mean to prevent Ts and Tp impact.
(In reply to comment #9)

It's fine to show a description of features the first time for a new user, but not every time a new tab is opened!

And a stripped down version of the Google startpage is a search box, Firefox already has a search box!
(In reply to comment #7)
> (In reply to comment #1)
> >  * The url is never shown in the location bar
> 
> Would the user be able to bookmark this page so if they wanted as a homepage
> they will be able to use it?  No point in only showing the content when only
> opening a new tab.

use a about:tabs or about:start like Alfred Kayser calls it please

About Speed Dial / Fast Dial or how you want to call it;
Make in the preferences panel a new tab with the option show 4/9/16 choices and then the urls of the pages that you want to have.
Now we have tags, and I have made a tag for pages I am using (almost) every day,
in the list 'Most Visited' is the first page 'Google' and this is good to start Firefox with, but it isn't for browsing with (the search field of Firefox does it)
Target Milestone: --- → Future
How many of the ideas in Aza Raskin's blog (http://www.azarask.in/blog/post/firefox-31-new-tab-spec/) are likely to make it into this bug? Some of the ideas sound really excellent in principle. The contextual actions, like a map search of address details on the previously selected tab. The quick access strip that only contains sites that start history strands sounds like it may address to observation in comment 8 and avoid useless pages like gmail redirects. 

As long as the page loads virtually instantly then many of those ideas sound good. I am not sure how far off they are from being implemented - even in a try-server build.
This isn't going to make 3.1 if it doesn't get in for 3.1b2?
Note that the 'Ctrl-Tab' thing also has been removed from 3.1 (temporarily disabled), as it was seen as not yet completed / bug free.
So, starting a new feature like the 'about:blank/start' thing is probably also something not for 3.1.

Note, that the CtrlTab feature also has 3 rows of 3 previews, which also needs to be configurable, so this needs to be based on the same configs
(In reply to comment 14)
> Note that the 'Ctrl-Tab' thing also has been removed from 3.1 (temporarily
> disabled), as it was seen as not yet completed / bug free.

I tried the add-on Showcase and it was a very configurable alternative to cover say All-Tabs features.

We should also think about adding back an option to configure the new tab or "home" tab to always load just your "homepage" if you wanted - (useful for portals for organizations and end users who don't like blank new tabs and want to keep users focused to their site), as in providing a better option to re-implementing a wontfix bug 269664.  

-Just a note so we don't get lost:

If this morphs to a one size fits all tab (which users don't expect, but) and no separate home button, there should be a new easy and consistent way for users to recognize and access their "homepage" from this new functionality to load their "homepage" ie:

A home header, A home link, A home button, A home icon, A home image
Attached patch prototype v1 (obsolete) — Splinter Review
This feature is coming back to life, and it's being tracked here: https://wiki.mozilla.org/New_Tab_page

Here's a prototype I made that populates the new tab page with the same list of sites as the autocomplete dropdown. Since Test Pilot studies have indicated that a large percentage of users use that dropdown list, this seems like a reasonable place to start. Removing sites from this list has the same effect as removing them from the awesomebar popup (removing the page from browser history).
Assignee: mano → margaret.leibovic
Attachment #338928 - Attachment is obsolete: true
Built from http://hg.mozilla.org/projects/ux/rev/dd1a9622f0dd
Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110607 Firefox/7.0a1

Top sites on new tab invisible.

Error: NewTabPage is undefined
Source File: about:newtab Line: 1

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://browser/content/newTab.js :: <TOP_LEVEL> :: line 43"  data: no]
The import() in newTab.js is wrong - newTabUtils.jsm should probably be renamed to NewTabUtils.jsm, and the import() call should use "resource:///" instead of "resource://gre/" since this is a browser-only module (only matters for Firefox when omnijar is enabled, I think).
work now, just rename newTabUtils.jsm to NewTabUtils.jsm.
Summary: New Tab Feature → New Tab Page feature
If we're sticking with about:newtab as the url for this page, the following line also needs to be updated:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1549
It should be possible to add some feeds to the new tab page.
http://upload.7pmtech.com/view/full/90_fltn2
Sorry, wrong link. Here's the correct one:
http://upload.7pmtech.com/files/90_fltn2/2.jpg
You want to say in such a form as now, work is over?
(In reply to Tiger.711 from comment #24)
> You want to say in such a form as now, work is over?

I'm not actively working on this anymore. This feature is going to get more attention shortly, but I'm not going to be the one working on it.
Assignee: margaret.leibovic → nobody
Assignee: nobody → ttaubert
I'm thinking the following would be nice:
Desktop - https://addons.mozilla.org/en-US/firefox/addon/desktop/

thumbnails on new browser tab with full control on their layout(movable), sub-folders, and sync with Firefox Sync!
Previews are broken, they're all the same.
It would be nice if a preview to the logos of sites (often it's logo.png) or possibility to use your own image as a preview.

Will be able to change change the background image?
Would it be possible to make it aero-glass, like in panorama?
Will be able to add bookmarks? Like in Fast Dial and similar extensions.
Depends on: 497543
May I put forward this proposal for the thumbnails on the new tab page? Currently there are at least three types of thumbnails across Firefox (Panorama, Fennec and those proposed for this).

This takes some of the ideas that Faaborg/Margaret put forward and integrates them into the latest proposal for thumbnails on the new tab page.
As comment 30 hints at, is it an idea to combine new tab page and the 'panorama/tabview' into one thing?
(In reply to Alfred Kayser from comment #31)
> As comment 30 hints at, is it an idea to combine new tab page and the
> 'panorama/tabview' into one thing?

Just to clarify, I was not hinting at combining the new tab page and panorama, but was rather hoping to have a consistent thumbnail style across the entire product line. My proposal merely build on the good work that Margaret done and integrated that into the current new tab page proposal while drawing some inspiration from panorama.

As an aside, I would recommend the new tab page doesn't use a white background but rather appears as an extension to the navigation/bookmarks toolbar.
(In reply to Paul [sabret00the] from comment #30)
> May I put forward this proposal for the thumbnails on the new tab page?
> Currently there are at least three types of thumbnails across Firefox
> (Panorama, Fennec and those proposed for this).

I like your suggestion on first sight but there's some underlying work to be done first. I've created a thumbnail service in bug 497543 that needs to be used by all of these parts first (it's already used by the New Tab Page). After that is done the UX team would need to start figuring out a common design for all these "clients" (which is not too easy I think). I think this should definitely be handled in its own bug.
Blocks: 699362
Hovered thumbnails should look more different from others. Perhaps the default opacity should be reduced.
Really nice implementation as of now expect the fact IMO the thumbnails can take lil more space and can be lil less congested ... 

Also , i think if a thumbnail is dragged outside the window area , it should be opened in new window , that's how i see it and will be more easy for use. 
I noticed that their size doesn't change with window size , can't we use CSS or something like that , as demoed everywhere , it will be good to promote these technologies IF POSSIBLE.
I noticed that there is a delay when the page loads up all the thumbnails from their respective websites. Caching this be somewhere will improve the perceived performance.

For some sites like gmail.com, where the url always redirects to some other page, the new tab page keeps showing http://gmail.com as the thumbnail label instead of the page redirected to. Please fix this.

Some UI suggestions:
The page label should be displayed below the thumbnail like in panorama and not over it to allow better visibility. The URL should be hidden by default like in other browsers and may be displayed in a tooltip if needed.

The black bars look ugly and should be replaced by just buttons as Paul posted above. Also these buttons may be displayed only on hover. The top left corner may show the page favicon and be replaced by the pin option on hover.
I agree with the thumbnail review proposal, only that I suggest, the pin should always be visible if a cell is pinned, so that the user knows that it is pinned just by looking at it.
The grid should be 2X4 instead of 3X3 to make better use of screen space, and allow bigger thumbnails. This will also allow placing the site title below the thumbnail and not over it.
I think, that the grid should be customizable.
Is it reasonable to expect the layout to adapt to screensize based on CSS media queries?
Tim, on OS X, dropping a thumbnail is janky, because the OS does a slow animation for drags between the mouse being released and when dragend is called, which is perceived as an unnecessary delay in our use cases. I ran into this problem when implementing tab animations too. To circumvent this, I recommend calling event.preventDefault() in a dragover handler on the document, so that the OS thinks that the drop is accepted and skips the animation.
(In reply to Frank Yan (:fryn) from comment #41)
> Tim, on OS X, dropping a thumbnail is janky, because the OS does a slow
> animation for drags between the mouse being released and when dragend is
> called, which is perceived as an unnecessary delay in our use cases. I ran
> into this problem when implementing tab animations too. To circumvent this,
> I recommend calling event.preventDefault() in a dragover handler on the
> document, so that the OS thinks that the drop is accepted and skips the
> animation.

Ok, fixed in my patch queue :) Thanks!
Attached patch patch v1 (WIP) (obsolete) — Splinter Review
Asking for UX review. This is especially about basics, effects and drag/drop behavior. The following improvement bugs exist which we want to tackle on their own to get this basic feature reviewed. We could (and probably would) of course defer landing the whole feature until any or all of these bugs have been fixed:

bug 705000, bug 705001

Please use tomorrow's UX Nightly (should be 2011-11-25).
Attachment #535455 - Attachment is obsolete: true
Attachment #576861 - Flags: ui-review?(jboriss)
Using the latest hourly's.
Are the two buttons on top right intentionally left transparent ?
I am talking about the buttons to reset the New Tab Page and to toggle the display of the 9 tabs.

Also, while dragging any thumbnail, the mouse pointer flickers and changes back and forth from being a dragging mouse pointer to a normal mouse pointer.
Depends on: 704882
Depends on: 505521
Depends on: 705907
Depends on: 705911
(In reply to scrapmachines from comment #44)
> Are the two buttons on top right intentionally left transparent ?
> Also, while dragging any thumbnail, the mouse pointer flickers and changes
> back and forth from being a dragging mouse pointer to a normal mouse pointer.

FTR, that's on Windows. Thanks for reporting, should be fixed with today's nightly.
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
Attachment #576861 - Attachment is obsolete: true
Attachment #576861 - Flags: ui-review?(jboriss)
Attached patch Part 2 - Assets / Images (obsolete) — Splinter Review
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
(In reply to Tim Taubert [:ttaubert] from comment #47)
> (In reply to scrapmachines from comment #44)
> > Are the two buttons on top right intentionally left transparent ?
> > Also, while dragging any thumbnail, the mouse pointer flickers and changes
> > back and forth from being a dragging mouse pointer to a normal mouse pointer.
> 
> FTR, that's on Windows. Thanks for reporting, should be fixed with today's
> nightly.

Totally works on Windwos now , both the icons on the top right and the flickering.
Great work. It should be ready now to land to mc.
Attachment #579643 - Flags: feedback?(jwein)
Attachment #579644 - Flags: feedback?(bmcbride)
Attachment #579645 - Flags: feedback?(fryn)
Attachment #579646 - Flags: feedback?(mak77)
Attachment #579647 - Flags: feedback?(dietrich)
Comment on attachment 579647 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

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

need to document all the functions in head.js, including an overview of how the tests in this dir work (aSitePattern, etc).
Attachment #579647 - Flags: feedback?(dietrich) → feedback+
Comment on attachment 579643 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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

Looks great!

There are many places where double underscores are used to prefix named functions that can be switched to use a single underscore, and a few places where |__defineGetter__| is used that can be switched to use |Object.defineProperty| instead.

Also, there are a number of |addEventListener| calls that don't appear to have matching |removeEventListener| calls. These may be innocuous, but it is worth double-checking.

::: browser/base/content/newtab/cells.js
@@ +39,5 @@
> +// Class: Cell
> +// This class represents a cell contained in the grid.
> +function Cell(aNode) {
> +  this._node = aNode;
> +  this._node.__newtabCell = this;

We should probably use a single underscore prefix here, such as |this._node._newtabCell|

@@ +58,5 @@
> +  get index() {
> +    let index = Grid.cells.indexOf(this);
> +
> +    // cache this value, overwrite the getter
> +    this.__defineGetter__("index", function () index);

Can you use Object.defineProperty() here? According to this blog post, Object.defineProperty is the preferred approach now: http://whereswalden.com/2010/04/16/more-spidermonkey-changes-ancient-esoteric-very-rarely-used-syntax-for-creating-getters-and-setters-is-being-removed/

@@ +69,5 @@
> +    let prev = this.node.previousElementSibling;
> +    prev = prev && prev.__newtabCell;
> +
> +    // cache this value, overwrite the getter
> +    this.__defineGetter__("previousSibling", function () prev);

Same Object.defineProperty() question here.

@@ +80,5 @@
> +    let next = this.node.nextElementSibling;
> +    next = next && next.__newtabCell;
> +
> +    // cache this value, overwrite the getter
> +    this.__defineGetter__("nextSibling", function () next);

Ditto.

@@ +88,5 @@
> +
> +  // the site contained in the cell, if any
> +  get site() {
> +    let firstChild = this.node.firstElementChild;
> +    return firstChild && firstChild.__newtabSite;

Same nit about the double underscores here.

::: browser/base/content/newtab/drag.js
@@ +141,5 @@
> +  //
> +  // Parameters:
> +  //   aSite - the site that's being dragged
> +  //   aEvent - the 'dragstart' event
> +  _setDragData: function Drag__setDragData(aSite, aEvent) {

Can you change to a single underscore here on the named function?

@@ +154,5 @@
> +    dt.setData("text/html", "<a href=\"" + url + "\">" + url + "</a>");
> +
> +    // create and use an empty drag image. we don't want to use the default
> +    // drag image with its default opacity.
> +    let img = document.createElement("div");

Is this supposed to be a div or an img? Can you change the variable name to match the tag name?

::: browser/base/content/newtab/drop.js
@@ +36,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +
> +// a little delay that prevents the grid from being too sensitive when dragging
> +// sites around
> +const DELAY_REARRANGE = 250;

Can you please rename this to DELAY_REARRANGE_MS? I prefer having constant numbers with units.

@@ +101,5 @@
> +  // Re-pins all pinned sites in their (new) positions.
> +  //
> +  // Parameters:
> +  //   aCell - the drop target cell
> +  _repinSitesAfterDrop: function Drop__repinSitesAfterDrop(aCell) {

Single underscore here?

@@ +110,5 @@
> +      return aSite && aSite.isPinned();
> +    });
> +
> +    // re-pin all shifted pinned cells
> +    pinnedSites.forEach(function (aSite) aSite.pin(sites.indexOf(aSite)));

This file is included from a "use strict"; file. So |this| should be set to the passed argument, which I believe is |undefined|. Can you pass a |thisArg| to |forEach| so future authors will know what to expect.

@@ +120,5 @@
> +  //
> +  // Parameters:
> +  //   aCell - the drop target cell
> +  //   aEvent - the 'dragexit' event
> +  _pinDraggedSite: function Drop__pinDraggedSite(aCell, aEvent) {

Single underscore here?

@@ +142,5 @@
> +  // Time a rearrange with a little delay.
> +  //
> +  // Parameters:
> +  //   aCell - the drop target cell
> +  _delayedRearrange: function Drop__delayedRearrange(aCell) {

Ditto.

@@ +164,5 @@
> +
> +  // ----------
> +  // Function: _cancelDelayedArrange
> +  // Cancels a timed rearrange, if any.
> +  _cancelDelayedArrange: function Drop__cancelDelayedArrange() {

Ditto.

@@ +177,5 @@
> +  // Rearrange all sites in the grid depending on the current drop target.
> +  //
> +  // Parameters:
> +  //   aCell - the drop target cell
> +  _rearrange: function Drop__rearrange(aCell) {

Ditto.

::: browser/base/content/newtab/dropPreview.js
@@ +169,5 @@
> +    return range;
> +  },
> +
> +  // ----------
> +  // Function: _filterPinnedSites

This looks like an incorrect copy/paste.

@@ +176,5 @@
> +  //
> +  // Parameters:
> +  //   aSites - the array of sites to check
> +  //   aCell - the drop target cell
> +  _hasOverflownPinnedSite:

Hmm... overflown vs. overflowed... Can we change this to |_hasOverflowedPinnedSite| and change the other mentions to use overflowed instead? Overflown reminds me too much of airplanes missing runways :(

@@ +199,5 @@
> +
> +  // ----------
> +  // Function: _repositionOverflownPinnedSite
> +  // We have a overflown pinned site that we need to re-position so that it's
> +  // visible again. We try to find a lower-prio cell (empty or containing

s/lower-prio/lower-priority

::: browser/base/content/newtab/dropTargetShim.js
@@ +69,5 @@
> +    Grid.lock();
> +  },
> +
> +  // ----------
> +  // Function: _start

Copy/paste mistake?

::: browser/base/content/newtab/grid.js
@@ +88,5 @@
> +  // Function: refresh
> +  // Refreshes the grid and re-creates all sites.
> +  refresh: function Grid_refresh() {
> +    // remove all sites
> +    this.cells.forEach(function (cell) {

Please add a |this| argument to the forEach call.

@@ +140,5 @@
> +  // ----------
> +  // Function: _draw
> +  // Draws the grid, creates all sites and puts them into their cells.
> +  _draw: function Grid__draw() {
> +    let self = this;

Do we need to add the |self| variable here? Shouldn't |this| be consistent within the function? Maybe this was left over from a previous version that used a closure?

::: browser/base/content/newtab/helper.js
@@ +59,5 @@
> +  debug("trace: " + aMsg + "\n" + stack.join("\n"));
> +}
> +
> +// ##########
> +// Class: Batch

This is a cool little class. It took me a minute to understand the goal of the class, but now that I get it, I think it is pretty neat :)

@@ +108,5 @@
> +  // Checks if the batch has finished.
> +  _check: function Batch__check() {
> +    if (this._count == 0 && this._callback)
> +      this._callback();
> +  }

There seems to be an odd invariant here. Suppose a user of the Batch class does the following (pseudocode follows):
let b = new Batch(function() console.log('callback'));
b.close();
b.pop();
delete b;

In this scenario, it looks like the callback will be invoked twice. While no sane author would intend to write the code this way, it should be trivial to restrict the callback from being invoked twice.

::: browser/base/content/newtab/newTab.css
@@ +31,5 @@
> +.toolbar-button {
> +  position: absolute;
> +  cursor: pointer;
> +  background-color: transparent;
> +  background-image: url(chrome://browser/skin/newtab/toolbar.png);

This should probably be moved to a theme specific CSS file (in /browser/themes/).

@@ +83,5 @@
> +#toolbar-button-reset:active {
> +  background-position: -21px -24px;
> +}
> +
> +.modified #toolbar-button-reset, .disabled #toolbar-button-show {

We should probably stay away from using the descendent selector. Can you apply these classes to the descendent elements? In other words, adjust the code so these selectors can be |#toolbar-button-reset.modified, #toolbar-button-show.disabled|.

@@ +88,5 @@
> +  opacity: 1;
> +  pointer-events: auto;
> +}
> +
> +.disabled #toolbar-button-hide, .disabled #toolbar-button-reset {

Ditto.

@@ +143,5 @@
> +  width: 201px;
> +  height: 127px;
> +  outline: none;
> +  background-color: #ececec;
> +  box-shadow: 0 0 0 #000;

The box-shadow can't be seen here because it has no offset and no blur radius. This is probably here for the transition, but I did some simple tests and I think you can remove this line.

I tested with these URIs:
data:text/html,<style>div{-moz-transition:200ms;box-shadow:0 0 0 black;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div>
data:text/html,<style>div{-moz-transition:200ms;box-shadow:none;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div>
data:text/html,<style>div{-moz-transition:200ms;} div:hover{box-shadow:0 100px 2px black}</style><div style="background-color:red; width:400px; height:200px;">box-shadow</div>

@@ +193,5 @@
> +  font-weight: 700;
> +  font-size: 70%;
> +  text-decoration: none;
> +  line-height: 1.2em;
> +  background: rgba(0, 0, 0, 0.8);

Please change this to |background-color: rgba(0,0,0,0.8);|.

@@ +284,5 @@
> +/* DRAG & DROP */
> +.drag-img {
> +  width: 1px;
> +  height: 1px;
> +  background: none #fff;

Can you use |background-color: #fff;| here instead?

@@ +285,5 @@
> +.drag-img {
> +  width: 1px;
> +  height: 1px;
> +  background: none #fff;
> +  opacity: 0.01;

I'm not sure why the opacity is 0.01. Is this for a transition?

::: browser/base/content/newtab/newTab.xhtml
@@ +49,5 @@
> +]>
> +
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +  <head>
> +    <title>New Tab</title>

This text should probably be moved out to a DTD file.

::: browser/base/content/newtab/toolbar.js
@@ +74,5 @@
> +    Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, false);
> +  },
> +
> +  // ----------
> +  // Function: hide

Copy/paste mistake?

@@ +80,5 @@
> +  //
> +  // Parameters:
> +  //   aCallback - the callback to call when the page has been reset
> +  reset: function Toolbar_reset(aCallback) {
> +    if (aCallback && typeof aCallback != "function")

There is similar code throughout this patch that takes a callback argument, checks for truthiness, and invokes it if possible. This is the only place within the patch that checks the |typeof| the callback. Does |Toolbar_reset| sometimes get called with a non-function |aCallback| argument? If not, should this check be removed or should the other callback invocations check the |typeof| the callback?
Attachment #579643 - Flags: feedback?(jwein) → feedback+
Comment on attachment 579645 [details] [diff] [review]
Part 3 - about:newtab integration

This looks good, but I highly recommend we add code either to hide the favicon in the tab for the New Tab Page or to suppress the throbber, since the throbber makes opening new tabs feel slower.

Also, the New Tab Page is very high contrast right now. That, combined with the brief blank page shown to the user before the page is rendered, is quite distracting. Could we mitigate that by toning down the contrast (via colors or opacity)?
Attachment #579645 - Flags: feedback?(fryn) → feedback+
No longer depends on: 505521
Comment on attachment 579644 [details] [diff] [review]
Part 2 - Assets / Images

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

These jar.mn files look SPLENDID!
Attachment #579644 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 579643 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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

::: browser/base/content/newtab/animations.js
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Since these files are being #include'd, and the base file has the same license, it'd be nice to have the licenses here preprocessed away so you don't end up with a file with a dozen licenses - which seems like a waste of space.

::: browser/base/content/newtab/cells.js
@@ +41,5 @@
> +function Cell(aNode) {
> +  this._node = aNode;
> +  this._node.__newtabCell = this;
> +
> +  // register drag-and-drop event handlers

General nit: Capital first letter, full stop at the end.

@@ +45,5 @@
> +  // register drag-and-drop event handlers
> +  ["dragenter", "dragover", "dragexit", "drop"].forEach(function (aType) {
> +    let method = "_" + aType;
> +    this[method] = this[method].bind(this);
> +    this._node.addEventListener(aType, this[method], false);

Could make this more deliberate and potentially less error prone (maintenance-wise) by having the prefix be "_on" or "on", and capitalizing the event names (addEventListener is case-insensitive), so you end up pointing to functions like _onDragOver.

::: browser/base/content/newtab/grid.js
@@ +133,5 @@
> +      '  <input class="button strip-button strip-button-block" type="button"/>' +
> +      '</span>';
> +
> +    let fragment = this._siteFragment = document.createDocumentFragment();
> +    fragment.appendChild(site);

This feels awkward - no need for the temporary variable |fragment|.

::: browser/base/content/newtab/helper.js
@@ +40,5 @@
> +// Logs a given debug message to the console.
> +//
> +// Parameters:
> +//   aMsg - the log message
> +function debug(aMsg) {

This appears to be unused.

@@ +51,5 @@
> +// Logs a given debug message to the console and prints the current stack trace.
> +//
> +// Parameters:
> +//   aMsg - the log message
> +function trace(aMsg) {

This appears to be unused.

::: browser/base/content/newtab/newTab.css
@@ +128,5 @@
> +  margin: 0 0 15px 16px;
> +}
> +
> +.cell:nth-child(3n+3) {
> +  margin-right: 0;

This doesn't seem RTL friendly.

::: browser/base/content/newtab/newTab.xhtml
@@ +47,5 @@
> +  <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd" >
> +  %browserDTD;
> +]>
> +
> +<html xmlns="http://www.w3.org/1999/xhtml">

HTML5 is the new hotness.

::: browser/base/content/newtab/page.js
@@ +38,5 @@
> +// ##########
> +// Class: Page
> +// This singleton represents the whole 'New Tab Page' and takes care of
> +// initializing all its components.
> +let Page = {

Since this is used as a global variable (Grid, Animations, etc too), it's name should reflect it. "Page" makes it look like you should use |new Page()|, which is misleading. gPage or similar would be better.

Alternatively, you could do |let gPage = new Page("#toolbar", "#grid")|. That would also let you put this (and a bunch of other files here) in a JSM - would be interesting to see if doing that could reduce the load time by a few milliseconds (which can really matter here), since the JS wouldn't need to be parsed every time.

::: browser/base/content/newtab/updater.js
@@ +38,5 @@
> +// ##########
> +// Class: Updater
> +// This singleton provides functionality to update the current grid to a new
> +// set of pinned and blocked sites. It adds, moves and removes sites.
> +let Updater = {

I wonder if this should be merged with Grid.
Attachment #579643 - Flags: feedback+
Comment on attachment 579646 [details] [diff] [review]
Part 4 - Shared Module

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

::: browser/base/content/NewTabUtils.jsm
@@ +60,5 @@
> +  "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager");
> +
> +// ##########
> +// Class: Storage
> +// Singleton that provides storage functionality.

My kingdom for consistent code documentation style. And by that I mean javadoc-style, which is the predominant style for JS files in the tree (I have no data to back that up).

@@ +71,5 @@
> +      return this._storage;
> +
> +    let uri = Services.io.newURI("about:newtab", null, null);
> +    let principal = gScriptSecurityManager.getCodebasePrincipal(uri);
> +    let storage = gStorageManager.getLocalStorageForPrincipal(principal, "");

Looks like this won't use PrivateBrowsingStorage if the first about:newtab is opened when the browser is already in private-browsing mode. eg, if Firefox is started with the -privatebrowsing command line switch (or whatever it's called).

@@ +132,5 @@
> +    return {
> +      observe: function () {
> +        if (gPrivateBrowsing.privateBrowsingEnabled) {
> +          // create a temporary storage for the pb mode
> +          self._storage = new PrivateBrowsingStorage(self.storage);

Add a comment here saying why resetCache()/update() is only called when leaving private browsing mode, as it isn't immediately obvious.

@@ +293,5 @@
> +  //
> +  // Parameters:
> +  //   aLink - the link to check
> +  isPinned: function PinnedLinks_isPinned(aLink) {
> +    return (-1 < this._indexOfLink(aLink));

|this._indexOfLink(aLink) != -1| makes it more obvious what the code is doing, IMO.

@@ +474,5 @@
> +// ##########
> +// Class: Testing
> +// Singleton that provides testing functionality and wrapped access to core
> +// objects to mock certain values.
> +let Testing = {

Is there any way around needing this? I cringe at shipping code that's only useful for automated tests.

@@ +532,5 @@
> +  Pages: Pages,
> +  Links: Links,
> +  Testing: Testing,
> +  PinnedLinks: PinnedLinks,
> +  BlockedLinks: BlockedLinks

This would look less like constructors if they were (for instance) NewTabUtils.pages instead of NewTabUtils.Pages. Also, pages/page is subtle - allPages/page is less subtle.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +457,4 @@
>      var searchParamParts = aSearchParam.split(" ");
>      this._enableActions = searchParamParts.indexOf("enable-actions") != -1;
>  
> +    if (searchParamParts.indexOf("newtab-maxresults") != -1)

This feels a little wrong/hackish. Calling it something like "guarentee-more-results" or something might be better. I bet Macro has thoughts (hopefully better than mine).

@@ +457,5 @@
>      var searchParamParts = aSearchParam.split(" ");
>      this._enableActions = searchParamParts.indexOf("enable-actions") != -1;
>  
> +    if (searchParamParts.indexOf("newtab-maxresults") != -1)
> +      this._maxRichResults = 100;

And this should probably be a global constant.
Attachment #579646 - Flags: feedback+
(In reply to Dietrich Ayala (:dietrich) from comment #54)
> need to document all the functions in head.js, including an overview of how
> the tests in this dir work (aSitePattern, etc).

Done.
Attachment #579647 - Attachment is obsolete: true
Attachment #581427 - Flags: review?(dietrich)
We need a way to get more than the default 25 results from the nsPlacesAutoComplete service.
Attachment #581428 - Flags: review?(mak77)
(In reply to Frank Yan (:fryn) from comment #56)
> This looks good, but I highly recommend we add code either to hide the
> favicon in the tab for the New Tab Page or to suppress the throbber, since
> the throbber makes opening new tabs feel slower.

Done.

> Also, the New Tab Page is very high contrast right now. That, combined with
> the brief blank page shown to the user before the page is rendered, is quite
> distracting. Could we mitigate that by toning down the contrast (via colors
> or opacity)?

I think that's gonna be handled in bug 705000.
Attachment #579645 - Attachment is obsolete: true
Attachment #581432 - Flags: review?(fryn)
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
(In reply to Blair McBride (:Unfocused) from comment #59)
> My kingdom for consistent code documentation style. And by that I mean
> javadoc-style, which is the predominant style for JS files in the tree (I
> have no data to back that up).

Documentation style changed to javadoc.

> Looks like this won't use PrivateBrowsingStorage if the first about:newtab
> is opened when the browser is already in private-browsing mode. eg, if
> Firefox is started with the -privatebrowsing command line switch (or
> whatever it's called).

True, I didn't consider this. Fixed.

> Add a comment here saying why resetCache()/update() is only called when
> leaving private browsing mode, as it isn't immediately obvious.

Done.

> Is there any way around needing this? I cringe at shipping code that's only
> useful for automated tests.

Turns out that was legacy anyway so we don't need it anymore. Removed.

> This would look less like constructors if they were (for instance)
> NewTabUtils.pages instead of NewTabUtils.Pages. Also, pages/page is subtle -
> allPages/page is less subtle.

Fixed.
Attachment #579646 - Attachment is obsolete: true
Attachment #581436 - Flags: review?(bmcbride)
Attachment #579646 - Flags: feedback?(mak77)
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
(In reply to Jared Wein [:jwein and :jaws] from comment #55)
> ::: browser/base/content/newtab/newTab.css
> > +  background-image: url(chrome://browser/skin/newtab/toolbar.png);
> 
> This should probably be moved to a theme specific CSS file (in /browser/themes/).

Done.

> We should probably stay away from using the descendent selector. Can you apply
> these classes to the descendent elements? In other words, adjust the code so
> these selectors can be |#toolbar-button-reset.modified, #toolbar-button-show.disabled|.

Fixed using attributes.

> The box-shadow can't be seen here because it has no offset and no blur radius.
> This is probably here for the transition, but I did some simple tests and I think
> you can remove this line.

> @@ +285,5 @@
> > +.drag-img {
> > +  opacity: 0.01;
> 
> I'm not sure why the opacity is 0.01. Is this for a transition?

I want this to be hidden so that the DnD implementation does not use its default drag image. 'visibility: hidden' and 'opacity: 0' don't work - that seems to be equal to hidden. So this only works with 'opacity: 0.01'.

Fixed all other minor CSS issues.

(In reply to Blair McBride (:Unfocused) from comment #58)
> ::: browser/base/content/newtab/newTab.css
> @@ +128,5 @@
> > +.cell:nth-child(3n+3) {
> > +  margin-right: 0;
> 
> This doesn't seem RTL friendly.

Fixed using -moz-margin-end.
Attachment #579644 - Attachment is obsolete: true
Attachment #581441 - Flags: review?(jwein)
Comment on attachment 581441 [details] [diff] [review]
Part 2 - Assets / CSS / Images

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

Looks good, although I should defer to someone who has review privileges on this.
Attachment #581441 - Flags: review?(jwein)
Attachment #581441 - Flags: review?(dao)
Attachment #581441 - Flags: feedback+
Comment on attachment 581432 [details] [diff] [review]
Part 3 - about:newtab integration

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

(In reply to Tim Taubert [:ttaubert] from comment #62)
> (In reply to Frank Yan (:fryn) from comment #56)
> > Also, the New Tab Page is very high contrast right now. That, combined with
> > the brief blank page shown to the user before the page is rendered, is quite
> > distracting. Could we mitigate that by toning down the contrast (via colors
> > or opacity)?
> 
> I think that's gonna be handled in bug 705000.

Okay, but I really, really want the thumbnail contrast problem addressed ASAP.
By thumbnail contract, I also mean the thumbnail itself, not just its label.
I don't want us to be both late to the new tab page party and worse in UX.

::: browser/app/profile/firefox.js
@@ +1084,5 @@
>  pref("browser.panorama.animate_zoom", true);
>  
> +// New Tab feature
> +pref("browser.newtab.enabled", true);
> +

Nit: I recommend changing the comment to "Enable New Tab Page (about:newtab)" or something like that, since new tab creation in general has been around for a while.

::: browser/base/content/tabbrowser.xml
@@ +536,4 @@
>                  if (aWebProgress.DOMWindow == this.mBrowser.contentWindow)
>                    this.mBrowser.userTypedClear += 2;
>  
> +                if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") {

Interesting way of suppressing the throbber. :P
Attachment #581432 - Flags: review?(fryn) → review+
Comment on attachment 581432 [details] [diff] [review]
Part 3 - about:newtab integration

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-              if (title && title != "about:blank") {
>+              if (title && title != "about:blank" && title != "about:newtab") {

It might be handy to have symbolic functions that:
- checks whether a given URL should be considered "blank" (for the moment that would just check for about:blank and about:newtab)
- returns the URL to use for new tab pages (to be used in the ~3 places in this patch that currently hardcode about:newtab)

>             var blank = !aURI || (aURI == "about:blank");
> 
>-            if (blank)
>+            if (blank || aURI == "about:newtab")

Can't you just change add this condition to the setting of "blank"? That way mBlank will be set correctly too.
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
(In reply to Jared Wein [:jwein and :jaws] from comment #55)
> > +    <title>New Tab</title>
> This text should probably be moved out to a DTD file.

Yeah, of course. Done.

> There is similar code throughout this patch that takes a callback argument,
> checks for truthiness, and invokes it if possible. This is the only place
> within the patch that checks the |typeof| the callback. Does |Toolbar_reset|
> sometimes get called with a non-function |aCallback| argument? If not,
> should this check be removed or should the other callback invocations check
> the |typeof| the callback?

Actually this check was here because I directly used this function as an event handler. So when called directly we could pass a callback but this could have also been an event. I changed this so that these functions never get events (as they don't need them) and we can just check for |if (aCallback)|.

> We should probably use a single underscore prefix here, such as
> |this._node._newtabCell|

I'm attaching an object to a DOM node so I figured it might be better to use a double underscore prefix as that's consistent with the code base I know that does the same (SessionStore, Panorama).

> Can you use Object.defineProperty() here?

Good point, using defineProperty() now.

> > +  _setDragData: function Drag__setDragData(aSite, aEvent) {
> Can you change to a single underscore here on the named function?

Done.

> > +    let img = document.createElement("div");
> Is this supposed to be a div or an img? Can you change the variable name to match
> the tag name?

Changed.

> > +const DELAY_REARRANGE = 250;
> Can you please rename this to DELAY_REARRANGE_MS? I prefer having constant numbers
> with units.

Done.

> > +    pinnedSites.forEach(function (aSite) aSite.pin(sites.indexOf(aSite)));
> This file is included from a "use strict"; file. So |this| should be set to the
> passed argument, which I believe is |undefined|. Can you pass a |thisArg| to
> |forEach| so future authors will know what to expect.

Done.

> > +  _hasOverflownPinnedSite:
> Hmm... overflown vs. overflowed... Can we change this to
> |_hasOverflowedPinnedSite| and change the other mentions to use overflowed
> instead? Overflown reminds me too much of airplanes missing runways :(

Changed.

> In this scenario, it looks like the callback will be invoked twice. While no sane
> author would intend to write the code this way, it should be trivial to restrict
> the callback from being invoked twice.

Fixed the Batch class.

(In reply to Blair McBride (:Unfocused) from comment #58)
> Since these files are being #include'd, and the base file has the same
> license, it'd be nice to have the licenses here preprocessed away so you
> don't end up with a file with a dozen licenses - which seems like a waste of
> space.

Done.

> General nit: Capital first letter, full stop at the end.

Fixed.

> Could make this more deliberate and potentially less error prone
> (maintenance-wise) by having the prefix be "_on" or "on", and capitalizing
> the event names (addEventListener is case-insensitive), so you end up
> pointing to functions like _onDragOver.

Yep, that looks better, fixed.

> Since this is used as a global variable (Grid, Animations, etc too), it's
> name should reflect it. "Page" makes it look like you should use |new
> Page()|, which is misleading. gPage or similar would be better.

Renamed all singletons.

> > +let Updater = {
> I wonder if this should be merged with Grid.

I actually like it more separated so that the whole Grid isn't messed up with update code. It's a bit cleaner IMHO.
Attachment #579643 - Attachment is obsolete: true
Attachment #581451 - Flags: review?(bmcbride)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #67)
> >             var blank = !aURI || (aURI == "about:blank");
> > 
> >-            if (blank)
> >+            if (blank || aURI == "about:newtab")
> 
> Can't you just change add this condition to the setting of "blank"? That way
> mBlank will be set correctly too.

The problem is that `blank` is also used for other things that should not apply to about:newtab (e.g. when `blank` is true, we don't bother to stop the about:blank load in anticipation of another document being loaded immediately afterward).

I'm not sure what `mBlank` is used for though.
Yes, I tried setting 'blank' first but that didn't work out well.
Comment on attachment 581428 [details] [diff] [review]
Part 6 - Extend nsPlacesAutoComplete to get more results

discussed on IRC, I think the PlacesAutocomplete overhead is excessive for this task, while it could follow the same code path as Win 7 jumplists and still get a frecency ordered list. Clearing the request for now.
Attachment #581428 - Flags: review?(mak77)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #67)
> Can't you just change add this condition to the setting of "blank"? That way
> mBlank will be set correctly too.

The progress listener sets |mBlank = false| after the first STATE_STOP so that doesn't work either with about:newtab and its images.
Comment on attachment 581451 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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

::: browser/base/content/newtab/animations.js
@@ +42,5 @@
> + */
> +let gAnimations = {
> +  /**
> +   * Fades in the given node from 0 to 1 opacity.
> +   * @param aNode the node to animate

Global nit: Comments describing params/return values should be full sentences too. ie:
* @param aNode The node to animate.

@@ +79,5 @@
> +  _start: function CssAnimations_start() {
> +    let node = this._node;
> +    let name = this._name;
> +    let classes = node.classList;
> +    let callback = this._callback;

You set this._node, this._name, and this._callback in the constructor, but then just pull them out again here (which is only called by the constructor). Should either directly use this._node etc (preferred, IMO), or just pass them as parameters (or merge _start into the constructor, for that matter).

::: browser/base/content/newtab/drop.js
@@ +102,5 @@
> +    let sites = gDropPreview.rearrange(aCell);
> +
> +    // Filter out pinned sites.
> +    let pinnedSites = sites.filter(function (aSite) {
> +      return aSite && aSite.isPinned();

Can aSite ever be null? Seems like it should be guaranteed by rearrange() to always be non-null.

::: browser/base/content/newtab/dropPreview.js
@@ +93,5 @@
> +   * @param aSites the array of sites containing the dragged site
> +   * @param aCell the drop target cell
> +   */
> +  _repositionPinnedSites:
> +    function DropPreview_repositionPinnedSites(aSites, aCell) {

Splitting it like this makes it look like a typo or an inner function.

Reads easier if you split this line like so (even if it means breaking 80 characters, IMO):

_repositionPinnedSites: function DropPreview_repositionPinnedSites(aSites,
                                                                   aCell) {

With aCell lined up with aSites. Couple of other places like this too.

@@ +95,5 @@
> +   */
> +  _repositionPinnedSites:
> +    function DropPreview_repositionPinnedSites(aSites, aCell) {
> +
> +    // collect all pinned sites.

Capital first letter.

@@ +164,5 @@
> +    return range;
> +  },
> +
> +  /**
> +   * Checks if the given array of sites contains a pinned sites that has

"a pinned sites"?

@@ +200,5 @@
> +   */
> +  _repositionOverflowedPinnedSite:
> +    function DropPreview_repositionOverflowedPinnedSite(aSites, aCell) {
> +
> +    // Try to find a lower-prio cell (empty or containing an unpinned site).

s/lower-prio/lower-priority/ ?

@@ +238,5 @@
> +    // Search (beginning with the last site in the grid) for a site that is
> +    // empty or unpinned (an thus lower-priority) and can be pushed out of the
> +    // grid instead of the pinned site.
> +    for (let i = cells.length - 1; i >= 0; i--) {
> +      // the cell that is our drop target is not a good choice

Capital first latter, final full stop.

::: browser/base/content/newtab/grid.js
@@ +112,5 @@
> +  /**
> +   * Locks the grid to block all pointer events.
> +   */
> +  lock: function Grid_lock() {
> +    this.node.setAttribute("locked", "");

Set the value to "true" - makes it look more deliberate whenever someone looks at the generated DOM.

::: browser/base/content/newtab/page.js
@@ +57,5 @@
> +    else
> +      this._setDisabled(true);
> +
> +    // Register the preference observer.
> +    Services.prefs.addObserver(PREF_NEWTAB_ENABLED, this, true);

This should probably be in the JSM (AllPages), so only one observer is ever added. Could cache the value of isEnabled there too, to save getting it from the prefs service every time.

@@ +80,5 @@
> +  /**
> +   * Checks if the page is modified and sets the CSS class accordingly
> +   */
> +  checkIfModified: function Page_checkIfModified() {
> +    let classes = document.body.classList;

This appears to be unused.

@@ +141,5 @@
> +  _setDisabled: function Page_setDisabled(aValue) {
> +    let nodes = [gGrid.node];
> +
> +    ["show", "hide", "reset"].forEach(function (aButton) {
> +      let id = "toolbar-button-" + aButton;

Can you get these via a class instead? If an addon wants to add any buttons there, there's currently no easy way for them to have their buttons observe the disabled state.

::: browser/base/content/newtab/sites.js
@@ +74,5 @@
> +
> +  /**
> +   * The site's parent cell.
> +   */
> +  get cell() this.node.parentNode && this.node.parentNode.__newtabCell,

This deserves some brackets.

::: browser/base/content/newtab/transformations.js
@@ +123,5 @@
> +   *        unfreeze - unfreeze the site after rearranging
> +   *        callback - the callback to call when finished
> +   */
> +  rearrangeSites:
> +    function Transformation_rearrangeSites(aSites, aOptions) {

This linebreak is unneeded (the line is less than 80 characters anyway).

@@ +191,5 @@
> +   */
> +  _hideSite: function Transformation_hideSite(aSite, aCallback) {
> +    gAnimations.fadeOut(aSite.node, function () {
> +      aSite.node.setAttribute("hidden", "");
> +      aCallback();

Need to null-check aCallback here, since rearrangeSites allows for it to be null.
Attachment #581451 - Flags: review?(bmcbride) → review+
Comment on attachment 581436 [details] [diff] [review]
Part 4 - Shared Module

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

::: browser/base/content/NewTabUtils.jsm
@@ +85,5 @@
> +  /**
> +   * Initializes the storage.
> +   */
> +  init: function Storage_init() {
> +    Services.obs.addObserver(this._createPrivateBrowsingObserver(),

Why do you need a function to generate this? Can't Storage just directly implement nsIObserver?
Attachment #581436 - Flags: review?(bmcbride) → review+
Removed two unnecessary about:newtab occurrences that enforced a blank tab title.
Attachment #581432 - Attachment is obsolete: true
Attachment #581570 - Flags: review?(gavin.sharp)
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
(In reply to Blair McBride (:Unfocused) from comment #74)
> Why do you need a function to generate this? Can't Storage just directly
> implement nsIObserver?

I have no idea why I did this. Done, much cleaner now.

(In reply to Marco Bonardo [:mak] from comment #71)
> discussed on IRC, I think the PlacesAutocomplete overhead is excessive for
> this task, while it could follow the same code path as Win 7 jumplists and
> still get a frecency ordered list.

Thanks for your advice! This is a lot better now without the need to extend nsPlacesAutoComplete. REDIRECTS_MODE_TARGET is just awesome (I thought there's now way to get a behavior like this).
Attachment #581428 - Attachment is obsolete: true
Attachment #581436 - Attachment is obsolete: true
Attachment #581579 - Flags: review?(mak77)
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
Moved the preference observer to the JSM.
Attachment #581579 - Attachment is obsolete: true
Attachment #581588 - Flags: review?(mak77)
Attachment #581579 - Flags: review?(mak77)
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
(In reply to Blair McBride (:Unfocused) from comment #73)
> Global nit: Comments describing params/return values should be full
> sentences too.

Fixed.

> > +  _start: function CssAnimations_start() {
> > +    let node = this._node;
> > +    let name = this._name;
> > +    let classes = node.classList;
> > +    let callback = this._callback;
> 
> You set this._node, this._name, and this._callback in the constructor, but
> then just pull them out again here (which is only called by the
> constructor). Should either directly use this._node etc (preferred, IMO), or
> just pass them as parameters (or merge _start into the constructor, for that
> matter).

Fixed by merging the CssAnimations class into gAnimation. The code really didn't justify it being a single class, so it's now a single method instead.

> > +    let sites = gDropPreview.rearrange(aCell);
> > +
> > +    // Filter out pinned sites.
> > +    let pinnedSites = sites.filter(function (aSite) {
> > +      return aSite && aSite.isPinned();
> 
> Can aSite ever be null? Seems like it should be guaranteed by rearrange() to
> always be non-null.

Yes it can, there can be empty cells in the grid which are returned as 'null' by Grid.sites.

> > +  _repositionPinnedSites:
> > +    function DropPreview_repositionPinnedSites(aSites, aCell) {
> 
> Splitting it like this makes it look like a typo or an inner function.
> 
> Reads easier if you split this line like so (even if it means breaking 80
> characters, IMO):
> 
> _repositionPinnedSites: function DropPreview_repositionPinnedSites(aSites,
>                                                                    aCell) {

Splitting lines like this is not uncommon (e.g. in SessionStore, Panorama). Is there a convention to follow for future code?

> > +    this.node.setAttribute("locked", "");
> 
> Set the value to "true" - makes it look more deliberate whenever someone
> looks at the generated DOM.

Fixed (for all state attributes).

> > +    Services.prefs.addObserver(PREF_NEWTAB_ENABLED, this, true);
> 
> This should probably be in the JSM (AllPages), so only one observer is ever
> added. Could cache the value of isEnabled there too, to save getting it from
> the prefs service every time.

Good point, moved to the JSM.

> > +    ["show", "hide", "reset"].forEach(function (aButton) {
> > +      let id = "toolbar-button-" + aButton;
> 
> Can you get these via a class instead? If an addon wants to add any buttons
> there, there's currently no easy way for them to have their buttons observe
> the disabled state.

Gosh, that looks horrible (I should've got some sleep). Fixed by querying the toolbar button class.

> > +  _hideSite: function Transformation_hideSite(aSite, aCallback) {
> > +    gAnimations.fadeOut(aSite.node, function () {
> > +      aSite.node.setAttribute("hidden", "");
> > +      aCallback();
> 
> Need to null-check aCallback here, since rearrangeSites allows for it to be
> null.

Yes, good catch.
Attachment #581451 - Attachment is obsolete: true
Attachment #581594 - Flags: review?(dietrich)
Comment on attachment 581588 [details] [diff] [review]
Part 4 - Shared Module

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

I don't see anything blocking or problematic, just minor comments.

::: browser/base/content/NewTabUtils.jsm
@@ +49,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");
> +
> +// The preference that tells whether this feature is enabled.
> +const PREF_NEWTAB_ENABLED = "browser.newtab.enabled";

I don't know if this was raised before, but the pref should be more like browser.newtabpage.enable imo, newtab is a far more generic concept.

@@ +52,5 @@
> +// The preference that tells whether this feature is enabled.
> +const PREF_NEWTAB_ENABLED = "browser.newtab.enabled";
> +
> +// The maximum number of results we want to retrieve from the places history.
> +const PLACES_RESULTS_LIMIT = 100;

nit: s/places/history/ imo, so "retrieve from history" and HISTORY_RESULTS_LIMIT
Places is just one of the possible history implementations, ideally.

@@ +91,5 @@
> +  /**
> +   * Initializes the storage.
> +   */
> +  init: function Storage_init() {
> +    Services.obs.addObserver(this, "private-browsing-transition-complete", true);

shouldn't this be in the storage lazy getter? why do we have to start listening before we have a storage?

@@ +107,5 @@
> +    try {
> +      value = JSON.parse(this.storage.getItem(aName));
> +    } catch (e) {}
> +
> +    return value || aDefault;

you may avoid the local var by
try {
  return JSON.parse...
} catch(e) {}
return aDefault;

@@ +132,5 @@
> +   * browsing mode changes.
> +   */
> +  observe: function Storage_observe() {
> +    if (gPrivateBrowsing.privateBrowsingEnabled) {
> +      // When switching to private browsing mode we keep the current state

according to your needs, you may rather listen to "private-browsing" and check if the data argument is "enter" or "exit", should be cheaper than checking privateBrowsingEnabled

@@ +183,5 @@
> +  getItem: function PrivateBrowsingStorage_getItem(aName) {
> +    if (aName in this._data)
> +      return this._data[aName];
> +
> +    return null;

I think return this._data[aName] || null; would work and not warn

@@ +253,5 @@
> +   * value changes.
> +   */
> +  observe: function AllPages_observe() {
> +    let enabled = Services.prefs.getBoolPref(PREF_NEWTAB_ENABLED);
> +    this._pages.forEach(function (aPage) aPage.setDisabled(!enabled));

Is there a reason you have setDisabled and not a disabled setter on a page?

@@ +289,5 @@
> +  pin: function PinnedLinks_pin(aLink, aIndex) {
> +    // Clear the link's old position, if any.
> +    this.unpin(aLink);
> +
> +    this.links[aIndex] = aLink;

Are pinned links a fixed list? I mean, this will replace whatever was at this position, rather than making space for a new entry. I assume this is the wanted behavior?

@@ +300,5 @@
> +   */
> +  unpin: function PinnedLinks_unpin(aLink) {
> +    let index = this._indexOfLink(aLink);
> +
> +    if (-1 < index) {

nit: I'd prefer a index != -1 check, I was wondering what this was trying to check
nit: the newline doesn't seem useful.

@@ +331,5 @@
> +  _indexOfLink: function PinnedLinks_indexOfLink(aLink) {
> +    for (let i = 0; i < this.links.length; i++) {
> +      let link = this.links[i];
> +
> +      if (link && link.url == aLink.url)

nit: the newline doesn't seem useful.

@@ +385,5 @@
> +   * Checks whether the list of blocked links is empty.
> +   * @return Whether the list is empty.
> +   */
> +  isEmpty: function BlockedLinks_isEmpty() {
> +    return !Object.keys(this.links).length;

nit: == 0 is usually preferred for length checks

@@ +399,5 @@
> +
> +/**
> + * Singleton that serves as the default link provider for the grid. It queries
> + * the places history to retrieve the most frequently visited sites.
> + */

ditto s/the places//

@@ +421,5 @@
> +    let callback = {
> +      handleResult: function (aResultSet) {
> +        let row;
> +
> +        while (row = aResultSet.getNextRow()) {

You can
for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow())

@@ +470,5 @@
> +
> +    this._provider.getLinks(function (aLinks) {
> +      self._links = aLinks;
> +      aCallback && aCallback();
> +    });

would bind work, instead of self?
this._provider.getLinks((function (aLinks) {}).bind(this));

@@ +478,5 @@
> +   * Gets the current set of links contained in the grid.
> +   * @return The links in the grid.
> +   */
> +  getLinks: function Links_getLinks() {
> +    let pinnedLinks = PinnedLinks.links.concat();

If you are using concat to get a copy of the array, I think the usual way in the codebase is Array.slice(PinnedLinks.links);

@@ +494,5 @@
> +    // Append the remaining links if any.
> +    if (links.length)
> +      pinnedLinks = pinnedLinks.concat(links);
> +
> +    return pinnedLinks;

is there a reason PinnedLinks couldn't handle this internally by making space for new entries and compacting on removal?
Attachment #581588 - Flags: review?(mak77) → review+
Whiteboard: [a11y-ping=davidb]
The two buttons on top right are again transparent on windows 7 with this recent patch on the latest UX tinderbox.
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
(In reply to scrapmachines from comment #80)
> The two buttons on top right are again transparent on windows 7 with this
> recent patch on the latest UX tinderbox.

Sorry, just a stupid typo :( Will update the UX branch in some minutes.
Attachment #581441 - Attachment is obsolete: true
Attachment #581715 - Flags: review?(dao)
Attachment #581441 - Flags: review?(dao)
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #79)
> I don't know if this was raised before, but the pref should be more like
> browser.newtabpage.enable imo, newtab is a far more generic concept.

True, fixed.

> > +  init: function Storage_init() {
> > +    Services.obs.addObserver(this, "private-browsing-transition-complete", true);
> 
> shouldn't this be in the storage lazy getter? why do we have to start
> listening before we have a storage?

Yeah, this can be added lazily, done.

> you may avoid the local var by
> try {
>   return JSON.parse...
> } catch(e) {}
> return aDefault;

I didn't change that because I actually want a default value if getItem() returns null (which is the case when no value is set).

> according to your needs, you may rather listen to "private-browsing" and
> check if the data argument is "enter" or "exit", should be cheaper than
> checking privateBrowsingEnabled

Done.

> > +    let enabled = Services.prefs.getBoolPref(PREF_NEWTAB_ENABLED);
> > +    this._pages.forEach(function (aPage) aPage.setDisabled(!enabled));
> 
> Is there a reason you have setDisabled and not a disabled setter on a page?

There was a Page.isEnabled() so I turned both of them into get/set enabled().

> Are pinned links a fixed list? I mean, this will replace whatever was at
> this position, rather than making space for a new entry. I assume this is
> the wanted behavior?

Yes, they're a fixed list, additional to the list of links. Replacing the old entry is what we want.

> > +    // Append the remaining links if any.
> > +    if (links.length)
> > +      pinnedLinks = pinnedLinks.concat(links);
> > +
> > +    return pinnedLinks;
> 
> is there a reason PinnedLinks couldn't handle this internally by making
> space for new entries and compacting on removal?

This is the place where the list of pinned links is merged with the current list of links (excluding blocked links). So this is nothing that can be handled by PinnedLinks.
Attachment #581588 - Attachment is obsolete: true
Combined Page.isEnabled() and Page.setDisabled() into get/set enabled().
Attachment #581427 - Attachment is obsolete: true
Attachment #581730 - Flags: review?(dietrich)
Attachment #581427 - Flags: review?(dietrich)
Changed the preference name to browser.newtabpage.enabled.
Attachment #581570 - Attachment is obsolete: true
Attachment #581731 - Flags: review?(gavin.sharp)
Attachment #581570 - Flags: review?(gavin.sharp)
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
Combined Page.isEnabled() and Page.setDisabled() into get/set enabled(). Changed the preference name to browser.newtabpage.enabled.
Attachment #581594 - Attachment is obsolete: true
Attachment #581733 - Flags: review?(dietrich)
Attachment #581594 - Flags: review?(dietrich)
Comment on attachment 581731 [details] [diff] [review]
Part 3 - about:newtab integration

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// Enable the New Tab Page (about:newtab)
>+pref("browser.newtabpage.enabled", true);

Where does this pref actually get read? I see an observer in the module in the other patch, but it just sets "enabled" properties on elements in _pages, and I don't see where this has any effect. Shouldn't toggling this pref result in us reverting to using about:blank for new tabs?

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

>-                      "chrome,all,dialog=no", "about:blank");
>+                      "chrome,all,dialog=no", "about:newtab");

>-  gBrowser.loadOneTab("about:blank", {inBackground: false});
>+  gBrowser.loadOneTab("about:newtab", {inBackground: false});

How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in browser.js, and using it in these two places as well as the addTab call in tabbrowser.xml? If you want the pref to impact this, you could also make it a lazy getter.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-                if (!this.mBlank) {
>+                if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") {

What was wrong with my suggestion of changing the definition of "blank" (and therefore mBlank) to include about:newtab?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86)
> Where does this pref actually get read? I see an observer in the module in
> the other patch, but it just sets "enabled" properties on elements in
> _pages, and I don't see where this has any effect. Shouldn't toggling this
> pref result in us reverting to using about:blank for new tabs?

It's accessed in page.js (part 1). toolbar.js (part 1) sets this pref so that all pages get notified of this change.

> >-                      "chrome,all,dialog=no", "about:blank");
> >+                      "chrome,all,dialog=no", "about:newtab");
> 
> >-  gBrowser.loadOneTab("about:blank", {inBackground: false});
> >+  gBrowser.loadOneTab("about:newtab", {inBackground: false});
> 
> How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in
> browser.js, and using it in these two places as well as the addTab call in
> tabbrowser.xml? If you want the pref to impact this, you could also make it
> a lazy getter.

Will do that.

> >-                if (!this.mBlank) {
> >+                if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") {
> 
> What was wrong with my suggestion of changing the definition of "blank" (and
> therefore mBlank) to include about:newtab?

As Frank said:

(In reply to Frank Yan (:fryn) from comment #69)
> The problem is that `blank` is also used for other things that should not
> apply to about:newtab (e.g. when `blank` is true, we don't bother to stop
> the about:blank load in anticipation of another document being loaded
> immediately afterward).

The problem with mBlank is that the progress listener sets |mBlank = false| after the first STATE_STOP so that doesn't work either with about:newtab and its images (the throbber will be turned on with the first resource that is loaded).
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86)
> Shouldn't toggling this
> pref result in us reverting to using about:blank for new tabs?

No, we'll have a blank page without the grid that has a tiny icon in the upper right corner to easily re-enable the 'New Tab Page'. The whole page won't be initialized (but of course parsed) if it's pref'ed off.
(In reply to Tim Taubert [:ttaubert] from comment #88)
> (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #86)
> > Shouldn't toggling this
> > pref result in us reverting to using about:blank for new tabs?
> 
> No, we'll have a blank page without the grid that has a tiny icon in the
> upper right corner to easily re-enable the 'New Tab Page'. The whole page
> won't be initialized (but of course parsed) if it's pref'ed off.

Ah, I somewhat misunderstood this too. I guess the following comment in the code should better explain that it simply toggles the content of the page, not the usage of about:newtab vs. about:blank.

>+// Enable the New Tab Page (about:newtab)
>+pref("browser.newtabpage.enabled", true);
(In reply to Frank Yan (:fryn) from comment #89)
> Ah, I somewhat misunderstood this too. I guess the following comment in the
> code should better explain that it simply toggles the content of the page,
> not the usage of about:newtab vs. about:blank.

Done.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86)
> How about adding a |const BROWSER_NEW_TAB_URL = "about:newtab";| in
> browser.js, and using it in these two places as well as the addTab call in
> tabbrowser.xml?

Done.

> >-                if (!this.mBlank) {
> >+                if (!this.mBlank && this.mBrowser.userTypedValue != "about:newtab") {
> 
> What was wrong with my suggestion of changing the definition of "blank" (and
> therefore mBlank) to include about:newtab?

I didn't change this, yet. Is there a better way of doing this? Should we add another argument? Should we prevent setting mBlank to false?
Attachment #581731 - Attachment is obsolete: true
Attachment #581760 - Flags: review?(gavin.sharp)
Attachment #581731 - Flags: review?(gavin.sharp)
(In reply to Tim Taubert [:ttaubert] from comment #78)
> Splitting lines like this is not uncommon (e.g. in SessionStore, Panorama).
> Is there a convention to follow for future code?

Maybe it's just uncommon/not used in the code I usually touch. I don't think we have any formal convention for that... I just find one style easier to read/understand-at-a-glance than the other.
Can you set BROWSER_NEW_TAB_URL to about:blank depending on the pref?
How about introducing a second pref? So there's one pref to toggle this feature (temporary) via the button in about:newtab. The second pref (not exposed via UI) switches back to about:blank like you said to satisfy people not wanting it at all. That should of course prevent the whole module from loading and initializing.
I'm thinking the primary use case for reverting to about:blank would be to turn off the feature on aurora if needed. Also, if that pref is a string, add-ons can utilize it.
Ok, that sounds like a good idea. So we'll just introduce a new pref for a new tab's url. So it's easy to pref it off on aurora and extensible.
Comment on attachment 581715 [details] [diff] [review]
Part 2 - Assets / CSS / Images

Please move aesthetic / non-functional styling to the theme css files.

Get rid of the descendent selectors; for body[dir=rtl], follow bug 700036.

Break the line after each comma in selectors.

Why are you using -moz-animation-* instead of -moz-transition?
Attachment #581715 - Flags: review?(dao) → review-
Introduced the pref 'browser.newtab.url'. Discovered some additional places where we should use BROWSER_NEW_TAB_URL instead of 'about:blank' to get a consistent new tab behavior.
Attachment #581760 - Attachment is obsolete: true
Attachment #582058 - Flags: review?(gavin.sharp)
Attachment #581760 - Flags: review?(gavin.sharp)
(In reply to Tim Taubert [:ttaubert] from comment #90)
> > What was wrong with my suggestion of changing the definition of "blank" (and
> > therefore mBlank) to include about:newtab?
> 
> I didn't change this, yet. Is there a better way of doing this? Should we
> add another argument? Should we prevent setting mBlank to false?

I was suggesting changing the value of the "var blank" in addTab() to be true if the URL is about:newtab. That variable gets passed to mTabProgressListener as mBlank, so changing it there should make your other tabbrowser change unnecessary. I haven't looked closely into what other side effects that may have, but it seems to make sense conceptually.
Comment on attachment 582058 [details] [diff] [review]
Part 3 - about:newtab integration

>+__defineGetter__("BROWSER_NEW_TAB_URL", function() {
>+  return gPrefService.getCharPref("browser.newtab.url", "about:blank");
>+});

I don't think the pref needs to be live. If we wanted to support this, an observer would be the way to go.
Oops. Yeah, I also don't think it needs to be live. I actually meant to implement a lazy getter...
Limi asked me to remove the history dropmarker from the URL bar as a part of this patch because that's needed to avoid UI redundancy. Removing the history dropmarker was one side-goal of the New Tab Page.
Attachment #582241 - Flags: review?(dao)
(In reply to Tim Taubert [:ttaubert] from comment #101)
> Created attachment 582241 [details] [diff] [review]
> Part 6 - Remove the history dropmarker from the URL bar

This is again an unnecessary change. Removing something which has been present for years, without any major gains, appears to be meaningless.
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
Removed gAnimation/animations.js because that's actually legacy code and we can achieve the same with CSS transformations. Rewrote the New Tab Page to be a XUL file, but using the HTML namespace as default. That enables us to use the :-moz-local-dir(rtl) selector and avoid descendant selectors.
Attachment #581733 - Attachment is obsolete: true
Attachment #582247 - Flags: review?(dietrich)
Attachment #581733 - Flags: review?(dietrich)
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #96)
> Please move aesthetic / non-functional styling to the theme css files.

Done.

> Get rid of the descendent selectors; for body[dir=rtl], follow bug 700036.

Done.

> Why are you using -moz-animation-* instead of -moz-transition?

That was some legacy code. I removed the CSS animations and switched to using CSS transitions.
Attachment #581715 - Attachment is obsolete: true
Attachment #582251 - Flags: review?(dao)
I have found a bug with the version of this patch that is pushed to the UX branch. 

STR:
1. Maximize the window
2. Open a new tab
3. Try to tear off the tab to make it a new window.

Actual results: The tab will not tear off. Even more odd, is that if the tab is dragged on top of the grid of sites, the grid will make space for the tab and if the tab is dropped on the grid then the "New Tab page" tab will become part of the grid.

Expected results: The tab is torn off and creates a new window.
(In reply to Jared Wein [:jwein and :jaws] from comment #105)
> I have found a bug with the version of this patch that is pushed to the UX
> branch. 
> 
> STR:
> 1. Maximize the window
> 2. Open a new tab
> 3. Try to tear off the tab to make it a new window.
> 
> Actual results: The tab will not tear off. Even more odd, is that if the tab
> is dragged on top of the grid of sites, the grid will make space for the tab
> and if the tab is dropped on the grid then the "New Tab page" tab will
> become part of the grid.
> 
> Expected results: The tab is torn off and creates a new window.

Works fine for me on build with changeset f0f24e6d7587 .
The New Tab tears off well and works like any other torn off tab. Although the New Tab can be added in the New Tab page's Tiles which then becomes a blank Tile with no link. Just a white Rectangle. This should not happen.
New Tab page should not allow drop of New Tab page itself on the Tiles.
Attached patch Part 1 - XHTML Page / Scripts (obsolete) — Splinter Review
(In reply to Jared Wein [:jwein and :jaws] from comment #105)
> Actual results: The tab will not tear off. Even more odd, is that if the tab
> is dragged on top of the grid of sites, the grid will make space for the tab
> and if the tab is dropped on the grid then the "New Tab page" tab will
> become part of the grid.

Thanks for reporting this! I didn't check the dataTransfer type when accepting drops, fixed. I'll update the patch on the UX branch later today.
Attachment #582247 - Attachment is obsolete: true
Attachment #582663 - Flags: review?(dietrich)
Attachment #582247 - Flags: review?(dietrich)
Fixed a bug in isTabEmpty(). BROWSER_NEW_TAB_URL is now a lazy getter.
Attachment #582058 - Attachment is obsolete: true
Attachment #582901 - Flags: review?(gavin.sharp)
Attachment #582058 - Flags: review?(gavin.sharp)
Comment on attachment 582901 [details] [diff] [review]
Part 3 - about:newtab integration

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

> function isTabEmpty(aTab) {

>-         !browser.contentDocument.body.hasChildNodes() &&
>-         !aTab.hasAttribute("busy");
>+         (!body || !body.hasChildNodes()) && !aTab.hasAttribute("busy");

Were you actually hitting body being null here, or are you just being cautious?

nit: leaving the hasAttribute check on its own line makes this easier to read, IMO.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-                if (!this.mBlank) {
>+                if (!this.mBlank && this.mBrowser.userTypedValue != BROWSER_NEW_TAB_URL) {

I still think we should investigate modifying mBlank to avoid the need for a specific userTypedValue check here, but we can do that in a followup bug I guess.
Attachment #582901 - Flags: review?(gavin.sharp) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #109)
> Were you actually hitting body being null here, or are you just being
> cautious?

I actually hit that while testing locally. The New Tab Page seems to have no document.body so that threw here and I added a check for that.

> I still think we should investigate modifying mBlank to avoid the need for a
> specific userTypedValue check here, but we can do that in a followup bug I
> guess.

Ok, I'll file a bug.
Comment on attachment 582663 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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

overall comment: i like that things are broken up into separate include files. however the fact that the split-out code relies on globals (gGid, gDrag, etc) is confusing, since it's not clear where those are defined and managed.

i'd prefer that those globals be passed into object/method scope via ctor, individually or via a configuration object, rather than having the split-out code use them directly. however, that can be done as a followup refactoring since it's mostly just be variable renaming.

r=me with the noted changes made, and questions answered.

::: browser/base/content/newtab/cells.js
@@ +41,5 @@
> + * This class represents a cell contained in the grid.
> + */
> +function Cell(aNode) {
> +  this._node = aNode;
> +  this._node.__newtabCell = this;

what's the double underscore about?

@@ +61,5 @@
> +  /**
> +   * The cell's offset in the grid.
> +   */
> +  get index() {
> +    let index = gGrid.cells.indexOf(this);

can you pass the grid into the ctor instead of using a global?

::: browser/base/content/newtab/helper.js
@@ +38,5 @@
> +#endif
> +
> +/**
> + * This class makes it easy to wait until a batch of callbacks has finished.
> + */

kind of weird that this is called helper.js but only has a single api for batching. maybe should call it batch.js?

wrt the api itself:

* should add an example
* seems confusing that it doesn't handle the contents of the batch, but maybe that's just me

::: browser/base/content/newtab/sites.js
@@ +40,5 @@
> +/**
> + * This class represents a site that is contained in a cell and can be pinned,
> + * moved around or deleted.
> + */
> +function Site(aNode, aLink) {

nit: rename file to site.js?

@@ +80,5 @@
> +    return parentNode && parentNode.__newtabCell;
> +  },
> +
> +  /**
> +   * Pins the site on it's current or a given index.

nit: no apostrophe

@@ +168,5 @@
> +   * Adds event handlers for the site and its buttons.
> +   */
> +  _addEventHandlers: function Site_addEventHandlers() {
> +    // Register drag-and-drop event handlers.
> +    ["DragStart", /*"Drag",*/ "DragEnd"].forEach(function (aType) {

why the commented out bit?

::: browser/base/content/newtab/toolbar.js
@@ +63,5 @@
> +  /**
> +   * Enables the 'New Tab Page' feature.
> +   */
> +  show: function Toolbar_show() {
> +    Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, true);

i'd prefer have the enable/disable implementation centralized somewhere (page.js?) and then toggled via an api. for example, this code would call page.enable/disable() or toggle a boolean property there.
Attachment #582663 - Flags: review?(dietrich) → review+
i see the grid on new tab also when browser.newtabpage.enabled is false
(In reply to Tim Taubert [:ttaubert] from comment #101)
> Part 6 - Remove the history dropmarker from the URL bar

We should move this to another bug.
Attachment #582241 - Attachment is obsolete: true
Attachment #582241 - Flags: review?(dao)
Sorry, have to ask for review again. BrowserOpenTab() recently changed and this revealed that we need to fix utilityOverlay.js as well.
Attachment #582901 - Attachment is obsolete: true
Attachment #583448 - Flags: review?(gavin.sharp)
Blocks: 712602
Comment on attachment 583448 [details] [diff] [review]
Part 3 - about:newtab integration

Fixing some further integration bugs reported by the UX team (blinking 'about:newtab' in the location bar and similar stuff). Cancelling review for now.
Attachment #583448 - Flags: review?(gavin.sharp)
The new tab page needs scrollbars, both vertical and horizontal. Another option could be to resize thumbnails as we do in Panorama.
Comment on attachment 582251 [details] [diff] [review]
Part 2 - Assets / CSS / Images

>+++ b/browser/base/content/newtab/newTab.css

>+body {
>+  position: relative;

>+#toolbar {
>+  position: absolute;

If you use "fixed" here, you can remove "position: relative" from the body.

>+  top: 8px;
>+  right: 8px;

should be in themes

>+  list-style-type: none;

What is this doing here?

>+.site {

>+  outline: none;
>+  -moz-transition: 200ms ease-out;
>+  -moz-transition-property: top, left, box-shadow, opacity;

This looks like it should be in themes as well. You don't seem to set box-shadow here, for instance.

>+.site:hover .site-img,
>+.site[ontop] .site-img {

>+.site:hover:not([frozen]) .site-strip {

child selector

>+.drag-element {
>+  width: 1px;
>+  height: 1px;
>+  background-color: #fff;
>+  opacity: 0.01;
>+}

More theme stuff? Not sure what this is doing, exactly. Maybe add a comment.

>diff --git a/browser/base/content/newtab/strip.png b/browser/base/content/newtab/strip.png

belongs in themes

>--- /dev/null
>+++ b/browser/locales/en-US/chrome/browser/newTab.dtd
>@@ -0,0 +1,7 @@
>+<!ENTITY % brandDTD
>+    SYSTEM "chrome://branding/locale/brand.dtd">
>+  %brandDTD;
>+
>+<!-- These strings are used in the about:newtab page -->
>+
>+<!ENTITY newtab.pageTitle "New Tab">

I think a couple of things on this page need tooltips.

Speaking of which, has a11y been reviewed?

Oh, and yes, some sensible overflow behavior would be nice.
Attachment #582251 - Flags: review?(dao)
1) Introduced isNewTabURL() to get rid of all the recurring (url == 'about:blank' || url == BROWSER_NEW_TAB_URL) checks.

2) Some a11y tests do crazy stuff with the tabbrowser and yield that isNewTabURL() isn't defined so I moved the method to gBrowser.isNewTabURL() and replaced BROWSER_NEW_TAB_URL with gBrowser.newTabURL instead of having them defined in browser.js.

3) After the initial 'about:blank' load was stopped the patch now re-sets mBlank=true because that has been set to false by STATUS_STOP when tabbrowser calls browser.stop() before loading the actual URI. So we can now re-use the 'blank' variable for the mTabProgressListener.

4) Replaced 'about:blank' occurrences in browser-places.js and browser-fullZoom.js with gBrowser.isNewTabURL().
Attachment #583448 - Attachment is obsolete: true
Attachment #583802 - Flags: review?(gavin.sharp)
Comment on attachment 583802 [details] [diff] [review]
Part 3 - about:newtab integration

utilityOverlay.js can't always access gBrowser and it's not always loaded with browser.js either, which means that BROWSER_NEW_TAB_URL / isNewTabURL() need to be defined there.

Please don't let those crazy a11y tests dictate the implementation. You can define dummy functions there or let them include utilityOverlay.js or do whatever local hack it takes to get them pass.
Attachment #583802 - Flags: review?(gavin.sharp) → review-
Is it okay to define isNewTabURL() and BROWSER_NEW_TAB_URL in utilityOverlay.js? Can tabbrowser.xml assume that this is loaded? Does the tabbrowser need to define its own functions to stay an independent XUL element or does it already have dependencies like this?
(In reply to Tim Taubert [:ttaubert] from comment #120)
> Is it okay to define isNewTabURL() and BROWSER_NEW_TAB_URL in
> utilityOverlay.js? Can tabbrowser.xml assume that this is loaded?

Yes.

> Does the
> tabbrowser need to define its own functions to stay an independent XUL
> element or does it already have dependencies like this?

It already has similar dependencies and we had to fix up these tests before.
Moved BROWSER_NEW_TAB_URL and isNewTabURL() to utilityOverlay.js. Fixed a11y tests by including utilityOverlay.js.
Attachment #583802 - Attachment is obsolete: true
Attachment #583972 - Flags: review?(gavin.sharp)
Comment on attachment 583972 [details] [diff] [review]
Part 3 - about:newtab integration

>+  return Services.prefs.getCharPref("browser.newtab.url", "about:blank");

getCharPref doesn't take a second argument.
Devs -- you might like to have a look at bug 713461 for more ideas.
(In reply to Dão Gottwald [:dao] from comment #123)
> >+  return Services.prefs.getCharPref("browser.newtab.url", "about:blank");
> getCharPref doesn't take a second argument.

Sorry, expected a default argument. Fixed.
Attachment #583972 - Attachment is obsolete: true
Attachment #584434 - Flags: review?(gavin.sharp)
Attachment #583972 - Flags: review?(gavin.sharp)
Fixed a test-only mem leak.
Attachment #581730 - Attachment is obsolete: true
Attachment #584436 - Flags: review?(dietrich)
Attachment #581730 - Flags: review?(dietrich)
Comment on attachment 584436 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

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

::: browser/base/content/test/newtab/Makefile.in
@@ +44,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +include $(topsrcdir)/config/rules.mk
> +
> +_BROWSER_FILES = \
> +                 browser_newtab_block.js \

nit: these should line up with something.

::: browser/base/content/test/newtab/head.js
@@ +228,5 @@
> +
> +/**
> + * Simulates a drop and drop operation.
> + * @param aDropTarget the cell that is the drop target
> + * @param aDragSource the cell that contains the dragged site (optional)

are these nsISomethings? please add actual type to this and to pin/blockCell as well.
Attachment #584436 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #128)
> > +_BROWSER_FILES = \
> > +                 browser_newtab_block.js \
> 
> nit: these should line up with something.

Fixed.

> > + * Simulates a drop and drop operation.
> > + * @param aDropTarget the cell that is the drop target
> > + * @param aDragSource the cell that contains the dragged site (optional)
> 
> are these nsISomethings? please add actual type to this and to pin/blockCell
> as well.

These are objects (instanceof Cell) custom to the new tab page.
Attachment #584436 - Attachment is obsolete: true
Attached patch Part 1 - XUL Page / Scripts (obsolete) — Splinter Review
(In reply to Dietrich Ayala (:dietrich) from comment #111)
> i'd prefer that those globals be passed into object/method scope via ctor,
> individually or via a configuration object, rather than having the split-out
> code use them directly. however, that can be done as a followup refactoring
> since it's mostly just be variable renaming.

Ok, I'll file a follow-up bug for this.

> > +function Cell(aNode) {
> > +  this._node = aNode;
> > +  this._node.__newtabCell = this;
> 
> what's the double underscore about?

I use underscores when attaching JS objects to DOM nodes. That's a convention we use in Panorama (and SS iirc) as well.

> > +  get index() {
> > +    let index = gGrid.cells.indexOf(this);
> 
> can you pass the grid into the ctor instead of using a global?

Done.

> kind of weird that this is called helper.js but only has a single api for
> batching. maybe should call it batch.js?

Renamed.

> wrt the api itself:
> 
> * should add an example

Added.

> * seems confusing that it doesn't handle the contents of the batch, but
> maybe that's just me

What exactly do you mean with "contents"? Basically this class just handles a collection of callbacks.

> > +function Site(aNode, aLink) {
> 
> nit: rename file to site.js?

I'm not totally objecting this but I as the files are named 'sites.js' and 'cells.js' this says that these are not singletons but there will be many of them created.

> > +  _addEventHandlers: function Site_addEventHandlers() {
> > +    // Register drag-and-drop event handlers.
> > +    ["DragStart", /*"Drag",*/ "DragEnd"].forEach(function (aType) {
> 
> why the commented out bit?

This is related to bug 505521. Once that is implemented we could use the coordinates from the drag event. For the time being we need to use a workaround (which I noted in some comments) and therefore I commented it out.

> > +  show: function Toolbar_show() {
> > +    Services.prefs.setBoolPref(PREF_NEWTAB_ENABLED, true);
> 
> i'd prefer have the enable/disable implementation centralized somewhere
> (page.js?) and then toggled via an api. for example, this code would call
> page.enable/disable() or toggle a boolean property there.

Yeah that's a good point. I moved all the pref toggling and reading to the JSM. The AllPages object does now implement a 'enabled' getter/setter.
Attachment #582663 - Attachment is obsolete: true
Attachment #585394 - Flags: review?(dietrich)
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
Asking for review wrt to the changes of the AllPages object.
Attachment #581729 - Attachment is obsolete: true
Attachment #585395 - Flags: review?(dietrich)
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #117)
> >+body {
> >+  position: relative;
> 
> >+#toolbar {
> >+  position: absolute;
> 
> If you use "fixed" here, you can remove "position: relative" from the body.

Because of the HTML/XUL mix here 'position:absolute' doesn't really work for elements without the body having 'position:relative'. This also breaks drag and drop completely. Maybe it's because the layout engine doesn't really find a root element to position to?

> >+  top: 8px;
> >+  right: 8px;
> 
> should be in themes

Moved.

> >+  list-style-type: none;
> 
> What is this doing here?

The grid with its sites/thumbnails is a list (ul/li). Without this some ugly bullets appear.

> >+.site {
> 
> >+  outline: none;
> >+  -moz-transition: 200ms ease-out;
> >+  -moz-transition-property: top, left, box-shadow, opacity;
> 
> This looks like it should be in themes as well. You don't seem to set
> box-shadow here, for instance.

Moved.

> >+.site:hover .site-img,
> >+.site[ontop] .site-img {
> 
> >+.site:hover:not([frozen]) .site-strip {
> 
> child selector

Done.

> >+.drag-element {
> >+  width: 1px;
> >+  height: 1px;
> >+  background-color: #fff;
> >+  opacity: 0.01;
> >+}
> 
> More theme stuff? Not sure what this is doing, exactly. Maybe add a comment.

Added a comment.

> >diff --git a/browser/base/content/newtab/strip.png b/browser/base/content/newtab/strip.png
> 
> belongs in themes

Moved.

> I think a couple of things on this page need tooltips.

Added tooltips for all buttons.

> Speaking of which, has a11y been reviewed?

No, we want to do this in a follow-up bug (a11y and ARIA). This whole feature will land pref'ed off until we think it's 'shippable' (i.e. all important follow-up bugs are fixed).

> Oh, and yes, some sensible overflow behavior would be nice.

Indeed, that unfortunately got lost with the XHTML -> XUL transition... Fixed.
Attachment #582251 - Attachment is obsolete: true
Attachment #585398 - Flags: review?(dao)
(In reply to Tim Taubert [:ttaubert] from comment #130)
> > > +function Cell(aNode) {
> > > +  this._node = aNode;
> > > +  this._node.__newtabCell = this;
> > 
> > what's the double underscore about?
> 
> I use underscores when attaching JS objects to DOM nodes. That's a
> convention we use in Panorama (and SS iirc) as well.

We usually use two underscores when tacking data to external objects that your code doesn't really control. For objects you own, a single underscore suffices.
Attached patch Part 1 - XUL Page / Scripts (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #133)
> We usually use two underscores when tacking data to external objects that
> your code doesn't really control. For objects you own, a single underscore
> suffices.

Ok, thanks for clearing this up! I replaced the double underscores with single ones.
Attachment #585394 - Attachment is obsolete: true
Attachment #585454 - Flags: review?(dietrich)
Attachment #585394 - Flags: review?(dietrich)
Comment on attachment 585454 [details] [diff] [review]
Part 1 - XUL Page / Scripts

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

mostly just naming and comment changes for clarity. r=me with these fixes, given that there aren't any major changes involved.

::: browser/base/content/newtab/cells.js
@@ +37,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +#endif
> +
> +/**
> + * This class represents a cell contained in the grid.

should clarify that it manages the dom node for the cell, not the actual cell content (ie, the Site).

might also want to note that it's basically a read-only class. all manipulation of both position and content happens elsewhere.

@@ +46,5 @@
> +  this._node._newtabCell = this;
> +
> +  // Register drag-and-drop event handlers.
> +  ["DragEnter", "DragOver", "DragExit", "Drop"].forEach(function (aType) {
> +    let method = "_on" + aType;

why are these prefixed with underscore? they're called from external code, so aren't really private.

::: browser/base/content/newtab/drag.js
@@ +37,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +#endif
> +
> +/**
> + * This singleton implements site dragging functionality.

should this singleton work at the level of cells, not sites? not sure why this code needs to be aware of a cell's contents - looking at it, it's all about the actual container itself.

drop.js handles cells in the drop code, but sites in the drag code. can this be made consistent, or is that not possible for some reason?

::: browser/base/content/newtab/grid.js
@@ +82,5 @@
> +
> +  /**
> +   * Creates a new site in the grid.
> +   * @param aLink The new site's link.
> +   * @param aCell The new site's parent cell.

when you say 'parent', you mean it's the cell that the site will be created in, right? maybe 'containing' would be better than 'parent'? parent connotes the ability to have multiple children.

where does the cell come from?

what happens as a result of this? is an existing cell pushed off the grid? should document the side-effects.

::: browser/base/content/newtab/newTab.js
@@ +43,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Geometry.jsm");
> +Cu.import("resource:///modules/PageThumbs.jsm");
> +Cu.import("resource:///modules/NewTabUtils.jsm");

worth lazy-loading these?

::: browser/base/content/newtab/page.js
@@ +87,5 @@
> +
> +  /**
> +   * Checks if the page is modified and sets the CSS class accordingly
> +   */
> +  checkIfModified: function Page_checkIfModified() {

the method name doesn't indicate any side-effects. this doesn't just check, it sets a value as well. maybe updateModifiedFlag or something ungainly like that?

@@ +116,5 @@
> +
> +    // Workaround to prevent a delay on MacOSX due to a slow drop animation.
> +    let doc = document.documentElement;
> +    doc.addEventListener("dragover", this._onDragOver, false);
> +    doc.addEventListener("drop", this._onDrop, false);

these aren't really private, so shouldn't be underscore prefixed.

::: browser/base/content/newtab/transformations.js
@@ +37,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +#endif
> +
> +/**
> + * This singleton allows to transform the grid by moving sites around.

should say whether it works on sites, cells, site-nodes or cell-nodes, or a mix of the above.

::: browser/base/content/newtab/updater.js
@@ +79,5 @@
> +   * their positions).
> +   * @param aLinks The array of links contained in the current grid.
> +   * @return The remaining sites.
> +   */
> +  _findRemainingSites: function Updater_findRemainingSites(aLinks) {

can you clarify the name and comment to say what these are remaining from?
what's the datatype of aLinks?
what's the datatype of the return value?

@@ +111,5 @@
> +   * @param aSites The array of sites to move.
> +   */
> +  _moveSiteNodes: function Updater_moveSiteNodes(aSites) {
> +    let cells = gGrid.cells;
> +    let sites = aSites.slice(0, cells.length);

imagine i'm a dev looking at this code for the first time, trying to fix a bug or add a feature... why does this need to happen? can you add some more commentary to explain the context of this call?
Attachment #585454 - Flags: review?(dietrich) → review+
Comment on attachment 585395 [details] [diff] [review]
Part 4 - Shared Module

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

f+. let's get these fixes done and questions answered, then i'll do another pass.

::: browser/modules/NewTabUtils.jsm
@@ +54,5 @@
> +
> +// The maximum number of results we want to retrieve from history.
> +const HISTORY_RESULTS_LIMIT = 100;
> +
> +// Define some lazyily loaded services.

typo

@@ +62,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager",
> +  "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gStorageManager",
> +  "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager");

move all these lazy getters to services.jsm? (iirc pb is already there)

@@ +92,5 @@
> +      if (gPrivateBrowsing.privateBrowsingEnabled)
> +        this._storage = new PrivateBrowsingStorage(this._storage);
> +
> +      // Register an observer to listen for private browsing mode changes.
> +      if (!this._observing) {

since this whole code block will only ever be called once (because _storage gets set above), this is unnecessary right?

also, instead of having these |if| blocks and saving _storage and _observing, could instead memo-ize the whole getter, eg:

get storage() {
  let storage = ...;
  this.__defineGetter__('storage', storage);
}

update: nm, i see below in the pb listener that you're blowing away the private vars. i don't like how this is all set up, seems more complex that it should be. that said, i don't see a much clearer way at the moment :(

@@ +122,5 @@
> +   * Sets the storage value for a given key.
> +   * @param aName The storage key.
> +   * @param aValue The value to set.
> +   */
> +  set: function Storage_set(aName, aValue) {

any kind of key validation need to be done here?

@@ +210,5 @@
> +
> +/**
> + * Singleton that serves as a registry for all open 'New Tab Page's.
> + */
> +let AllPages = {

thought for a followup - any reason to not just create a single hidden page, and swap it into view as needed, the way that we do for jetpack panel/widget content?

@@ +440,5 @@
> +  /**
> +   * Gets the current set of links delivered by this provider.
> +   * @param aCallback The callback to call when the links have been retrieved.
> +   */
> +  getLinks: function PlacesProvider_getLinks(aCallback) {

should document the datatype of what's passed to aCallback. should also document what you mean by 'link' in the Links singleton below.
Attachment #585395 - Flags: review?(dietrich) → feedback+
(In reply to Dietrich Ayala (:dietrich) from comment #136)
> @@ +210,5 @@
> > +
> > +/**
> > + * Singleton that serves as a registry for all open 'New Tab Page's.
> > + */
> > +let AllPages = {
> 
> thought for a followup - any reason to not just create a single hidden page,
> and swap it into view as needed, the way that we do for jetpack panel/widget
> content?

Hm, related: bug 715612. Imagine when I restore my 364 tab session. We should look into not actually doing anything for hidden tabs. Could either show new-tab-page content on-demand in those cases, or just fix it as above.
Not sure how easy it would be to swap a single page to the actively shown tab. We can't just swap the docShells because that would include the whole tab's history.

Restoring your big session shouldn't be a problem. SS creates 'about:blank' tabs (not about:newtab) and then waits for them to actually be restored.
Attached patch Part 4 - Shared Module (obsolete) — Splinter Review
(In reply to Dietrich Ayala (:dietrich) from comment #136)
> > +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager",
> > +  "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");
> > +
> > +XPCOMUtils.defineLazyServiceGetter(this, "gStorageManager",
> > +  "@mozilla.org/dom/storagemanager;1", "nsIDOMStorageManager");
> 
> move all these lazy getters to services.jsm? (iirc pb is already there)

Moved.

> > +      if (gPrivateBrowsing.privateBrowsingEnabled)
> > +        this._storage = new PrivateBrowsingStorage(this._storage);
> > +
> > +      // Register an observer to listen for private browsing mode changes.
> > +      if (!this._observing) {
> 
> since this whole code block will only ever be called once (because _storage
> gets set above), this is unnecessary right?
> 
> also, instead of having these |if| blocks and saving _storage and
> _observing, could instead memo-ize the whole getter, eg:
> 
> get storage() {
>   let storage = ...;
>   this.__defineGetter__('storage', storage);
> }
> 
> update: nm, i see below in the pb listener that you're blowing away the
> private vars. i don't like how this is all set up, seems more complex that
> it should be. that said, i don't see a much clearer way at the moment :(

I split it into .domStorage and .currentStorage. Looks a bit cleaner now IMO.

> > +   * Sets the storage value for a given key.
> > +   * @param aName The storage key.
> > +   * @param aValue The value to set.
> > +   */
> > +  set: function Storage_set(aName, aValue) {
> 
> any kind of key validation need to be done here?

The key must be a string but as this internal API is only used by the module itself, should we really check its type? I added that we're expecting a string here in the @param comment.

> > +/**
> > + * Singleton that serves as a registry for all open 'New Tab Page's.
> > + */
> > +let AllPages = {
> 
> thought for a followup - any reason to not just create a single hidden page,
> and swap it into view as needed, the way that we do for jetpack panel/widget
> content?

Please see comment #138.

> > +  /**
> > +   * Gets the current set of links delivered by this provider.
> > +   * @param aCallback The callback to call when the links have been retrieved.
> > +   */
> > +  getLinks: function PlacesProvider_getLinks(aCallback) {
> 
> should document the datatype of what's passed to aCallback. should also
> document what you mean by 'link' in the Links singleton below.

Done.
Attachment #585395 - Attachment is obsolete: true
Attachment #586278 - Flags: review?(dietrich)
(In reply to Dietrich Ayala (:dietrich) from comment #135)
> > +/**
> > + * This class represents a cell contained in the grid.
> 
> should clarify that it manages the dom node for the cell, not the actual
> cell content (ie, the Site).
> 
> might also want to note that it's basically a read-only class. all
> manipulation of both position and content happens elsewhere.

Done.

> > +  // Register drag-and-drop event handlers.
> > +  ["DragEnter", "DragOver", "DragExit", "Drop"].forEach(function (aType) {
> > +    let method = "_on" + aType;
> 
> why are these prefixed with underscore? they're called from external code,
> so aren't really private.

Fixed.

> ::: browser/base/content/newtab/drag.js
> > +/**
> > + * This singleton implements site dragging functionality.
> 
> should this singleton work at the level of cells, not sites? not sure why
> this code needs to be aware of a cell's contents - looking at it, it's all
> about the actual container itself.
> 
> drop.js handles cells in the drop code, but sites in the drag code. can this
> be made consistent, or is that not possible for some reason?

Cells are static and will never move around, be removed or change their positions. Sites float around, get deleted and created. So we're always dragging sites onto cells.

This is needed to have all these grid transformation effects and to allow dropping arbitrary links onto the grid. We could have fewer sites than cells and a user could drag a bookmark and drop it onto a cell. We'll then create a new site and put it into the target cell.

> > +   * Creates a new site in the grid.
> > +   * @param aLink The new site's link.
> > +   * @param aCell The new site's parent cell.
> 
> when you say 'parent', you mean it's the cell that the site will be created
> in, right? maybe 'containing' would be better than 'parent'? parent connotes
> the ability to have multiple children.

Fixed.

> where does the cell come from?
> 
> what happens as a result of this? is an existing cell pushed off the grid?
> should document the side-effects.

Cells in the grid are static. So even with not a single thumbnail visible we still have 9 cells created because they're static containers of sites moving around dynamically and being created/deleted.

> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/Geometry.jsm");
> > +Cu.import("resource:///modules/PageThumbs.jsm");
> > +Cu.import("resource:///modules/NewTabUtils.jsm");
> 
> worth lazy-loading these?

I used defineLazyModuleGetter() for Geometry.jsm as that's only used when transforming the grid. All other JSMs will be needed when creating the grid (i.e. loading the page), so they're not worth lazy-loading.

> > +   * Checks if the page is modified and sets the CSS class accordingly
> > +   */
> > +  checkIfModified: function Page_checkIfModified() {
> 
> the method name doesn't indicate any side-effects. this doesn't just check,
> it sets a value as well. maybe updateModifiedFlag or something ungainly like
> that?

Changed to updateModifiedFlag(), I think that's expressive.

> > +    doc.addEventListener("dragover", this._onDragOver, false);
> > +    doc.addEventListener("drop", this._onDrop, false);
> 
> these aren't really private, so shouldn't be underscore prefixed.

Fixed.

> ::: browser/base/content/newtab/transformations.js
> > + * This singleton allows to transform the grid by moving sites around.
> 
> should say whether it works on sites, cells, site-nodes or cell-nodes, or a
> mix of the above.

Fixed.

> ::: browser/base/content/newtab/updater.js
> > +  _findRemainingSites: function Updater_findRemainingSites(aLinks) {
> 
> can you clarify the name and comment to say what these are remaining from?
> what's the datatype of aLinks?
> what's the datatype of the return value?

Done.

> > +  _moveSiteNodes: function Updater_moveSiteNodes(aSites) {
> > +    let cells = gGrid.cells;
> > +    let sites = aSites.slice(0, cells.length);
> 
> imagine i'm a dev looking at this code for the first time, trying to fix a
> bug or add a feature... why does this need to happen? can you add some more
> commentary to explain the context of this call?

Done.
Attachment #585454 - Attachment is obsolete: true
Attachment #586278 - Flags: review?(dietrich) → review+
Disabling the New Tab Page by default because that's how we're gonna land it.
Attachment #584434 - Attachment is obsolete: true
Attachment #586988 - Flags: review?(gavin.sharp)
Attachment #584434 - Flags: review?(gavin.sharp)
Blocks: 716538
No longer depends on: 705907
No longer depends on: 704882
No longer depends on: 705911
Blocks: 717044
Comment on attachment 585398 [details] [diff] [review]
Part 2 - Assets / CSS / Images

>+++ b/browser/base/content/newtab/newTab.css

>+#vbox:not([page-disabled]) {

Please find a better id for this.

>+.button,
>+.button::-moz-focus-inner {
>+  cursor: pointer;
>+}

What's the point of ::-moz-focus-inner here?

>+++ b/browser/themes/gnomestripe/newtab/newTab.css

>+#body {
>+  font-family: -moz-window, sans-serif;

When would the sans-serif fallback be needed?

>+.button,
>+.button::-moz-focus-inner {
>+  padding: 0;
>+  border: 0 none;
>+}

Is there still some kind of focus indicator?
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #142)
> >+#vbox:not([page-disabled]) {
> 
> Please find a better id for this.

Sorry, that was a bad choice. Changed to #scrollbox because that's what it's for.

> >+.button,
> >+.button::-moz-focus-inner {
> >+  cursor: pointer;
> >+}
> 
> What's the point of ::-moz-focus-inner here?

Um, that's legacy from before I moved theme specific rules to separate files. Removed.

> >+#body {
> >+  font-family: -moz-window, sans-serif;
> 
> When would the sans-serif fallback be needed?

Also legacy from the very beginning when we used a custom font that wasn't available on all machines. Removed.

> >+.button,
> >+.button::-moz-focus-inner {
> >+  padding: 0;
> >+  border: 0 none;
> >+}
> 
> Is there still some kind of focus indicator?

That's the actual place where we want to remove the 'inner outline'.
Attachment #585398 - Attachment is obsolete: true
Attachment #587682 - Flags: review?(dao)
Attachment #585398 - Flags: review?(dao)
(In reply to Tim Taubert [:ttaubert] from comment #143)
> > >+.button,
> > >+.button::-moz-focus-inner {
> > >+  padding: 0;
> > >+  border: 0 none;
> > >+}
> > 
> > Is there still some kind of focus indicator?
> 
> That's the actual place where we want to remove the 'inner outline'.

This doesn't seem to answer my question. Are these buttons keyboard-focusable? Is there a focus indicator?
Yes, they are. The focus indicator is also visible after clicking those buttons. I guess we shouldn't remove the focus indicator because of accessibility? I don't oppose having it at all but it's pretty ugly when using only the mouse to click those buttons.
Yes, keyboard focus requires an indicator. :-moz-focusring should let you target keyboard focus vs. mouse focus.
So I had a look at bug 418521 and seems like this behavior is default. I mainly developed on Linux where the focus indicators are always shown and I don't liked that. But if that's the default we should probably don't touch it? Also, I won't even be able to detect if the element has been focused via keyboard (and not mouse) on Linux... What do you think?
Comment on attachment 586278 [details] [diff] [review]
Part 4 - Shared Module

>diff --git a/browser/modules/NewTabUtils.jsm b/browser/modules/NewTabUtils.jsm

>+function PrivateBrowsingStorage(aStorage) {
>+  this._data = {};

Seems like this should use Dict.jsm.

>diff --git a/toolkit/content/Services.jsm b/toolkit/content/Services.jsm

>+  ["privateBrowsing", "@mozilla.org/privatebrowsing;1", "nsIPrivateBrowsingService"],

This will fail for apps that don't have a private browsing service. You could do a run-time check to decide whether it should be included, but I think it's easier/better to just avoid adding this.
Comment on attachment 586988 [details] [diff] [review]
Part 3 - about:newtab integration

Perhaps we should use "blank page URL" rather than "new tab URL", in the utilityOverlay constant name and helper function? It isn't only used for "new tabs". I guess it doesn't matter too much, but we probably can't rename it later :)

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

>+  NewTabUtils.init();

Do you really want this to be called for each browser window? It seems like maybe init() should return early if the cache has already been populated.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-            var blank = !aURI || (aURI == "about:blank");
>+            var blank = !aURI || isNewTabURL(aURI);

>-            if (!blank) {
>+            if (aURI && aURI != "about:blank") {

>-              // pretend the user typed this so it'll be available till
>-              // the document successfully loads
>-              b.userTypedValue = aURI;
>+              if (blank) {
>+                // The progress listener received a stop status change. Reset
>+                // the mBlank status to true to prevent the throbber from being
>+                // shown when we're still loading a new tab URL.
>+                tabListener.mBlank = true;
>+              } else {
>+                // pretend the user typed this so it'll be available till
>+                // the document successfully loads
>+                b.userTypedValue = aURI;
>+              }

Can you move these changes to a followup ("make sure throbber doesn't appear for about:newtab"?). I want to look into this in more detail, and it doesn't need to block the rest of this landing. You can change the |if (blank)| check to if (blank || aURI && isNewTabURL(aURI)) to fix the tab title setting part.
Attachment #586988 - Flags: review?(gavin.sharp) → review+
(In reply to Tim Taubert [:ttaubert] from comment #147)
> So I had a look at bug 418521 and seems like this behavior is default. I
> mainly developed on Linux where the focus indicators are always shown and I
> don't liked that. But if that's the default we should probably don't touch
> it?

yep
Attached patch Part 2 - Assets / CSS / Images (obsolete) — Splinter Review
Removed the ::-moz-focus-inner selectors.
Attachment #587682 - Attachment is obsolete: true
Attachment #588071 - Flags: review?(dao)
Attachment #587682 - Flags: review?(dao)
Comment on attachment 588071 [details] [diff] [review]
Part 2 - Assets / CSS / Images

I tested this on Windows.

font-family: -moz-window; doesn't seem to work, I'm getting Times New Roman.

I can tab through the items, but other than the status panel, I get no indication which item is focused. I also can't seem to active items with Enter.

With the new tab page hidden, the empty page has a scroll bar if the window is small enough.
Attachment #588071 - Flags: review?(dao) → review-
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #148)
> >diff --git a/browser/modules/NewTabUtils.jsm b/browser/modules/NewTabUtils.jsm
> 
> >+function PrivateBrowsingStorage(aStorage) {
> >+  this._data = {};
> 
> Seems like this should use Dict.jsm.

Why exactly would it be better to use this? The API differs from nsIDOMStorage and as I need to clone the DOMStorage data there is also no way to easily initialize a Dict with a plain JavaScript object so I'm not really seeing the benefits here. Am I missing something?

> >diff --git a/toolkit/content/Services.jsm b/toolkit/content/Services.jsm
> 
> >+  ["privateBrowsing", "@mozilla.org/privatebrowsing;1", "nsIPrivateBrowsingService"],
> 
> This will fail for apps that don't have a private browsing service. You
> could do a run-time check to decide whether it should be included, but I
> think it's easier/better to just avoid adding this.

Removed this, thanks.
(In reply to Dão Gottwald [:dao] from comment #152)
> font-family: -moz-window; doesn't seem to work, I'm getting Times New Roman.

No idea why that couldn't be working but I wonder if we should just use 'sans-serif'. That looks much better for me.

> I can tab through the items, but other than the status panel, I get no
> indication which item is focused. I also can't seem to active items with
> Enter.

Need to fix this.

> With the new tab page hidden, the empty page has a scroll bar if the window
> is small enough.

Darn, that got lost when renaming #vbox to #scrollbox. I forgot to adapt a selector in the code.
Updating patch.
Attachment #586278 - Attachment is obsolete: true
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149)
> >+  NewTabUtils.init();
> 
> Do you really want this to be called for each browser window? It seems like
> maybe init() should return early if the cache has already been populated.

No we don't want that... Fixed, thanks!
Patch updated with the following:

* Hiding scrollbars when the grid is hidden.
* Using sans-serif as default font.
* Sites are now focusable and have an outline when focused. When focused the thumbnails fade to full opacity.

Keyboard navigation:

It's now possible to use the keyboard to properly navigate the New Tab Page. To update the tab indices for focusable elements changes to Part 2 were necessary and I'll update the patch in a minute. The pin and remove buttons on every site are not focusable for now as they're currently hidden in the black strip at the top. Bug 716532 will address this.
Attachment #588071 - Attachment is obsolete: true
Attachment #589189 - Flags: review?(dao)
Sorry, have to ask for review again because of some changes necessary to allow keyboard navigation. Hopefully interdiff can help here.

Changes:

* When hiding/showing/modifying the grid make sure that the right buttons are focusable (and some are not) and the sites have the right tab indices.
* Let the toolbar pass the focus to the next button when the currently focused button is soon to be hidden.
* Make pin/remove buttons not focusable for now - this is tackled in bug 716532.
* Some other minor fixes might be included, not sure.

I'll file a follow-up bug about writing a test for keyboard navigation and 'focus flow' to make sure we don't regress this.
Attachment #586378 - Attachment is obsolete: true
Attachment #589193 - Flags: review?(dietrich)
Fixed a typo and renamed "updateTabIndeces" to "updateTabIndices".
Attachment #589193 - Attachment is obsolete: true
Attachment #589197 - Flags: review?(dietrich)
Attachment #589193 - Flags: review?(dietrich)
(In reply to Tim Taubert [:ttaubert] from comment #153)
> Why exactly would it be better to use this? The API differs from
> nsIDOMStorage and as I need to clone the DOMStorage data there is also no
> way to easily initialize a Dict with a plain JavaScript object so I'm not
> really seeing the benefits here. Am I missing something?

I meant that PrivateBrowsingStorage's _data member should be a Dict, not that you should replace PrivateBrowsingStorage itself with a Dict. You can initialize it using set() in the loop, just as you're initializing the current object. We should generally always use Dict.jsm rather than plain objects when you want a simple key-data store, to avoid problems with e.g. |'toSource' in {}|.
Comment on attachment 589197 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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

::: browser/base/content/newtab/toolbar.js
@@ +66,5 @@
> +
> +  /**
> +   * Passes the focus from the current button to the next.
> +   * @param aCurrent The button that currently has focus.
> +   * @param aNext The button that is focused next.

please document that these are parts of the element ids.
Attachment #589197 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #162)
> please document that these are parts of the element ids.

Done.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #160)
> I meant that PrivateBrowsingStorage's _data member should be a Dict, not
> that you should replace PrivateBrowsingStorage itself with a Dict. You can
> initialize it using set() in the loop, just as you're initializing the
> current object. We should generally always use Dict.jsm rather than plain
> objects when you want a simple key-data store, to avoid problems with e.g.
> |'toSource' in {}|.

Ok, gotcha, done.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149)
> Perhaps we should use "blank page URL" rather than "new tab URL", in the
> utilityOverlay constant name and helper function? It isn't only used for
> "new tabs". I guess it doesn't matter too much, but we probably can't rename
> it later :)

I totally concur that it's definitely better to rename isNewTabURL() to isBlankPageURL() because that's what we're basically testing.

But IMHO we shouldn't rename BROWSER_NEW_TAB_URL to BROWSER_BLANK_PAGE_URL because that gives a false impression. This "const" is a wrapper for the 'browser.newtab.url' pref which basically defines the URL used for new tabs and not every blank page (because that's still about:blank in most cases).
Dão: it would be great if you have time to review the last patch today or tomorrow. We'd like to land the New Tab Page (pref'ed off) this week to give it some Nightly bake time before the Aurora uplift.

Thanks!
Attachment #589189 - Flags: review?(dao) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #149)
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> 
> >-            var blank = !aURI || (aURI == "about:blank");
> >+            var blank = !aURI || isNewTabURL(aURI);
> 
> >-            if (!blank) {
> >+            if (aURI && aURI != "about:blank") {
> 
> >-              // pretend the user typed this so it'll be available till
> >-              // the document successfully loads
> >-              b.userTypedValue = aURI;
> >+              if (blank) {
> >+                // The progress listener received a stop status change. Reset
> >+                // the mBlank status to true to prevent the throbber from being
> >+                // shown when we're still loading a new tab URL.
> >+                tabListener.mBlank = true;
> >+              } else {
> >+                // pretend the user typed this so it'll be available till
> >+                // the document successfully loads
> >+                b.userTypedValue = aURI;
> >+              }
> 
> Can you move these changes to a followup ("make sure throbber doesn't appear
> for about:newtab"?). I want to look into this in more detail, and it doesn't
> need to block the rest of this landing.

Ok, re-opened bug 716108 and will attach a patch.
w00t \o/
FTR, this feature has landed but is currently disabled by default. If you'd like to help testing and want to use it *now* so please do the following:

0) get a nightly build
1) set browser.newtab.url to 'about:newtab'
2) restart the browser
3) open a new tab and show the grid by clicking the button in the upper right

Alternatively you could try the UX branch, it's enabled by default there.
Please do NOT report any issues you notice here. Please file new bugs. Thanks!
Blocks: 561749
Depends on: 721389
No longer blocks: 561749
Is there a bug filed for undo-ing removal of a site/thumbnail from the new tab page?
you can click the refresh button at top right of the new tab page
(In reply to bogas04 from comment #180)
> you can click the refresh button at top right of the new tab page

What if I want to restore the last removed site. There is a similar option in Google Chrome.

Also the refresh icon doesn't convey the idea of reset.
(In reply to Siddhartha Dugar (sdrocking) from comment #181)
> What if I want to restore the last removed site. There is a similar option
> in Google Chrome.

Please go ahead and file a bug.

> Also the refresh icon doesn't convey the idea of reset.

Feel free and file a bug about that, too.

(This is not the right place guys, file new bugs please...)
Depends on: 722234
Depends on: 722235
(In reply to Tim Taubert [:ttaubert] from comment #182)
> Please go ahead and file a bug.

Filed bug 722234 and bug 722235. Please confirm.
Depends on: 722263
Depends on: 722273
Blocks: 722660
No longer blocks: 722660
Blocks: 722663
Depends on: 722672
Depends on: 721417
So I expect to find this in the nighties?
Yes, see bug 716538.
Depends on: 724089
Blocks: 724217
Depends on: 724239
Blocks: 724408
Depends on: 724497
Depends on: 724504
Depends on: 724796
Depends on: 724848
Depends on: 725200
Blocks: 725579
Blocks: 725670
No longer blocks: 725670
Depends on: 725670
Blocks: 725694
Depends on: 725756
Depends on: 725996
Depends on: 726009
Depends on: 726267
Depends on: 723298
Depends on: 726105
Depends on: 724539
Depends on: 725393
Depends on: 726628
Depends on: 721020
Depends on: 727120
Depends on: 727393
Depends on: 727469
Depends on: 722650
Depends on: 719675
Depends on: 719035
Depends on: 706707
Depends on: 705001
Depends on: 716543
Depends on: 727668
No longer blocks: 723981
Depends on: 723981
No longer blocks: 724408
No longer depends on: 726267
No longer depends on: 727120
Depends on: 728663
Depends on: 729024
Depends on: 710440
Depends on: 729063
Depends on: 729878
Depends on: 731157
No longer depends on: 731157
Depends on: 731157
Depends on: 731635
Depends on: 734280
Blocks: 734900
Depends on: 735294
Blocks: 735630
No longer blocks: 735630
Depends on: 735630
Depends on: 735984
Depends on: 735987
Depends on: 736211
Depends on: 736474
Depends on: 736476
Depends on: 736709
Depends on: 736839
Depends on: 737358
Depends on: 738167
Depends on: 736796
Depends on: 736724
Depends on: 739387
Depends on: 739820
Bug 740340 asks to revert patch "Part 3" unless it's actually working. As is, the pref doesn't work, and the changes break existing addons that hook up a newtab page.
Depends on: 740340
Depends on: 743613
Blocks: 748125
Blocks: 748163
Depends on: 749038
Blocks: 739820
No longer depends on: 739820
Depends on: 752841
Depends on: 752839
Depends on: 754731
Depends on: 756000
Depends on: 756961
Depends on: 757386
Depends on: 760773
Depends on: 763259
Depends on: 764768
Depends on: 765729
No longer depends on: 739387
Depends on: 766990
Depends on: 768969
Depends on: 776477
Depends on: 786555
Depends on: 787081
Depends on: 798443
Depends on: 800775
No longer depends on: 800775
Depends on: 802220
No longer depends on: 802220
Depends on: 832996
Depends on: 757455
Depends on: 755996
Depends on: 851408
Depends on: 854087
Depends on: 855603
Depends on: 908110
Depends on: 863296
Depends on: 862941
No longer depends on: 908110
Depends on: 832624
Depends on: 818024
Depends on: 794716
Depends on: 960711
FYI: There's a new "New Tab Page" component for bugs.
Component: Tabbed Browser → New Tab Page
No longer depends on: 787081
Depends on: 1342765
You need to log in before you can comment on or make changes to this bug.