Find working in app_shell.

Find working in app_shell.

This CL moves the functionality of WebViewGuest::Find() out of the chrome-specific delegate, and into extensions, so that app_shell can also use it.

BUG=352293
Committed: https://crrev.com/9ae8cb708f79233346cc52f865293be3f0cd1b40
Cr-Commit-Position: refs/heads/master@{#295536}

[lfg, 2014-09-16 22:14:56]

lfg@chromium.org changed reviewers:
+ hanxi@chromium.org

[lfg, 2014-09-16 22:14:57]

hanxi@chromium.org: Please review changes in 

Everything.

[Xi Han, 2014-09-17 22:09:40]

lgtm

[lfg, 2014-09-17 22:11:10]

lfg@chromium.org changed reviewers:
+ fsamuel@chromium.org, rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-17 22:17:31]

extensions lgtm

https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
(right):

https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:24: explicit
ChromeWebViewGuestDelegate(
Tangential comment: Might be worth adding a note here that web_view_guest must
outlive - and is not owned by - the delegate instance.

[lfg, 2014-09-17 22:34:42]

https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
(right):

https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:24: explicit
ChromeWebViewGuestDelegate(
On 2014/09/17 22:17:31, Ken Rockot wrote:
> Tangential comment: Might be worth adding a note here that web_view_guest must
> outlive - and is not owned by - the delegate instance.

No sure I followed this. The stronger guarantee here, is that the delegate is
actually owned by the web_view_guest, so that guarantees that the web_view_guest
will outlive it.

[Ken Rockot(use gerrit already), 2014-09-17 22:40:20]

On 2014/09/17 22:34:42, lfg wrote:
>
https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
> File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
> (right):
> 
>
https://codereview.chromium.org/579563002/diff/20001/chrome/browser/guest_vie...
> chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:24:
explicit
> ChromeWebViewGuestDelegate(
> On 2014/09/17 22:17:31, Ken Rockot wrote:
> > Tangential comment: Might be worth adding a note here that web_view_guest
must
> > outlive - and is not owned by - the delegate instance.
> 
> No sure I followed this. The stronger guarantee here, is that the delegate is
> actually owned by the web_view_guest, so that guarantees that the
web_view_guest
> will outlive it.

Ah, that's probably clearer.

Also forgot to note before, please edit the CL description to
provide a little more context.

[Fady Samuel, 2014-09-18 16:49:48]

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_find_helper.cc:90:
WebViewInternalFindFunction* find_function) {
Why are you making this change?

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_find_helper.h (right):

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_find_helper.h:106:
WebViewInternalFindFunction* find_function);
scoped_refptr?

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.h:168:
WebViewInternalFindFunction* find_function);
Can we restore this back to being a scoped_refptr?

[lfg, 2014-09-18 17:09:24]

On 2014/09/18 16:49:48, Fady Samuel wrote:
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):
> 
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> extensions/browser/guest_view/web_view/web_view_find_helper.cc:90:
> WebViewInternalFindFunction* find_function) {
> Why are you making this change?
> 
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> File extensions/browser/guest_view/web_view/web_view_find_helper.h (right):
> 
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> extensions/browser/guest_view/web_view/web_view_find_helper.h:106:
> WebViewInternalFindFunction* find_function);
> scoped_refptr?
> 
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> File extensions/browser/guest_view/web_view/web_view_guest.h (right):
> 
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> extensions/browser/guest_view/web_view/web_view_guest.h:168:
> WebViewInternalFindFunction* find_function);
> Can we restore this back to being a scoped_refptr?

We could, but let me explain the reason why I changed this.

Since those objects live on the stack, all that the scoped_refptr does is
increment the refcount when the function enters, and decrement when the function
exits, so it increases code size and decreases performance for no real benefit.
Note that in the FindInfo member, I kept it as a scoped_refptr, because it makes
sense (that object lives on the heap).

A second benefit of passing as a raw ptr is that we don't need to include the
header for that type, a simple forward declaration is enough.

Functionality-wise, there's no difference, so if you still feel strong about
having the argument passed as a scoped_refptr I'll change it.

[lfg, 2014-09-18 17:41:22]

On 2014/09/18 17:09:24, lfg wrote:
> On 2014/09/18 16:49:48, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > File extensions/browser/guest_view/web_view/web_view_find_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > extensions/browser/guest_view/web_view/web_view_find_helper.cc:90:
> > WebViewInternalFindFunction* find_function) {
> > Why are you making this change?
> > 
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > File extensions/browser/guest_view/web_view/web_view_find_helper.h (right):
> > 
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > extensions/browser/guest_view/web_view/web_view_find_helper.h:106:
> > WebViewInternalFindFunction* find_function);
> > scoped_refptr?
> > 
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > File extensions/browser/guest_view/web_view/web_view_guest.h (right):
> > 
> >
>
https://codereview.chromium.org/579563002/diff/20001/extensions/browser/guest...
> > extensions/browser/guest_view/web_view/web_view_guest.h:168:
> > WebViewInternalFindFunction* find_function);
> > Can we restore this back to being a scoped_refptr?
> 
> We could, but let me explain the reason why I changed this.
> 
> Since those objects live on the stack, all that the scoped_refptr does is
> increment the refcount when the function enters, and decrement when the
function
> exits, so it increases code size and decreases performance for no real
benefit.
> Note that in the FindInfo member, I kept it as a scoped_refptr, because it
makes
> sense (that object lives on the heap).
> 
> A second benefit of passing as a raw ptr is that we don't need to include the
> header for that type, a simple forward declaration is enough.
> 
> Functionality-wise, there's no difference, so if you still feel strong about
> having the argument passed as a scoped_refptr I'll change it.

PTAL, I switched the find_function to be passed as a scoped_refptr.

[Fady Samuel, 2014-09-18 18:18:40]

lgtm with nit.

https://codereview.chromium.org/579563002/diff/40001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_find_helper.h (right):

https://codereview.chromium.org/579563002/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_find_helper.h:159: // Weak
pointer used to access the find info of fin.
nit: This comment doesn't make sense and isn't very useful. I'd just remove it.

[lfg, 2014-09-18 19:21:45]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-09-18 19:23:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/579563002/60001

[commit-bot: I haz the power, 2014-09-18 19:58:22]

Committed patchset #4 (id:60001) as 5c63ada9d7b93e29742f9130f875567179db82c5

[commit-bot: I haz the power, 2014-09-18 19:58:50]

Patchset 4 (id:??) landed as
https://crrev.com/9ae8cb708f79233346cc52f865293be3f0cd1b40
Cr-Commit-Position: refs/heads/master@{#295536}