Clean up activity log URLs when history is cleaned.

Clean up URLs from activity log when a user cleans history via the history UI or using the extensions api.


BUG=253367
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221436

[karenlees, 2013-09-03 21:22:09]

Hi,

I'm not sure if these are the best places to make the changes, so suggestions
welcome. I tried modifying the history_service.cc file but am not allowed to add
dependencies there.

We want to clean URLs from the activity log when a user deletes URLs from their
history.

Thanks,

Karen

[Bernhard Bauer, 2013-09-03 21:55:42]

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:280: if (activity_log) {
When is this NULL?

Also, nit: braces aren't required around single-line statements (and left out
for example above). So for consistency's sake I'd also leave them out here.

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/ui/webui/hi...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/ui/webui/hi...
chrome/browser/ui/webui/history_ui.cc:626: extensions::ActivityLog* activity_log
= extensions::ActivityLog::GetInstance(
Nit: This might look a bit nicer if you break after the equals sign.

[Matt Perry, 2013-09-03 22:16:23]

lgtm

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:281:
activity_log->RemoveURLs(restrict_urls);
FYI, restrict_urls might be empty, in which case this will purge everything

[felt, 2013-09-03 23:03:46]

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/history/history_api.cc:449: // Also clean out from
the activity log.
the activity log api needed to declare its dependency with
DeclareFactoryDependencies:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

but I noticed that this API doesn't even do that for the HistoryService. so...
maybe you don't need to here? but you should keep an eye out for problems in the
browser tests if you remove the if.

[karenlees, 2013-09-04 21:35:05]

I've set the try bots running on the latest version and I'll also run all the
browser to tests to make sure they don't fail at the new DCHECK.

Matt - do I need to add the DeclareFactoryDependencies or will it be ok without?

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:280: if (activity_log) {
On 2013/09/03 21:55:42, Bernhard Bauer wrote:
> When is this NULL?
> 
> Also, nit: braces aren't required around single-line statements (and left out
> for example above). So for consistency's sake I'd also leave them out here.

Removed check. If it is not initialized before this code is called then it could
be NULL but discussing more with Adrienne it should be ok without the NULL check
in this file.

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:281:
activity_log->RemoveURLs(restrict_urls);
On 2013/09/03 22:16:23, Matt Perry wrote:
> FYI, restrict_urls might be empty, in which case this will purge everything

If the restrict URLs is empty the activity log method will clear all URLs, so it
should be consistent with how the history clearing is done.  If empty shouldn't
cause all URLs to be deleted from this part of the code I can add an if
condition here.  I wasn't sure what you meant by the comment.

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/ui/webui/hi...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/ui/webui/hi...
chrome/browser/ui/webui/history_ui.cc:626: extensions::ActivityLog* activity_log
= extensions::ActivityLog::GetInstance(
On 2013/09/03 21:55:42, Bernhard Bauer wrote:
> Nit: This might look a bit nicer if you break after the equals sign.

Done.

[Matt Perry, 2013-09-04 21:38:23]

Adrienne's right - you should declare the factory dependencies in HistoryAPI. It
might work without, but it's a race condition.

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/23785002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:281:
activity_log->RemoveURLs(restrict_urls);
On 2013/09/04 21:35:05, karenlees wrote:
> On 2013/09/03 22:16:23, Matt Perry wrote:
> > FYI, restrict_urls might be empty, in which case this will purge everything
> 
> If the restrict URLs is empty the activity log method will clear all URLs, so
it
> should be consistent with how the history clearing is done.  If empty
shouldn't
> cause all URLs to be deleted from this part of the code I can add an if
> condition here.  I wasn't sure what you meant by the comment. 

Just making sure it was intended. Sounds like it was.

[karenlees, 2013-09-04 22:03:48]

Added dependency.

[Matt Perry, 2013-09-04 22:17:53]

lgtm

[Bernhard Bauer, 2013-09-04 22:21:14]

browsing_data LGTM.

[felt, 2013-09-04 23:00:05]

lgtm

[commit-bot: I haz the power, 2013-09-04 23:02:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/karenlees@chromium.org/23785002/22001

[commit-bot: I haz the power, 2013-09-04 23:24:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[karenlees, 2013-09-04 23:27:43]

On 2013/09/04 23:24:45, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on ios_dbg_simulator.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

I think the failure is unrelated to my code:
   xcodebuild: error: SDK "iphonesimulator6.1" cannot be located.


I'll try sending to the CQ again first thing tomorrow.

[commit-bot: I haz the power, 2013-09-05 14:09:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/karenlees@chromium.org/23785002/22001

[commit-bot: I haz the power, 2013-09-05 15:31:44]

Change committed as 221436