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

Issue 3477001: Add pyauto hook for getting and manipulating the data underneath the NTP.... (Closed)

Created:
10 years, 3 months ago by kkania

Modified:
9 years, 7 months ago

Reviewers:
Paweł Hajdan Jr., sky, Nirnimesh

CC:
chromium-reviews, brettw-cc_chromium.org, ben+cc_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

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

Patch Set 4 : '' #

Created: 10 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+780 lines, -158 lines)			Patch
M	chrome/browser/automation/automation_provider_observers.h	View	1 2 3	3 chunks	+26 lines, -0 lines	0 comments	Download
M	chrome/browser/automation/automation_provider_observers.cc	View	1 2 3	3 chunks	+60 lines, -0 lines	0 comments	Download
M	chrome/browser/automation/testing_automation_provider.h	View	1 2 3	1 chunk	+32 lines, -0 lines	0 comments	Download
M	chrome/browser/automation/testing_automation_provider.cc	View	1 2 3	3 chunks	+108 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/new_tab_ui.h	View	1 2 3	2 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/new_tab_ui.cc	View	1 2 3	5 chunks	+53 lines, -104 lines	0 comments	Download
M	chrome/browser/history/top_sites.h	View	1 2 3	6 chunks	+27 lines, -4 lines	0 comments	Download
M	chrome/browser/history/top_sites.cc	View	1 2 3	8 chunks	+55 lines, -10 lines	0 comments	Download
M	chrome/browser/history/top_sites_unittest.cc	View	3	20 chunks	+98 lines, -40 lines	0 comments	Download
A	chrome/test/functional/ntp.py	View	1	1 chunk	+88 lines, -0 lines	0 comments	Download
A	chrome/test/pyautolib/ntp_model.py	View	1	1 chunk	+213 lines, -0 lines	0 comments	Download
M	chrome/test/pyautolib/pyauto.py	View	1 2 3	3 chunks	+13 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Nirnimesh

Thanks for putting this together. I've taken only a brief look so far. I'll get ...

10 years, 3 months ago (2010-09-21 01:52:18 UTC) #2

Paweł Hajdan Jr.

Drive-by. Definitely fix the issues Nirnimesh has pointed out, especially about the unneeded refcounting. http://codereview.chromium.org/3477001/diff/1/4 ...

10 years, 3 months ago (2010-09-21 08:51:20 UTC) #3

Nirnimesh

http://codereview.chromium.org/3477001/diff/1/2 File chrome/browser/automation/automation_provider_observers.cc (right): http://codereview.chromium.org/3477001/diff/1/2#newcode1326 chrome/browser/automation/automation_provider_observers.cc:1326: MessageLoop::current()->PostTask(FROM_HERE, We probably won't need this if we explicitly ...

10 years, 3 months ago (2010-09-21 20:09:35 UTC) #4

kkania

http://codereview.chromium.org/3477001/diff/1/2 File chrome/browser/automation/automation_provider_observers.cc (right): http://codereview.chromium.org/3477001/diff/1/2#newcode1299 chrome/browser/automation/automation_provider_observers.cc:1299: AutomationProvider* automation, IPC::Message* reply_message, On 2010/09/21 01:52:19, Nirnimesh wrote: ...

10 years, 3 months ago (2010-09-22 23:42:02 UTC) #5

http://codereview.chromium.org/3477001/diff/1/2
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3477001/diff/1/2#newcode1299
chrome/browser/automation/automation_provider_observers.cc:1299:
AutomationProvider* automation, IPC::Message* reply_message,
On 2010/09/21 01:52:19, Nirnimesh wrote:
> one arg per line please

Done.

http://codereview.chromium.org/3477001/diff/1/2#newcode1311
chrome/browser/automation/automation_provider_observers.cc:1311:
scoped_ptr<ListValue> list_value(new ListValue());
On 2010/09/21 01:52:19, Nirnimesh wrote:
> I'm not sure you need the scoped_ptr here.
> A regular ListValue* would do. Since you're going to push it into reply_, this
> list value will be owned by it and get freed when reply_ goes out of scope.

Done.

http://codereview.chromium.org/3477001/diff/1/2#newcode1316
chrome/browser/automation/automation_provider_observers.cc:1316:
DictionaryValue* dict = new DictionaryValue();
On 2010/09/21 01:52:19, Nirnimesh wrote:
> nit: You don't need ()

