Introduce WebViewGuestDelegate.

Introduce WebViewGuestDelegate.


BUG=352290
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291002

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291133

[Fady Samuel, 2014-08-18 20:11:23]

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

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:50:
content::WebContents* contents) {
Add a TODO to investigate which of these observers should move to the extension
module in the future.

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

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:24: const
extensions::WebViewGuest& web_view_guest);
I feel like this should be a pointer to allow for modifying the WebViewGuest in
the future from the delegate?

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:28: virtual
void OnDidInitialize() OVERRIDE;
Huge nitpick: Could we please put this in alphabetical order? It makes it easier
to find a particular method in the future.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (left):

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:348: #if
defined(OS_CHROMEOS)
Remove this?

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:994: #if
defined(OS_CHROMEOS)
Remove this. This is already decided on the other end.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:662: current_zoom_factor_ =
web_view_guest_delegate_->GetZoomLevel();
Let's move zoom entirely out until it is supported in extensions.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:979:
web_view_guest_delegate_->OnSetZoom(zoom_factor);
This conceptually to me is weird. Either zoom is available or it is not. I think
you should move all the code below into the delegate implementation.

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

https://codereview.chromium.org/481003002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:23: explicit
WebViewGuestDelegate();
Doesn't need to be explicit.

https://codereview.chromium.org/481003002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:39: virtual
double GetZoomLevel();
Can we get rid of this?

[Xi Han, 2014-08-18 22:23:57]

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

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:50:
content::WebContents* contents) {
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Add a TODO to investigate which of these observers should move to the
extension
> module in the future.

Done.

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

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:24: const
extensions::WebViewGuest& web_view_guest);
On 2014/08/18 20:11:22, Fady Samuel wrote:
> I feel like this should be a pointer to allow for modifying the WebViewGuest
in
> the future from the delegate?

Done.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:28: virtual
void OnDidInitialize() OVERRIDE;
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Huge nitpick: Could we please put this in alphabetical order? It makes it
easier
> to find a particular method in the future.

Done.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (left):

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:348: #if
defined(OS_CHROMEOS)
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Remove this?

Done.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:994: #if
defined(OS_CHROMEOS)
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Remove this. This is already decided on the other end.

Done.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:662: current_zoom_factor_ =
web_view_guest_delegate_->GetZoomLevel();
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Let's move zoom entirely out until it is supported in extensions.

Done.

https://codereview.chromium.org/481003002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:979:
web_view_guest_delegate_->OnSetZoom(zoom_factor);
On 2014/08/18 20:11:22, Fady Samuel wrote:
> This conceptually to me is weird. Either zoom is available or it is not. I
think
> you should move all the code below into the delegate implementation.

Done.

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

https://codereview.chromium.org/481003002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:23: explicit
WebViewGuestDelegate();
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Doesn't need to be explicit.

Done.

https://codereview.chromium.org/481003002/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:39: virtual
double GetZoomLevel();
On 2014/08/18 20:11:22, Fady Samuel wrote:
> Can we get rid of this?

Done.

[Xi Han, 2014-08-20 13:07:56]

PTAL, thanks.

