Fix the allHistory flag on chrome.history.onVisitRemoved event.

Fix the allHistory flag on chrome.history.onVisitRemoved event.

Calling chrome.history.deleteAll results in chrome.history.onVisitRemoved
events to be dispatched with the allHistory flag being false instead of true.
This fix changes the call to ExpireHistoryBetween so that begin_time and
end_time are null which results in the history backend using a special path
to delete all history and dispatch the right event.

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

[msimonides, 2013-07-08 07:34:36]

I have made the fix in the history API but I wonder if the issue isn't in the
history backend itself.
Maybe the backend should set the all_history flag event on
NOTIFICATION_HISTORY_URLS_DELETED when all entries are removed regardless of how
it was called.

[Matt Perry, 2013-07-08 21:18:51]

The change seems unrelated to the CL description. Also, is there a bug entry for
this?

[msimonides, 2013-07-09 13:53:39]

On 2013/07/08 21:18:51, Matt Perry wrote:
> The change seems unrelated to the CL description. Also, is there a bug entry
for
> this?

I have filed an issue here:
https://code.google.com/p/chromium/issues/detail?id=258445

The change is related to the description but maybe my description is lacking
context.

What happens is this:
HistoryDeleteAllFunction::RunAsyncImpl calls
HistoryService::ExpireHistoryBetween which in turn calls
HistoryBackend::ExpireHistoryBetween. Based on whether begin_time and end_time
are null Time values or not this method calls either DeleteAllHistory or
expirer_.ExpireHistoryBetween.
Both DeleteAllHistory and expirer_.ExpireHistoryBetween send the
NOTIFICATION_HISTORY_URLS_DELETED notification. The details for this
notification, URLsModifiedDetails, has a bool all_history flag. The
DeleteAllHistory method sets the all_history flag to true while the
expirer_.ExpireHistoryBetween method sets it to false.
The history API observes the NOTIFICATION_HISTORY_URLS_DELETED and in
HistoryEventRouter::HistoryUrlsRemoved translates the all_history flag to the
allHistory property on the argument to chrome.history.onVisitRemoved listener.

Therefore passing null Time values to the HistoryService::ExpireHistoryBetween
is different than passing a regular interval, even if the regular interval is
since the Unix epoch till the present moment and results in deleting all the
history entries. The former causes DeleteAllHistory to be called and the latter
results in the expirer object being used. And the visible result is in the value
of the allHistory flag.
And that's why this quick-fix looks the way it does.

As I have mentioned in my previous message, the issue actually may be in the
HistoryBackend - maybe the expirer_.ExpireHistoryBetween method should set the
all_history flag to true if all history entries get cleared.

[Matt Perry, 2013-07-09 18:11:07]

Makes sense! Thanks for the explanation.

Please change the BUG= line to BUG=<bug number>.

Then LGTM

[commit-bot: I haz the power, 2013-07-10 07:02:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/18755003/1

[commit-bot: I haz the power, 2013-07-10 07:36:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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.

[msimonides, 2013-07-11 07:50:21]

Added a compilation fixup

[msimonides, 2013-07-11 09:36:17]

On 2013/07/11 07:50:21, msimonides wrote:
> Added a compilation fixup

Looks like I cannot run try builds :-/
It should compile now though.

[commit-bot: I haz the power, 2013-07-12 07:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/18755003/26001

[commit-bot: I haz the power, 2013-07-12 11:05:01]

Change committed as 211376