Adding chrome::NOTIFICATION_BROWSING_DATA_REMOVED.

Adding chrome::NOTIFICATION_BROWSING_DATA_REMOVED.

After BrowsingDataRemover removes browsing data, it now triggers a
chrome::NOTIFICATION_BROWSING_DATA_REMOVED, which uses the profile
in which data was removed as it's |source|, and a struct containing the
beginning of the removal's timeframe, and the removal mask
(BrowsingDataRemover::NotificationDetail) as it's |details|.

BUG=107202
TEST=BrowsingDataRemover unit tests.


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

[Mike West, 2011-12-13 10:30:33]

Hi Jochen, can you take a quick look at this? It should implement the
notification we discussed yesterday... It's the first time I'm touching the
notification system, so I'd appreciate a keen eye to make sure I'm not screwing
things up (I'm particularly not sure about where ownership of the |details|
object should lie...).

Bernhard, FYI.

Dominic, I'm planning on using this to test the `chrome.clear.*` APIs: would you
be satisfied if those API tests simply ensure that the BrowsingDataRemover is
called successfully? Or do you think we should do crazy things like creating
cookies and checking that they've been removed after the method comes back?
Since we test that already in the BDR unit tests, I'm hoping we can avoid it for
the extension API. :)

Thanks, all!

-Mike

[battre, 2011-12-13 12:16:33]

Yes, using the signal SGTM.

http://codereview.chromium.org/8907015/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/8907015/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.h:45: typedef std::pair<base::Time, int>
BrowsingDataRemoverNotificationDetail;
Can you make this a struct? Neither the meaning of base::Time nor int seem clear
to me from this definition.

[Mike West, 2011-12-13 13:07:54]

Thanks Dominic.

http://codereview.chromium.org/8907015/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/8907015/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.h:45: typedef std::pair<base::Time, int>
BrowsingDataRemoverNotificationDetail;
Good idea. Done.

[jochen (gone - plz use gerrit), 2011-12-13 13:18:47]

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.cc:390:
content::Source<Profile>(profile_->GetOriginalProfile()),
can you invoke the browsing data deleter on the incognito profile? Will it only
remove incognito data then? Then the source should also be the incognito profile

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:83: struct NotificationDetail {
NotificationDetails

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:85: int removal_mask);
for structs, we usually also provide a constructor with no parameters

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:324: base::Time
called_with_time_;
what about just storing a NotificationDetails object here, instead of the
individual fields?

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:353:
EXPECT_EQ(GetRemovalMask(), BrowsingDataRemover::REMOVE_COOKIES);
EXPECT_EQ takes expected, actual as parameters IIRC, not the other way round
(here and everywhere else in this file)

[Mike West, 2011-12-13 14:23:26]

Thanks Jochen.

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.cc:390:
content::Source<Profile>(profile_->GetOriginalProfile()),
On 2011/12/13 13:18:48, jochen wrote:
> can you invoke the browsing data deleter on the incognito profile? Will it
only
> remove incognito data then? Then the source should also be the incognito
profile

Yup. I should just use the profile here, not the originalprofile. Changed.

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:83: struct NotificationDetail {
On 2011/12/13 13:18:48, jochen wrote:
> NotificationDetails

Done.

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.h:85: int removal_mask);
On 2011/12/13 13:18:48, jochen wrote:
> for structs, we usually also provide a constructor with no parameters

Done.

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:324: base::Time
called_with_time_;
On 2011/12/13 13:18:48, jochen wrote:
> what about just storing a NotificationDetails object here, instead of the
> individual fields?

Done, though I'm not convinced it's any prettier. :)

http://codereview.chromium.org/8907015/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover_unittest.cc:353:
EXPECT_EQ(GetRemovalMask(), BrowsingDataRemover::REMOVE_COOKIES);
On 2011/12/13 13:18:48, jochen wrote:
> EXPECT_EQ takes expected, actual as parameters IIRC, not the other way round
> (here and everywhere else in this file)

Needle, haystack. Haystack, needle. What... you mean C++ isn't PHP? :)

Done.

[jochen (gone - plz use gerrit), 2011-12-13 14:26:29]

lgtm

[commit-bot: I haz the power, 2011-12-13 15:51:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/8907015/3003

[commit-bot: I haz the power, 2011-12-13 17:11:11]

Change committed as 114225