Reduce the public method footprint of GuestViewBase and derived types.

This patch reduces the public method footprint of GuestViewBase and derived types.

BUG=534320
Committed: https://crrev.com/d7523c2b968283cfdcfe46cf2569b871ed2216f8
Cr-Commit-Position: refs/heads/master@{#355420}

[paulmeyer, 2015-10-09 20:54:18]

paulmeyer@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-10-09 22:35:20]

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.h (right):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:86: void DidAttach(int
guest_proxy_routing_id) final;
Do DidAttach and WillAttach need to be public? Can they be private?

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:65:
void CreateWebContents(const base::DictionaryValue& create_params,
Why are these protected and not private?

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:118: void
StartFindInternal(
Internal things should not be public? Why are these public? Maybe they're not
actually internal? What happens if a client decides to call these directly?

[Fady Samuel, 2015-10-10 01:02:12]

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:151: ScriptExecutor*
script_executor() { return script_executor_.get(); }
Does this need to be public?

[Fady Samuel, 2015-10-10 10:58:01]

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.cc (left):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.cc:729:
EmbedderZoomChanged(data.old_zoom_level, data.new_zoom_level);
Why was this deleted?

[paulmeyer, 2015-10-16 21:13:03]

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.cc (left):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.cc:729:
EmbedderZoomChanged(data.old_zoom_level, data.new_zoom_level);
On 2015/10/10 10:58:01, Fady Samuel wrote:
> Why was this deleted?

Because it does nothing and is never overridden.

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.h (right):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:86: void DidAttach(int
guest_proxy_routing_id) final;
On 2015/10/09 22:35:19, Fady Samuel wrote:
> Do DidAttach and WillAttach need to be public? Can they be private?

They need to be public because they are both called from
guest_view_message_filter.cc.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:65:
void CreateWebContents(const base::DictionaryValue& create_params,
On 2015/10/09 22:35:19, Fady Samuel wrote:
> Why are these protected and not private?

Because they are overridden by TestMimeHandlerViewGuest.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:118: void
StartFindInternal(
On 2015/10/09 22:35:19, Fady Samuel wrote:
> Internal things should not be public? Why are these public? Maybe they're not
> actually internal? What happens if a client decides to call these directly?

I originally named these without the "Internal" suffix, but Raymes later added
it. Perhaps this was done because they are called from web_view_internal_api.cc
(though other APIs called from there are not labelled that way)? They are
definitely not internal to WebViewGuest though.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:151: ScriptExecutor*
script_executor() { return script_executor_.get(); }
On 2015/10/10 01:02:12, Fady Samuel wrote:
> Does this need to be public?

Yes, since it is also called in web_view_internal_api.cc.

[Fady Samuel, 2015-10-16 21:17:29]

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.cc (left):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.cc:729:
EmbedderZoomChanged(data.old_zoom_level, data.new_zoom_level);
On 2015/10/16 21:13:03, Paul Meyer wrote:
> On 2015/10/10 10:58:01, Fady Samuel wrote:
> > Why was this deleted?
> 
> Because it does nothing and is never overridden.

Acknowledged.

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.h (right):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:86: void DidAttach(int
guest_proxy_routing_id) final;
On 2015/10/16 21:13:03, Paul Meyer wrote:
> On 2015/10/09 22:35:19, Fady Samuel wrote:
> > Do DidAttach and WillAttach need to be public? Can they be private?
> 
> They need to be public because they are both called from
> guest_view_message_filter.cc.

I'd make GuestViewMessageFilter a friend instead.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:65:
void CreateWebContents(const base::DictionaryValue& create_params,
On 2015/10/16 21:13:03, Paul Meyer wrote:
> On 2015/10/09 22:35:19, Fady Samuel wrote:
> > Why are these protected and not private?
> 
> Because they are overridden by TestMimeHandlerViewGuest.

Make TestMimeHandlerViewGuest a friend class instead and move to private.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:118: void
StartFindInternal(
On 2015/10/16 21:13:03, Paul Meyer wrote:
> On 2015/10/09 22:35:19, Fady Samuel wrote:
> > Internal things should not be public? Why are these public? Maybe they're
not
> > actually internal? What happens if a client decides to call these directly?
> 
> I originally named these without the "Internal" suffix, but Raymes later added
> it. Perhaps this was done because they are called from
web_view_internal_api.cc
> (though other APIs called from there are not labelled that way)? They are
> definitely not internal to WebViewGuest though.

Yea, I'd name them back if possible.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:151: ScriptExecutor*
script_executor() { return script_executor_.get(); }
On 2015/10/16 21:13:03, Paul Meyer wrote:
> On 2015/10/10 01:02:12, Fady Samuel wrote:
> > Does this need to be public?
> 
> Yes, since it is also called in web_view_internal_api.cc.

Acknowledged.

[paulmeyer, 2015-10-21 19:41:00]

Description was changed from

==========
This patch reduces the public method footprint of GuestViewBase and derived
types.

Also, methods in the .cc files are rearranged to match their orders in the
respective .h files.

BUG=534320
==========

to

==========
This patch reduces the public method footprint of GuestViewBase and derived
types.

BUG=534320
==========

[paulmeyer, 2015-10-21 19:45:13]

PTAL

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
File components/guest_view/browser/guest_view_base.h (right):

https://codereview.chromium.org/1392343002/diff/1/components/guest_view/brows...
components/guest_view/browser/guest_view_base.h:86: void DidAttach(int
guest_proxy_routing_id) final;
On 2015/10/16 21:17:28, Fady Samuel wrote:
> On 2015/10/16 21:13:03, Paul Meyer wrote:
> > On 2015/10/09 22:35:19, Fady Samuel wrote:
> > > Do DidAttach and WillAttach need to be public? Can they be private?
> > 
> > They need to be public because they are both called from
> > guest_view_message_filter.cc.
> 
> I'd make GuestViewMessageFilter a friend instead.

Done.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:65:
void CreateWebContents(const base::DictionaryValue& create_params,
On 2015/10/16 21:17:28, Fady Samuel wrote:
> On 2015/10/16 21:13:03, Paul Meyer wrote:
> > On 2015/10/09 22:35:19, Fady Samuel wrote:
> > > Why are these protected and not private?
> > 
> > Because they are overridden by TestMimeHandlerViewGuest.
> 
> Make TestMimeHandlerViewGuest a friend class instead and move to private.

Done.

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1392343002/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.h:118: void
StartFindInternal(
On 2015/10/16 21:17:29, Fady Samuel wrote:
> On 2015/10/16 21:13:03, Paul Meyer wrote:
> > On 2015/10/09 22:35:19, Fady Samuel wrote:
> > > Internal things should not be public? Why are these public? Maybe they're
> not
> > > actually internal? What happens if a client decides to call these
directly?
> > 
> > I originally named these without the "Internal" suffix, but Raymes later
added
> > it. Perhaps this was done because they are called from
> web_view_internal_api.cc
> > (though other APIs called from there are not labelled that way)? They are
> > definitely not internal to WebViewGuest though.
> 
> Yea, I'd name them back if possible.

I discovered while renaming that StopFinding() collides with the inherited
method from BrowserPluginGuestDelegate, though THAT method really shouldn't have
just been called StopFinding, since what it really is is the method that
attempts to allow the BPG to handle the StopFinding request for the embedder.
So, I renamed that one too to reflect this better.

[Fady Samuel, 2015-10-21 20:06:04]

LGTM

[paulmeyer, 2015-10-21 20:16:14]

paulmeyer@chromium.org changed reviewers:
+ creis@chromium.org

[paulmeyer, 2015-10-21 20:16:14]

+creis@ for OWNER review of browser_plugin_guest_delegate.h/cc

[Charlie Reis, 2015-10-21 20:53:58]

content/public LGTM.

[paulmeyer, 2015-10-21 20:54:48]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-10-21 20:55:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1392343002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1392343002/20001

[commit-bot: I haz the power, 2015-10-21 22:58:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-21 22:59:13]

Patchset 2 (id:??) landed as
https://crrev.com/d7523c2b968283cfdcfe46cf2569b871ed2216f8
Cr-Commit-Position: refs/heads/master@{#355420}