Last Comment Bug 455553 - New Tab Page feature
: New Tab Page feature
Status: RESOLVED FIXED
[a11y-ping=davidb]
: addon-compat
Product: Firefox
Classification: Client Software
Component: New Tab Page (show other bugs)
: Trunk
: All All
: -- normal with 13 votes (vote)
: Firefox 12
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 457034 561749 601549 693264 693567 713461 (view as bug list)
Depends on: 641346 706707 710440 723968 723981 725756 727393 727469 727668 729024 732384 735294 735630 736211 736474 737358 743613 751390 754731 755996 756000 756961 763259 764768 766990 776477 794716 818024 832624 855603 863296 497543 705001 716543 719035 719675 721020 721389 721392 721417 722234 722235 722263 722273 722650 722672 723298 723957 724089 724239 724494 724497 724504 724539 724796 724848 725200 725393 725670 725996 726009 726105 726628 728663 729063 729878 731157 731635 734280 734952 735564 735984 735987 736018 736476 736709 736724 736796 736839 738167 740340 749038 752839 752841 757386 757455 760773 765729 768969 786555 798443 832996 851408 854087 862941 960711
Blocks: 712602 725579 725694 734900 699362 716538 717044 722663 723972 724217 739820 748125 748163 748299
  Show dependency treegraph
 
Reported: 2008-09-16 10:55 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2015-07-17 14:06 PDT (History)
89 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prototype v0.01 (18.68 KB, patch)
2008-09-16 13:26 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
prototype v1 (13.23 KB, patch)
2011-05-26 13:29 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
Thumbnail Review Proposal (111.23 KB, image/png)
2011-11-01 08:51 PDT, Paul [pwd]
no flags Details
patch v1 (WIP) (150.79 KB, patch)
2011-11-24 18:37 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (87.50 KB, patch)
2011-12-07 02:36 PST, Tim Taubert [:ttaubert]
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
Part 2 - Assets / Images (14.08 KB, patch)
2011-12-07 02:37 PST, Tim Taubert [:ttaubert]
bmcbride: feedback+
Details | Diff | Splinter Review
Part 3 - about:newtab integration (4.17 KB, patch)
2011-12-07 02:37 PST, Tim Taubert [:ttaubert]
fryn: feedback+
Details | Diff | Splinter Review
Part 4 - Shared Module (15.94 KB, patch)
2011-12-07 02:38 PST, Tim Taubert [:ttaubert]
bmcbride: feedback+
Details | Diff | Splinter Review
Part 5 - New Tab Page tests and test suite (22.17 KB, patch)
2011-12-07 02:38 PST, Tim Taubert [:ttaubert]
dietrich: feedback+
Details | Diff | Splinter Review
Part 5 - New Tab Page tests and test suite (24.64 KB, patch)
2011-12-13 14:12 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 6 - Extend nsPlacesAutoComplete to get more results (1011 bytes, patch)
2011-12-13 14:16 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (4.58 KB, patch)
2011-12-13 14:21 PST, Tim Taubert [:ttaubert]
fryn: review+
Details | Diff | Splinter Review
Part 4 - Shared Module (14.17 KB, patch)
2011-12-13 14:29 PST, Tim Taubert [:ttaubert]
bmcbride: review+
Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (21.39 KB, patch)
2011-12-13 14:42 PST, Tim Taubert [:ttaubert]
jaws: feedback+
Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (81.26 KB, patch)
2011-12-13 15:13 PST, Tim Taubert [:ttaubert]
bmcbride: review+
Details | Diff | Splinter Review
Part 3 - about:newtab integration (3.93 KB, patch)
2011-12-14 02:48 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 4 - Shared Module (14.24 KB, patch)
2011-12-14 03:24 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 4 - Shared Module (15.10 KB, patch)
2011-12-14 04:27 PST, Tim Taubert [:ttaubert]
mak77: review+
Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (80.73 KB, patch)
2011-12-14 04:55 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (21.39 KB, patch)
2011-12-14 11:02 PST, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
Part 4 - Shared Module (15.22 KB, patch)
2011-12-14 11:47 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 5 - New Tab Page tests and test suite (24.63 KB, patch)
2011-12-14 11:50 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (3.94 KB, patch)
2011-12-14 11:53 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (80.57 KB, patch)
2011-12-14 11:55 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (4.21 KB, patch)
2011-12-14 13:18 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (12.06 KB, patch)
2011-12-15 11:52 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 6 - Remove the history dropmarker from the URL bar (9.60 KB, patch)
2011-12-16 04:25 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (78.72 KB, patch)
2011-12-16 04:50 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (30.19 KB, patch)
2011-12-16 05:45 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (79.27 KB, patch)
2011-12-18 06:40 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 3 - about:newtab integration (11.91 KB, patch)
2011-12-19 11:45 PST, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Splinter Review
Part 3 - about:newtab integration (12.62 KB, patch)
2011-12-21 04:11 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (19.25 KB, patch)
2011-12-22 07:58 PST, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
Part 3 - about:newtab integration (23.69 KB, patch)
2011-12-22 17:04 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (23.69 KB, patch)
2011-12-27 07:40 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 5 - New Tab Page tests and test suite (24.73 KB, patch)
2011-12-27 07:42 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 5 - New Tab Page tests and test suite (24.53 KB, patch)
2012-01-02 06:28 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XUL Page / Scripts (80.12 KB, patch)
2012-01-03 06:30 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 4 - Shared Module (15.97 KB, patch)
2012-01-03 06:36 PST, Tim Taubert [:ttaubert]
dietrich: feedback+
Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (38.79 KB, patch)
2012-01-03 06:44 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XUL Page / Scripts (80.12 KB, patch)
2012-01-03 10:24 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 4 - Shared Module (16.63 KB, patch)
2012-01-05 16:58 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
Part 1 - XUL/HTML Page and Scripts (59.47 KB, patch)
2012-01-06 03:38 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 3 - about:newtab integration (23.70 KB, patch)
2012-01-09 07:18 PST, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (38.74 KB, patch)
2012-01-11 06:44 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (38.75 KB, patch)
2012-01-12 09:28 PST, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
Part 4 - Shared Module (16.54 KB, patch)
2012-01-17 06:45 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 2 - Assets / CSS / Images (38.73 KB, patch)
2012-01-17 07:14 PST, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
Part 1 - XUL/HTML Page and Scripts (61.61 KB, patch)
2012-01-17 07:21 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Part 1 - XHTML Page / Scripts (61.61 KB, patch)
2012-01-17 07:42 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-16 10:55:05 PDT

    
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-16 13:26:34 PDT
Created attachment 338928 [details] [diff] [review]
prototype v0.01

 * 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.
