Do not open file on UI thread in VisitedLinkMaster

Do not open file on UI thread in VisitedLinkMaster
while writing table after rebuilding.

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

[sky, 2012-07-18 14:45:41]

Brett should review this one. I'm removing myself as a reivewer.

[brettw, 2012-07-23 17:45:13]

Sorry I've been slow, I'll try to get to this soon.

[brettw, 2012-07-27 22:11:14]

lgtm

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_master.cc:205: FreeURLTable();
Can you add a comment here that the file_ pointer will be freed as a side effect
of freeing the URL table?

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_master.cc:707: PostIOTask(FROM_HERE,
base::Bind(&AsyncClose, file_));
After this call can you null out the file_ pointer and comment that deleting of
it will be taken care of by AsyncClose?

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
File chrome/browser/visitedlink/visitedlink_master.h (right):

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_master.h:133: typedef FILE* FilePtr;
I think I prefer just seeing FILE** in the various places since then there's
nothing to look up what it means. So I'd probably delete this. Can you also add
that this class owns the pointer.

https://chromiumcodereview.appspot.com/10800005/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_master.h:344: FilePtr* file_;
Can you clarify the comment here? There are now three cases, file_ is NULL
(rebuilding and haven't written a new version as in the old comment), and that
it exists but points to NULL (because it's being opened on a background thread).

[commit-bot: I haz the power, 2012-07-27 22:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10800005/1

[brettw, 2012-07-27 22:12:06]

Sorry, didn't mean LGTM, pressed the wrong button. I'd like to double check this
one last time before it goes in.

I've convinced myself that this is correct. Thanks and sorry for taking so long.
I just had some comment suggestions above.

[pivanof, 2012-07-29 02:58:06]

brettw: I've updated the patch. Please take a look.

[brettw, 2012-07-31 21:22:46]

LGTM, just one minor style comment. Thanks!

https://chromiumcodereview.appspot.com/10800005/diff/6005/chrome/browser/visi...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

https://chromiumcodereview.appspot.com/10800005/diff/6005/chrome/browser/visi...
chrome/browser/visitedlink/visitedlink_master.cc:70: if (!(*file)) {
Can you just write DLOG_IF(ERROR, !(*file)) << "...";

[pivanof, 2012-07-31 21:57:12]

On 2012/07/31 21:22:46, brettw wrote:
>
https://chromiumcodereview.appspot.com/10800005/diff/6005/chrome/browser/visi...
> File chrome/browser/visitedlink/visitedlink_master.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10800005/diff/6005/chrome/browser/visi...
> chrome/browser/visitedlink/visitedlink_master.cc:70: if (!(*file)) {
> Can you just write DLOG_IF(ERROR, !(*file)) << "...";

Updated.

[commit-bot: I haz the power, 2012-07-31 21:57:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10800005/15004

[commit-bot: I haz the power, 2012-07-31 23:19:04]

Change committed as 149310