Make it possible to hide "most visited" on nnnnnnntp

Make it possible to hide "most visited" on nnnnnnntp

This implements http://folder/glen/chrome/spec/101_ntp/8_wish&sidebar mocks 2, 5, 6

It's a bit lame to use a bit in the "sections" bitmask to serialize the "should show?" information, but the damage here was done when someone decided to that "THUBMS bit missing" should mean "show a list instead" (instead of adding another bit for that).

BUG=55148
TEST=Hover "Most Visited" bar. Close button should appear. Clicking it should hide "Most Visited" area and add a button at the window edge. That button should have a menu that makes it possible to make the "Most Visited" area visible again. Also do this while multiple NTPs are open; the changes made in the front NTP should be synced to the background NTPs.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61095

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61259

[Nico, 2010-09-20 03:47:57]

According to Glen, it'd be aweeeesome if this makes m7.

I'm not the ninjaest js guy out there, so keep that in mind while reviewing. I'm
good at making up superlatives though.

[Glen Murphy, 2010-09-20 04:07:30]

+ Aaron

On Sun, Sep 19, 2010 at 8:47 PM,  <thakis@chromium.org> wrote:
> Reviewers: Miranda Callahan,
>
> Message:
> According to Glen, it'd be aweeeesome if this makes m7.
>
> I'm not the ninjaest js guy out there, so keep that in mind while reviewing.
> I'm
> good at making up superlatives though.
>
> Description:
> Make it possible to hide "most visited" on nnnnnnntp
>
> This implements http://folder/glen/chrome/spec/101_ntp/8_wish&sidebar mocks
> 2,
> 5, 6
>
> It's a bit lame to use a bit in the "sections" bitmask to serialize the
> "should
> show?" information, but the damage here was done when someone decided to
> that
> "THUBMS bit missing" should mean "show a list instead" (instead of adding
> another bit for that).
>
> BUG=55148
> TEST=Hover "Most Visited" bar. Close button should appear. Clicking it
> should
> hide "Most Visited" area and add a button at the window edge. That button
> should
> have a menu that makes it possible to make the "Most Visited" area visible
> again.
>
> Please review this at http://codereview.chromium.org/3455007/show
>
> Affected files:
>  M chrome/browser/dom_ui/shown_sections_handler.h
>  M chrome/browser/dom_ui/shown_sections_handler.cc
>  M chrome/browser/resources/new_new_tab.css
>  M chrome/browser/resources/new_new_tab.html
>  M chrome/browser/resources/new_new_tab.js
>  M chrome/browser/resources/new_tab_theme.css
>  M chrome/browser/resources/ntp/most_visited.js
>
>
>

[Miranda Callahan, 2010-09-20 04:12:53]

LGTM after my minor comments, but you should wait for the ok from a js ninja, (I
see Glen just added aa).

http://codereview.chromium.org/3455007/diff/14008/18002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/14008/18002#newcode19
chrome/browser/dom_ui/shown_sections_handler.h:19: // they are shown as list.
nit1: s/this/THUMB 
nit2: s/as list/as a list

http://codereview.chromium.org/3455007/diff/14008/18002#newcode27
chrome/browser/dom_ui/shown_sections_handler.h:27: // If this is set, the most
visited pages aren't shown at all.
nit: s/this/NO_MOST_VISITED

http://codereview.chromium.org/3455007/diff/14008/18003
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3455007/diff/14008/18003#newcode328
chrome/browser/resources/new_new_tab.css:328: /**/
Is a comment missing betwixt the **?

http://codereview.chromium.org/3455007/diff/14008/18003#newcode337
chrome/browser/resources/new_new_tab.css:337: visibility:hidden;
one space after :

http://codereview.chromium.org/3455007/diff/14008/18003#newcode347
chrome/browser/resources/new_new_tab.css:347: visibility:visible;
as above, space after :

http://codereview.chromium.org/3455007/diff/14008/18003#newcode382
chrome/browser/resources/new_new_tab.css:382: font-size: 16px;  /* FIXME */
FIXME?

