Add pyauto hook for getting and manipulating the data underneath the NTP.

Add pyauto hook for getting and manipulating the data underneath the NTP.
BUG=49113
TEST=none

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

[kkania, 2010-11-16 20:00:23]

sky: top_sites*
nirnimesh: all else

Patch #1 was taken from http://codereview.chromium.org/3477001/, with the hook
in TopSites slightly modified and the unittests redone due to the refactor

Patch #2 is a few miscellaneous changes

[sky, 2010-11-16 21:43:08]

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/dom_ui/new_t...
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/dom_ui/new_t...
chrome/browser/dom_ui/new_tab_ui.cc:530: const TabRestoreService::Entries&
entries, ListValue* entry_list_value) {
The changes to new_tab_ui seem completely separate to the top sites changes. Can
they be part of a separate patch?

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:444: void
TopSites::RefreshAndCallback(Callback0::Type* callback) {
Instead of exposing this method, maintaining handle_to_wait_for_ and
refresh_callback_, make StartQueryForMostVisited public, have it return the
handle and make OnTopSitesAvailableFromHistory fire a notification with the
handle so that NTPInfoObserver can listen for that.

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:564: StartQueryForMostVisited();
Why do we need TimerFired?

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:928: backend_->DoEmptyRequest(
Why does this need to wait until the backend has been updated?

[Nirnimesh, 2010-11-16 22:25:05]

http://codereview.chromium.org/5088001/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/5088001/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1413: :
automation_(automation),
I think this needs to be indented only 4 chars

http://codereview.chromium.org/5088001/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.cc:1433:
recently_closed_list);
nit: this might fit above

http://codereview.chromium.org/5088001/diff/2001/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/5088001/diff/2001/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:929: 
Please add a comment stating the purpose

http://codereview.chromium.org/5088001/diff/2001/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/5088001/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:201: def GetFileURLForDataPath(relative_path):
this function is duplicate

http://codereview.chromium.org/5088001/diff/17001/chrome/test/functional/ntp.py
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/test/functional/ntp....
chrome/test/functional/ntp.py:1: #!/usr/bin/python
Please add this test in PYAUTO_TESTS

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
File chrome/test/pyautolib/ntp_model.py (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:7: """NTPModel: python representation of the
NTP.
A "model" should not have methods to manipulate itself or perform "actions". It
should just represent the data. To that end, I think all these methods should be
moved to pyauto.py directly (use 'NTP' as prefix or substring for each method
name). All automation messages will be in one place. It comes out better in the
generated documentation which the testers are used to.

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:33: sender: a pyautolib.PyUITestBase which
can send JSON requests
not true. _GetResultFromJSONRequest lives in PyUITest (not PyUITestBase)

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:191: 
please add a TODO about adding apps info

[kkania, 2010-11-17 01:11:34]

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/dom_ui/new_t...
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/dom_ui/new_t...
chrome/browser/dom_ui/new_tab_ui.cc:530: const TabRestoreService::Entries&
entries, ListValue* entry_list_value) {
On 2010/11/16 21:43:08, sky wrote:
> The changes to new_tab_ui seem completely separate to the top sites changes.
Can
> they be part of a separate patch?

I have a test method called GetNTPInfo in the TestingAutomationProvider that
uses this to get the recently closed info that is sent to the NTP. The same
method also grabs info about the most visited section. 

It is possible to split this up, but they are related to the same hook. Would
you like to see this separated into two still?

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:444: void
TopSites::RefreshAndCallback(Callback0::Type* callback) {
On 2010/11/16 21:43:08, sky wrote:
> Instead of exposing this method, maintaining handle_to_wait_for_ and
> refresh_callback_, make StartQueryForMostVisited public, have it return the
> handle and make OnTopSitesAvailableFromHistory fire a notification with the
> handle so that NTPInfoObserver can listen for that.

Done.

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:564: StartQueryForMostVisited();
On 2010/11/16 21:43:08, sky wrote:
> Why do we need TimerFired?

OneShotTimer expects a void method.

http://codereview.chromium.org/5088001/diff/17001/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:928: backend_->DoEmptyRequest(
On 2010/11/16 21:43:08, sky wrote:
> Why does this need to wait until the backend has been updated?

After thinking about it again, I think this is not necessary given the things we
currently want to test in the NTP. Will remove.

http://codereview.chromium.org/5088001/diff/17001/chrome/test/functional/ntp.py
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/test/functional/ntp....
chrome/test/functional/ntp.py:1: #!/usr/bin/python
On 2010/11/16 22:25:05, Nirnimesh wrote:
> Please add this test in PYAUTO_TESTS

Done.

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
File chrome/test/pyautolib/ntp_model.py (right):

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:7: """NTPModel: python representation of the
NTP.
On 2010/11/16 22:25:05, Nirnimesh wrote:
> A "model" should not have methods to manipulate itself or perform "actions".
It
> should just represent the data. To that end, I think all these methods should
be
> moved to pyauto.py directly (use 'NTP' as prefix or substring for each method
> name). All automation messages will be in one place. It comes out better in
the
> generated documentation which the testers are used to.

Done.

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:33: sender: a pyautolib.PyUITestBase which
can send JSON requests
On 2010/11/16 22:25:05, Nirnimesh wrote:
> not true. _GetResultFromJSONRequest lives in PyUITest (not PyUITestBase)

Done.

http://codereview.chromium.org/5088001/diff/17001/chrome/test/pyautolib/ntp_m...
chrome/test/pyautolib/ntp_model.py:191: 
On 2010/11/16 22:25:05, Nirnimesh wrote:
> please add a TODO about adding apps info

Done.

[Nirnimesh, 2010-11-17 01:25:49]

PyAuto/Automation bits LGTM.

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:28: import exceptions
not needed. 'IndexError' is already in scope

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:202: def GetFileURLForDataPath(relative_path):
this is still duplicated

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1828: Note:
Move the note before Args:

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1898: def CloseDefaultNTPThumbnails(self):
s/Close/Remove/ ?

Where is it used?

[sky, 2010-11-17 16:30:59]

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:444: if (!profile_)
DCHECK(loaded_).

Any pyauto test needing top sites should block until loaded.

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:451: request_handle =
hs->QueryMostVisitedURLs(
return hs->...

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:457: return request_handle;
return NULL

[kkania, 2010-11-17 22:16:07]

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:444: if (!profile_)
On 2010/11/17 16:31:00, sky wrote:
> DCHECK(loaded_).
> 
> Any pyauto test needing top sites should block until loaded.

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:451: request_handle =
hs->QueryMostVisitedURLs(
On 2010/11/17 16:31:00, sky wrote:
> return hs->...

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/browser/history/top_...
chrome/browser/history/top_sites.cc:457: return request_handle;
On 2010/11/17 16:31:00, sky wrote:
> return NULL

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:28: import exceptions
On 2010/11/17 01:25:49, Nirnimesh wrote:
> not needed. 'IndexError' is already in scope

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:202: def GetFileURLForDataPath(relative_path):
On 2010/11/17 01:25:49, Nirnimesh wrote:
> this is still duplicated

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1828: Note:
On 2010/11/17 01:25:49, Nirnimesh wrote:
> Move the note before Args:

Done.

http://codereview.chromium.org/5088001/diff/37015/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:1898: def CloseDefaultNTPThumbnails(self):
On 2010/11/17 01:25:49, Nirnimesh wrote:
> s/Close/Remove/ ?
> 
> Where is it used?

Renamed it, use it in a new test

[sky, 2010-11-17 22:36:01]

LGTM

http://codereview.chromium.org/5088001/diff/59001/chrome/browser/history/top_...
File chrome/browser/history/top_sites.h (right):

http://codereview.chromium.org/5088001/diff/59001/chrome/browser/history/top_...
chrome/browser/history/top_sites.h:149: bool loaded() { return loaded_; }
const

http://codereview.chromium.org/5088001/diff/59001/chrome/browser/history/top_...
chrome/browser/history/top_sites.h:303: void
InvokeRefreshCallback(CancelableRequestProvider::Handle handle);
This isn't needed anymore, right?