Issue 25870002: Abstract BrowserThread knowledge out of AutofillWebDataService.

Issue 25870002: Abstract BrowserThread knowledge out of AutofillWebDataService. (Closed)

Created:
7 years, 2 months ago by blundell

Modified:
7 years, 2 months ago

Reviewers:
Nicolas Zea, benm (inactive), Jói

CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, sgurun-gerrit only

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Abstract BrowserThread knowledge out of AutofillWebDataService. Instead, the AutofillWebDataService now takes in MessageLoopProxy's specifying the UI thread and DB thread that it should use in its constructor. BUG=303020 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227384

Patch Set 1 #

Patch Set 2 : Fix build failures #

Patch Set 3 : Fix android_webview #

Patch Set 4 : Fix up test constructor #

Total comments: 2

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+61 lines, -35 lines)			Patch
M	android_webview/browser/aw_form_database_service.cc	View	1 2	1 chunk	+4 lines, -1 line	0 comments	Download
M	chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/sync/profile_sync_service_autofill_unittest.cc	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/webdata/web_data_service_factory.cc	View		2 chunks	+6 lines, -4 lines	0 comments	Download
M	components/autofill/core/browser/autocomplete_history_manager_unittest.cc	View	1 2 3	2 chunks	+3 lines, -1 line	0 comments	Download
M	components/autofill/core/browser/webdata/autofill_webdata_service.h	View	1 2 3	3 chunks	+14 lines, -2 lines	2 comments	Download
M	components/autofill/core/browser/webdata/autofill_webdata_service.cc	View	1 2 3	4 chunks	+24 lines, -24 lines	0 comments	Download
M	components/autofill/core/browser/webdata/web_data_service_unittest.cc	View	1	1 chunk	+4 lines, -1 line	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

blundell

joi: For the CL as a whole zea: For chrome/browser/sync benm: For android_webview

7 years, 2 months ago (2013-10-03 16:24:02 UTC) #1

Jói

LGTM https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/browser/webdata/autofill_webdata_service.h File components/autofill/core/browser/webdata/autofill_webdata_service.h (right): https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/browser/webdata/autofill_webdata_service.h#newcode45 components/autofill/core/browser/webdata/autofill_webdata_service.h:45: AutofillWebDataService(scoped_refptr<base::MessageLoopProxy> ui_thread, const scoped_refptr<...>& here and following lines.

7 years, 2 months ago (2013-10-03 16:33:55 UTC) #2

blundell

Thanks! https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/browser/webdata/autofill_webdata_service.h File components/autofill/core/browser/webdata/autofill_webdata_service.h (right): https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/browser/webdata/autofill_webdata_service.h#newcode45 components/autofill/core/browser/webdata/autofill_webdata_service.h:45: AutofillWebDataService(scoped_refptr<base::MessageLoopProxy> ui_thread, isherman@ advised me the other way ...

7 years, 2 months ago (2013-10-03 16:43:33 UTC) #3

Jói

Interesting question. I'm not sure there is an authoritative answer. I would assume that passing ...

7 years, 2 months ago (2013-10-03 17:29:43 UTC) #4

Interesting question. I'm not sure there is an authoritative answer.

I would assume that passing it by value would incur one additional
copy, which would incur one additional pair of AddRef/Release, so
passing by reference would save a bit of unnecessary work. If you
decide to store a scoped pointer, you are going to be making a copy of
it, and for copy semantics a const reference is fine.

On the other hand, there is this bug, until it is solved it is
probably more performant to pass it by value when it's in a function
that gets bound to using base::Bind: http://crbug.com/251937.

There is also this old thread, but I don't think we commonly pass
refcounted objects any more without using the smart pointer:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/noOt34XStEU/i5qB8...

A rough approximation of frequency is a code search with these two patterns:
For roughly all scoped_refptr parameters: scoped_refptr[^,]+>[^\(]*[,\)]
For just the const reference scoped_refptr parameters:
const\s+scoped_refptr[^,]+>\s*&[^\(]*[,\)]

