Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9358031:
Added new adaptive "Suggest" tab on the New Tab Page, behing the flag, for the experiments. (Closed)
|
Created:
8 years, 10 months ago by GeorgeY Modified:
6 years, 3 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded new adaptive "Suggest" tab on the New Tab Page, behing the flag, for the experiments.
To experiment with presentation edit chrome/browser/resources/ntp4/suggested_page.css|.js
To experiment with data edit chrome/browser/ui/webui/ntp/suggested_page_handler.h|.cc
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124340
Patch Set 1 #Patch Set 2 : Fix clearing of the data #Patch Set 3 : remove extra line #
Total comments: 55
Patch Set 4 : Removed "most visited" references #
Total comments: 8
Patch Set 5 : Adressed the comments. #
Total comments: 4
Patch Set 6 : adressed 2 out of 3 comments #Patch Set 7 : added dynamic loading of the JS and CSS #
Total comments: 23
Patch Set 8 : Addressed comments #
Total comments: 6
Patch Set 9 : Adressed comments 2 #
Total comments: 2
Patch Set 10 : #Patch Set 11 : Moved suggestions before apps, fixed already present js bug, alphabetised .css #
Total comments: 3
Patch Set 12 : Made apps page to add new pages correctly #Patch Set 13 : Simplified code to add app pages in succession #Patch Set 14 : put the suggested before apps page #Patch Set 15 : Fixed bug in card_slider.js which prevented correct card be selected in dome cases #
Total comments: 80
Patch Set 16 : Addressed Dan's comments #
Total comments: 13
Patch Set 17 : Addressed Dan's comments #
Total comments: 4
Patch Set 18 : adressed latest coments #Patch Set 19 : fix the unit-tests #Messages
Total messages: 35 (0 generated)
This is the experimental page behind the flag that we're going to experiment with.
drive by essay https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.js:431: function setSuggestedPages(data, hasBlacklistedUrls) { unused param from copy pasta? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.js:562: var setSuggestedPages = ntp4.setSuggestedPages; alpha https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/page_list_view.js:63: * @type {!Element|undefined} I think just !Element is fine here (as that means it could be null), but I see you're just following convention. Long term these should probably all be changed to null instead of undefined, I think... https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. Why did you duplicate this style? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:3: * found in the LICENSE file. nit: * found in the LICENSE file. */ https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:15: -webkit-box-orient: vertical; -webkit rules should go at the top, alpha sorted, then followed by standardized rules, alpha sorted. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); You'll obviously want to de-duplicate a lot of this code. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:144: // ['NTP_Suggested', this.index, 8]); remove https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:153: cr.isMac && e.metaKey && e.keyCode == 8) { // Cmd + Backspace shouldn't this be fn + Backspace on mac? (perhaps better question for estade) https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:356: * @param {Object} newData The new Suggested page list. these should be {Array}s https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:364: for (var j = 0; j < newData.length; j++) { nit: why j on outer for? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:420: }; no ; https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/new_tab_ui.cc:258: return false; why not return CommandLine::ForCurrentProcess()->HasSwitch( switches::kEnableSuggestedTabPage); ? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/suggested_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:55: profile->GetChromeURLDataManager()->AddDataSource(thumbnail_src); nit: make look like line below https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:56: nit: remove newline https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:60: // TODO(georgey) change it to provide our data. nit: Capitalize and describe what "it" means in this context (like by replacing it with a noun, not pronoun) here and through out (the other 3 or 4 occurrences). https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:61: history::TopSites* ts = profile->GetTopSites(); nit: ts could be thumbnail source, make more descriptive var name https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:80: base::Unretained(this))); nit: no newline https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:95: if (!NewTabUI::IsSuggestedPageEnabled()) I don't think these preventative measures are necessary (as they seem like they'd only be triggered by running chrome.send('getSuggested') from the webkit console, in which case it's easier for the silly hacker to just close their browser than to crash it via this means). If anything, you should probably be DCHECK()ing, IMO, but I'm not strongly opinionated on the matter. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:112: *(pages_value_.get()), nit: can't one just use pages_value_.release()? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:121: if (ts) { when is this NULL? https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:139: DCHECK(args->GetSize() != 0); nit: > 0 (I understand this is a size_t, but say someday we change ListValue to have a size of -1 if it hasn't been initialized or something...) https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:146: // TODO(georgey) clear blacklist. I'd prefer you just wait until the TODO() is done before adding this method (no need to make many devs and bots compile it in the mean time or possible leave it around as crufty code later). https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:153: pages_value_.reset(new ListValue); Don't know what the style guide says about optional () for default constructors, but I'd put them as you're doing this in JavaScript (as does everywhere else in the file). https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:155: const history::MostVisitedURL& url = data[data.size() - i - 1]; nit: name this something different so it doesn't look like you accidentally copy/pasta'd url.url. below, IMO https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:187: // TODO(georgey) blacklist an URL. same for these two stubs https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/suggested_page_handler.h (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/common/chrome... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/common/chrome... chrome/common/chrome_switches.cc:552: // Enables suggested new tab page for internal experiments. nit: // Enables experimental suggested pane in New Tab page. (it's not internal if it gets committed to chromium...right?)
it seems the UI is exactly the same as most visited. Can you instead refactor the code such that you can share the UI code but just swap out the data source? http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/new_tab.js:562: var setSuggestedPages = ntp4.setSuggestedPages; do not add to this list. see TODO http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/page_list_view.js:550: this.shownPageIndex = this.getAppsPageIndex(page); you copy pasta'd the wrong line here. Copy L547 http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... File chrome/browser/resources/ntp4/suggested_page.js (right): http://codereview.chromium.org/9358031/diff/6001/chrome/browser/resources/ntp... chrome/browser/resources/ntp4/suggested_page.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I prefer this feature be called SuggestionsPage, not SuggestedPage
We want to experiment with both UI presentation and data sources (I actually have a 50% cl that does the second part). So the current Most Visited page is a very nice *template* for further changes. It is highly unlikely that it will be the same in the end, but in that case it would be easily merged/reused. It is just a starting point currently. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.js:431: function setSuggestedPages(data, hasBlacklistedUrls) { On 2012/02/09 01:43:06, Dan Beam wrote: > unused param from copy pasta? nope. We want to blacklist URLs in the suggested page eventually, just not in the first experiments. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.js:562: var setSuggestedPages = ntp4.setSuggestedPages; On 2012/02/09 01:43:06, Dan Beam wrote: > alpha What do you mean by that. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/page_list_view.js:63: * @type {!Element|undefined} On 2012/02/09 01:43:06, Dan Beam wrote: > I think just !Element is fine here (as that means it could be null), but I see > you're just following convention. Long term these should probably all be > changed to null instead of undefined, I think... noted for when we do update it. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/09 01:43:06, Dan Beam wrote: > Why did you duplicate this style? Modified copy of the Most Visited - see above. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:3: * found in the LICENSE file. On 2012/02/09 01:43:06, Dan Beam wrote: > nit: > > * found in the LICENSE file. */ Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:15: -webkit-box-orient: vertical; On 2012/02/09 01:43:06, Dan Beam wrote: > -webkit rules should go at the top, alpha sorted, then followed by standardized > rules, alpha sorted. Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); On 2012/02/09 01:43:06, Dan Beam wrote: > You'll obviously want to de-duplicate a lot of this code. Yes, eventually - see above. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:144: // ['NTP_Suggested', this.index, 8]); On 2012/02/09 01:43:06, Dan Beam wrote: > remove Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:356: * @param {Object} newData The new Suggested page list. On 2012/02/09 01:43:06, Dan Beam wrote: > these should be {Array}s Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:364: for (var j = 0; j < newData.length; j++) { On 2012/02/09 01:43:06, Dan Beam wrote: > nit: why j on outer for? Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:420: }; On 2012/02/09 01:43:06, Dan Beam wrote: > no ; Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/new_tab_ui.cc:258: return false; On 2012/02/09 01:43:06, Dan Beam wrote: > why not > return CommandLine::ForCurrentProcess()->HasSwitch( > switches::kEnableSuggestedTabPage); > ? sure https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/suggested_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:55: profile->GetChromeURLDataManager()->AddDataSource(thumbnail_src); On 2012/02/09 01:43:06, Dan Beam wrote: > nit: make look like line below Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:56: On 2012/02/09 01:43:06, Dan Beam wrote: > nit: remove newline Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:60: // TODO(georgey) change it to provide our data. On 2012/02/09 01:43:06, Dan Beam wrote: > nit: Capitalize and describe what "it" means in this context (like by replacing > it with a noun, not pronoun) here and through out (the other 3 or 4 > occurrences). Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:61: history::TopSites* ts = profile->GetTopSites(); On 2012/02/09 01:43:06, Dan Beam wrote: > nit: ts could be thumbnail source, make more descriptive var name Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:80: base::Unretained(this))); On 2012/02/09 01:43:06, Dan Beam wrote: > nit: no newline Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:95: if (!NewTabUI::IsSuggestedPageEnabled()) On 2012/02/09 01:43:06, Dan Beam wrote: > I don't think these preventative measures are necessary (as they seem like > they'd only be triggered by running chrome.send('getSuggested') from the webkit > console, in which case it's easier for the silly hacker to just close their > browser than to crash it via this means). If anything, you should probably be > DCHECK()ing, IMO, but I'm not strongly opinionated on the matter. Yep, remnants of the bug when this function was always called - removed. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:112: *(pages_value_.get()), On 2012/02/09 01:43:06, Dan Beam wrote: > nit: can't one just use pages_value_.release()? Nope, you get memory leak. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:121: if (ts) { On 2012/02/09 01:43:06, Dan Beam wrote: > when is this NULL? AFAIK, for TestProfileImpl https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:139: DCHECK(args->GetSize() != 0); On 2012/02/09 01:43:06, Dan Beam wrote: > nit: > 0 (I understand this is a size_t, but say someday we change ListValue to > have a size of -1 if it hasn't been initialized or something...) Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:146: // TODO(georgey) clear blacklist. On 2012/02/09 01:43:06, Dan Beam wrote: > I'd prefer you just wait until the TODO() is done before adding this method (no > need to make many devs and bots compile it in the mean time or possible leave it > around as crufty code later). We *do* want to add it before the first internal release. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:153: pages_value_.reset(new ListValue); On 2012/02/09 01:43:06, Dan Beam wrote: > Don't know what the style guide says about optional () for default constructors, > but I'd put them as you're doing this in JavaScript (as does everywhere else in > the file). Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:155: const history::MostVisitedURL& url = data[data.size() - i - 1]; On 2012/02/09 01:43:06, Dan Beam wrote: > nit: name this something different so it doesn't look like you accidentally > copy/pasta'd url.url. below, IMO Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.cc:187: // TODO(georgey) blacklist an URL. On 2012/02/09 01:43:06, Dan Beam wrote: > same for these two stubs see above https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... File chrome/browser/ui/webui/ntp/suggested_page_handler.h (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/browser/ui/we... chrome/browser/ui/webui/ntp/suggested_page_handler.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/09 01:43:06, Dan Beam wrote: > 2012 Done. https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/common/chrome... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/4001/chrome/common/chrome... chrome/common/chrome_switches.cc:552: // Enables suggested new tab page for internal experiments. On 2012/02/09 01:43:06, Dan Beam wrote: > nit: // Enables experimental suggested pane in New Tab page. > (it's not internal if it gets committed to chromium...right?) Done. https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.js:562: var setSuggestedPages = ntp4.setSuggestedPages; On 2012/02/09 01:54:23, Evan Stade wrote: > do not add to this list. see TODO Done. https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... chrome/browser/resources/ntp4/page_list_view.js:550: this.shownPageIndex = this.getAppsPageIndex(page); On 2012/02/09 01:54:23, Evan Stade wrote: > you copy pasta'd the wrong line here. Copy L547 Shoot. missed it - done :) https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/09 01:54:23, Evan Stade wrote: > I prefer this feature be called SuggestionsPage, not SuggestedPage Done.
https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/10 00:00:36, GeorgeY wrote: > On 2012/02/09 01:54:23, Evan Stade wrote: > > I prefer this feature be called SuggestionsPage, not SuggestedPage > > Done. I still see "suggested" in most places. https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.html:59: <script src="suggested_page.js"></script> do not include this or any new resources in the new tab page if the feature is not enabled. It slows down the load. https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:36: .filler { you can't just copy and then fiddle with the css. Rules like this one will apply across the whole page; filler class is being used by most visited so if you change something here it will apply to the most visited page as well.
https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/6001/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/11 01:56:05, Evan Stade wrote: > On 2012/02/10 00:00:36, GeorgeY wrote: > > On 2012/02/09 01:54:23, Evan Stade wrote: > > > I prefer this feature be called SuggestionsPage, not SuggestedPage > > > > Done. > > I still see "suggested" in most places. renamed all https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... chrome/browser/resources/ntp4/new_tab.html:59: <script src="suggested_page.js"></script> On 2012/02/11 01:56:05, Evan Stade wrote: > do not include this or any new resources in the new tab page if the feature is > not enabled. It slows down the load. Done. https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... File chrome/browser/resources/ntp4/suggested_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/4003/chrome/browser/resou... chrome/browser/resources/ntp4/suggested_page.css:36: .filler { On 2012/02/11 01:56:05, Evan Stade wrote: > you can't just copy and then fiddle with the css. Rules like this one will apply > across the whole page; filler class is being used by most visited so if you > change something here it will apply to the most visited page as well. Done.
http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> do the same thing for this one as you did for the js. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:118: suggestions_script.src = "chrome://newtab/suggestions_page.js"; src can just be suggestions_page.js (it's relative) use single quotes in javascript http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:119: document.getElementsByTagName('head')[0].appendChild(suggestions_script); document.querySelector('head') http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:546: localStrings: localStrings, don't export. Just create new LocalStrings wherever you're using this. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), this will now put the new app page after the suggestions page http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/suggestions_page.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/suggestions_page.js:425: ntp4.localStrings.getString('suggestions'), random amount of indent http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.h:68: SUGGESTIONS_PAGE_ID = 4 << kPageIdOffset, you must update histograms.xml as well.
http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/17 20:07:35, Evan Stade wrote: > do the same thing for this one as you did for the js. oh nevermind. I see what you did.
http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/17 20:08:14, Evan Stade wrote: > On 2012/02/17 20:07:35, Evan Stade wrote: > > do the same thing for this one as you did for the js. > > oh nevermind. I see what you did. Yes. Apparently the css refused to load anywhere else (adding of <link...> in js did not help - it works if you add css code directly, but not if it is just reference to a file). This way it does not take the time or space, and it is similar to what other places doing for CSS (though they *always* load something). http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:118: suggestions_script.src = "chrome://newtab/suggestions_page.js"; On 2012/02/17 20:07:35, Evan Stade wrote: > src can just be suggestions_page.js (it's relative) > > use single quotes in javascript Done. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:119: document.getElementsByTagName('head')[0].appendChild(suggestions_script); On 2012/02/17 20:07:35, Evan Stade wrote: > document.querySelector('head') Done. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:546: localStrings: localStrings, On 2012/02/17 20:07:35, Evan Stade wrote: > don't export. Just create new LocalStrings wherever you're using this. Done. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/17 20:07:35, Evan Stade wrote: > this will now put the new app page after the suggestions page You mean vice versa? The suggested page is not loaded yet and after it loads it will be added at the end. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/suggestions_page.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/suggestions_page.js:425: ntp4.localStrings.getString('suggestions'), On 2012/02/17 20:07:35, Evan Stade wrote: > random amount of indent Sorry, changed the call, forgot to adjust the indent. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.h:68: SUGGESTIONS_PAGE_ID = 4 << kPageIdOffset, On 2012/02/17 20:07:35, Evan Stade wrote: > you must update histograms.xml as well. Should I do this in the same cl? I always thought that by default out-of-range histograms go to out-of-range bucket. And considering that it does not send anything yet anyway...
is there really no crbug to track this? if not, please file one. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/21 23:32:23, GeorgeY wrote: > On 2012/02/17 20:08:14, Evan Stade wrote: > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > do the same thing for this one as you did for the js. > > > > oh nevermind. I see what you did. > > Yes. Apparently the css refused to load anywhere else (adding of <link...> in js > did not help - it works if you add css code directly, but not if it is just > reference to a file). This way it does not take the time or space, and it is > similar to what other places doing for CSS (though they *always* load > something). > does this cause a "resource failed to load" error for the no suggestions case? http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/21 23:32:23, GeorgeY wrote: > On 2012/02/17 20:07:35, Evan Stade wrote: > > this will now put the new app page after the suggestions page > You mean vice versa? The suggested page is not loaded yet and after it loads it > will be added at the end. > no. This is called when a new app page is added after the NTP has been loaded (i.e. a second open NTP creates a new page). http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.h:68: SUGGESTIONS_PAGE_ID = 4 << kPageIdOffset, On 2012/02/21 23:32:23, GeorgeY wrote: > On 2012/02/17 20:07:35, Evan Stade wrote: > > you must update histograms.xml as well. > Should I do this in the same cl? I always thought that by default out-of-range > histograms go to out-of-range bucket. And considering that it does not send > anything yet anyway... > no, you have to do it in a separate CL, but you can do that and have it up for review simultaneously. http://codereview.chromium.org/9358031/diff/24001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9358031/diff/24001/chrome/app/generated_resour... chrome/app/generated_resources.grd:9048: <message name="IDS_NEW_TAB_SUGGESTED" SUGGESTIONS http://codereview.chromium.org/9358031/diff/24001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/suggestions_page.js (right): http://codereview.chromium.org/9358031/diff/24001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/suggestions_page.js:20: function Suggested() { Suggestions http://codereview.chromium.org/9358031/diff/24001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9358031/diff/24001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:558: const char kEnableSuggestionsTabPage[] = "enable-suggested-ntp"; nit: enable-ntp-suggestions
http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.html (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/23 02:10:20, Evan Stade wrote: > On 2012/02/21 23:32:23, GeorgeY wrote: > > On 2012/02/17 20:08:14, Evan Stade wrote: > > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > > do the same thing for this one as you did for the js. > > > > > > oh nevermind. I see what you did. > > > > Yes. Apparently the css refused to load anywhere else (adding of <link...> in > js > > did not help - it works if you add css code directly, but not if it is just > > reference to a file). This way it does not take the time or space, and it is > > similar to what other places doing for CSS (though they *always* load > > something). > > > > does this cause a "resource failed to load" error for the no suggestions case? No, no error that I see - I think empty .css is OK. http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/23 02:10:20, Evan Stade wrote: > On 2012/02/21 23:32:23, GeorgeY wrote: > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > this will now put the new app page after the suggestions page > > You mean vice versa? The suggested page is not loaded yet and after it loads > it > > will be added at the end. > > > > no. This is called when a new app page is added after the NTP has been loaded > (i.e. a second open NTP creates a new page). I think I understand what you mean: if there are more apps than fit on one page they will be added after the Suggestions NTP, which is currently the last one, so multiple app pages will be "split up" with Suggestions page in the middle. Am I correct? How do I mitigate that? http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right): http://codereview.chromium.org/9358031/diff/16001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_handler.h:68: SUGGESTIONS_PAGE_ID = 4 << kPageIdOffset, On 2012/02/23 02:10:20, Evan Stade wrote: > On 2012/02/21 23:32:23, GeorgeY wrote: > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > you must update histograms.xml as well. > > Should I do this in the same cl? I always thought that by default out-of-range > > histograms go to out-of-range bucket. And considering that it does not send > > anything yet anyway... > > > > no, you have to do it in a separate CL, but you can do that and have it up for > review simultaneously. cl 3970015 http://codereview.chromium.org/9358031/diff/24001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/9358031/diff/24001/chrome/app/generated_resour... chrome/app/generated_resources.grd:9048: <message name="IDS_NEW_TAB_SUGGESTED" On 2012/02/23 02:10:20, Evan Stade wrote: > SUGGESTIONS Done. http://codereview.chromium.org/9358031/diff/24001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/suggestions_page.js (right): http://codereview.chromium.org/9358031/diff/24001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/suggestions_page.js:20: function Suggested() { On 2012/02/23 02:10:20, Evan Stade wrote: > Suggestions Renamed to suggestion to better reflect that it is one tile http://codereview.chromium.org/9358031/diff/24001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/9358031/diff/24001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:558: const char kEnableSuggestionsTabPage[] = "enable-suggested-ntp"; On 2012/02/23 02:10:20, Evan Stade wrote: > nit: enable-ntp-suggestions Done.
is there really no crbug to track this? if not, please file one. https://chromiumcodereview.appspot.com/9358031/diff/16001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/16001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/24 01:52:07, GeorgeY wrote: > On 2012/02/23 02:10:20, Evan Stade wrote: > > On 2012/02/21 23:32:23, GeorgeY wrote: > > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > > this will now put the new app page after the suggestions page > > > You mean vice versa? The suggested page is not loaded yet and after it loads > > it > > > will be added at the end. > > > > > > > no. This is called when a new app page is added after the NTP has been loaded > > (i.e. a second open NTP creates a new page). > > I think I understand what you mean: if there are more apps than fit on one page > they will be added after the Suggestions NTP, which is currently the last one, > so multiple app pages will be "split up" with Suggestions page in the middle. Am > I correct? yes. > How do I mitigate that? by changing this function.
https://chromiumcodereview.appspot.com/9358031/diff/16001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/16001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:378: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/24 02:11:36, Evan Stade wrote: > On 2012/02/24 01:52:07, GeorgeY wrote: > > On 2012/02/23 02:10:20, Evan Stade wrote: > > > On 2012/02/21 23:32:23, GeorgeY wrote: > > > > On 2012/02/17 20:07:35, Evan Stade wrote: > > > > > this will now put the new app page after the suggestions page > > > > You mean vice versa? The suggested page is not loaded yet and after it > loads > > > it > > > > will be added at the end. > > > > > > > > > > no. This is called when a new app page is added after the NTP has been > loaded > > > (i.e. a second open NTP creates a new page). > > > > I think I understand what you mean: if there are more apps than fit on one > page > > they will be added after the Suggestions NTP, which is currently the last one, > > so multiple app pages will be "split up" with Suggestions page in the middle. > Am > > I correct? > > yes. > > > How do I mitigate that? > > by changing this function. Done + fixed the bug in the appendTile function.
http://codereview.chromium.org/9358031/diff/31001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/31001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:391: this.appendTilePage(new ntp4.AppsPage(), this is still just appending the new apps page... you have to pass a reference node
http://codereview.chromium.org/9358031/diff/31001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/31001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:391: this.appendTilePage(new ntp4.AppsPage(), On 2012/02/24 22:49:45, Evan Stade wrote: > this is still just appending the new apps page... you have to pass a reference > node I changed the code above to add suggestions page prior to apps page, so the apps page will be at the end. See the last cl lines 206-211 in this file
http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:206: if (typeof ntp4.SuggestionsPage != 'undefined' && this is a hack.
http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/page_list_view.js (right): http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/page_list_view.js:206: if (typeof ntp4.SuggestionsPage != 'undefined' && On 2012/02/24 23:28:01, Evan Stade wrote: > this is a hack. I will not argue with you what is a hack or not here, but I changed the code.
On 2012/02/25 01:30:54, GeorgeY wrote: > http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... > File chrome/browser/resources/ntp4/page_list_view.js (right): > > http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... > chrome/browser/resources/ntp4/page_list_view.js:206: if (typeof > ntp4.SuggestionsPage != 'undefined' && > On 2012/02/24 23:28:01, Evan Stade wrote: > > this is a hack. > I will not argue with you what is a hack or not here, but I changed the code. I thought you decided to place the suggestions page before the apps pages? When I said it was a hack, I was referring to your methodology, not the intent. Use the parameter which exists for that very purpose. When I applied this patch locally, then opened two NTPs, and dragged an app to a new app page, I got a lot of console errors and the two NTPs were in different states.
On 2012/02/27 19:19:38, Evan Stade wrote: > On 2012/02/25 01:30:54, GeorgeY wrote: > > > http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... > > File chrome/browser/resources/ntp4/page_list_view.js (right): > > > > > http://codereview.chromium.org/9358031/diff/32001/chrome/browser/resources/nt... > > chrome/browser/resources/ntp4/page_list_view.js:206: if (typeof > > ntp4.SuggestionsPage != 'undefined' && > > On 2012/02/24 23:28:01, Evan Stade wrote: > > > this is a hack. > > I will not argue with you what is a hack or not here, but I changed the code. > > I thought you decided to place the suggestions page before the apps pages? When > I said it was a hack, I was referring to your methodology, not the intent. Use > the parameter which exists for that very purpose. When I applied this patch > locally, then opened two NTPs, and dragged an app to a new app page, I got a lot > of console errors and the two NTPs were in different states. The order does not matter, but I put it before the apps for conformity. There was a bug in card_slider which prevented correct insertions in some cases. All works now.
https://chromiumcodereview.appspot.com/9358031/diff/32001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/32001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:424: var localStrings = new LocalStrings; why no parens here but parens below when using new? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> Can we load this conditionally as well? <link rel="stylesheet" i18n-values="href:suggestionsPageStyle"> https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:120: } Seems easier just to do this in i18n-values unless there's timing issues. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:528: } this should not be required https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:341: } easier to understand, IMO: var lastAppsPage = this.appsPages[this.appsPages.length - 1]; var lastAppsPageIndex = Array.prototype.indexOf.call(this.tilePages, lastAppsPage); var nextPageAfterApps = lastAppsPageIndex != -1 ? this.tilePages[lastAppsPageIndex + 1] : null; https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:560: getFirstAppsPage: function() { You don't use this anywhere and this can be accomplished via this.appsPage[0] (this.appsPage is a live NodeList, so it'll never be undefined)? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.css:5: .suggestions { seems like almost all of this could be shared with most visited CSS... is there a reason why we're not doing this? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:5: cr.define('ntp4', function() { this may be changing to ntp (from ntp4) soon, btw https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:63: '<span class="title"></span>'; should me make a template for this? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); SUGGESTIONS_PAGE? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:164: doc https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:171: } }; https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:176: } }; https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:178: var undoAll = { I'm also not totally sure about whether there should be \n after { here, but I'm not sure it's specified either... https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:183: } }; https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:194: * animate. remove indented line https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:233: * data for this tile (for dragging outside of the NTP). @param {Event.DataTransfer} dataTransfer The drag event data store. (or something to this effect) https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:302: * Update the tiles after a change to |data_|. nit: |this.data_| https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:424: var localStrings = new LocalStrings; new LocalStrings(); IMO, as you use calling parens on new objects everywhere else https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:429: ntp4.getNewTabView().getFirstAppsPage()); Move this into page_list_view.js and only do after DOM is ready or this script is loaded, IMO. If you change to i18n-values this might be easier, but I can't remember when processValues() runs. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:431: ntp4.getNewTabView().updateSliderCards(); I don't think this is required as appendTilePage() should automatically call this. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:440: * selected. Why is this necessary? In theory if you're inserting into the middle of some cards, the cardSlider will do the right thing already. If you've found a case this doesn't work, please explain it to me and perhaps we can make a fix rather than an override behavior. Also, if this ends up staying, you should probably convert all these to opt_propertyBag and pass {forceChange: true} as this @param instead (I wrote this code and regret making these optional params). https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:450: if (opt_forceChange != 'undefined' && opt_forceChange) typeof opt_forceChange https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:143: html_source->AddResource("suggestions_page.js", "text/javascript", nit: should this be application/javascript? if you're just following precedent, that's fine, but text/javascript is the old mime type for .js files. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:344: scoped_refptr<RefCountedStaticMemory> resource_bytes( nit: indent is weird https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:367: std::string NewTabUI::NewTabHTMLSource::GetMimeType(const std::string& s) can you rename |s| to something more descriptive? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.h (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.h:95: Profile* profile_; nit: newline https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:40: #include "ui/base/l10n/l10n_util.h" are all these still necessary? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:149: page_value->SetBoolean("filler", true); if they're always fillers, can you remove this logic? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:173: // Most visited urls changed, query again. s/most visited/ https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.h (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:27: // The handler for Javascript messages related to the "most visited" view. s/"most visited"/"suggestions"/ (or "suggested", whatever you and estade decided) https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:67: // Send a request to the HistoryService to get the most visited pages. s/most visited/suggest{ions,ed}/ https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:71: void SetPagesValueFromTopSites(const history::MostVisitedURLList& data); should these be a history::MostVisitedURLList? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:91: // The most visited URLs, in priority order. s/most visited/ ... you get the idea
Please finish the review - I addressed the comments. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/28 20:14:24, Dan Beam wrote: > Can we load this conditionally as well? > > <link rel="stylesheet" i18n-values="href:suggestionsPageStyle"> It is loaded conditionally (see the code) https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:120: } On 2012/02/28 20:14:24, Dan Beam wrote: > Seems easier just to do this in i18n-values unless there's timing issues. ? This is the way it is done elsewhere. Not sure it can be easier https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:528: } On 2012/02/28 20:14:24, Dan Beam wrote: > this should not be required It is. Either newTabView or getNewTabView needs to be exported. This way is clearer https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:341: } On 2012/02/28 20:14:24, Dan Beam wrote: > easier to understand, IMO: > > var lastAppsPage = this.appsPages[this.appsPages.length - 1]; > var lastAppsPageIndex = > Array.prototype.indexOf.call(this.tilePages, lastAppsPage); > var nextPageAfterApps = lastAppsPageIndex != -1 ? > this.tilePages[lastAppsPageIndex + 1] : null; there is a bug in your code: this.appsPages.length can be 0. So I think my way is clearer :) https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:560: getFirstAppsPage: function() { On 2012/02/28 20:14:24, Dan Beam wrote: > You don't use this anywhere and this can be accomplished via this.appsPage[0] > (this.appsPage is a live NodeList, so it'll never be undefined)? Yes it is used. See suggestions_page.js https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.css (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.css:5: .suggestions { On 2012/02/28 20:14:24, Dan Beam wrote: > seems like almost all of this could be shared with most visited CSS... is there > a reason why we're not doing this? Already explained twice above: this *will* change before the final release. It is right now a clone of the most visited, but it will not be in the end. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:5: cr.define('ntp4', function() { On 2012/02/28 20:14:24, Dan Beam wrote: > this may be changing to ntp (from ntp4) soon, btw Hopefully, I'll commit before then :). https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:63: '<span class="title"></span>'; On 2012/02/28 20:14:24, Dan Beam wrote: > should me make a template for this? May be, but then again - the problem is *this* file starts changing as soon as i commit it in both presentation and data source. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); On 2012/02/28 20:14:24, Dan Beam wrote: > SUGGESTIONS_PAGE? Removed this altogether as it works this way now, but it will not be when released to external user. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:164: On 2012/02/28 20:14:24, Dan Beam wrote: > doc Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:171: } On 2012/02/28 20:14:24, Dan Beam wrote: > }; Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:176: } On 2012/02/28 20:14:24, Dan Beam wrote: > }; Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:183: } On 2012/02/28 20:14:24, Dan Beam wrote: > }; Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:194: * animate. On 2012/02/28 20:14:24, Dan Beam wrote: > remove indented line Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:233: * data for this tile (for dragging outside of the NTP). On 2012/02/28 20:14:24, Dan Beam wrote: > @param {Event.DataTransfer} dataTransfer The drag event data store. > > (or something to this effect) Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:302: * Update the tiles after a change to |data_|. On 2012/02/28 20:14:24, Dan Beam wrote: > nit: |this.data_| Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:424: var localStrings = new LocalStrings; On 2012/02/28 20:14:24, Dan Beam wrote: > new LocalStrings(); IMO, as you use calling parens on new objects everywhere > else Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:429: ntp4.getNewTabView().getFirstAppsPage()); On 2012/02/28 20:14:24, Dan Beam wrote: > Move this into page_list_view.js and only do after DOM is ready or this script > is loaded, IMO. If you change to i18n-values this might be easier, but I can't > remember when processValues() runs. That is how Evan suggested to do this. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:431: ntp4.getNewTabView().updateSliderCards(); On 2012/02/28 20:14:24, Dan Beam wrote: > I don't think this is required as appendTilePage() should automatically call > this. It does not. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:440: * selected. On 2012/02/28 20:14:24, Dan Beam wrote: > Why is this necessary? In theory if you're inserting into the middle of some > cards, the cardSlider will do the right thing already. If you've found a case > this doesn't work, please explain it to me and perhaps we can make a fix rather > than an override behavior. > > Also, if this ends up staying, you should probably convert all these to > opt_propertyBag and pass {forceChange: true} as this @param instead (I wrote > this code and regret making these optional params). I just fixed a *bug* that was here: if you inserted a card with index prior to currently selected one it moved selected one to a new index and it was not re-selected correctly. I am fine with changing it to property bag in the other change list, but this one is very big as it is and needs to be committed ASAP. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:450: if (opt_forceChange != 'undefined' && opt_forceChange) On 2012/02/28 20:14:24, Dan Beam wrote: > typeof opt_forceChange Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:143: html_source->AddResource("suggestions_page.js", "text/javascript", On 2012/02/28 20:14:24, Dan Beam wrote: > nit: should this be application/javascript? if you're just following precedent, > that's fine, but text/javascript is the old mime type for .js files. sure https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:344: scoped_refptr<RefCountedStaticMemory> resource_bytes( On 2012/02/28 20:14:24, Dan Beam wrote: > nit: indent is weird Four spaces - what is the problem? https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:367: std::string NewTabUI::NewTabHTMLSource::GetMimeType(const std::string& s) On 2012/02/28 20:14:24, Dan Beam wrote: > can you rename |s| to something more descriptive? Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.h (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.h:95: Profile* profile_; On 2012/02/28 20:14:24, Dan Beam wrote: > nit: newline Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:40: #include "ui/base/l10n/l10n_util.h" On 2012/02/28 20:14:24, Dan Beam wrote: > are all these still necessary? For now yes, as it mirrors the MostVisited behavior - in the new cl which changes the behavior, some of these are removed. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:149: page_value->SetBoolean("filler", true); On 2012/02/28 20:14:24, Dan Beam wrote: > if they're always fillers, can you remove this logic? OK, it was removed in the other cl anyway, removed here as well https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:173: // Most visited urls changed, query again. On 2012/02/28 20:14:24, Dan Beam wrote: > s/most visited/ Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.h (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:27: // The handler for Javascript messages related to the "most visited" view. On 2012/02/28 20:14:24, Dan Beam wrote: > s/"most visited"/"suggestions"/ (or "suggested", whatever you and estade > decided) Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:67: // Send a request to the HistoryService to get the most visited pages. On 2012/02/28 20:14:24, Dan Beam wrote: > s/most visited/suggest{ions,ed}/ Done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:71: void SetPagesValueFromTopSites(const history::MostVisitedURLList& data); On 2012/02/28 20:14:24, Dan Beam wrote: > should these be a history::MostVisitedURLList? Yes, this is the container we use now https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.h:91: // The most visited URLs, in priority order. On 2012/02/28 20:14:24, Dan Beam wrote: > s/most visited/ ... you get the idea removed
lgtm
https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > Can we load this conditionally as well? > > > > <link rel="stylesheet" i18n-values="href:suggestionsPageStyle"> > It is loaded conditionally (see the code) But it seems inefficient to me as we'd still have to make a request and return a null reponse, but I guess setting this to about:blank is the same thing, so whatever (still would be best to just not insert at all unless necessary). https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:528: } On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > this should not be required > It is. Either newTabView or getNewTabView needs to be exported. This way is > clearer It doesn't need to be exported if you just use onload of the script and closure. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:341: } On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > easier to understand, IMO: > > > > var lastAppsPage = this.appsPages[this.appsPages.length - 1]; > > var lastAppsPageIndex = > > Array.prototype.indexOf.call(this.tilePages, lastAppsPage); > > var nextPageAfterApps = lastAppsPageIndex != -1 ? > > this.tilePages[lastAppsPageIndex + 1] : null; > > there is a bug in your code: this.appsPages.length can be 0. > So I think my way is clearer :) A bug in example code doesn't make your code clearer. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:63: '<span class="title"></span>'; On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > should me make a template for this? > May be, but then again - the problem is *this* file starts changing as soon as i > commit it in both presentation and data source. That's the case with other pages be we still do this (http://goo.gl/2cVEC). We can wait to see if the feature makes it out from behind a flag first. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > SUGGESTIONS_PAGE? > > Removed this altogether as it works this way now, but it will not be when > released to external user. Make sure you replace before you move to FieldTrial or whatever the non-behind-a-flag ramp-up method is. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:440: * selected. On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > Why is this necessary? In theory if you're inserting into the middle of some > > cards, the cardSlider will do the right thing already. If you've found a case > > this doesn't work, please explain it to me and perhaps we can make a fix > rather > > than an override behavior. > > > > Also, if this ends up staying, you should probably convert all these to > > opt_propertyBag and pass {forceChange: true} as this @param instead (I wrote > > this code and regret making these optional params). > I just fixed a *bug* that was here: if you inserted a card with index prior to > currently selected one it moved selected one to a new index and it was not > re-selected correctly. I am fine with changing it to property bag in the other > change list, but this one is very big as it is and needs to be committed ASAP. I don't see a bug fix, I see an override. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:344: scoped_refptr<RefCountedStaticMemory> resource_bytes( On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > nit: indent is weird > Four spaces - what is the problem? It's fine. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:40: #include "ui/base/l10n/l10n_util.h" On 2012/02/28 21:17:42, GeorgeY wrote: > On 2012/02/28 20:14:24, Dan Beam wrote: > > are all these still necessary? > > For now yes, as it mirrors the MostVisited behavior - in the new cl which > changes the behavior, some of these are removed. The reason I ask is because I searched for l10n in this file and found nothing so it seems like some of these are stale. https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:119: document.querySelector('head').appendChild(suggestions_script); Why not something along the lines of suggestions_script.onload = function() { newTabView.appendTilePage(new ntp4.SuggestionsPage(), localStrings.getString('suggestions'), false, newTabView.appsPages[0]); chrome.send('getSuggestions'); }; ? https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:561: if (typeof this.tilePages == 'undefined') I don't see how this can ever be undefined (as initialize() will always be called first synchronously before loading the script that would use this). https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:564: if (this.tilePages[i] instanceof ntp4.AppsPage) assuming this.appsPages and this.tilePages always exists, this does the same thing: this.appsPages[0] or this.appsPages[0] || null if you really want to return null https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:614: this.updatePageSwitchers(); I think this is a better place to call updateSliderCard() if (!this.isStartingUp_()) { this.updatePageSwitchers(); this.updateSliderCards(); }
https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:442: selectCard: function(newCardIndex, opt_animate, opt_dontNotify, btw, you should probably be preferring a param per line when you overflow 80 char wrap, i.e.: selectCard: function(newCardIndex, opt_animate, opt_dontNotify, opt_forceChange) { // func body }, both in JS and C++.
https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:382: int resource_id) { matching what you've done here https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.h (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.h:88: void AddResource(const char* resource, const char *mime_type, here as well
https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.html (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.html:29: <link rel="stylesheet" href="chrome://newtab/suggestions_page.css"> On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > Can we load this conditionally as well? > > > > > > <link rel="stylesheet" i18n-values="href:suggestionsPageStyle"> > > It is loaded conditionally (see the code) > > But it seems inefficient to me as we'd still have to make a request and return a > null reponse, but I guess setting this to about:blank is the same thing, so > whatever (still would be best to just not insert at all unless necessary). The problem is that dynamically adding .css similarly to .js does not do the request to chrome. This way request happens, but just returned immediately if the .css is empty. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:528: } On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > this should not be required > > It is. Either newTabView or getNewTabView needs to be exported. This way is > > clearer > > It doesn't need to be exported if you just use onload of the script and closure. done. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:341: } On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > easier to understand, IMO: > > > > > > var lastAppsPage = this.appsPages[this.appsPages.length - 1]; > > > var lastAppsPageIndex = > > > Array.prototype.indexOf.call(this.tilePages, lastAppsPage); > > > var nextPageAfterApps = lastAppsPageIndex != -1 ? > > > this.tilePages[lastAppsPageIndex + 1] : null; > > > > there is a bug in your code: this.appsPages.length can be 0. > > So I think my way is clearer :) > > A bug in example code doesn't make your code clearer. What i meant is that if the 3 line code is buggy, it is *not* simple. Changed it your way, though https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:63: '<span class="title"></span>'; On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > should me make a template for this? > > May be, but then again - the problem is *this* file starts changing as soon as > i > > commit it in both presentation and data source. > > That's the case with other pages be we still do this (http://goo.gl/2cVEC). We > can wait to see if the feature makes it out from behind a flag first. OK. Again this template *will* change (not '*may* change') https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:140: ntp4.APP_LAUNCH.NTP_MOST_VISITED]); On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > SUGGESTIONS_PAGE? > > > > Removed this altogether as it works this way now, but it will not be when > > released to external user. > > Make sure you replace before you move to FieldTrial or whatever the > non-behind-a-flag ramp-up method is. Yes we will add the UMA stats later, for now we still haven't decided *what* we want to display. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:440: * selected. On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > Why is this necessary? In theory if you're inserting into the middle of > some > > > cards, the cardSlider will do the right thing already. If you've found a > case > > > this doesn't work, please explain it to me and perhaps we can make a fix > > rather > > > than an override behavior. > > > > > > Also, if this ends up staying, you should probably convert all these to > > > opt_propertyBag and pass {forceChange: true} as this @param instead (I wrote > > > this code and regret making these optional params). > > I just fixed a *bug* that was here: if you inserted a card with index prior to > > currently selected one it moved selected one to a new index and it was not > > re-selected correctly. I am fine with changing it to property bag in the other > > change list, but this one is very big as it is and needs to be committed ASAP. > > I don't see a bug fix, I see an override. Bug: insert a card *prior* to the selected card. Card is moved to the newCardIndex, while still selected, so this.cards_[newCardIndex].classList.contains('selected-card') returns true, thus failing to change sected status on the card forever. This happens only when new card is inserted, so I added this parameter to override this behavior in this case. https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/45001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:40: #include "ui/base/l10n/l10n_util.h" On 2012/02/28 22:30:12, Dan Beam wrote: > On 2012/02/28 21:17:42, GeorgeY wrote: > > On 2012/02/28 20:14:24, Dan Beam wrote: > > > are all these still necessary? > > > > For now yes, as it mirrors the MostVisited behavior - in the new cl which > > changes the behavior, some of these are removed. > > The reason I ask is because I searched for l10n in this file and found nothing > so it seems like some of these are stale. Oh, cool, more for me to remove :) haven't gone through the original set, did it now. https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/ntp4/new_tab.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/new_tab.js:119: document.querySelector('head').appendChild(suggestions_script); On 2012/02/28 22:30:12, Dan Beam wrote: > Why not something along the lines of > > suggestions_script.onload = function() { > newTabView.appendTilePage(new ntp4.SuggestionsPage(), > localStrings.getString('suggestions'), > false, > newTabView.appsPages[0]); > chrome.send('getSuggestions'); > }; > > ? Oh, this works as well, done. https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:561: if (typeof this.tilePages == 'undefined') On 2012/02/28 22:30:12, Dan Beam wrote: > I don't see how this can ever be undefined (as initialize() will always be > called first synchronously before loading the script that would use this). Function removed https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:614: this.updatePageSwitchers(); On 2012/02/28 22:30:12, Dan Beam wrote: > I think this is a better place to call updateSliderCard() > > if (!this.isStartingUp_()) { > this.updatePageSwitchers(); > this.updateSliderCards(); > } Tried it (and removed calls to updateSliderCards() everywhere else) and the page gets completely broken in some cases https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/shared/js/cr/ui/card_slider.js:442: selectCard: function(newCardIndex, opt_animate, opt_dontNotify, On 2012/02/28 22:59:22, Dan Beam wrote: > btw, you should probably be preferring a param per line when you overflow 80 > char wrap, i.e.: > > selectCard: function(newCardIndex, > opt_animate, > opt_dontNotify, > opt_forceChange) { > // func body > }, > > both in JS and C++. Done. https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/new_tab_ui.cc:382: int resource_id) { On 2012/02/28 23:01:23, Dan Beam wrote: > matching what you've done here :)
lgtm w/nits https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... File chrome/browser/resources/ntp4/page_list_view.js (right): https://chromiumcodereview.appspot.com/9358031/diff/49002/chrome/browser/reso... chrome/browser/resources/ntp4/page_list_view.js:614: this.updatePageSwitchers(); On 2012/02/29 00:15:45, GeorgeY wrote: > On 2012/02/28 22:30:12, Dan Beam wrote: > > I think this is a better place to call updateSliderCard() > > > > if (!this.isStartingUp_()) { > > this.updatePageSwitchers(); > > this.updateSliderCards(); > > } > Tried it (and removed calls to updateSliderCards() everywhere else) and the page > gets completely broken in some cases Weird, OK, it's fine to omit this. https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:49: * for a blank thumbnail. can you add a TODO() to make this a template? https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:137: * Allow blacklisting suggestions site using the keyboard. * @param {Event} e The keydown event.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/georgey@chromium.org/9358031/56001
Try job failure for 9358031-56001 (retry) on mac_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/georgey@chromium.org/9358031/56001
Sorry forgot to publish... https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... File chrome/browser/resources/ntp4/suggestions_page.js (right): https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:49: * for a blank thumbnail. On 2012/02/29 00:47:02, Dan Beam wrote: > can you add a TODO() to make this a template? Done. https://chromiumcodereview.appspot.com/9358031/diff/50008/chrome/browser/reso... chrome/browser/resources/ntp4/suggestions_page.js:137: * Allow blacklisting suggestions site using the keyboard. On 2012/02/29 00:47:02, Dan Beam wrote: > * @param {Event} e The keydown event. Done.
Try job failure for 9358031-56001 (retry) (retry) on mac_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/georgey@chromium.org/9358031/67001
Change committed as 124340
Message was sent while issue was closed.
On 2012/03/01 02:51:33, I haz the power (commit-bot) wrote: > Change committed as 124340 ntp.SuggestionsPage() still exists in new_tab.js code as part of chrome://apps code, though suggestions_page.js file is not included in chrome://apps. Should we do something with that? Right now I only experience compilation problems like ## /usr/local/google/home/vitalyp/src/chrome/browser/resources/ntp4/new_tab.js:182: ERROR - Property SuggestionsPage never defined on ntp ## newTabView.appendTilePage(new ntp.SuggestionsPage(), ## ^ |