http://codereview.chromium.org/3455007/diff/14008/18004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/14008/18004#newcode75
chrome/browser/resources/new_new_tab.html:75: // 128 is used to hide "most
visited", but it's not a section.
But it's in the "Section" enum in ShownSectionsHandler, so shouldn't it be in
this Section var as well, as opposed to below?

[Aaron Boodman, 2010-09-20 04:20:47]

I know it's a pita, but please try to remember to post screen shots and/or mocks
to a public url.

[Aaron Boodman, 2010-09-20 04:20:47]

I know it's a pita, but please try to remember to post screen shots and/or mocks
to a public url.

[Nico, 2010-09-20 04:53:20]

aa: Here are four screenshots that describe comprehensively describe the patch:
http://imgur.com/AVf2yl&OdZKw&hssIE&4Kgvo

(Hovering a section h2 lets a close button appear, clicking that "minimizes" the
section, and choosing the last item from the minimized section's popup menu
restores the section again.)

http://codereview.chromium.org/3455007/diff/14008/18002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/14008/18002#newcode19
chrome/browser/dom_ui/shown_sections_handler.h:19: // they are shown as list.
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> nit1: s/this/THUMB 
> nit2: s/as list/as a list

Done.

http://codereview.chromium.org/3455007/diff/14008/18002#newcode27
chrome/browser/dom_ui/shown_sections_handler.h:27: // If this is set, the most
visited pages aren't shown at all.
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> nit: s/this/NO_MOST_VISITED

Done.

http://codereview.chromium.org/3455007/diff/14008/18003
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3455007/diff/14008/18003#newcode328
chrome/browser/resources/new_new_tab.css:328: /**/
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> Is a comment missing betwixt the **?

No, I added them as helper for me, but then forgot to remove them. Done now.

http://codereview.chromium.org/3455007/diff/14008/18003#newcode337
chrome/browser/resources/new_new_tab.css:337: visibility:hidden;
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> one space after :

Done.

http://codereview.chromium.org/3455007/diff/14008/18003#newcode347
chrome/browser/resources/new_new_tab.css:347: visibility:visible;
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> as above, space after :

Done.

http://codereview.chromium.org/3455007/diff/14008/18003#newcode382
chrome/browser/resources/new_new_tab.css:382: font-size: 16px;  /* FIXME */
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> FIXME?

Done.

http://codereview.chromium.org/3455007/diff/14008/18004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/14008/18004#newcode75
chrome/browser/resources/new_new_tab.html:75: // 128 is used to hide "most
visited", but it's not a section.
On 2010/09/20 04:12:53, Miranda Callahan wrote:
> But it's in the "Section" enum in ShownSectionsHandler, so shouldn't it be in
> this Section var as well, as opposed to below?

No, because there's code like "for (i = 0; section = sections[i]; i++)" in the
js, and it breaks if i add something to this object that isn't really a section.

[Aaron Boodman, 2010-09-20 05:45:40]

On 2010/09/20 04:53:20, Nico wrote:
> aa: Here are four screenshots that describe comprehensively describe the
patch:
> http://imgur.com/AVf2yl&OdZKw&hssIE&4Kgvo

Thank you! (I am in a hotel that is somehow preventing vpn ... grr)

[Aaron Boodman, 2010-09-20 06:09:53]

Big-picture comment: It seems weird to me that this only works for MV, not apps.
I think it should be generalized to work with all sections.

http://codereview.chromium.org/3455007/diff/14008/18005
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3455007/diff/14008/18005#newcode210
chrome/browser/resources/new_new_tab.js:210: footerHeight += button.fixedHeight;
Where does 'fixedHeight' get assigned? Do you mean 'offsetHeight'?

http://codereview.chromium.org/3455007/diff/27001/28003
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3455007/diff/27001/28003#newcode330
chrome/browser/resources/new_new_tab.css:330: top: -1px;
Using pixels for top here will make this not vertically centered when people use
custom font sizes. Instead set this at top:50%, margin-top:-npx, where n is
~half the size of the close dingbat. See .disclosure for an example.

http://codereview.chromium.org/3455007/diff/27001/28003#newcode348
chrome/browser/resources/new_new_tab.css:348: #most-visited-close {
Kind of a nit, but it doesn't seem like the wrapper is needed here. Could you
just position the background at 5px 5px within the parent?

