Issue 8550010: base::Bind() conversion for webkit.

Issue 8550010: base::Bind() conversion for webkit. (Closed)

Created:
9 years, 1 month ago by dcheng

Modified:
9 years, 1 month ago

Reviewers:
csilv, awong, tony

CC:
chromium-reviews, Erik does not do reviews, brettw-cc_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

base::Bind() conversion for webkit. BUG=none TEST=trybots Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111060

Patch Set 4 : . #

Created: 9 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+103 lines, -124 lines)			Patch
M	chrome/renderer/chrome_render_view_observer.cc	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/renderer/extensions/extension_helper.cc	View		3 chunks	+7 lines, -3 lines	0 comments	Download
M	webkit/glue/alt_error_page_resource_fetcher.cc	View		2 chunks	+4 lines, -2 lines	0 comments	Download
M	webkit/glue/cpp_bound_class.h	View		7 chunks	+18 lines, -21 lines	0 comments	Download
M	webkit/glue/cpp_bound_class.cc	View	1 2 3	7 chunks	+20 lines, -28 lines	0 comments	Download
M	webkit/glue/image_resource_fetcher.h	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	webkit/glue/image_resource_fetcher.cc	View	1 2 3	3 chunks	+9 lines, -8 lines	0 comments	Download
M	webkit/glue/resource_fetcher.h	View		4 chunks	+7 lines, -6 lines	0 comments	Download
M	webkit/glue/resource_fetcher.cc	View	1 2 3	3 chunks	+7 lines, -8 lines	0 comments	Download
M	webkit/glue/resource_fetcher_unittest.cc	View	1 2	2 chunks	+5 lines, -3 lines	0 comments	Download
M	webkit/glue/webthread_impl.cc	View	1	2 chunks	+13 lines, -16 lines	0 comments	Download
M	webkit/glue/weburlloader_impl.cc	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	webkit/support/webkit_support.cc	View	1 2	3 chunks	+4 lines, -22 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

dcheng

(Sorry, I stole extension_helper and chrome_render_view_observer from you because of the ResourceFetcher dependencies)

9 years, 1 month ago (2011-11-20 18:50:30 UTC) #1

csilv

dcheng, looking pretty good, just a minor changes for me. ajwong, could you give this ...

9 years, 1 month ago (2011-11-21 21:26:31 UTC) #3

awong

Wow...fun looking code. Thanks for tackling it! My main concern is with the BindMethod() call. ...

9 years, 1 month ago (2011-11-21 21:46:56 UTC) #4

dcheng

http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.cc File webkit/glue/cpp_bound_class.cc (right): http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.cc#newcode198 webkit/glue/cpp_bound_class.cc:198: const NPVariant* args, On 2011/11/21 21:46:56, awong wrote: > ...

9 years, 1 month ago (2011-11-21 22:04:16 UTC) #5

awong

LGTM Feel free to fix BindMethod() in a separate change. Or if you can't do ...

9 years, 1 month ago (2011-11-21 22:29:20 UTC) #6

LGTM

Feel free to fix BindMethod() in a separate change.  Or if you can't do it,
let me know so I can clean it up.

Thanks!

-Albert

On Mon, Nov 21, 2011 at 2:04 PM, <dcheng@chromium.org> wrote:

>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/cpp_bound_class.cc<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.cc>
> File webkit/glue/cpp_bound_class.cc (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/cpp_bound_class.cc#**newcode198<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.cc#newcode198>
> webkit/glue/cpp_bound_class.**cc:198: const NPVariant* args,
> On 2011/11/21 21:46:56, awong wrote:
>
>> Fix indenting?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/cpp_bound_class.cc#**newcode255<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.cc#newcode255>
> webkit/glue/cpp_bound_class.**cc:255: if (old_callback != methods_.end()
> && callback.is_null()) {
> On 2011/11/21 21:26:31, csilv wrote:
>
>> If the callback is null, there's no reason to call methods_.find, so I
>>
> would
>
>> move that check before line 254.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/cpp_bound_class.h<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.h>
> File webkit/glue/cpp_bound_class.h (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/cpp_bound_class.h#**newcode26<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/cpp_bound_class.h#newcode26>
> webkit/glue/cpp_bound_class.h:**26: #include "base/bind.h"
> On 2011/11/21 21:46:56, awong wrote:
>
>> We try pretty hard to keep bind.h out of headers.
>>
>
>  I think BindMethod() is now largely obsolete as people can just call
>>
> Bind(),
>
>> then pass to BindCallback.  How big of a change is that?
>>
>
> It's not a huge change, but it might make sense in a separate change
> since it spans a lot of directories (chrome/renderer, content/renderer,
> net,
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/image_resource_fetcher.cc<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/image_resource_fetcher.cc>
> File webkit/glue/image_resource_**fetcher.cc (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/image_resource_fetcher.**cc#newcode7<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/image_resource_fetcher.cc#newcode7>
> webkit/glue/image_resource_**fetcher.cc:7: #include "base/bind.h"
> On 2011/11/21 21:26:31, csilv wrote:
>
>> add #include "base/bind_helpers.h"
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/image_resource_fetcher.**cc#newcode58<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/image_resource_fetcher.cc#newcode58>
> webkit/glue/image_resource_**fetcher.cc:58: callback_.Reset();
> On 2011/11/21 21:46:56, awong wrote:
>
>> This works, but semantically is slightly funny.  I'd just do
>>
>
>  // Take a reference to the callback as a stack variable
>> // before running the callback as it may lead to
>> // our destruction.
>> Callback callback = callback_;
>> callback.Run(this, bitmap);
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/resource_fetcher.cc<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/resource_fetcher.cc>
> File webkit/glue/resource_fetcher.**cc (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/resource_fetcher.cc#**newcode65<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/resource_fetcher.cc#newcode65>
> webkit/glue/resource_fetcher.**cc:65: // our destruction.
> On 2011/11/21 21:46:56, awong wrote:
>
>> Update similar to image_resource_fetcher.cc
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/resource_fetcher_**unittest.cc<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/resource_fetcher_unittest.cc>
> File webkit/glue/resource_fetcher_**unittest.cc (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
glue/resource_fetcher_**unittest.cc#newcode7<http://codereview.chromium.org/8550010/diff/2001/webkit/glue/resource_fetcher_unittest.cc#newcode7>
> webkit/glue/resource_fetcher_**unittest.cc:7: #include "base/bind.h"
> On 2011/11/21 21:26:31, csilv wrote:
>
>> add #include base/bind_helpers.h
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
support/webkit_support.cc<http://codereview.chromium.org/8550010/diff/2001/webkit/support/webkit_support.cc>
> File webkit/support/webkit_support.**cc (right):
>
> http://codereview.chromium.**org/8550010/diff/2001/webkit/**
>
support/webkit_support.cc#**newcode9<http://codereview.chromium.org/8550010/diff/2001/webkit/support/webkit_support.cc#newcode9>
> webkit/support/webkit_support.**cc:9: #include "base/bind.h"
> On 2011/11/21 21:26:31, csilv wrote:
>
>> add #include base/bind_helpers.h
>>
>
> Done.
>
>
http://codereview.chromium.**org/8550010/<http://codereview.chromium.org/8550...
>

commit-bot: I haz the power

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

9 years, 1 month ago (2011-11-21 22:56:54 UTC) #8

Change committed as 111060

Expand Messages | Collapse Messages