base::Bind: More converts, mostly in WebUI.

base::Bind: More converts, mostly in WebUI.

BUG=none
TEST=none

R=csilv@chromium.org,ajwong@chromium.org

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

[csilv, 2011-10-04 01:43:13]

http://codereview.chromium.org/8113025/diff/1/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/8113025/diff/1/base/callback.h#newcode28
base/callback.h:28: // In programming languages, a closure is a first-class
function where all its
I assume this should be edited somewhere else as per line 3.

[awong, 2011-10-04 01:48:09]

looks pretty good, with the exception of the replacement of the cache clearing
task.

http://codereview.chromium.org/8113025/diff/1/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/8113025/diff/1/base/callback.h#newcode28
base/callback.h:28: // In programming languages, a closure is a first-class
function where all its
Please modify callback.h.pump

and then execute from src

./testing/gtest/scripts/pump.py base/callback.h.pump

This should regenerate this file (which hopefully will look the same).  Finally,
checkin both files.

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chrome_...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chrome_...
chrome/browser/ui/webui/chrome_url_data_manager.cc:54:
base::Bind(AddDataSourceOnIOThread,
Use "&AddDataSourceOnIOThread" insteads. It's slightly more proper.

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:106: #if 0
Should this just be removed?

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:626:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback);
I don't think is quite right....

StartClearingDnsCache() is invoked on the UI thread right?  Then
weak_factory_.GetWeakPtr() is going to generate a WeakPtr that can only be
deferenced on the UI thread.  Thus, callback.Run() can only be called on the UI
thread, yet it is being posted to the IO thread.  I think this will DCHECK.

This is different from the previous behavior where ClearDnsCacheTaskOnIOThread
was a disassociated Task that always runs regardless of whether or not the
originating SigninScreenHandler object is still alive. The liveliness of
SigninScreenHandler was only tested the NRM() for OnDnsCleaered was posted back
to the UI thread.

To simulate the old behavior you'd want to make ClearDnsCache either a static
method, or just a file-static function.

However, looking at this more, I think this is a really good candidate for
PostTaskAndReply.  Try this:

namespace {
void ClearDnsCache(IOThread* io_thread) {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  if (browser_shutdown::IsTryingToQuit())
    return;

  io_thread->globals()->dnsrr_resolver.get()->OnIPAddressChanged();
}
}  // namespace

Then in StartClearingDnsCache()

BrowserThread::PostTaskAndReply(BrowserThread::IO, FROM_HERE
   base::Bind(&ClearDnsCache, g_browser_process->io_thread()),
   base::Bind(&SigninScreenHandler::OnDnsCleared, weak_factory_.GetWeakPtr());

See message_loop_proxy.h's file comment for PostTaskAndReply() for details, but
here's the summary.

The first Closure, ClearDnsCache, is the "task."  It is run on the target
thread.

The second Closure, OnDnsCleared, is the "reply."  It is run on the current
thread.

Both closures and all arguments are deleted on the current thread.

This ensures you can safely use a WeakPtr<> to make the reply no-op itself if
the receiving object has died while task is running.

Let me know if that makes sense.

[James Hawkins, 2011-10-04 02:09:20]

http://codereview.chromium.org/8113025/diff/1/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/8113025/diff/1/base/callback.h#newcode28
base/callback.h:28: // In programming languages, a closure is a first-class
function where all its
On 2011/10/04 01:48:09, awong wrote:
> Please modify callback.h.pump
> 
> and then execute from src
> 
> ./testing/gtest/scripts/pump.py base/callback.h.pump
> 
> This should regenerate this file (which hopefully will look the same). 
Finally,
> checkin both files.

Done.

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chrome_...
File chrome/browser/ui/webui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chrome_...
chrome/browser/ui/webui/chrome_url_data_manager.cc:54:
base::Bind(AddDataSourceOnIOThread,
On 2011/10/04 01:48:09, awong wrote:
> Use "&AddDataSourceOnIOThread" insteads. It's slightly more proper.

Done.

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:106: #if 0
On 2011/10/04 01:48:09, awong wrote:
> Should this just be removed?

Done.

http://codereview.chromium.org/8113025/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:626:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback);
On 2011/10/04 01:48:09, awong wrote:
> I don't think is quite right....
> 
> StartClearingDnsCache() is invoked on the UI thread right?  Then
> weak_factory_.GetWeakPtr() is going to generate a WeakPtr that can only be
> deferenced on the UI thread.  Thus, callback.Run() can only be called on the
UI
> thread, yet it is being posted to the IO thread.  I think this will DCHECK.
> 
> This is different from the previous behavior where ClearDnsCacheTaskOnIOThread
> was a disassociated Task that always runs regardless of whether or not the
> originating SigninScreenHandler object is still alive. The liveliness of
> SigninScreenHandler was only tested the NRM() for OnDnsCleaered was posted
back
> to the UI thread.
> 
> To simulate the old behavior you'd want to make ClearDnsCache either a static
> method, or just a file-static function.
> 
> However, looking at this more, I think this is a really good candidate for
> PostTaskAndReply.  Try this:
> 
> namespace {
> void ClearDnsCache(IOThread* io_thread) {
>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
>   if (browser_shutdown::IsTryingToQuit())
>     return;
> 
>   io_thread->globals()->dnsrr_resolver.get()->OnIPAddressChanged();
> }
> }  // namespace
> 
> Then in StartClearingDnsCache()
> 
> BrowserThread::PostTaskAndReply(BrowserThread::IO, FROM_HERE
>    base::Bind(&ClearDnsCache, g_browser_process->io_thread()),
>    base::Bind(&SigninScreenHandler::OnDnsCleared, weak_factory_.GetWeakPtr());
> 
> See message_loop_proxy.h's file comment for PostTaskAndReply() for details,
but
> here's the summary.
> 
> The first Closure, ClearDnsCache, is the "task."  It is run on the target
> thread.
> 
> The second Closure, OnDnsCleared, is the "reply."  It is run on the current
> thread.
> 
> Both closures and all arguments are deleted on the current thread.
> 
> This ensures you can safely use a WeakPtr<> to make the reply no-op itself if
> the receiving object has died while task is running.
> 
> Let me know if that makes sense.

Done.

[awong, 2011-10-04 02:14:48]

LGTM

[csilv, 2011-10-04 02:48:50]

lgtm