[Fady Samuel, 2014-08-20 14:45:09]

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:131:
OnHandleContextMenu(const content::ContextMenuParams& params) {
HandleContextMenu

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:153: const
std::string& event_zoom_change) {
This seems weird to me. Extensions shouldn't know about the event name if they
don't know how to zoom. I would move this to chrome.

https://codereview.chromium.org/481003002/diff/80001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/481003002/diff/80001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:21: class
WebViewGuestDelegate {
Methods here should be pure virtual. WebViewGuest should be able to handle the
case with a NULL delegate.

[Xi Han, 2014-08-20 15:46:01]

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:131:
OnHandleContextMenu(const content::ContextMenuParams& params) {
On 2014/08/20 14:45:09, Fady Samuel wrote:
> HandleContextMenu

Done.

https://codereview.chromium.org/481003002/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:153: const
std::string& event_zoom_change) {
On 2014/08/20 14:45:09, Fady Samuel wrote:
> This seems weird to me. Extensions shouldn't know about the event name if they
> don't know how to zoom. I would move this to chrome.

Done.

https://codereview.chromium.org/481003002/diff/80001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/481003002/diff/80001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:21: class
WebViewGuestDelegate {
On 2014/08/20 14:45:09, Fady Samuel wrote:
> Methods here should be pure virtual. WebViewGuest should be able to handle the
> case with a NULL delegate.

Done.

[Fady Samuel, 2014-08-20 16:18:20]

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:418: if
(!web_view_guest_delegate_.get())
nit: I think you can drop the .get()

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:559: if
(!web_view_guest_delegate_.get())
nit: I think you can drop the .get()

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:895: void
WebViewGuest::InjectChromeVoxIfNeeded(
This doesn't seem to be used in WebViewGuest. Please remove. You probably also
don't need the delegate call.

[Xi Han, 2014-08-20 18:15:24]

PTAL, thanks.

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:418: if
(!web_view_guest_delegate_.get())
On 2014/08/20 16:18:20, Fady Samuel wrote:
> nit: I think you can drop the .get()

Done.

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:559: if
(!web_view_guest_delegate_.get())
On 2014/08/20 16:18:19, Fady Samuel wrote:
> nit: I think you can drop the .get()

Done.

https://codereview.chromium.org/481003002/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:895: void
WebViewGuest::InjectChromeVoxIfNeeded(
On 2014/08/20 16:18:19, Fady Samuel wrote:
> This doesn't seem to be used in WebViewGuest. Please remove. You probably also
> don't need the delegate call. 

Remove this function in WebViewGuest, and rename the one in delegate as
OnDocumentLoadedInFrame.

[Fady Samuel, 2014-08-20 20:15:07]

https://codereview.chromium.org/481003002/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:691: if
(!render_frame_host->GetParent() && web_view_guest_delegate_)
This is confusing. I would move this check to the delegate. Otherwise, it's not
clear why you're doing it.

[Xi Han, 2014-08-20 20:40:22]

https://codereview.chromium.org/481003002/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:691: if
(!render_frame_host->GetParent() && web_view_guest_delegate_)
On 2014/08/20 20:15:07, Fady Samuel wrote:
> This is confusing. I would move this check to the delegate. Otherwise, it's
not
> clear why you're doing it.

Done.

[Fady Samuel, 2014-08-20 22:06:43]

https://codereview.chromium.org/481003002/diff/220001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/220001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:692:
web_view_guest_delegate_->OnDocumentLoadedInFrame(render_frame_host);
Pass in the parameter verbatim.

[Fady Samuel, 2014-08-20 22:09:04]

lgtm

https://codereview.chromium.org/481003002/diff/220001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/481003002/diff/220001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:692:
web_view_guest_delegate_->OnDocumentLoadedInFrame(render_frame_host);
On 2014/08/20 22:06:43, Fady Samuel wrote:
> Pass in the parameter verbatim. 

Nm misread

[Xi Han, 2014-08-20 22:09:53]

kalman@chromium.org: Please review changes in:

-extensions/extensions.gyp

Thanks!

[not at google - send to devlin, 2014-08-21 00:40:57]

lgtm

[Xi Han, 2014-08-21 01:29:20]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-21 01:29:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/481003002/220001

[commit-bot: I haz the power, 2014-08-21 04:45:50]

Committed patchset #7 (220001) as 291002

[Xi Han, 2014-08-21 14:39:57]

PTAL

[Fady Samuel, 2014-08-21 15:35:53]

lgtm

[Xi Han, 2014-08-21 15:36:56]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:39:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/481003002/280001

[commit-bot: I haz the power, 2014-08-21 17:40:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-21 18:10:55]

Committed patchset #9 (280001) as 291133