Merge local_omnibox_popup into local_ntp. Render the Google logo and fakebox if Google is the sear…

Merge local_omnibox_popup into local_ntp.  Render the Google logo and fakebox if Google is the search provider.  Prevent the most visited/fakebox animation from triggering when the NTP is revealed.

BUG=230172, 225894, 231137
COLLABORATOR=samarth@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195323

[jeremycho, 2013-04-15 16:26:13]

dbeam for local_ntp - most of the diffs are from copying over
local_omnibox_popup.js/css

samarth for c/b/s and c/b/u/s.

[samarth, 2013-04-15 19:01:34]

A couple of other comments:

1) You should be able to delete local_omnibox_popup, no?

2) You need to call hideBars()/showBars() at the correct times to make the
bookmarks bar go away when the dropdown is shown. You can check with Michael
about how to do so.

Thanks,
Samarth

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:13: var isGooglePage = false;
See
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/brows...
for an example of how to check the search engine in C++ code.

I'd prefer to add that check in this CL so we can test both versions easily.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:94: * The fakebox.
Please add comments explaining what a "fakebox" is.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:465: // The Google page uses a
fixed layout instead.
Can we use the same basic layout for both modes, for simplicity?  Also, IIRC,
using a non-fixed layout leads to things jumping around on the NTP when the bmb
appears or disappears.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:507: * @param {boolean} show
True to show the NTP.
How about |visible| instead of |show|.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:581: //  Suggestion
Implementation
s/Suggestion/Dropdown/

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:1007: if (isGooglePage) {
More robust to check for the existence of the fakebox instead?

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:1033:
window.removeEventListener(
nit: fits one line?

https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:119: filename ==
kWhiteGoogleLogoFilename;
This was fine when we had a couple of files but it's getting a bit out of hand
now. How about defining a struct with the following fields?
const char* filename;
int resource_identifier;
const char* mime_type;

You could just define an array of these structures and simplify all the code in
this file using it.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:465:
CreateOverlay(chrome::kChromeSearchLocalNtpUrl,
Replace this (and below) with a GetLocalInstantURL() helper that does the
is-google check.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:933: if (overlay_ &&
overlay_->IsLocalOverlay())
Replace all instances of IsLocalOverlay with IsLocalNTP (even better, replace
them both with IsLocalInstantPage).

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1272: instant_url =
chrome::kChromeSearchLocalNtpUrl;
Use the GetLocalInstantURL() function here too.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1291: instant_url =
chrome::kChromeSearchLocalNtpUrl;
Use the GetLocalInstantURL() function here too.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1618: bool
InstantController::UseInstantTabToShowSuggestions() const {
You can just inline this check wherever this function is used.

[jeremycho, 2013-04-16 01:42:12]

Addressed comments and added the 2x logo images.  PTAL.

On 2013/04/15 19:01:34, samarth wrote:
> A couple of other comments:
> 
> 1) You should be able to delete local_omnibox_popup, no?

Eventually, but perhaps that should wait until Jered's changes are committed?

> 2) You need to call hideBars()/showBars() at the correct times to make the
> bookmarks bar go away when the dropdown is shown. You can check with Michael
> about how to do so.

Added.

> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> File chrome/browser/resources/local_ntp/local_ntp.js (right):
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:13: var isGooglePage = false;
> See
>
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/brows...
> for an example of how to check the search engine in C++ code.
> 
> I'd prefer to add that check in this CL so we can test both versions easily.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:94: * The fakebox.
> Please add comments explaining what a "fakebox" is.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:465: // The Google page uses a
> fixed layout instead.
> Can we use the same basic layout for both modes, for simplicity?  Also, IIRC,
> using a non-fixed layout leads to things jumping around on the NTP when the
bmb
> appears or disappears.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:507: * @param {boolean} show
> True to show the NTP.
> How about |visible| instead of |show|.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:581: //  Suggestion
> Implementation
> s/Suggestion/Dropdown/
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:1007: if (isGooglePage) {
> More robust to check for the existence of the fakebox instead?
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_ntp/local_ntp.js:1033:
> window.removeEventListener(
> nit: fits one line?
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
> File chrome/browser/search/local_ntp_source.cc (right):
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
> chrome/browser/search/local_ntp_source.cc:119: filename ==
> kWhiteGoogleLogoFilename;
> This was fine when we had a couple of files but it's getting a bit out of hand
> now. How about defining a struct with the following fields?
> const char* filename;
> int resource_identifier;
> const char* mime_type;
> 
> You could just define an array of these structures and simplify all the code
in
> this file using it.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> File chrome/browser/ui/search/instant_controller.cc (right):
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:465:
> CreateOverlay(chrome::kChromeSearchLocalNtpUrl,
> Replace this (and below) with a GetLocalInstantURL() helper that does the
> is-google check.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:933: if (overlay_ &&
> overlay_->IsLocalOverlay())
> Replace all instances of IsLocalOverlay with IsLocalNTP (even better, replace
> them both with IsLocalInstantPage).
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:1272: instant_url =
> chrome::kChromeSearchLocalNtpUrl;
> Use the GetLocalInstantURL() function here too.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:1291: instant_url =
> chrome::kChromeSearchLocalNtpUrl;
> Use the GetLocalInstantURL() function here too.
> 
>
https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:1618: bool
> InstantController::UseInstantTabToShowSuggestions() const {
> You can just inline this check wherever this function is used.

[jeremycho, 2013-04-16 01:42:37]

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:13: var isGooglePage = false;
On 2013/04/15 19:01:34, samarth wrote:
> See
>
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/brows...
> for an example of how to check the search engine in C++ code.
> 
> I'd prefer to add that check in this CL so we can test both versions easily.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:94: * The fakebox.
On 2013/04/15 19:01:34, samarth wrote:
> Please add comments explaining what a "fakebox" is.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:465: // The Google page uses a
fixed layout instead.
That makes sense to me - will discuss on the bug thread
(https://code.google.com/p/chromium/issues/detail?id=229806).

On 2013/04/15 19:01:34, samarth wrote:
> Can we use the same basic layout for both modes, for simplicity?  Also, IIRC,
> using a non-fixed layout leads to things jumping around on the NTP when the
bmb
> appears or disappears.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:507: * @param {boolean} show
True to show the NTP.
On 2013/04/15 19:01:34, samarth wrote:
> How about |visible| instead of |show|.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:581: //  Suggestion
Implementation
On 2013/04/15 19:01:34, samarth wrote:
> s/Suggestion/Dropdown/

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:1007: if (isGooglePage) {
On 2013/04/15 19:01:34, samarth wrote:
> More robust to check for the existence of the fakebox instead?

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_ntp/local_ntp.js:1033:
window.removeEventListener(
On 2013/04/15 19:01:34, samarth wrote:
> nit: fits one line?

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:119: filename ==
kWhiteGoogleLogoFilename;
On 2013/04/15 19:01:34, samarth wrote:
> This was fine when we had a couple of files but it's getting a bit out of hand
> now. How about defining a struct with the following fields?
> const char* filename;
> int resource_identifier;
> const char* mime_type;
> 
> You could just define an array of these structures and simplify all the code
in
> this file using it.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:465:
CreateOverlay(chrome::kChromeSearchLocalNtpUrl,
On 2013/04/15 19:01:34, samarth wrote:
> Replace this (and below) with a GetLocalInstantURL() helper that does the
> is-google check.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:933: if (overlay_ &&
overlay_->IsLocalOverlay())
On 2013/04/15 19:01:34, samarth wrote:
> Replace all instances of IsLocalOverlay with IsLocalNTP (even better, replace
> them both with IsLocalInstantPage).
Done.  Will do the rename in a followup.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1272: instant_url =
chrome::kChromeSearchLocalNtpUrl;
On 2013/04/15 19:01:34, samarth wrote:
> Use the GetLocalInstantURL() function here too.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1291: instant_url =
chrome::kChromeSearchLocalNtpUrl;
On 2013/04/15 19:01:34, samarth wrote:
> Use the GetLocalInstantURL() function here too.

Done.

https://codereview.chromium.org/13905008/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:1618: bool
InstantController::UseInstantTabToShowSuggestions() const {
On 2013/04/15 19:01:34, samarth wrote:
> You can just inline this check wherever this function is used.

Done.

[Dan Beam, 2013-04-16 03:01:44]

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:42: -webkit-border-radius: 1px;
border-radius

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:48: border-top: 1px solid
#a0a0a0;
border-top-color (assuming this works)

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:49: box-shadow: inset 0 1px 2px
rgba(0,0,0,0.1);
rgba(0, 0, 0, 0.1);

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:59: border-top: 1px solid
#909090;
same nit

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:70: #fakebox>input {
#fakebox > input {

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:76: top: 0;
what you trying to accomplish here?

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:88: @-webkit-keyframes blink {
just

61.54% {
  opacity: 1;
},
100% {
  opacity: 0;
}

should work I think

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:348: border: 0;
nit: border-width: 0; or border: none;

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:66: //
=============================================================================
why isn't this defined as a LocalNtp class or something along these lines?

[jeremycho, 2013-04-16 16:18:33]

Thanks for the comments.  PTAL.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:42: -webkit-border-radius: 1px;
On 2013/04/16 03:01:44, Dan Beam wrote:
> border-radius

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:48: border-top: 1px solid
#a0a0a0;
On 2013/04/16 03:01:44, Dan Beam wrote:
> border-top-color (assuming this works)

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:49: box-shadow: inset 0 1px 2px
rgba(0,0,0,0.1);
On 2013/04/16 03:01:44, Dan Beam wrote:
> rgba(0, 0, 0, 0.1);

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:59: border-top: 1px solid
#909090;
On 2013/04/16 03:01:44, Dan Beam wrote:
> same nit

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:70: #fakebox>input {
On 2013/04/16 03:01:44, Dan Beam wrote:
> #fakebox > input {

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:76: top: 0;
On 2013/04/16 03:01:44, Dan Beam wrote:
> what you trying to accomplish here?

To have the input expand into the entire fakebox (for pasting text).

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:88: @-webkit-keyframes blink {
On 2013/04/16 03:01:44, Dan Beam wrote:
> just
> 
> 61.54% {
>   opacity: 1;
> },
> 100% {
>   opacity: 0;
> }
> 
> should work I think

Without 61.55%, I'm seeing the cursor fade due to interpolation.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:348: border: 0;
On 2013/04/16 03:01:44, Dan Beam wrote:
> nit: border-width: 0; or border: none;

Done.

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://chromiumcodereview.appspot.com/13905008/diff/13001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:66: //
=============================================================================
On 2013/04/16 03:01:44, Dan Beam wrote:
> why isn't this defined as a LocalNtp class or something along these lines?

This code is only used here and initialized immediately, so the additional
encapsulation seemed unnecessary - it doesn't feel like OO programming (I see
similar cases throughout chrome/browser/resources).

[samarth, 2013-04-16 16:55:09]

A few more comments but it's looking good.  I'll patch it in and play with the
demo some more later today.

Thanks!
Samarth

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:966: isGooglePage =
location.href.indexOf('isGoogle' != -1);
You can just define this inline as a constant at the top, no?

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
File chrome/browser/search/local_ntp_source.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:64: std::string stripped_path =
StripParameters(path);
const std::string

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:67: identifier =
kResources[i].identifier;
You can just declare identifier here (or skip that variable altogether).

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:80: std::string stripped_path =
StripParameters(path);
const

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:82: if (stripped_path ==
kResources[i].filename) {
nit: no braces

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:95: std::string filename =
request->url().path().substr(1);
How come you don't need to strip the query params here?

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:97: if (filename ==
kResources[i].filename) {
nit: no braces (It's chrome C++ style to omit braces for single-line blocks).

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:105: std::string
LocalNtpSource::StripParameters(const std::string& path) const{
Instead of a private method, just define this at the top of this file in an
anonymous namespace.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
File chrome/browser/search/search.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/search.cc:339: entry->GetVirtualURL() ==
GURL(chrome::kChromeSearchLocalNtpUrl) ||
How about just entry->GetVirtualURL() == GetLocalInstantURL()?

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/search.cc:477: if (!default_provider)
I think you still want to use chrome::kChromeSearchLocalNtpUrl in this case. 
Something is probably bad if this is true but we still want to give them a local
NTP.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:555: // Unless we are talking to
the local overlay, skip SearchProvider, since
This check is no longer accurate (note that it currently assumes that only the
overlay can be local).

I would define a second bool:
const bool using_local_page = (instant_tab_ && instant_tab_->IsLocal()) ||
overlay_->IsLocal();

The check below then will be if (from_search_provider && !using_local_page)

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1123: if (instant_tab_ &&
nit: fits on one line?

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1136: bool can_use_instant_tab =
instant_tab_ &&
nit: fits on one line?

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_page.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_page.cc:20: bool InstantPage::IsLocalNTP()
const {
How about renaming to just IsLocal?  (or IsLocalPage if you think that's
clearer; InstantPage::IsLocalPage feels a little redundant to me)

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_page.cc:26: bool InstantPage::IsLocalOverlay()
const {
You can delete this method.

[samarth, 2013-04-16 16:56:58]

On 2013/04/16 01:42:12, jeremycho wrote:
> On 2013/04/15 19:01:34, samarth wrote:
> > A couple of other comments:
> > 
> > 1) You should be able to delete local_omnibox_popup, no?
> 
> Eventually, but perhaps that should wait until Jered's changes are committed?'

No, if your change lands first, Jered will need to make his changes in the local
ntp file.  If Jered's lands first, you'll want need to merge his changes into
the merged file.  Either way, we should delete the omnibox popup file in this
change.

Thanks,
Samarth

[jeremycho, 2013-04-16 20:39:09]

Will delete local_omnibox_popup in the next patch set.  Because I'm not a
committer, I've been unable to move .png files, so I'll do that in a follow-up
CL.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:966: isGooglePage =
location.href.indexOf('isGoogle' != -1);
On 2013/04/16 16:55:10, samarth wrote:
> You can just define this inline as a constant at the top, no?

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
File chrome/browser/search/local_ntp_source.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:64: std::string stripped_path =
StripParameters(path);
On 2013/04/16 16:55:10, samarth wrote:
> const std::string

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:67: identifier =
kResources[i].identifier;
On 2013/04/16 16:55:10, samarth wrote:
> You can just declare identifier here (or skip that variable altogether).

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:80: std::string stripped_path =
StripParameters(path);
On 2013/04/16 16:55:10, samarth wrote:
> const

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:82: if (stripped_path ==
kResources[i].filename) {
On 2013/04/16 16:55:10, samarth wrote:
> nit: no braces

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:95: std::string filename =
request->url().path().substr(1);
On 2013/04/16 16:55:10, samarth wrote:
> How come you don't need to strip the query params here?

The request doesn't contain them, unlike the path parameter in the other
methods.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:97: if (filename ==
kResources[i].filename) {
On 2013/04/16 16:55:10, samarth wrote:
> nit: no braces (It's chrome C++ style to omit braces for single-line blocks).

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/local_ntp_source.cc:105: std::string
LocalNtpSource::StripParameters(const std::string& path) const{
On 2013/04/16 16:55:10, samarth wrote:
> Instead of a private method, just define this at the top of this file in an
> anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
File chrome/browser/search/search.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/search.cc:339: entry->GetVirtualURL() ==
GURL(chrome::kChromeSearchLocalNtpUrl) ||
On 2013/04/16 16:55:10, samarth wrote:
> How about just entry->GetVirtualURL() == GetLocalInstantURL()?

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/sea...
chrome/browser/search/search.cc:477: if (!default_provider)
On 2013/04/16 16:55:10, samarth wrote:
> I think you still want to use chrome::kChromeSearchLocalNtpUrl in this case. 
> Something is probably bad if this is true but we still want to give them a
local
> NTP.

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:555: // Unless we are talking to
the local overlay, skip SearchProvider, since
On 2013/04/16 16:55:10, samarth wrote:
> This check is no longer accurate (note that it currently assumes that only the
> overlay can be local).
> 
> I would define a second bool:
> const bool using_local_page = (instant_tab_ && instant_tab_->IsLocal()) ||
> overlay_->IsLocal();
> 
> The check below then will be if (from_search_provider && !using_local_page)

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1123: if (instant_tab_ &&
On 2013/04/16 16:55:10, samarth wrote:
> nit: fits on one line?

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1136: bool can_use_instant_tab =
instant_tab_ &&
On 2013/04/16 16:55:10, samarth wrote:
> nit: fits on one line?

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_page.cc (right):

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_page.cc:20: bool InstantPage::IsLocalNTP()
const {
On 2013/04/16 16:55:10, samarth wrote:
> How about renaming to just IsLocal?  (or IsLocalPage if you think that's
> clearer; InstantPage::IsLocalPage feels a little redundant to me)

Done.

https://chromiumcodereview.appspot.com/13905008/diff/17001/chrome/browser/ui/...
chrome/browser/ui/search/instant_page.cc:26: bool InstantPage::IsLocalOverlay()
const {
On 2013/04/16 16:55:10, samarth wrote:
> You can delete this method.

Done.

[jeremycho, 2013-04-16 22:39:44]

Deleted local_omnibox_popup.

+sky for chrome_browser.gypi, browser_resources.grd

We're shooting to enable this by default for M28, which means committing this
hopefully by April 19th.

[samarth, 2013-04-17 00:19:34]

Two other issues I noticed:

(1) Open local NTP, type a URL, hit enter.  We see a flash of the NTP before the
page loads.  This is different from current google.com behavior.  You should
double check with Gideon, but I think we should follow that behavior.

(2) There are small layout differences between the google.com home page and the
Google local NTP.  It's only noticeable if you look at them side by side (I'll
email you screenshots separately).  

Both of these are minor enough, and this change is big enough, that I'd prefer
you fix them in follow up changes (unless the fix is trivial).

Thanks,
Samarth

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:307: #suggestions-box {
FYI, Kausalya just made some changes here in crrev.com/14080012 so be careful
when you sync past her change.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:10: * @type {boolean}
@const also?

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:12: var isGooglePage =
location.href.indexOf('isGoogle' != -1);
This should be location.href.indexOf(..)  != -1

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:574: return true;
indent

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:492: if (!ntp_ ||
(ShouldSwitchToLocalNTP() && !ntp_->IsLocal()))
There is a bug here.  To reproduce:
0) Enable local-only...
1) Open NTP
2) Switch search provider to Yahoo
3) Open NTP

You'll get the Google local NTP in step 3.  You can fix this by changing the
!ntp->IsLocal() check to ntp_->instant_url() != GetLocalInstantURL().  In other
words, you do want to reload the local NTP if its URL has changed.  (I'd just
move this clause into ShouldSwitchToLocalNTP().  Note also that you will need to
add an instant_url() member to InstantNTP, like the one that exists for
InstantOverlay.)

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:289: //  determined. For
|ignore_blacklist| look at comments in GetInstantURL().
nit: no space before "determined"

[jeremycho, 2013-04-17 00:59:23]

Thanks for testing - I'll look at the additional issues you mentioned, but agree
it's more important to get this in soon.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:307: #suggestions-box {
Acknowledged.  display:inline-block is not what we want here.
On 2013/04/17 00:19:34, samarth wrote:
> FYI, Kausalya just made some changes here in crrev.com/14080012 so be careful
> when you sync past her change.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:10: * @type {boolean}
On 2013/04/17 00:19:34, samarth wrote:
> @const also?

Done.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:12: var isGooglePage =
location.href.indexOf('isGoogle' != -1);
On 2013/04/17 00:19:34, samarth wrote:
> This should be location.href.indexOf(..)  != -1

Ouch!  Done.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:574: return true;
On 2013/04/17 00:19:34, samarth wrote:
> indent

Done.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:492: if (!ntp_ ||
(ShouldSwitchToLocalNTP() && !ntp_->IsLocal()))
On 2013/04/17 00:19:34, samarth wrote:
> There is a bug here.  To reproduce:
> 0) Enable local-only...
> 1) Open NTP
> 2) Switch search provider to Yahoo
> 3) Open NTP
> 
> You'll get the Google local NTP in step 3.  You can fix this by changing the
> !ntp->IsLocal() check to ntp_->instant_url() != GetLocalInstantURL().  In
other
> words, you do want to reload the local NTP if its URL has changed.  (I'd just
> move this clause into ShouldSwitchToLocalNTP().  Note also that you will need
to
> add an instant_url() member to InstantNTP, like the one that exists for
> InstantOverlay.)

Done.  Bug seems fixed.

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:289: //  determined. For
|ignore_blacklist| look at comments in GetInstantURL().
On 2013/04/17 00:19:34, samarth wrote:
> nit: no space before "determined"

Done.

[jeremycho, 2013-04-17 16:05:14]

ping

[samarth, 2013-04-17 16:24:11]

LGTM

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://codereview.chromium.org/13905008/diff/37001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:307: #suggestions-box {
On 2013/04/17 00:59:23, jeremycho wrote:
> Acknowledged.  display:inline-block is not what we want here.
> On 2013/04/17 00:19:34, samarth wrote:
> > FYI, Kausalya just made some changes here in crrev.com/14080012 so be
careful
> > when you sync past her change.
> 

This was to fix crbug.com/229839. If that doesn't work with the new layout, you
can fix it in JS. The bug is that renderSuggestions() sometimes adds a div to
the suggestions box but then adds no suggestion. We could either not add an
empty div in those cases or fix getDropdownHeight to return 0 when there are no
suggestions.

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1617: bool localNtpUrlChanged =
This was really confusing for me to reason about. How about instead just doing
this check at the top:
// Don't switch if already using the correct local NTP.
if (ntp_->instant_url() == GetLocalInstantURL())
  return false;

and then leaving the checks below untouched.

[samarth, 2013-04-17 17:24:58]

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:492: if (!ntp_ ||
ShouldSwitchToLocalNTP())
A few more comments here actually:
1) Please also move the !ntp_ check into the ShouldSwitchToLocalNTP
2) The comment above doesn't really add much; I would just kill it
3) We should add tests for the ShouldSwitch.. logic.  A follow up change for
that is OK.  If you haven't written instant extended interactive ui tests
before, I can walk you through what needs to be done.

[jeremycho, 2013-04-17 20:14:33]

Updated the JS to fix crbug.com/229839.  I wasn't able to reproduce the NTP
flash issue you mentioned.

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:492: if (!ntp_ ||
ShouldSwitchToLocalNTP())
On 2013/04/17 17:24:59, samarth wrote:
> A few more comments here actually:
> 1) Please also move the !ntp_ check into the ShouldSwitchToLocalNTP
> 2) The comment above doesn't really add much; I would just kill it
> 3) We should add tests for the ShouldSwitch.. logic.  A follow up change for
> that is OK.  If you haven't written instant extended interactive ui tests
> before, I can walk you through what needs to be done.

Done.  Left a TODO for the test.

https://codereview.chromium.org/13905008/diff/47001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1617: bool localNtpUrlChanged =
On 2013/04/17 16:24:11, samarth wrote:
> This was really confusing for me to reason about. How about instead just doing
> this check at the top:
> // Don't switch if already using the correct local NTP.
> if (ntp_->instant_url() == GetLocalInstantURL())
>   return false;
> 
> and then leaving the checks below untouched.

Done.

[Dan Beam, 2013-04-17 23:28:58]

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:340: .selected:hover {
why is :hover needed?

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:542: }
needs moar \n's

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:550:
document.body.classList.remove('custom-theme');
CLASSES.CUSTOM_THEME if you're gonna do this everywhere else...

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:571: function
isFakeboxClick(event) {
return fakebox.contains(event.target);

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:584: function
fakeboxAnimationDone() {
do you need to check the target or the property that was transitioned here?

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:652: * @type {Array.<number>}
!Array

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:704: * @param {Array}
nativeSuggestions An array of native suggestions to render.
!Array

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:739: * @param {Object}
suggestion A suggestion.
!Object

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:772: selectedIndex =
--selectedIndex < -1 ? numSuggestions - 1 : selectedIndex;
nit: this is clearer, IMO

if (increment) {
  if (selectedIndex > numSuggestions)
    selectedIndex = -1;
  else
    ++selectedIndex;
} else {
  if (selectedIndex < 0)
    selectedIndex = numSuggestions - 1;
  else
    --selectedIndex;
}

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:778:
newSelection.classList.add('selected');
nit: CLASSES.SELECTED

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:806: nativeSuggestions[0],
searchboxApiHandle.verbatim))
if (shouldSelectSuggestion(
        nativeSuggestions[0], searchboxApiHandle.verbatim)) {
  selectedIndex = 0;
}
\n

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:822: if ($('suggestionStyle'))
IDS.SUGGESTION_STYLE

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:829: style.id =
'suggestionStyle';
suggestion-style

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:847: * @return
{WindowOpenDisposition} The desired behavior for
!WindowOpenDisposition

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:871: * @param {Object} e The key
being pressed.
s/Object/!Event/

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:875: case 38:  // Up arrow
nit: end with .

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:878: case 40:  // Down arrow
same

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:1039: 
^H

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:1063: function
addDelayedTransitions() {
can document.webkitHidden still be true here?

https://chromiumcodereview.appspot.com/13905008/diff/58001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.js:1067: }
\n

https://chromiumcodereview.appspot.com/13905008/diff/61001/chrome/browser/res...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://chromiumcodereview.appspot.com/13905008/diff/61001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:89: 61.54% {
0% {

https://chromiumcodereview.appspot.com/13905008/diff/61001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:93: 100% {
61.55% {

https://chromiumcodereview.appspot.com/13905008/diff/61001/chrome/browser/res...
chrome/browser/resources/local_ntp/local_ntp.css:98: -webkit-animation: blink
1.3s linear infinite;
-webkit-animation-timing-function: step-start;

[jeremycho, 2013-04-18 05:56:20]

Thanks for the comments.  PTAL.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:340: .selected:hover {
On 2013/04/17 23:28:58, Dan Beam wrote:
> why is :hover needed?

To override .suggestion:hover.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:542: }
On 2013/04/17 23:28:58, Dan Beam wrote:
> needs moar \n's

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:550:
document.body.classList.remove('custom-theme');
On 2013/04/17 23:28:58, Dan Beam wrote:
> CLASSES.CUSTOM_THEME if you're gonna do this everywhere else...

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:571: function
isFakeboxClick(event) {
On 2013/04/17 23:28:58, Dan Beam wrote:
> return fakebox.contains(event.target);

That didn't work - I believe because of the input field.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:584: function
fakeboxAnimationDone() {
On 2013/04/17 23:28:58, Dan Beam wrote:
> do you need to check the target or the property that was transitioned here?

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:652: * @type {Array.<number>}
On 2013/04/17 23:28:58, Dan Beam wrote:
> !Array

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:704: * @param {Array}
nativeSuggestions An array of native suggestions to render.
On 2013/04/17 23:28:58, Dan Beam wrote:
> !Array

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:739: * @param {Object}
suggestion A suggestion.
On 2013/04/17 23:28:58, Dan Beam wrote:
> !Object

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:772: selectedIndex =
--selectedIndex < -1 ? numSuggestions - 1 : selectedIndex;
On 2013/04/17 23:28:58, Dan Beam wrote:
> nit: this is clearer, IMO
> 
> if (increment) {
>   if (selectedIndex > numSuggestions)
>     selectedIndex = -1;
>   else
>     ++selectedIndex;
> } else {
>   if (selectedIndex < 0)
>     selectedIndex = numSuggestions - 1;
>   else
>     --selectedIndex;
> }

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:778:
newSelection.classList.add('selected');
On 2013/04/17 23:28:58, Dan Beam wrote:
> nit: CLASSES.SELECTED

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:806: nativeSuggestions[0],
searchboxApiHandle.verbatim))
On 2013/04/17 23:28:58, Dan Beam wrote:
> if (shouldSelectSuggestion(
>         nativeSuggestions[0], searchboxApiHandle.verbatim)) {
>   selectedIndex = 0;
> }
> \n

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:822: if ($('suggestionStyle'))
On 2013/04/17 23:28:58, Dan Beam wrote:
> IDS.SUGGESTION_STYLE

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:829: style.id =
'suggestionStyle';
On 2013/04/17 23:28:58, Dan Beam wrote:
> suggestion-style

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:847: * @return
{WindowOpenDisposition} The desired behavior for
On 2013/04/17 23:28:58, Dan Beam wrote:
> !WindowOpenDisposition

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:871: * @param {Object} e The key
being pressed.
On 2013/04/17 23:28:58, Dan Beam wrote:
> s/Object/!Event/

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:875: case 38:  // Up arrow
On 2013/04/17 23:28:58, Dan Beam wrote:
> nit: end with .

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:878: case 40:  // Down arrow
On 2013/04/17 23:28:58, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1039: 
On 2013/04/17 23:28:58, Dan Beam wrote:
> ^H

Done.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1063: function
addDelayedTransitions() {
On 2013/04/17 23:28:58, Dan Beam wrote:
> can document.webkitHidden still be true here?

No - webkitvisibilitychange from a hidden state implies visibility.

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1067: }
On 2013/04/17 23:28:58, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/13905008/diff/61001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.css (right):

https://codereview.chromium.org/13905008/diff/61001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:89: 61.54% {
On 2013/04/17 23:28:59, Dan Beam wrote:
> 0% {

Done.

https://codereview.chromium.org/13905008/diff/61001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:93: 100% {
On 2013/04/17 23:28:59, Dan Beam wrote:
> 61.55% {

Done.

https://codereview.chromium.org/13905008/diff/61001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.css:98: -webkit-animation: blink
1.3s linear infinite;
On 2013/04/17 23:28:59, Dan Beam wrote:
> -webkit-animation-timing-function: step-start;

Done.

[Dan Beam, 2013-04-18 20:05:13]

lgtm but please don't duplicate contains() logic if you can help it, I see no
reason why that shouldn't work

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:571: function
isFakeboxClick(event) {
On 2013/04/18 05:56:21, jeremycho wrote:
> On 2013/04/17 23:28:58, Dan Beam wrote:
> > return fakebox.contains(event.target);
> 
> That didn't work - I believe because of the input field.

I don't understand a) why it wouldn't work, and b) your markup structure, so
"because of the input field" isn't very telling to me.

https://codereview.chromium.org/13905008/diff/68003/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/68003/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:546: 
^H

[samarth, 2013-04-18 20:14:12]

Jeremy: can you rebase once more? The patch doesn't apply at head and I'd like
to play with it some more.

Thanks!
Samarth

[jeremycho, 2013-04-18 20:26:18]

Thanks for the review!

@sky - can you stamp for chrome_browser.gypi, browser_resources.grd?

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:571: function
isFakeboxClick(event) {
On 2013/04/18 20:05:14, Dan Beam wrote:
> On 2013/04/18 05:56:21, jeremycho wrote:
> > On 2013/04/17 23:28:58, Dan Beam wrote:
> > > return fakebox.contains(event.target);
> > 
> > That didn't work - I believe because of the input field.
> 
> I don't understand a) why it wouldn't work, and b) your markup structure, so
> "because of the input field" isn't very telling to me.

Sorry, it does actually work (I was trying it over NX last night and mistook the
bad latency for it not working).

https://codereview.chromium.org/13905008/diff/68003/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/68003/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:546: 
On 2013/04/18 20:05:15, Dan Beam wrote:
> ^H

Done.

[jeremycho, 2013-04-18 22:21:50]

@thakis can you stamp for chrome_browser.gypi, browser_resources.grd?  @sky is
OOO.

[Nico, 2013-04-18 22:29:32]

lgtm

[samarth, 2013-04-19 14:52:23]

OK, I fixed the test failure in PS 16. I spent a bunch of time last night
debugging the test and it's almost definitely not really caused by your CL.  The
infinite reload on shutdown was happening sporadically before and I think your
change is just changing the timing characteristics enough to tickle it in that
test.

Sreeram: can you sanity check my fix in PS 16? Basically, if we see too many
failures for the local Instant page failures, we should stop reloading
automatically, and wait for an explicit user action (Update() or
ReleaseNTPContents()) before reloading. I still think we need a better fix to
stop reloading at all during shutdown but this fixes the worst of it.

In the meantime, I'll continue to pound on this change some more once I'm back
in the office.

Thanks,
Samarth

[samarth, 2013-04-19 16:39:51]

Jeremy, played with it some more and it looks pretty good. You can address
almost all of the comments below separately except for the bug where backspacing
away leaves the overlay open.

Thanks,
Samarth

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:798: var show = lastInputValue
== '';
show -> showNTP

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:798: var show = lastInputValue
== '';
FYI, this isn't quite right. This means if you backspace away, the NTP reappears
and the BMB comes back, moving this down which is jarring.

We should only do this on ESC. Unfortunately there is no way to discern ESC
right now (see crbug.com/231744). But we should fix this soon.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:802: if (show) {
We should also explicitly call showOverlay(0) to handle the case where this is
being used as an overlay. Otherwise, the empty overlay will stick around.

Actually it's because you're return early: just make sure you call keep the
calls to showOverlay from L823-824 in this branch as well.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:818:
searchboxApiHandle.hideBars();
To be nitpicky, the sequence is:
1) Hide the NTP
2) call hideBars
3) wait for bars to be hidden (there's the onbarshidden handler)
4) then show suggestions

On a release, this is all fast enough that I really can't see any jank but we
should still fix it in a follow-up change.

[jeremycho, 2013-04-19 17:00:46]

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:798: var show = lastInputValue
== '';
Acknowledged.
On 2013/04/19 16:39:52, samarth wrote:
> FYI, this isn't quite right. This means if you backspace away, the NTP
reappears
> and the BMB comes back, moving this down which is jarring.
> 
> We should only do this on ESC. Unfortunately there is no way to discern ESC
> right now (see crbug.com/231744). But we should fix this soon.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:798: var show = lastInputValue
== '';
On 2013/04/19 16:39:52, samarth wrote:
> show -> showNTP

Done.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:802: if (show) {
On 2013/04/19 16:39:52, samarth wrote:
> We should also explicitly call showOverlay(0) to handle the case where this is
> being used as an overlay. Otherwise, the empty overlay will stick around.
> 
> Actually it's because you're return early: just make sure you call keep the
> calls to showOverlay from L823-824 in this branch as well.

Done.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:818:
searchboxApiHandle.hideBars();
Acknowledged.
On 2013/04/19 16:39:52, samarth wrote:
> To be nitpicky, the sequence is:
> 1) Hide the NTP
> 2) call hideBars
> 3) wait for bars to be hidden (there's the onbarshidden handler)
> 4) then show suggestions
> 
> On a release, this is all fast enough that I really can't see any jank but we
> should still fix it in a follow-up change.

[sreeram, 2013-04-19 17:04:59]

Am just starting to review instant_controller.cc, but some comments on the rest
(I didn't look at any js/css/html).

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search.cc:473: GetDefaultSearchProvider();
GetDefaultSearchProviderTemplateURL()

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:319: SetGoogleAsSearchProvider();
This persists to the next iteration of the loop, right? That seems bad.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:346: SetGoogleAsSearchProvider();
-ditto-

[sreeram, 2013-04-19 17:12:41]

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:574: (instant_tab_ &&
instant_tab_->IsLocal()) || overlay_->IsLocal();
Not quite right. You need:
    (instant_tab_ && instant_tab_->IsLocal()) ||
    (!instant_tab_ && overlay_ && overlay_->IsLocal());
The "&& overlay_" is just paranoia. The "!instant_tab_" clause is what's
required.

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
We never preload the local NTP. So why would this ever be true?

[sreeram, 2013-04-19 17:14:36]

PS17 lost some of Samarth's changes in PS16. When multiple people collaborate on
a CL, it's important that each one "syncs" to the latest patch before uploading
their new patch.

[samarth, 2013-04-19 17:18:11]

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1495: const std::string&
instant_url = ntp_->instant_url();
Just chatted with Sreeram and we came up with a better fix for this. I should
just send a separate change for that though. Can you revert my changes in this
function and in the function below? You can just disable the broken test by
adding DISABLED_ at the beginning of the test name and I'll re-enable it with my
fix (please add a TODO there assigned to me to re-enable it).

[samarth, 2013-04-19 17:20:36]

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
On 2013/04/19 17:12:42, sreeram wrote:
> We never preload the local NTP. So why would this ever be true?

We only skip preloading when local_only_ is set. So if you're using the online
version and switch providers, we may preload the local overlay.

But you raise a good point: we should never bother preloading the local NTP,
regardless of the local_only_ flag.

[sreeram, 2013-04-19 17:25:29]

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
On 2013/04/19 17:20:37, samarth wrote:
> On 2013/04/19 17:12:42, sreeram wrote:
> > We never preload the local NTP. So why would this ever be true?
> 
> We only skip preloading when local_only_ is set. So if you're using the online
> version and switch providers, we may preload the local overlay.

Yeah, we should just never preload either ntp_ or overlay_ with the local URL.
Let's just make it so in this CL itself.

[samarth, 2013-04-19 18:30:02]

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
On 2013/04/19 17:25:29, sreeram wrote:
> On 2013/04/19 17:20:37, samarth wrote:
> > On 2013/04/19 17:12:42, sreeram wrote:
> > > We never preload the local NTP. So why would this ever be true?
> > 
> > We only skip preloading when local_only_ is set. So if you're using the
online
> > version and switch providers, we may preload the local overlay.
> 
> Yeah, we should just never preload either ntp_ or overlay_ with the local URL.
> Let's just make it so in this CL itself.

I'd rather do that separately too, unless you feel strongly.  This is getting to
be pretty big and hairy as it is, and I'd rather not jam more things in it.

[sreeram, 2013-04-19 18:31:56]

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
On 2013/04/19 18:30:03, samarth wrote:
> I'd rather do that separately too, unless you feel strongly.  This is getting
to
> be pretty big and hairy as it is, and I'd rather not jam more things in it.

No prob. Let me know when the other comments have been addressed and I'll take
another look.

[jeremycho, 2013-04-19 19:55:07]

Ready for another look Sreeram.  Thanks.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search.cc:473: GetDefaultSearchProvider();
On 2013/04/19 17:05:00, sreeram wrote:
> GetDefaultSearchProviderTemplateURL()

Done.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:319: SetGoogleAsSearchProvider();
Fixed. 

On 2013/04/19 17:05:00, sreeram wrote:
> This persists to the next iteration of the loop, right? That seems bad.

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:346: SetGoogleAsSearchProvider();
Fixed. 
On 2013/04/19 17:05:00, sreeram wrote:
> -ditto-

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/89024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1495: const std::string&
instant_url = ntp_->instant_url();
On 2013/04/19 17:18:11, samarth wrote:
> Just chatted with Sreeram and we came up with a better fix for this. I should
> just send a separate change for that though. Can you revert my changes in this
> function and in the function below? You can just disable the broken test by
> adding DISABLED_ at the beginning of the test name and I'll re-enable it with
my
> fix (please add a TODO there assigned to me to re-enable it).

Done.

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:574: (instant_tab_ &&
instant_tab_->IsLocal()) || overlay_->IsLocal();
On 2013/04/19 17:12:42, sreeram wrote:
> Not quite right. You need:
>     (instant_tab_ && instant_tab_->IsLocal()) ||
>     (!instant_tab_ && overlay_ && overlay_->IsLocal());
> The "&& overlay_" is just paranoia. The "!instant_tab_" clause is what's
> required.

Done.

https://codereview.chromium.org/13905008/diff/93024/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1648:
browser_->profile()).spec()) {
Comments addressed.
On 2013/04/19 18:31:57, sreeram wrote:
> On 2013/04/19 18:30:03, samarth wrote:
> > I'd rather do that separately too, unless you feel strongly.  This is
getting
> to
> > be pretty big and hairy as it is, and I'd rather not jam more things in it.
> 
> No prob. Let me know when the other comments have been addressed and I'll take
> another look.

[sreeram, 2013-04-19 20:30:44]

lgtm

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:194: void SetSearchProvider(bool
isGoogle) {
Nit: is_google
(Yeah, the differing variable naming conventions make it hard when you're
constantly editing both JS and C++.)

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:302: {"chrome-search//foo",            
      false, "Chrome-search scheme"},
Nit: Missing ":" in the scheme.

[jeremycho, 2013-04-19 20:39:36]

Thanks for the review!

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:194: void SetSearchProvider(bool
isGoogle) {
On 2013/04/19 20:30:44, sreeram wrote:
> Nit: is_google
> (Yeah, the differing variable naming conventions make it hard when you're
> constantly editing both JS and C++.)

Done.

https://codereview.chromium.org/13905008/diff/98002/chrome/browser/search/sea...
chrome/browser/search/search_unittest.cc:302: {"chrome-search//foo",            
      false, "Chrome-search scheme"},
On 2013/04/19 20:30:44, sreeram wrote:
> Nit: Missing ":" in the scheme.

Done.

[jeremycho, 2013-04-19 21:33:36]

Uploaded a new patch with a one-line CSS change to fix suggestion truncation.

[samarth, 2013-04-19 23:02:18]

Committed patchset #20 manually as r195323 (presubmit successful).

[samarth, 2013-04-19 23:03:39]

FYI, committed manually to bypass presubmit checks for "_" in class names.  Try
jobs looked good and will watch the waterfall.

Samarth