Clear Browsing Data hook added and some small tests.

Clear Browsing Data hook added and some small tests. BUG=36176
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52402

[Nirnimesh, 2010-07-10 00:03:35]

LGTM, with nits.

Please run through try servers.

http://codereview.chromium.org/2909005/diff/5001/6002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/5001/6002#newcode445
chrome/browser/automation/automation_provider.h:445: // functionality to
RemoveBrowsingData but this call is syncronous.
s/syncronous/synchronous/

http://codereview.chromium.org/2909005/diff/5001/6004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/5001/6004#newcode625
chrome/browser/automation/automation_provider_observers.h:625:
~AutomationProviderBrowsingDataObserver() {}
you don't need this line

http://codereview.chromium.org/2909005/diff/5001/6006
File chrome/test/functional/browsing_data.py (right):

http://codereview.chromium.org/2909005/diff/5001/6006#newcode37
chrome/test/functional/browsing_data.py:37: last_hour = now - num_secs_in_hour -
1
don't be too strict. take a larger time margin, say 5 sec.
It's possible for python's time to be slightly off from chrome's time on
windows.

http://codereview.chromium.org/2909005/diff/5001/6006#newcode68
chrome/test/functional/browsing_data.py:68: history =
self.GetHistoryInfo().History()
merge this to line 70
self.assertEqual(1, len(self.GetHistoryInfo().History()))

http://codereview.chromium.org/2909005/diff/5001/6006#newcode69
chrome/test/functional/browsing_data.py:69: downloads =
self.GetDownloadsInfo().Downloads()
merge this into line 71
self.assertEqual(1, len(self.GetDownloadsInfo().Downloads()))