Done.

http://codereview.chromium.org/3477001/diff/1/2#newcode1326
chrome/browser/automation/automation_provider_observers.cc:1326:
MessageLoop::current()->PostTask(FROM_HERE,
On 2010/09/21 20:09:36, Nirnimesh wrote:
> We probably won't need this if we explicitly call 'delete this'

Done.

http://codereview.chromium.org/3477001/diff/1/3
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/3477001/diff/1/3#newcode853
chrome/browser/automation/automation_provider_observers.h:853: class
GetMostVisitedURLsObserver
On 2010/09/21 20:09:36, Nirnimesh wrote:
> rename to 'MostVisitedURLsObserver'? The 'Get' prefix is confusing.

I renamed to NTPInfoObserver since I put all the NTP stuff in it as you
recommended

http://codereview.chromium.org/3477001/diff/1/3#newcode854
chrome/browser/automation/automation_provider_observers.h:854: : public
base::RefCountedThreadSafe<GetMostVisitedURLsObserver> {
On 2010/09/21 01:52:19, Nirnimesh wrote:
> While it's neat to use ref counting, I'd recommend to follow the pattern
> followed by the other observers here, ie don't ref count, and 'delete this' in
> OnReceived

Done.

http://codereview.chromium.org/3477001/diff/1/3#newcode864
chrome/browser/automation/automation_provider_observers.h:864:
AutomationJSONReply reply_;
On 2010/09/21 20:09:36, Nirnimesh wrote:
> On 2010/09/21 01:52:19, Nirnimesh wrote:
> > It's only ever used in OnReceived. maybe you should move the declaration
> there.
> 
> I take this back. I see that removing this woud require making member
variables
> for IPC::Message* and AutomationProvider*, so this is fine.
> 

Done.

http://codereview.chromium.org/3477001/diff/1/3#newcode864
chrome/browser/automation/automation_provider_observers.h:864:
AutomationJSONReply reply_;
On 2010/09/21 01:52:19, Nirnimesh wrote:
> It's only ever used in OnReceived. maybe you should move the declaration
there.

Done.

http://codereview.chromium.org/3477001/diff/1/4
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/3477001/diff/1/4#newcode3719
chrome/browser/automation/testing_automation_provider.cc:3719: // JSON output:
On 2010/09/21 01:52:19, Nirnimesh wrote:
> Maybe you should just refer to the right function in pyauto.py.
> It's easier to maintain if it's at one place.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3746
chrome/browser/automation/testing_automation_provider.cc:3746: if (service) {
On 2010/09/21 08:51:20, Paweł Hajdan Jr. wrote:
> nit: Put failure case first and return early.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3750
chrome/browser/automation/testing_automation_provider.cc:3750: ListValue*
recently_closed_list = new ListValue();
On 2010/09/21 20:09:36, Nirnimesh wrote:
> I think it would be nicer if all data was constructed together in the observer
> at the time of replying, so as to minimize the chances of them being out of
> sync. Also, you'd not need to construct and count new values here. Just pass
> 'this' to the observer, and let it compute everything. The observer can then
> call provider_->profile()->GetTopSites().

I thought that it would be best if the observers were as lightweight as
possible. That way it would be possible to use combinations of them and have all
the related automation logic in the provider, not tidbits in random observers
here and there. Ideally, I think we should have some callback mechanism from the
observer to the provider so that the observer doesn't even know about the
IPC::Message or the AutomationProvider...

But, I will follow your recommendation anyways.

http://codereview.chromium.org/3477001/diff/1/4#newcode3757
chrome/browser/automation/testing_automation_provider.cc:3757: if (top_sites) {
On 2010/09/21 08:51:20, Paweł Hajdan Jr. wrote:
> nit: same here... and check all other places for similar issue.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3762
chrome/browser/automation/testing_automation_provider.cc:3762:
AutomationJSONReply(this, reply_message).SendError("No TopSites.");
On 2010/09/21 20:09:36, Nirnimesh wrote:
> Should this be treated as an error? or an empty list?
> If the former, need a better error message.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3777
chrome/browser/automation/testing_automation_provider.cc:3777:
args->GetInteger("old_index", &old_index)) {
On 2010/09/21 01:52:19, Nirnimesh wrote:
> If you invert this conditional, it'll let you return error here, thereby
keeping
> it together, and reducing the if-else blocks below.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3792
chrome/browser/automation/testing_automation_provider.cc:3792:
reply.SendError("Missing 'url', 'index', or 'old_index' key.");
On 2010/09/21 08:51:20, Paweł Hajdan Jr. wrote:
> The error message should be as specific as possible.

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3802
chrome/browser/automation/testing_automation_provider.cc:3802: if
(args->GetString("url", &url)) {
On 2010/09/21 01:52:19, Nirnimesh wrote:
> same here

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3821
chrome/browser/automation/testing_automation_provider.cc:3821: if
(args->GetString("url", &url)) {
On 2010/09/21 01:52:19, Nirnimesh wrote:
> and here

Done.

http://codereview.chromium.org/3477001/diff/1/4#newcode3840
chrome/browser/automation/testing_automation_provider.cc:3840: if (top_sites) {
On 2010/09/21 01:52:19, Nirnimesh wrote:
> and here, but it doesn't really matter here.

Done.

http://codereview.chromium.org/3477001/diff/1/5
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/3477001/diff/1/5#newcode599
chrome/browser/automation/testing_automation_provider.h:599: // Gets the info
that is used to create a NTP.
On 2010/09/21 01:52:19, Nirnimesh wrote:
> This is slightly confusing.
> "Get info about the elements in the New Tab Page"

Done.

http://codereview.chromium.org/3477001/diff/1/5#newcode620
chrome/browser/automation/testing_automation_provider.h:620: void
UnpinNTPMostVisitedThumbnail(Browser* browser,
On 2010/09/21 01:52:19, Nirnimesh wrote:
> Is there a pin function as well?

You pin by calling MoveNTPMostVisitedThumbnail (see ntp_model.py). Do you want
me to explicitly add one?

http://codereview.chromium.org/3477001/diff/1/8
File chrome/browser/history/top_sites.cc (right):

http://codereview.chromium.org/3477001/diff/1/8#newcode98
chrome/browser/history/top_sites.cc:98: bool added =
AddPrepopulatedPages(&top_urls);
On 2010/09/21 20:09:36, Nirnimesh wrote:
> It's not obvious to me why these 2 changes were necessary.

Removed this as no longer necessary with my changed hook

http://codereview.chromium.org/3477001/diff/1/10
File chrome/test/data/dummy_pages/google.html (right):

http://codereview.chromium.org/3477001/diff/1/10#newcode2
chrome/test/data/dummy_pages/google.html:2: <head>
On 2010/09/21 01:52:19, Nirnimesh wrote:
> There are some sample html pages in chrome/test/data already. Could you use
any
> of those?

Done.

http://codereview.chromium.org/3477001/diff/1/12
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/3477001/diff/1/12#newcode10
chrome/test/functional/ntp.py:10: 
On 2010/09/21 20:09:36, Nirnimesh wrote:
> nit: need 2 blank lines at global scope

Done.

http://codereview.chromium.org/3477001/diff/1/12#newcode22
chrome/test/functional/ntp.py:22: ntp_info = self.GetNTPModel()._GetInfo()
On 2010/09/21 20:09:36, Nirnimesh wrote:
> this can be merged with line 25

Done.

http://codereview.chromium.org/3477001/diff/1/12#newcode35
chrome/test/functional/ntp.py:35: # Create some dummy file urls we can use in
the tests.
On 2010/09/21 20:09:36, Nirnimesh wrote:
> I'm not so sure whether this should be done by default in setUp, or whether
this
> could be refactored to another function, and the tests that need this could
call
> this function.

I think this will be used by the majority of tests and it won't hurt anything,
so might as well put it here. If it turns out different later, we can refactor.

http://codereview.chromium.org/3477001/diff/1/12#newcode36
chrome/test/functional/ntp.py:36: page_dir =
os.path.join(os.path.abspath(self.DataDir()), 'dummy_pages')
On 2010/09/21 20:09:36, Nirnimesh wrote:
> lines 36-45: use self.GetFileURLForPath

Done.

http://codereview.chromium.org/3477001/diff/1/12#newcode48
chrome/test/functional/ntp.py:48: """Tests that there are no sites or closed
tabs with a fresh profile,
On 2010/09/21 20:09:36, Nirnimesh wrote:
> No implementation-detail in first line of docstring please.
> This is the line that'll get printed out when the test is run.

Done.

http://codereview.chromium.org/3477001/diff/1/12#newcode92
chrome/test/functional/ntp.py:92: 
On 2010/09/21 20:09:36, Nirnimesh wrote:
> nit: need 2 blank lines at global scope

Done.

http://codereview.chromium.org/3477001/diff/1/13
File chrome/test/pyautolib/ntp_model.py (right):

http://codereview.chromium.org/3477001/diff/1/13#newcode7
chrome/test/pyautolib/ntp_model.py:7: """NTPModel: python representation of the
NTP. This does not refer to any
On 2010/09/21 01:52:19, Nirnimesh wrote:
> nit: recommend breaking the portion "This does not.." to a new line, after a
> blank line.

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode19
chrome/test/pyautolib/ntp_model.py:19: 
On 2010/09/21 20:09:36, Nirnimesh wrote:
> need another blank line

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode21
chrome/test/pyautolib/ntp_model.py:21: 
On 2010/09/21 20:09:36, Nirnimesh wrote:
> docstring please

I'm not quite sure what docstring is supposed to go here. Can you clarify?

http://codereview.chromium.org/3477001/diff/1/13#newcode22
chrome/test/pyautolib/ntp_model.py:22: def __init__(self, sender):
On 2010/09/21 01:52:19, Nirnimesh wrote:
> Please add Args: section. What is sender?

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode24
chrome/test/pyautolib/ntp_model.py:24: self.sender = sender
On 2010/09/21 01:52:19, Nirnimesh wrote:
> nit: s/sender/_sender/?

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode39
chrome/test/pyautolib/ntp_model.py:39: def GetThumbnailAt(self, index):
On 2010/09/21 01:52:19, Nirnimesh wrote:
> I'm not sure you need this method. After calling GetThumnails() (which is a
> list), it's easy (and common) for a python user to pull out a particular
> element.

Hehe, good point.

http://codereview.chromium.org/3477001/diff/1/13#newcode40
chrome/test/pyautolib/ntp_model.py:40: """Returns the thumbnail dict at the
given index. See |GetThumbnails| for
On 2010/09/21 20:09:36, Nirnimesh wrote:
> Please break out the 2nd line onwards into a new paragraph.

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode53
chrome/test/pyautolib/ntp_model.py:53: def MoveThumbnail(self, thumbnail,
new_index):
On 2010/09/21 01:52:19, Nirnimesh wrote:
> I'm not sure this is the right place for this function.
> Model should just *contain* the data, not have actions.
> Recommend moving all these action functions to pyauto.py.

I didn't want to crowd pyauto.py with methods that will hardly be used in tests
outside of NTP tests. Is there another way I can separate this out? Or can I
rename this class to something more appropriate?

http://codereview.chromium.org/3477001/diff/1/13#newcode85
chrome/test/pyautolib/ntp_model.py:85: def PinThumbnail(self, thumbnail):
On 2010/09/21 01:52:19, Nirnimesh wrote:
> please document |thumbnail| here and everywhere below

Done.

http://codereview.chromium.org/3477001/diff/1/13#newcode88
chrome/test/pyautolib/ntp_model.py:88: index = self.GetThumbnailIndex(thumbnail)
On 2010/09/21 01:52:19, Nirnimesh wrote:
> please merge this with the next line

Done.

http://codereview.chromium.org/3477001/diff/1/15
File chrome/test/pyautolib/pyauto_errors.py (right):

http://codereview.chromium.org/3477001/diff/1/15#newcode13
chrome/test/pyautolib/pyauto_errors.py:13: class
NTPThumbnailNotShownError(RuntimeError):
On 2010/09/21 01:52:19, Nirnimesh wrote:
> How about moving this to ntp_model.py?
> It's good if it stays close to where it's used.

Done.

sky

TopSites is currently disabled and I'm in the process of refactoring it. Could you hold ...

10 years, 3 months ago (2010-09-22 23:51:41 UTC) #6

sure

Expand Messages | Collapse Messages