http://codereview.chromium.org/3455007/diff/27001/28003#newcode380
chrome/browser/resources/new_new_tab.css:380: font-size: 16px;
This is out of date now. I recently went through and set these all to
percentages so that they would work correctly with custom font sizes.

Does font-size: inherit work?

http://codereview.chromium.org/3455007/diff/27001/28004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/27001/28004#newcode78
chrome/browser/resources/new_new_tab.html:78: var NO_MOST_VISITED = 128;
Shouldn't the apps section be closeable too? Why the special case for MV?

I think it would be cleaner to change the data model such that each section can
be expanded, hidden, or collapsed. We have the power to migrate the data, we
have existing code that does it for older versions.

That way code that deals with this bit mask now and in the future won't have to
remember the special case of 128, and the apps section will be consistent.

http://codereview.chromium.org/3455007/diff/27001/28005
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3455007/diff/27001/28005#newcode384
chrome/browser/resources/new_new_tab.js:384: //shownSections &=
~NO_MOST_VISITED;
Remove dead code?

http://codereview.chromium.org/3455007/diff/27001/28005#newcode699
chrome/browser/resources/new_new_tab.js:699: el.classList.add('reallyhidden');
We already have the disabled class to make a section completely invisible (I
know the naming is a bit wonky). It would be better to reuse that, rather than
have two.

http://codereview.chromium.org/3455007/diff/27001/28006
File chrome/browser/resources/new_tab_theme.css (right):

http://codereview.chromium.org/3455007/diff/27001/28006#newcode4
chrome/browser/resources/new_tab_theme.css:4: .close-wrapper {
This is only needed when an element needs to cover some other elements in the
foreground with the background. That doesn't seem to be the case for the close
button, since it is to the right of #main.

[arv (Not doing code reviews), 2010-09-20 18:59:12]

I agree with Aaron, it is strange that we only allow this for the MV section.
Other than that I think this is pretty good. I would be fine with this in M7 if
we can revisit this for M8 and make all sections behave the same.

http://codereview.chromium.org/3455007/diff/27001/28001
File chrome/browser/dom_ui/shown_sections_handler.cc (right):

http://codereview.chromium.org/3455007/diff/27001/28001#newcode101
chrome/browser/dom_ui/shown_sections_handler.cc:101: THUMB | RECENT | TIPS |
SYNC | APPS | NO_MOST_VISITED);
The default value should not contain NO_MOST_VISITED

http://codereview.chromium.org/3455007/diff/27001/28004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/27001/28004#newcode242
chrome/browser/resources/new_new_tab.html:242: var
most_visited_initially_visible = !(shownSections & NO_MOST_VISITED);
nit, no underscores

[Nico, 2010-09-20 21:56:24]