Comment 2 Simon Bünzli 2008-09-16 22:37:33 PDT
(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)?
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-17 05:36:29 PDT
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 4 Simon Bünzli 2008-09-17 07:18:45 PDT
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;
Comment 5 Dão Gottwald [:dao] 2008-09-25 11:18:43 PDT
*** Bug 457034 has been marked as a duplicate of this bug. ***
Comment 6 u88484 2008-09-25 11:24:23 PDT
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.
Comment 7 u88484 2008-09-25 11:26:21 PDT
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.
Comment 8 Γριφεγ 2008-09-25 14:38:08 PDT
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!
Comment 9 Alfred Kayser 2008-10-03 10:50:36 PDT
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.
Comment 10 Γριφεγ 2008-10-03 11:39:08 PDT
(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!
Comment 11 Maaren 2008-10-09 13:20:20 PDT
(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)
Comment 12 Luke Iliffe (Harlequin99) 2008-11-14 02:00:53 PST
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.
Comment 13 u88484 2008-11-20 13:49:40 PST
This isn't going to make 3.1 if it doesn't get in for 3.1b2?
Comment 14 Alfred Kayser 2008-11-20 23:45:18 PST
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
Comment 15 [not reading bugmail] 2009-09-18 15:05:25 PDT
(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
Comment 16 Dão Gottwald [:dao] 2010-10-05 05:25:37 PDT
*** Bug 601549 has been marked as a duplicate of this bug. ***
Comment 17 :Margaret Leibovic 2011-05-26 13:29:41 PDT
Created attachment 535455 [details] [diff] [review]
prototype v1

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).
Comment 18 Ekanan Ketunuti 2011-06-07 22:40:17 PDT
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]
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-07 22:51:11 PDT
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).
Comment 20 Ekanan Ketunuti 2011-06-07 23:13:16 PDT
work now, just rename newTabUtils.jsm to NewTabUtils.jsm.
Comment 21 Frank Yan (:fryn) 2011-06-27 11:44:43 PDT
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
Comment 22 Emil 2011-07-31 05:56:56 PDT
It should be possible to add some feeds to the new tab page.
http://upload.7pmtech.com/view/full/90_fltn2
Comment 23 Emil 2011-07-31 05:58:51 PDT
Sorry, wrong link. Here's the correct one:
http://upload.7pmtech.com/files/90_fltn2/2.jpg
Comment 24 Tiger.711 2011-09-29 07:32:28 PDT
You want to say in such a form as now, work is over?
Comment 25 :Margaret Leibovic 2011-09-29 08:22:28 PDT
(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.
Comment 26 pumay 2011-10-08 02:36:12 PDT
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!
Comment 27 Mats Palmgren (vacation) 2011-10-10 07:51:45 PDT
*** Bug 693264 has been marked as a duplicate of this bug. ***
Comment 28 Dão Gottwald [:dao] 2011-10-11 05:02:17 PDT
*** Bug 693567 has been marked as a duplicate of this bug. ***
Comment 29 Tiger.711 2011-10-22 02:07:09 PDT
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.
Comment 30 Paul [pwd] 2011-11-01 08:51:34 PDT
Created attachment 571021 [details]
Thumbnail Review Proposal

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.
Comment 31 Alfred Kayser 2011-11-01 09:53:38 PDT
As comment 30 hints at, is it an idea to combine new tab page and the 'panorama/tabview' into one thing?
Comment 32 Paul [pwd] 2011-11-01 10:09:52 PDT
(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.
Comment 33 Tim Taubert [:ttaubert] 2011-11-01 10:27:26 PDT
(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.
Comment 34 Siddhartha Dugar [:sdrocking] 2011-11-03 04:39:29 PDT
Hovered thumbnails should look more different from others. Perhaps the default opacity should be reduced.
Comment 35 bogas04 2011-11-04 02:51:21 PDT
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.
Comment 36 Siddhartha Dugar [:sdrocking] 2011-11-05 02:31:06 PDT
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.
Comment 37 Greg Karz 2011-11-11 07:36:11 PST
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.
Comment 38 Siddhartha Dugar [:sdrocking] 2011-11-12 19:44:16 PST
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.
Comment 39 Tiger.711 2011-11-12 21:18:26 PST
I think, that the grid should be customizable.
Comment 40 Mardeg 2011-11-12 21:52:20 PST
Is it reasonable to expect the layout to adapt to screensize based on CSS media queries?
Comment 41 Frank Yan (:fryn) 2011-11-15 12:53:25 PST
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.
Comment 42 Tim Taubert [:ttaubert] 2011-11-16 11:51:14 PST
(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!
Comment 43 Tim Taubert [:ttaubert] 2011-11-24 18:37:53 PST
Created attachment 576861 [details] [diff] [review]
patch v1 (WIP)

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).
Comment 44 Girish Sharma [:Optimizer] 2011-11-24 22:16:56 PST
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.
Comment 45 Dão Gottwald [:dao] 2011-11-25 03:18:51 PST
*** Bug 705001 has been marked as a duplicate of this bug. ***
Comment 46 Dão Gottwald [:dao] 2011-11-25 03:35:34 PST
*** Bug 705000 has been marked as a duplicate of this bug. ***
Comment 47 Tim Taubert [:ttaubert] 2011-12-07 02:34:17 PST
(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.
Comment 48 Tim Taubert [:ttaubert] 2011-12-07 02:36:08 PST
Created attachment 579643 [details] [diff] [review]
Part 1 - XHTML Page / Scripts
Comment 49 Tim Taubert [:ttaubert] 2011-12-07 02:37:01 PST
Created attachment 579644 [details] [diff] [review]
Part 2 - Assets / Images
Comment 50 Tim Taubert [:ttaubert] 2011-12-07 02:37:33 PST
Created attachment 579645 [details] [diff] [review]
Part 3 - about:newtab integration
Comment 51 Tim Taubert [:ttaubert] 2011-12-07 02:38:19 PST
Created attachment 579646 [details] [diff] [review]
Part 4 - Shared Module
Comment 52 Tim Taubert [:ttaubert] 2011-12-07 02:38:52 PST
Created attachment 579647 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite
Comment 53 Girish Sharma [:Optimizer] 2011-12-07 02:41:13 PST
(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.
Comment 54 Dietrich Ayala (:dietrich) 2011-12-07 11:17:15 PST
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).
Comment 55 Jared Wein [:jaws] (please needinfo? me) 2011-12-07 23:43:45 PST
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?
Comment 56 Frank Yan (:fryn) 2011-12-08 14:12:19 PST
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)?
Comment 57 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-13 00:37:14 PST
Comment on attachment 579644 [details] [diff] [review]
Part 2 - Assets / Images

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

These jar.mn files look SPLENDID!
Comment 58 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-13 01:56:35 PST
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.
Comment 59 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-13 02:24:57 PST
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.
Comment 60 Tim Taubert [:ttaubert] 2011-12-13 14:12:50 PST
Created attachment 581427 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

(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.
Comment 61 Tim Taubert [:ttaubert] 2011-12-13 14:16:30 PST
Created attachment 581428 [details] [diff] [review]
Part 6 - Extend nsPlacesAutoComplete to get more results

We need a way to get more than the default 25 results from the nsPlacesAutoComplete service.
Comment 62 Tim Taubert [:ttaubert] 2011-12-13 14:21:45 PST
Created attachment 581432 [details] [diff] [review]
Part 3 - about:newtab integration

(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.
Comment 63 Tim Taubert [:ttaubert] 2011-12-13 14:29:03 PST
Created attachment 581436 [details] [diff] [review]
Part 4 - Shared Module

(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.
Comment 64 Tim Taubert [:ttaubert] 2011-12-13 14:42:14 PST
Created attachment 581441 [details] [diff] [review]
Part 2 - Assets / CSS / Images

(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.
Comment 65 Jared Wein [:jaws] (please needinfo? me) 2011-12-13 14:50:53 PST
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.
Comment 66 Frank Yan (:fryn) 2011-12-13 15:04:35 PST
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
Comment 67 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 15:12:06 PST
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.
Comment 68 Tim Taubert [:ttaubert] 2011-12-13 15:13:59 PST
Created attachment 581451 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

(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.
Comment 69 Frank Yan (:fryn) 2011-12-13 15:22:09 PST
(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.
Comment 70 Tim Taubert [:ttaubert] 2011-12-13 15:24:50 PST
Yes, I tried setting 'blank' first but that didn't work out well.
Comment 71 Marco Bonardo [::mak] 2011-12-13 15:37:09 PST
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.
Comment 72 Tim Taubert [:ttaubert] 2011-12-13 15:51:36 PST
(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 73 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-13 19:26:58 PST
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.
Comment 74 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-13 23:55:07 PST
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?
Comment 75 Tim Taubert [:ttaubert] 2011-12-14 02:48:19 PST
Created attachment 581570 [details] [diff] [review]
Part 3 - about:newtab integration

Removed two unnecessary about:newtab occurrences that enforced a blank tab title.
Comment 76 Tim Taubert [:ttaubert] 2011-12-14 03:24:40 PST
Created attachment 581579 [details] [diff] [review]
Part 4 - Shared Module

(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).
Comment 77 Tim Taubert [:ttaubert] 2011-12-14 04:27:58 PST
Created attachment 581588 [details] [diff] [review]
Part 4 - Shared Module

Moved the preference observer to the JSM.
Comment 78 Tim Taubert [:ttaubert] 2011-12-14 04:55:26 PST
Created attachment 581594 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

(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.
Comment 79 Marco Bonardo [::mak] 2011-12-14 07:39:04 PST
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?
Comment 80 Girish Sharma [:Optimizer] 2011-12-14 09:13:24 PST
The two buttons on top right are again transparent on windows 7 with this recent patch on the latest UX tinderbox.
Comment 81 Tim Taubert [:ttaubert] 2011-12-14 11:02:14 PST
Created attachment 581715 [details] [diff] [review]
Part 2 - Assets / CSS / Images

(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.
Comment 82 Tim Taubert [:ttaubert] 2011-12-14 11:47:32 PST
Created attachment 581729 [details] [diff] [review]
Part 4 - Shared Module

(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.
Comment 83 Tim Taubert [:ttaubert] 2011-12-14 11:50:40 PST
Created attachment 581730 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

Combined Page.isEnabled() and Page.setDisabled() into get/set enabled().
Comment 84 Tim Taubert [:ttaubert] 2011-12-14 11:53:14 PST
Created attachment 581731 [details] [diff] [review]
Part 3 - about:newtab integration

Changed the preference name to browser.newtabpage.enabled.
Comment 85 Tim Taubert [:ttaubert] 2011-12-14 11:55:17 PST
Created attachment 581733 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

Combined Page.isEnabled() and Page.setDisabled() into get/set enabled(). Changed the preference name to browser.newtabpage.enabled.
Comment 86 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-14 12:08:08 PST
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?
Comment 87 Tim Taubert [:ttaubert] 2011-12-14 12:18:09 PST
(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).
Comment 88 Tim Taubert [:ttaubert] 2011-12-14 12:21:25 PST
(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.
Comment 89 Frank Yan (:fryn) 2011-12-14 12:28:09 PST
(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);
Comment 90 Tim Taubert [:ttaubert] 2011-12-14 13:18:30 PST
Created attachment 581760 [details] [diff] [review]
Part 3 - about:newtab integration

(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?
Comment 91 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-15 00:34:41 PST
(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.
Comment 92 Dão Gottwald [:dao] 2011-12-15 02:42:16 PST
Can you set BROWSER_NEW_TAB_URL to about:blank depending on the pref?
Comment 93 Tim Taubert [:ttaubert] 2011-12-15 02:46:42 PST
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.
Comment 94 Dão Gottwald [:dao] 2011-12-15 02:51:25 PST
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.
Comment 95 Tim Taubert [:ttaubert] 2011-12-15 03:05:23 PST
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 96 Dão Gottwald [:dao] 2011-12-15 04:20:52 PST
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?
Comment 97 Tim Taubert [:ttaubert] 2011-12-15 11:52:35 PST
Created attachment 582058 [details] [diff] [review]
Part 3 - about:newtab integration

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.
Comment 98 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-15 12:00:15 PST
(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 99 Dão Gottwald [:dao] 2011-12-15 12:13:49 PST
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.
Comment 100 Tim Taubert [:ttaubert] 2011-12-15 12:25:04 PST
Oops. Yeah, I also don't think it needs to be live. I actually meant to implement a lazy getter...
Comment 101 Tim Taubert [:ttaubert] 2011-12-16 04:25:50 PST
Created attachment 582241 [details] [diff] [review]
Part 6 - Remove the history dropmarker from the URL bar

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.
Comment 102 Siddhartha Dugar [:sdrocking] 2011-12-16 04:41:53 PST
(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.
Comment 103 Tim Taubert [:ttaubert] 2011-12-16 04:50:50 PST
Created attachment 582247 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

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.
Comment 104 Tim Taubert [:ttaubert] 2011-12-16 05:45:23 PST
Created attachment 582251 [details] [diff] [review]
Part 2 - Assets / CSS / Images

(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.
Comment 105 Jared Wein [:jaws] (please needinfo? me) 2011-12-17 21:27:27 PST
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.
Comment 106 Girish Sharma [:Optimizer] 2011-12-17 23:36:17 PST
(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.
Comment 107 Tim Taubert [:ttaubert] 2011-12-18 06:40:02 PST
Created attachment 582663 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

(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.
Comment 108 Tim Taubert [:ttaubert] 2011-12-19 11:45:50 PST
Created attachment 582901 [details] [diff] [review]
Part 3 - about:newtab integration

Fixed a bug in isTabEmpty(). BROWSER_NEW_TAB_URL is now a lazy getter.
Comment 109 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-19 17:51:12 PST
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.
Comment 110 Tim Taubert [:ttaubert] 2011-12-20 05:57:07 PST
(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 111 Dietrich Ayala (:dietrich) 2011-12-20 10:19:19 PST
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.
Comment 112 tabmix.onemen 2011-12-20 12:47:16 PST
i see the grid on new tab also when browser.newtabpage.enabled is false
Comment 113 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-20 17:08:26 PST
(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.
Comment 114 Tim Taubert [:ttaubert] 2011-12-21 04:11:34 PST
Created attachment 583448 [details] [diff] [review]
Part 3 - about:newtab integration

Sorry, have to ask for review again. BrowserOpenTab() recently changed and this revealed that we need to fix utilityOverlay.js as well.
Comment 115 Tim Taubert [:ttaubert] 2011-12-21 06:22:08 PST
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.
Comment 116 Siddhartha Dugar [:sdrocking] 2011-12-21 22:14:18 PST
The new tab page needs scrollbars, both vertical and horizontal. Another option could be to resize thumbnails as we do in Panorama.
Comment 117 Dão Gottwald [:dao] 2011-12-22 02:38:41 PST
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.
Comment 118 Tim Taubert [:ttaubert] 2011-12-22 07:58:19 PST
Created attachment 583802 [details] [diff] [review]
Part 3 - about:newtab integration

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().
Comment 119 Dão Gottwald [:dao] 2011-12-22 08:10:05 PST
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.
Comment 120 Tim Taubert [:ttaubert] 2011-12-22 09:21:10 PST
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?
Comment 121 Dão Gottwald [:dao] 2011-12-22 10:49:26 PST
(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.
Comment 122 Tim Taubert [:ttaubert] 2011-12-22 17:04:21 PST
Created attachment 583972 [details] [diff] [review]
Part 3 - about:newtab integration

Moved BROWSER_NEW_TAB_URL and isNewTabURL() to utilityOverlay.js. Fixed a11y tests by including utilityOverlay.js.
Comment 123 Dão Gottwald [:dao] 2011-12-23 04:14:53 PST
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.
Comment 124 Dão Gottwald [:dao] 2011-12-25 14:19:55 PST
*** Bug 713461 has been marked as a duplicate of this bug. ***
Comment 125 dE 2011-12-26 03:48:17 PST
Devs -- you might like to have a look at bug 713461 for more ideas.
Comment 126 Tim Taubert [:ttaubert] 2011-12-27 07:40:01 PST
Created attachment 584434 [details] [diff] [review]
Part 3 - about:newtab integration

(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.
Comment 127 Tim Taubert [:ttaubert] 2011-12-27 07:42:10 PST
Created attachment 584436 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

Fixed a test-only mem leak.
Comment 128 Dietrich Ayala (:dietrich) 2011-12-29 12:27:48 PST
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.
Comment 129 Tim Taubert [:ttaubert] 2012-01-02 06:28:58 PST
Created attachment 585280 [details] [diff] [review]
Part 5 - New Tab Page tests and test suite

(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.
Comment 130 Tim Taubert [:ttaubert] 2012-01-03 06:30:03 PST
Created attachment 585394 [details] [diff] [review]
Part 1 - XUL Page / Scripts

(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.
Comment 131 Tim Taubert [:ttaubert] 2012-01-03 06:36:21 PST
Created attachment 585395 [details] [diff] [review]
Part 4 - Shared Module

Asking for review wrt to the changes of the AllPages object.
Comment 132 Tim Taubert [:ttaubert] 2012-01-03 06:44:36 PST
Created attachment 585398 [details] [diff] [review]
Part 2 - Assets / CSS / Images

(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.
Comment 133 Dão Gottwald [:dao] 2012-01-03 07:35:12 PST
(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.
Comment 134 Tim Taubert [:ttaubert] 2012-01-03 10:24:02 PST
Created attachment 585454 [details] [diff] [review]
Part 1 - XUL Page / Scripts

(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.
Comment 135 Dietrich Ayala (:dietrich) 2012-01-04 16:28:28 PST
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?
Comment 136 Dietrich Ayala (:dietrich) 2012-01-05 13:33:59 PST
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.
Comment 137 Dietrich Ayala (:dietrich) 2012-01-05 14:51:41 PST
(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.
Comment 138 Tim Taubert [:ttaubert] 2012-01-05 14:59:45 PST
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.
Comment 139 Tim Taubert [:ttaubert] 2012-01-05 16:58:45 PST
Created attachment 586278 [details] [diff] [review]
Part 4 - Shared Module

(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.
Comment 140 Tim Taubert [:ttaubert] 2012-01-06 03:38:42 PST
Created attachment 586378 [details] [diff] [review]
Part 1 - XUL/HTML Page and Scripts

(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.
Comment 141 Tim Taubert [:ttaubert] 2012-01-09 07:18:47 PST
Created attachment 586988 [details] [diff] [review]
Part 3 - about:newtab integration

Disabling the New Tab Page by default because that's how we're gonna land it.
Comment 142 Dão Gottwald [:dao] 2012-01-11 05:13:03 PST
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?
Comment 143 Tim Taubert [:ttaubert] 2012-01-11 06:44:42 PST
Created attachment 587682 [details] [diff] [review]
Part 2 - Assets / CSS / Images

(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'.
Comment 144 Dão Gottwald [:dao] 2012-01-11 07:02:42 PST
(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?
Comment 145 Tim Taubert [:ttaubert] 2012-01-11 07:31:28 PST
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.
Comment 146 Dão Gottwald [:dao] 2012-01-11 08:08:20 PST
Yes, keyboard focus requires an indicator. :-moz-focusring should let you target keyboard focus vs. mouse focus.
Comment 147 Tim Taubert [:ttaubert] 2012-01-11 11:12:25 PST
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 148 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-11 13:04:30 PST
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 149 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-11 13:21:37 PST
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.
Comment 150 Dão Gottwald [:dao] 2012-01-12 06:13:51 PST
(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
Comment 151 Tim Taubert [:ttaubert] 2012-01-12 09:28:50 PST
Created attachment 588071 [details] [diff] [review]
Part 2 - Assets / CSS / Images

Removed the ::-moz-focus-inner selectors.
Comment 152 Dão Gottwald [:dao] 2012-01-13 04:18:11 PST
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.
Comment 153 Tim Taubert [:ttaubert] 2012-01-13 04:31:18 PST
(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.
Comment 154 Tim Taubert [:ttaubert] 2012-01-13 05:16:02 PST
(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.
Comment 155 Tim Taubert [:ttaubert] 2012-01-17 06:45:17 PST
Created attachment 589180 [details] [diff] [review]
Part 4 - Shared Module

Updating patch.
Comment 156 Tim Taubert [:ttaubert] 2012-01-17 06:48:27 PST
(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!
Comment 157 Tim Taubert [:ttaubert] 2012-01-17 07:14:40 PST
Created attachment 589189 [details] [diff] [review]
Part 2 - Assets / CSS / Images

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.
Comment 158 Tim Taubert [:ttaubert] 2012-01-17 07:21:56 PST
Created attachment 589193 [details] [diff] [review]
Part 1 - XUL/HTML Page and Scripts

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.
Comment 159 Tim Taubert [:ttaubert] 2012-01-17 07:42:32 PST
Created attachment 589197 [details] [diff] [review]
Part 1 - XHTML Page / Scripts

Fixed a typo and renamed "updateTabIndeces" to "updateTabIndices".
Comment 160 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-17 13:38:40 PST
(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 162 Dietrich Ayala (:dietrich) 2012-01-17 17:16:10 PST
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.
Comment 163 Tim Taubert [:ttaubert] 2012-01-18 06:54:30 PST
(In reply to Dietrich Ayala (:dietrich) from comment #162)
> please document that these are parts of the element ids.

Done.
Comment 164 Tim Taubert [:ttaubert] 2012-01-18 07:10:00 PST
(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.
Comment 165 Tim Taubert [:ttaubert] 2012-01-18 08:32:24 PST
(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).
Comment 166 Tim Taubert [:ttaubert] 2012-01-19 04:10:30 PST
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!
Comment 167 Tim Taubert [:ttaubert] 2012-01-19 17:33:44 PST
(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.
Comment 172 bogas04 2012-01-26 00:56:19 PST
w00t \o/
Comment 173 Tim Taubert [:ttaubert] 2012-01-26 01:00:51 PST
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.
Comment 174 Tim Taubert [:ttaubert] 2012-01-26 01:07:56 PST
Please do NOT report any issues you notice here. Please file new bugs. Thanks!
Comment 176 Dão Gottwald [:dao] 2012-01-26 10:18:17 PST
*** Bug 561749 has been marked as a duplicate of this bug. ***
Comment 179 Siddhartha Dugar [:sdrocking] 2012-01-29 22:28:55 PST
Is there a bug filed for undo-ing removal of a site/thumbnail from the new tab page?
Comment 180 bogas04 2012-01-29 22:51:47 PST
you can click the refresh button at top right of the new tab page
Comment 181 Siddhartha Dugar [:sdrocking] 2012-01-29 23:14:41 PST
(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.
Comment 182 Tim Taubert [:ttaubert] 2012-01-29 23:18:20 PST
(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...)
Comment 183 Siddhartha Dugar [:sdrocking] 2012-01-29 23:35:11 PST
(In reply to Tim Taubert [:ttaubert] from comment #182)
> Please go ahead and file a bug.

Filed bug 722234 and bug 722235. Please confirm.
Comment 184 dE 2012-02-03 05:34:08 PST
So I expect to find this in the nighties?
Comment 185 Tim Taubert [:ttaubert] 2012-02-03 05:46:15 PST
Yes, see bug 716538.
Comment 186 Ben Bucksch (:BenB) 2012-03-30 05:38:53 PDT
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.
Comment 187 Matthew N. [:MattN] 2014-05-20 22:19:47 PDT
FYI: There's a new "New Tab Page" component for bugs.

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