[Android] Upstream remaining changes to NTP UI code.

[Android] Upstream remaining changes to NTP UI code.

This disables the NTP suggestions subpage on Android, adds an NTP-ready handler on Android, removes the catch-all NTP handler on Android, and adds a JS hook to clear the recently closed tabs list.

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

[newt (away), 2012-10-02 13:07:31]

PTAL.  Thank you!

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc:118: if
(tab_restore_service_)
Not totally sure about this.  If tab_restore_service_ is null, then perhaps we
still want to clear the recently closed entries.  But currently, this is
triggered only when the user long presses on a recently closed entry and selects
"remove", at which point tab_restore_service_ already exists.

I could do something like in HandleGetRecentlyClosedTabs to make this work when
tab_restore_service_ is null.  Thoughts?

[Evan Stade, 2012-10-02 14:42:43]

mostly lg

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc:24:
web_ui()->RegisterMessageCallback("NTPUnexpectedNavigation", base::Bind(
why does this exist? just to make tests fail? You should just console.error()
and make sure the tests fail when there's an error (some webui tests already
do).

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:82: new
FaviconSource(profile, FaviconSource::FAVICON));
you need both of these on android?

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc:118: if
(tab_restore_service_)
On 2012/10/02 13:07:31, newt wrote:
> Not totally sure about this.  If tab_restore_service_ is null, then perhaps we
> still want to clear the recently closed entries.  But currently, this is
> triggered only when the user long presses on a recently closed entry and
selects
> "remove", at which point tab_restore_service_ already exists.
> 
> I could do something like in HandleGetRecentlyClosedTabs to make this work
when
> tab_restore_service_ is null.  Thoughts?

pull out an EnsureTabRestoreService function that inits (or attempts to)
tab_restore_service_

[newt (away), 2012-10-05 13:31:05]

Comments addressed.  PTAL.

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc:24:
web_ui()->RegisterMessageCallback("NTPUnexpectedNavigation", base::Bind(
Yes, it's for tests.  Many of our tests start by loading the NTP and then
navigating to a new page.  These tests hang or break in strange ways if the NTP
does extra navigation after sending notifyNTPReady.

These tests are changing quite a bit right now, so this callback might not even
be required in the near future.  I can file a bug to track this for future
fixing or marking obsolete.  Is that reasonable?

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:82: new
FaviconSource(profile, FaviconSource::FAVICON));
On 2012/10/02 14:42:43, Evan Stade wrote:
> you need both of these on android?

Yes.  Added comments to explain.  ANY is perhaps not the clearest name :(

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc:118: if
(tab_restore_service_)
On 2012/10/02 14:42:43, Evan Stade wrote:
> On 2012/10/02 13:07:31, newt wrote:
> > Not totally sure about this.  If tab_restore_service_ is null, then perhaps
we
> > still want to clear the recently closed entries.  But currently, this is
> > triggered only when the user long presses on a recently closed entry and
> selects
> > "remove", at which point tab_restore_service_ already exists.
> > 
> > I could do something like in HandleGetRecentlyClosedTabs to make this work
> when
> > tab_restore_service_ is null.  Thoughts?
> 
> pull out an EnsureTabRestoreService function that inits (or attempts to)
> tab_restore_service_

Done.

[Evan Stade, 2012-10-05 15:16:13]

lgtm

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc:24:
web_ui()->RegisterMessageCallback("NTPUnexpectedNavigation", base::Bind(
On 2012/10/05 13:31:05, newt wrote:
> Yes, it's for tests.  Many of our tests start by loading the NTP and then
> navigating to a new page.  These tests hang or break in strange ways if the
NTP
> does extra navigation after sending notifyNTPReady.
> 
> These tests are changing quite a bit right now, so this callback might not
even
> be required in the near future.  I can file a bug to track this for future
> fixing or marking obsolete.  Is that reasonable?

sure

http://codereview.chromium.org/11038014/diff/10001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc (right):

http://codereview.chromium.org/11038014/diff/10001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc:172: if
(!tab_restore_service_) {
nit:

if (tab_restore_service_)
  return

now the rest of the code is not indented;

[commit-bot: I haz the power, 2012-10-05 15:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/11038014/1012

[commit-bot: I haz the power, 2012-10-05 15:25:51]

Presubmit check for 11038014-1012 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[newt (away), 2012-10-05 15:38:33]

+brettw for

chrome/chrome_browser_ui.gypi
chrome/browser/browser_resources.grd
chrome/common/chrome_notification_types.h

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc (right):

http://codereview.chromium.org/11038014/diff/1011/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/android/new_tab_page_ready_handler.cc:24:
web_ui()->RegisterMessageCallback("NTPUnexpectedNavigation", base::Bind(
On 2012/10/05 15:16:13, Evan Stade wrote:
> On 2012/10/05 13:31:05, newt wrote:
> > Yes, it's for tests.  Many of our tests start by loading the NTP and then
> > navigating to a new page.  These tests hang or break in strange ways if the
> NTP
> > does extra navigation after sending notifyNTPReady.
> > 
> > These tests are changing quite a bit right now, so this callback might not
> even
> > be required in the near future.  I can file a bug to track this for future
> > fixing or marking obsolete.  Is that reasonable?
> 
> sure

filed http://crbug.com/154307

http://codereview.chromium.org/11038014/diff/10001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc (right):

http://codereview.chromium.org/11038014/diff/10001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc:172: if
(!tab_restore_service_) {
On 2012/10/05 15:16:13, Evan Stade wrote:
> nit:
> 
> if (tab_restore_service_)
>   return
> 
> now the rest of the code is not indented;

Done.

[nilesh, 2012-10-09 00:17:24]

Scott, Will you be able to help with the review?

for
chrome/chrome_browser_ui.gypi
chrome/browser/browser_resources.grd
chrome/common/chrome_notification_types.h

[sky, 2012-10-09 00:35:25]

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:154: #if defined(OS_ANDROID)
Why is this android specific?

[newt (away), 2012-10-09 09:49:26]

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:154: #if defined(OS_ANDROID)
On 2012/10/09 00:35:25, sky wrote:
> Why is this android specific?

We added the event to reduce flakiness for tests that start by loading the NTP. 
It's fired and listened for only on Android (We have separate html/js for the
Android NTP), so I saw no reason to compile it on other platforms.

[sky, 2012-10-09 15:59:13]

LGTM

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/11038014/diff/1012/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:154: #if defined(OS_ANDROID)
On 2012/10/09 09:49:27, newt wrote:
> On 2012/10/09 00:35:25, sky wrote:
> > Why is this android specific?
> 
> We added the event to reduce flakiness for tests that start by loading the
NTP. 
> It's fired and listened for only on Android (We have separate html/js for the
> Android NTP), so I saw no reason to compile it on other platforms.

Ok. From the description it seemed like it might be fired in cross platform
code, but if that's not the case the ifdef is fine.

[commit-bot: I haz the power, 2012-10-09 17:19:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/11038014/1012

[commit-bot: I haz the power, 2012-10-09 19:42:59]

Change committed as 160894