Remove custom Task implementations from VisitedLinkMaster and web_app.

Remove custom Task implementations from VisitedLinkMaster and web_app.

Fairly straightforward conversion except for two things:
- VisitedLinkMaster tries to be really clever and used StackVector. I converted
  it to use a plain old std::string since I don't think the cleverness adds
  anything.
- web_app::CreateShortcut() sole caller doesn't even use the success callback, so it was removed.

BUG=none
TEST=trybots

Do not submit. It's entirely possible we can just delete all the CreateShortcut
related goop in web_app.


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

[dcheng, 2011-12-08 08:22:07]

I'm going to try to find someone to figure out the reason we used StackVector in
VisitedLinkMaster. I don't think it's necessary though, and I think any attempt
to use it with base::Bind would just result in more overhead.

We could...
1. Construct a stack StackVector and bind a copy. This seems silly.
2. Create a helper class that wraps the StackVector creation. This ends up
resulting in at least two heap allocations at least though.

I figured it was best just to keep it simple and use std::string since we can't
do better than two heap allocations anyway.

[James Hawkins, 2011-12-08 18:19:15]

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:486:
base::IgnoreReturn<bool>(base::Bind(&TruncateFile, file_)));
Aren't we supposed to be using IgnoreResult now?

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:866: base::Bind(&AsyncWrite,
Save lines, put parameters onto the same line. The only time one must put
parameters on separate lines is in the function/method declaration.

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (left):

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:151: // Represents a task that
creates web application shortcut. This runs on
This change is very large; can you describe it in more detail?

[dcheng, 2011-12-08 18:37:07]

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:486:
base::IgnoreReturn<bool>(base::Bind(&TruncateFile, file_)));
On 2011/12/08 18:19:15, James Hawkins wrote:
> Aren't we supposed to be using IgnoreResult now?

Done. I didn't know about that =)

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:866: base::Bind(&AsyncWrite,
On 2011/12/08 18:19:15, James Hawkins wrote:
> Save lines, put parameters onto the same line. The only time one must put
> parameters on separate lines is in the function/method declaration.

I can't fit the temporary string ctor in the same line as the rest of the
parameters. It's over by 1 character =(

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (left):

http://codereview.chromium.org/8885007/diff/4001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:151: // Represents a task that
creates web application shortcut. This runs on
On 2011/12/08 18:19:15, James Hawkins wrote:
> This change is very large; can you describe it in more detail?

The actual diff is actually fairly small. The Task::Run() override used to call
CreateShortcutTask::CreateShortcut() and then call a callback with the result.

Since no one ever uses the callback, I removed the parameter and bind to the
original CreateShortcutTask::CreateShortcut() directly. If you look at the diffs
from new lines 155-327, you'll see it's largely the same as the original code.

[James Hawkins, 2011-12-08 18:43:55]

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/ui/views/crea...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/ui/views/crea...
chrome/browser/ui/views/create_application_shortcut_view.cc:380:
shortcut_info_);
Put this on the previous line.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/visitedlink/v...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:866: base::Bind(&AsyncWrite,
base::Bind(&AsyncWrite, file, offset,
           std::string(static_cast<const char*>(data), data_size)));

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:230: NOTREACHED();
Either remove these NOTREACHEDs or remove the returns.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:322: return;
No need to return

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:325: return;
No need to return

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (left):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:47: CreateShortcutCallback* callback);
What no one using this?

[dcheng, 2011-12-08 18:51:28]

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/ui/views/crea...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/ui/views/crea...
chrome/browser/ui/views/create_application_shortcut_view.cc:380:
shortcut_info_);
On 2011/12/08 18:43:56, James Hawkins wrote:
> Put this on the previous line.

Done.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/visitedlink/v...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/visitedlink/v...
chrome/browser/visitedlink/visitedlink_master.cc:866: base::Bind(&AsyncWrite,
On 2011/12/08 18:43:56, James Hawkins wrote:
> base::Bind(&AsyncWrite, file, offset,
>            std::string(static_cast<const char*>(data), data_size)));

Done.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:230: NOTREACHED();
On 2011/12/08 18:43:56, James Hawkins wrote:
> Either remove these NOTREACHEDs or remove the returns.

Done.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:322: return;
On 2011/12/08 18:43:56, James Hawkins wrote:
> No need to return

Done.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.cc:325: return;
On 2011/12/08 18:43:56, James Hawkins wrote:
> No need to return

Done.

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (left):

http://codereview.chromium.org/8885007/diff/6001/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:47: CreateShortcutCallback* callback);
On 2011/12/08 18:43:56, James Hawkins wrote:
> What no one using this?

There's one caller and it passes NULL.

[James Hawkins, 2011-12-08 18:55:53]

LGTM with nit.

http://codereview.chromium.org/8885007/diff/11/chrome/browser/visitedlink/vis...
File chrome/browser/visitedlink/visitedlink_master.cc (right):

http://codereview.chromium.org/8885007/diff/11/chrome/browser/visitedlink/vis...
chrome/browser/visitedlink/visitedlink_master.cc:867:
std::string(static_cast<const char*>(data), data_size)));
The start of parameter lines must be on the same column.

[brettw, 2011-12-08 19:32:26]

visitedlink master LGTM

[commit-bot: I haz the power, 2011-12-08 19:37:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8885007/2005

[commit-bot: I haz the power, 2011-12-08 21:17:23]

Change committed as 113662