This shows 588 const scoped_refptr parameters vs. 1041 passed by value
(1629 in total).

Might be worth asking on chromium-dev@. For now do what is consistent
locally, I'm fine with either.

Cheers,
Jói

On Thu, Oct 3, 2013 at 4:43 PM,  <blundell@chromium.org> wrote:
> Thanks!
>
>
>
>
https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/...
> File components/autofill/core/browser/webdata/autofill_webdata_service.h
> (right):
>
>
https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/...
> components/autofill/core/browser/webdata/autofill_webdata_service.h:45:
> AutofillWebDataService(scoped_refptr<base::MessageLoopProxy> ui_thread,
> isherman@ advised me the other way on a CL yesterday:
>
>
https://codereview.chromium.org/25783002/diff/1/components/autofill/core/brow...
>
> so I looked through the codebase, and both seem to be used with
> frequency. Is there a known preference for const references in this
> case?
>
>
> On 2013/10/03 16:33:55, Jói wrote:
>>
>> const scoped_refptr<...>& here and following lines.
>
>
> https://codereview.chromium.org/25870002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

benm (inactive)

On 2013/10/03 17:42:28, Nicolas Zea wrote: > c/b/sync/ LGTM a_w/ lgtm

7 years, 2 months ago (2013-10-06 11:10:06 UTC) #6

blundell

On 2013/10/03 17:29:43, Jói wrote: > Interesting question. I'm not sure there is an authoritative ...

7 years, 2 months ago (2013-10-07 05:37:11 UTC) #7

On 2013/10/03 17:29:43, Jói wrote:
> Interesting question. I'm not sure there is an authoritative answer.
> 
> I would assume that passing it by value would incur one additional
> copy, which would incur one additional pair of AddRef/Release, so
> passing by reference would save a bit of unnecessary work. If you
> decide to store a scoped pointer, you are going to be making a copy of
> it, and for copy semantics a const reference is fine.
> 
> On the other hand, there is this bug, until it is solved it is
> probably more performant to pass it by value when it's in a function
> that gets bound to using base::Bind: http://crbug.com/251937.
> 
> There is also this old thread, but I don't think we commonly pass
> refcounted objects any more without using the smart pointer:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/noOt34XStEU/i5qB8...
> 
> A rough approximation of frequency is a code search with these two patterns:
> For roughly all scoped_refptr parameters: scoped_refptr[^,]+>[^\(]*[,\)]
> For just the const reference scoped_refptr parameters:
> const\s+scoped_refptr[^,]+>\s*&[^\(]*[,\)]
> 
> This shows 588 const scoped_refptr parameters vs. 1041 passed by value
> (1629 in total).
> 
> Might be worth asking on chromium-dev@. For now do what is consistent
> locally, I'm fine with either.
> 
> Cheers,
> Jói
> 
> 
> On Thu, Oct 3, 2013 at 4:43 PM,  <mailto:blundell@chromium.org> wrote:
> > Thanks!
> >
> >
> >
> >
>
https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/...
> > File components/autofill/core/browser/webdata/autofill_webdata_service.h
> > (right):
> >
> >
>
https://codereview.chromium.org/25870002/diff/13001/components/autofill/core/...
> > components/autofill/core/browser/webdata/autofill_webdata_service.h:45:
> > AutofillWebDataService(scoped_refptr<base::MessageLoopProxy> ui_thread,
> > isherman@ advised me the other way on a CL yesterday:
> >
> >
>
https://codereview.chromium.org/25783002/diff/1/components/autofill/core/brow...
> >
> > so I looked through the codebase, and both seem to be used with
> > frequency. Is there a known preference for const references in this
> > case?
> >
> >
> > On 2013/10/03 16:33:55, Jói wrote:
> >>
> >> const scoped_refptr<...>& here and following lines.
> >
> >
> > https://codereview.chromium.org/25870002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks! I'll keep them passnig by value in this CL, as that's what I did in
AutofillWebDataBackendImpl, and I'll pose the question on chromium-dev for
discussion.