Thanks for the comments. Looks like I won't have time to clean this up and get
in today :-( If one of you wants to take it and run with it, feel free, else it
won't make m7 I guess.

[Nico, 2010-09-30 04:54:06]

Rewrote it to support closing all three sections. The code that stores this in
the profile is still missing, but the rest is mostly done, so I'm sending it out
now. Will ping this again once serialization is done, too. I plan to just add 3
more bits to the existing sections pref.

[Nico, 2010-09-30 06:14:11]

Now with serialization support.

If you missed it, there are some privacy concerns about launching without this
all of a sudden, so maybe this is for m7 after all.

[Aaron Boodman, 2010-09-30 15:20:40]

lgtm, thanks for doing this -- arv should take a look too.

http://codereview.chromium.org/3455007/diff/45001/46001
File chrome/browser/dom_ui/shown_sections_handler.cc (right):

http://codereview.chromium.org/3455007/diff/45001/46001#newcode24
chrome/browser/dom_ui/shown_sections_handler.cc:24: void
NotifySectionDisabled(int new_mode, int old_mode, Profile *profile) {
This code should be updated to record all the different state transitions, but
it isn't clear to me whether we should maintain the old one (as is done here)
for some reason.

Can you create a bug and assign to me to revisit this, and put a TODO
referencing the bug here?

http://codereview.chromium.org/3455007/diff/45001/46002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/45001/46002#newcode23
chrome/browser/dom_ui/shown_sections_handler.h:23: THUMB = 1,
Maybe call them MAXIMIZED_THUMB or FULL_THUMB or something, just to clarify?

http://codereview.chromium.org/3455007/diff/45001/46005
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3455007/diff/45001/46005#newcode21
chrome/browser/resources/new_new_tab.js:21: function
getSectionMenuButton(section_id) {
This function has the same name as the above.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode30
chrome/browser/resources/new_new_tab.js:30: if ((visible && !(shownSections &
mask)) ||
The word "mask" here makes this confusing. Maybe "hide_mask"?

http://codereview.chromium.org/3455007/diff/45001/46005#newcode36
chrome/browser/resources/new_new_tab.js:36: // Show this section and hide all
other sections - at most one section can
This code exists in toggleSectionAndAnimate(). It might be worth pulling out.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode88
chrome/browser/resources/new_new_tab.js:88: // TODO(thakis): Use better string
once m7 string freeze is over.
Create a bug for this?

[Aaron Boodman, 2010-09-30 15:21:32]

http://codereview.chromium.org/3455007/diff/45001/46002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/45001/46002#newcode30
chrome/browser/dom_ui/shown_sections_handler.h:30: NO_APPS = 0x400000,
Maybe it's just me, but I find the hex notation confusing. Consider 1 << x
instead?

[arv (Not doing code reviews), 2010-09-30 16:03:38]

LGTM overall. A bunch of style nits and a few minor optimizations and cleanups
wanted.

http://codereview.chromium.org/3455007/diff/45001/46002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/45001/46002#newcode28
chrome/browser/dom_ui/shown_sections_handler.h:28: NO_THUMB = 0x10000,
How about MINIMIZED_*?

http://codereview.chromium.org/3455007/diff/45001/46003
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3455007/diff/45001/46003#newcode359
chrome/browser/resources/new_new_tab.css:359: visibility: visible;
Maybe opacity: 1 and opacity: 0 with a -webkit-transition: opacity .15s

http://codereview.chromium.org/3455007/diff/45001/46003#newcode374
chrome/browser/resources/new_new_tab.css:374: border: 0px;
s/0px/0/

http://codereview.chromium.org/3455007/diff/45001/46003#newcode387
chrome/browser/resources/new_new_tab.css:387: -webkit-transform:rotate(90deg);
spave after :

http://codereview.chromium.org/3455007/diff/45001/46004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/45001/46004#newcode183
chrome/browser/resources/new_new_tab.html:183: class="disabled"
Can you use the disabled attribute instead?

http://codereview.chromium.org/3455007/diff/45001/46004#newcode228
chrome/browser/resources/new_new_tab.html:228: <menu id="apps-menu">
I would just put the closing </menu> at the same line.

http://codereview.chromium.org/3455007/diff/45001/46005
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3455007/diff/45001/46005#newcode12
chrome/browser/resources/new_new_tab.js:12: function
getSectionCloseButton(section_id) {
no underscores in JS (there are some exceptions but this is not one of them)

http://codereview.chromium.org/3455007/diff/45001/46005#newcode13
chrome/browser/resources/new_new_tab.js:13: return document.querySelectorAll(
querySelector

querySelectorAll returns a static node list of all the matching nodes.
querySelector only returns the first found matching element.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode17
chrome/browser/resources/new_new_tab.js:17: function
getSectionMenuButton(section_id) {
sectionId

http://codereview.chromium.org/3455007/diff/45001/46005#newcode18
chrome/browser/resources/new_new_tab.js:18: return document.querySelectorAll('#'
+ section_id + '-button')[0];
querySelector

http://codereview.chromium.org/3455007/diff/45001/46005#newcode31
chrome/browser/resources/new_new_tab.js:31: (!visible && (shownSections &
mask)))
All these extra parentheses makes my head spin... but that is because I'm used
to JS and the Google style guide says to only use parentheses where required by
syntax and semantix.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode34
chrome/browser/resources/new_new_tab.js:34: if (visible) {
if (visible && section) {

http://codereview.chromium.org/3455007/diff/45001/46005#newcode82
chrome/browser/resources/new_new_tab.js:82: function addClosedMenuFooter(menu,
section_id, mask, opt_section) {
Optional arguments is one of the cases where underscores are correct

http://codereview.chromium.org/3455007/diff/45001/46005#newcode190
chrome/browser/resources/new_new_tab.js:190: var click_handler = function(e) {
function clickHandler(e) {

} // no semicolon

http://codereview.chromium.org/3455007/diff/45001/46007
File chrome/browser/resources/ntp/most_visited.js (right):

http://codereview.chromium.org/3455007/diff/45001/46007#newcode618
chrome/browser/resources/ntp/most_visited.js:618: if (item.filler) {
negate and skip continue here

[Nico, 2010-09-30 19:28:17]

all done, submitting

http://codereview.chromium.org/3455007/diff/45001/46001
File chrome/browser/dom_ui/shown_sections_handler.cc (right):

http://codereview.chromium.org/3455007/diff/45001/46001#newcode24
chrome/browser/dom_ui/shown_sections_handler.cc:24: void
NotifySectionDisabled(int new_mode, int old_mode, Profile *profile) {
On 2010/09/30 15:20:40, Aaron Boodman wrote:
> This code should be updated to record all the different state transitions, but
> it isn't clear to me whether we should maintain the old one (as is done here)
> for some reason.
> 
> Can you create a bug and assign to me to revisit this, and put a TODO
> referencing the bug here?

Done.

http://codereview.chromium.org/3455007/diff/45001/46002
File chrome/browser/dom_ui/shown_sections_handler.h (right):

http://codereview.chromium.org/3455007/diff/45001/46002#newcode28
chrome/browser/dom_ui/shown_sections_handler.h:28: NO_THUMB = 0x10000,
On 2010/09/30 16:03:38, arv wrote:
> How about MINIMIZED_*?

Done.

http://codereview.chromium.org/3455007/diff/45001/46002#newcode30
chrome/browser/dom_ui/shown_sections_handler.h:30: NO_APPS = 0x400000,
On 2010/09/30 15:21:32, Aaron Boodman wrote:
> Maybe it's just me, but I find the hex notation confusing. Consider 1 << x
> instead?

Done.

http://codereview.chromium.org/3455007/diff/45001/46003
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3455007/diff/45001/46003#newcode359
chrome/browser/resources/new_new_tab.css:359: visibility: visible;
On 2010/09/30 16:03:38, arv wrote:
> Maybe opacity: 1 and opacity: 0 with a -webkit-transition: opacity .15s

Done. Nice!

http://codereview.chromium.org/3455007/diff/45001/46003#newcode374
chrome/browser/resources/new_new_tab.css:374: border: 0px;
On 2010/09/30 16:03:38, arv wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/3455007/diff/45001/46003#newcode387
chrome/browser/resources/new_new_tab.css:387: -webkit-transform:rotate(90deg);
On 2010/09/30 16:03:38, arv wrote:
> spave after :

Done.

http://codereview.chromium.org/3455007/diff/45001/46004
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3455007/diff/45001/46004#newcode228
chrome/browser/resources/new_new_tab.html:228: <menu id="apps-menu">
On 2010/09/30 16:03:38, arv wrote:
> I would just put the closing </menu> at the same line.

Done.

http://codereview.chromium.org/3455007/diff/45001/46005
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3455007/diff/45001/46005#newcode12
chrome/browser/resources/new_new_tab.js:12: function
getSectionCloseButton(section_id) {
On 2010/09/30 16:03:38, arv wrote:
> no underscores in JS (there are some exceptions but this is not one of them)

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode13
chrome/browser/resources/new_new_tab.js:13: return document.querySelectorAll(
On 2010/09/30 16:03:38, arv wrote:
> querySelector
> 
> querySelectorAll returns a static node list of all the matching nodes.
> querySelector only returns the first found matching element.

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode17
chrome/browser/resources/new_new_tab.js:17: function
getSectionMenuButton(section_id) {
On 2010/09/30 16:03:38, arv wrote:
> sectionId

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode18
chrome/browser/resources/new_new_tab.js:18: return document.querySelectorAll('#'
+ section_id + '-button')[0];
On 2010/09/30 16:03:38, arv wrote:
> querySelector
> 

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode21
chrome/browser/resources/new_new_tab.js:21: function
getSectionMenuButton(section_id) {
On 2010/09/30 15:20:40, Aaron Boodman wrote:
> This function has the same name as the above.

Luckily they do the same thing, only slightly differently :-) Removed the more
complex one.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode30
chrome/browser/resources/new_new_tab.js:30: if ((visible && !(shownSections &
mask)) ||
On 2010/09/30 15:20:40, Aaron Boodman wrote:
> The word "mask" here makes this confusing. Maybe "hide_mask"?

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode31
chrome/browser/resources/new_new_tab.js:31: (!visible && (shownSections &
mask)))
On 2010/09/30 16:03:38, arv wrote:
> All these extra parentheses makes my head spin... but that is because I'm used
> to JS and the Google style guide says to only use parentheses where required
by
> syntax and semantix.

Removed a couple. I had people demand them before elsewhere, because they
couldn't remember the precedence of || vs &&.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode34
chrome/browser/resources/new_new_tab.js:34: if (visible) {
On 2010/09/30 16:03:38, arv wrote:
> if (visible && section) {

No. |section| is Section.THUMB for most visited, Section.APPS for apps and
undefined for recently closed. We still want this branch to run for the recently
closed section.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode36
chrome/browser/resources/new_new_tab.js:36: // Show this section and hide all
other sections - at most one section can
On 2010/09/30 15:20:40, Aaron Boodman wrote:
> This code exists in toggleSectionAndAnimate(). It might be worth pulling out.

Done.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode88
chrome/browser/resources/new_new_tab.js:88: // TODO(thakis): Use better string
once m7 string freeze is over.
On 2010/09/30 15:20:40, Aaron Boodman wrote:
> Create a bug for this?

Removed the TODO instead, it doesn't look all that bad now that I look at it.

http://codereview.chromium.org/3455007/diff/45001/46005#newcode190
chrome/browser/resources/new_new_tab.js:190: var click_handler = function(e) {
On 2010/09/30 16:03:38, arv wrote:
> function clickHandler(e) {
> 
> } // no semicolon

Done.

http://codereview.chromium.org/3455007/diff/45001/46007
File chrome/browser/resources/ntp/most_visited.js (right):

http://codereview.chromium.org/3455007/diff/45001/46007#newcode618
chrome/browser/resources/ntp/most_visited.js:618: if (item.filler) {
On 2010/09/30 16:03:38, arv wrote:
> negate and skip continue here

Done.

[Nico, 2010-10-01 21:36:53]

I fixed the issues that got this reverted. The diff is very small, see bug.

Aaron, do you want to patch this in and play with it first, or should I just
land and you play with it once you pull the next time?

[Aaron Boodman, 2010-10-01 21:47:54]

On 2010/10/01 21:36:53, Nico wrote:
> I fixed the issues that got this reverted. The diff is very small, see bug.
> 
> Aaron, do you want to patch this in and play with it first, or should I just
> land and you play with it once you pull the next time?

I will patch in and have a play. Thanks for getting back to this!

[Aaron Boodman, 2010-10-01 22:08:52]

Can you make it so that the apps section opens when you install an app and it is
hidden? It is kind of weird that the NTP opens and nothing happens.

Also, I think the menus should be moved up about 2px when they are open, if
possible.

LGTM other than that.

[Nico, 2010-10-03 03:21:30]

On Fri, Oct 1, 2010 at 3:08 PM,  <aa@chromium.org> wrote:
> Can you make it so that the apps section opens when you install an app and
> it is
> hidden? It is kind of weird that the NTP opens and nothing happens.

Sent you a different CL for that.

> Also, I think the menus should be moved up about 2px when they are open, if
> possible.

I'm using the stock menu code, so there isn't much I can do about this.

> LGTM other than that.

Thanks, landed.

>
> http://codereview.chromium.org/3455007/show
>