http://codereview.chromium.org/2909005/diff/5001/6007
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2909005/diff/5001/6007#newcode761
chrome/test/pyautolib/pyauto.py:761: """Clear the specified browsing data.
Mention:

Implements the features available in the "Clear Browsing Data" UI.

[Nirnimesh, 2010-07-10 00:09:52]

Add BUG=36176 in the description before committing.

[Paweł Hajdan Jr., 2010-07-10 18:46:14]

Drive-by with an automation comment. Please let me take another look before you
commit. I didn't review the implementation, waiting to see the de-duplicated
code.

http://codereview.chromium.org/2909005/diff/5001/6002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/5001/6002#newcode447
chrome/browser/automation/automation_provider.h:447: void
ClearBrowsingData(Browser* browser,
Hello, code duplication. We already have RemoveBrowsingData. Could you somehow
merge these two methods?

[Nirnimesh, 2010-07-12 01:53:49]

http://codereview.chromium.org/2909005/diff/5001/6002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/5001/6002#newcode447
chrome/browser/automation/automation_provider.h:447: void
ClearBrowsingData(Browser* browser,
On 2010/07/10 18:46:14, Paweł Hajdan Jr. wrote:
> Hello, code duplication. We already have RemoveBrowsingData. Could you somehow
> merge these two methods?

RemoveBrowsingData is async (and is used by chrome_frame).
Alyssa calls this out in the comment above. Since RemoveBrowsingData is barely 3
lines it seems an overkill to attempt to refactor it.

[Paweł Hajdan Jr., 2010-07-13 03:13:39]

Okay, I'm fine with having our own version of this for testing purposes. Some
real review comments now. Just nits.

http://codereview.chromium.org/2909005/diff/20001/21001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2387
chrome/browser/automation/automation_provider.cc:2387: if
(string_to_mask_value.find(removal) == string_to_mask_value.end()) {
nit: Could you rather use ContainsKey from base/stl_util-inl.h?

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2396
chrome/browser/automation/automation_provider.cc:2396: if
(string_to_time_period.find(time_period) ==
nit: same here

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2412
chrome/browser/automation/automation_provider.cc:2412: json_return =
nit: Can we move the error case to the beginning and return early, to avoid
indenting main part of the method?

http://codereview.chromium.org/2909005/diff/20001/21002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/20001/21002#newcode445
chrome/browser/automation/automation_provider.h:445: // functionality to
RemoveBrowsingData but this call is synchronous.
nit: "but this call is" -> "but is" to avoid repeating "this call".

http://codereview.chromium.org/2909005/diff/20001/21004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/20001/21004#newcode622
chrome/browser/automation/automation_provider_observers.h:622: provider_ =
provider;
nit: Please use the initializer list syntax here.

[Alyssa, 2010-07-13 18:17:49]

Having issues getting my CLs to patch to the trybots, so I'll reply again when
that is resolved. But here are my changes based on both sets of feedback.

http://codereview.chromium.org/2909005/diff/5001/6002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/5001/6002#newcode445
chrome/browser/automation/automation_provider.h:445: // functionality to
RemoveBrowsingData but this call is syncronous.
On 2010/07/10 00:03:36, Nirnimesh wrote:
> s/syncronous/synchronous/

Done.

http://codereview.chromium.org/2909005/diff/5001/6004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/5001/6004#newcode625
chrome/browser/automation/automation_provider_observers.h:625:
~AutomationProviderBrowsingDataObserver() {}
On 2010/07/10 00:03:36, Nirnimesh wrote:
> you don't need this line

Done.

http://codereview.chromium.org/2909005/diff/5001/6006
File chrome/test/functional/browsing_data.py (right):

http://codereview.chromium.org/2909005/diff/5001/6006#newcode37
chrome/test/functional/browsing_data.py:37: last_hour = now - num_secs_in_hour -
1
On 2010/07/10 00:03:36, Nirnimesh wrote:
> don't be too strict. take a larger time margin, say 5 sec.
> It's possible for python's time to be slightly off from chrome's time on
> windows.

Done.

http://codereview.chromium.org/2909005/diff/5001/6006#newcode68
chrome/test/functional/browsing_data.py:68: history =
self.GetHistoryInfo().History()
On 2010/07/10 00:03:36, Nirnimesh wrote:
> merge this to line 70
> self.assertEqual(1, len(self.GetHistoryInfo().History()))

Done.

http://codereview.chromium.org/2909005/diff/5001/6006#newcode69
chrome/test/functional/browsing_data.py:69: downloads =
self.GetDownloadsInfo().Downloads()
On 2010/07/10 00:03:36, Nirnimesh wrote:
> merge this into line 71
> self.assertEqual(1, len(self.GetDownloadsInfo().Downloads()))

Done.

http://codereview.chromium.org/2909005/diff/20001/21001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2387
chrome/browser/automation/automation_provider.cc:2387: if
(string_to_mask_value.find(removal) == string_to_mask_value.end()) {
On 2010/07/13 03:13:39, Paweł Hajdan Jr. wrote:
> nit: Could you rather use ContainsKey from base/stl_util-inl.h?

Done.

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2396
chrome/browser/automation/automation_provider.cc:2396: if
(string_to_time_period.find(time_period) ==
On 2010/07/13 03:13:39, Paweł Hajdan Jr. wrote:
> nit: same here

Done.

http://codereview.chromium.org/2909005/diff/20001/21001#newcode2412
chrome/browser/automation/automation_provider.cc:2412: json_return =
On 2010/07/13 03:13:39, Paweł Hajdan Jr. wrote:
> nit: Can we move the error case to the beginning and return early, to avoid
> indenting main part of the method?

Pulled out sending the error into a new method so I could avoid the indentation
all over.

http://codereview.chromium.org/2909005/diff/20001/21002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/20001/21002#newcode445
chrome/browser/automation/automation_provider.h:445: // functionality to
RemoveBrowsingData but this call is synchronous.
On 2010/07/13 03:13:39, Paweł Hajdan Jr. wrote:
> nit: "but this call is" -> "but is" to avoid repeating "this call".

Done.

http://codereview.chromium.org/2909005/diff/20001/21004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/20001/21004#newcode622
chrome/browser/automation/automation_provider_observers.h:622: provider_ =
provider;
On 2010/07/13 03:13:39, Paweł Hajdan Jr. wrote:
> nit: Please use the initializer list syntax here.

Done.

[Nirnimesh, 2010-07-13 19:08:03]

http://codereview.chromium.org/2909005/diff/5002/21008
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2909005/diff/5002/21008#newcode2527
chrome/browser/automation/automation_provider.cc:2527: void
AutomationProvider::ReturnError(const std::string& err,
I have a CL in flight which does this refactoring, and some more. You can skip
adding this method for now.

http://codereview.chromium.org/2898001/show

http://codereview.chromium.org/2909005/diff/29001/30001#newcode2409
chrome/browser/automation/automation_provider.cc:2409: return;  //
BrowsingDataRemover deletes itself, observer sends the reply.
Did you mean AutomationProviderBrowsingDataObserver deletes itself?
Who deletes BrowsingDataRemover?

http://codereview.chromium.org/2909005/diff/29001/30004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/29001/30004#newcode622
chrome/browser/automation/automation_provider_observers.h:622:
:provider_(provider),
need space after ':'

http://codereview.chromium.org/2909005/diff/29001/30007
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2909005/diff/29001/30007#newcode846
chrome/test/pyautolib/pyauto.py:846: 
Need 2 blank lines at global scope in python

[Alyssa, 2010-07-13 20:45:24]

http://codereview.chromium.org/2909005/diff/5002/21008
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2909005/diff/5002/21008#newcode2527
chrome/browser/automation/automation_provider.cc:2527: void
AutomationProvider::ReturnError(const std::string& err,
On 2010/07/13 19:08:03, Nirnimesh wrote:
> I have a CL in flight which does this refactoring, and some more. You can skip
> adding this method for now.
> 
> http://codereview.chromium.org/2898001/show

Okay, I left a TODO in there for now if that's okay.

http://codereview.chromium.org/2909005/diff/29001/30001#newcode2409
chrome/browser/automation/automation_provider.cc:2409: return;  //
BrowsingDataRemover deletes itself, observer sends the reply.
On 2010/07/13 19:08:04, Nirnimesh wrote:
> Did you mean AutomationProviderBrowsingDataObserver deletes itself?
> Who deletes BrowsingDataRemover?

BrowsingDataRemover deletes itself using DeleteTask. (See comment in
chrome/browser/browsing_data_remover.h)

http://codereview.chromium.org/2909005/diff/29001/30004
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/2909005/diff/29001/30004#newcode622
chrome/browser/automation/automation_provider_observers.h:622:
:provider_(provider),
On 2010/07/13 19:08:04, Nirnimesh wrote:
> need space after ':'

Done.

http://codereview.chromium.org/2909005/diff/29001/30007
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/2909005/diff/29001/30007#newcode846
chrome/test/pyautolib/pyauto.py:846: 
On 2010/07/13 19:08:04, Nirnimesh wrote:
> Need 2 blank lines at global scope in python

Done.

[Nirnimesh, 2010-07-13 20:53:59]

LGTM.
(I think you need to wait for Pawel's LGTM on this CL)

http://codereview.chromium.org/2909005/diff/29001/30001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2909005/diff/29001/30001#newcode2409
chrome/browser/automation/automation_provider.cc:2409: return;  //
BrowsingDataRemover deletes itself, observer sends the reply.
On 2010/07/13 20:45:24, Alyssa wrote:
> On 2010/07/13 19:08:04, Nirnimesh wrote:
> > Did you mean AutomationProviderBrowsingDataObserver deletes itself?
> > Who deletes BrowsingDataRemover?
> 
> BrowsingDataRemover deletes itself using DeleteTask. (See comment in
> chrome/browser/browsing_data_remover.h)

Please comment this fact.

http://codereview.chromium.org/2909005/diff/41001/42001#newcode2420
chrome/browser/automation/automation_provider.cc:2420:
return_value->SetString(L"name", theme->name());
nit: return is redundant here

[Alyssa, 2010-07-14 17:53:57]

Pawel - Any more comments on this CL? Thanks for the review!

[Paweł Hajdan Jr., 2010-07-14 18:00:08]

I think it's good to go (LGTM). Please just see my comment below.

http://codereview.chromium.org/2909005/diff/31002/43002
File chrome/browser/automation/automation_provider.h (right):

http://codereview.chromium.org/2909005/diff/31002/43002#newcode470
chrome/browser/automation/automation_provider.h:470: void ReturnError(const
std::string& err, IPC::Message* reply_message);
I think this is now duplicating some helpers Nirnimesh has created. Not sure if
they are landed yet. Could you coordinate on that? I'm fine with landing this
as-is, but with a TODO to merge the helpers afterwards.

[Alyssa, 2010-07-14 18:12:22]

Good catch. I already removed the code that would duplicate Nirnimesh's work
(and put a TODO) but forgot to remove the function header. Removed it now.
Thanks!