Split TabContentsViewMac.

Split TabContentsViewMac.

BUG=93804, 95573
TEST=no change

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

[Avi (use Gerrit), 2012-02-02 23:31:18]

Jochen: for review
John, Joi, Elliot: FYI

This is a split just like Elliot did for GTK (see
http://codereview.chromium.org/8344061/). GTK required a subclassing pattern
while here I'm doing straight delegation.

Pre-renamed stuff to be WebContents* and moved/added to the public directory in
content where appropriate.

[jochen (gone - plz use gerrit), 2012-02-03 00:13:32]

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h:47: };
you have scoped ptrs in here. shouldn't this be DISALLOW_COPY_AND_ASSIGN then?

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h:58:
content::WebContentsViewMacDelegate* CreateWebContentsViewMacDelegate(
given the fancy namespace, why not just call it Create?

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm:19: }
}  // namespace $fancy

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm:36: 
only one empty line

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/moving_to_content/tab_contents_view_mac.mm
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/moving_to_content/tab_contents_view_mac.mm:357:
DLOG(ERROR) << "Implement context menus without chrome/ code";
Aren't error messages with chrome/ in it also some kind of chrome dependency? :)

What about just NOTREACHED() or "Cannot show context menus without a delegate"
or similar?

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/web_contents_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:23: 
nit. no empty line?

[jam, 2012-02-03 00:29:14]

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/render_widget_host_view_mac_delegate.h:19: 
nit: everything in content/public is in the content namespace. is this just
overlooked or can the objective c code not be in a namespace?

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/web_contents_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:38: virtual void
ShowContextMenu(const ContextMenuParams& params) = 0;
i think this isn't needed because WebContentsDelegate::HandleContextMenu is also
called and that's how the embedder hooks in (I would actually prefer if ended up
removing it from WebContentsView altogether, if chrome is only using it from the
delegate interface)

[Avi (use Gerrit), 2012-02-03 01:18:53]

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h:47: };
On 2012/02/03 00:13:33, jochen wrote:
> you have scoped ptrs in here. shouldn't this be DISALLOW_COPY_AND_ASSIGN then?

Done.

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.h:58:
content::WebContentsViewMacDelegate* CreateWebContentsViewMacDelegate(
On 2012/02/03 00:13:33, jochen wrote:
> given the fancy namespace, why not just call it Create?

This is the pattern we've used with tab_contents_view_mac.h,
sad_tab_controller.h, etc.

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm:19: }
On 2012/02/03 00:13:33, jochen wrote:
> }  // namespace $fancy

For a namespace that tightly wrapped around a shim? (NB tab_contents_view_mac)

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/chrome_web_contents_view_mac_delegate.mm:36: 
On 2012/02/03 00:13:33, jochen wrote:
> only one empty line

Done.

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
File chrome/browser/tab_contents/moving_to_content/tab_contents_view_mac.mm
(right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/chrome/browser/tab_c...
chrome/browser/tab_contents/moving_to_content/tab_contents_view_mac.mm:357:
DLOG(ERROR) << "Implement context menus without chrome/ code";
On 2012/02/03 00:13:33, jochen wrote:
> Aren't error messages with chrome/ in it also some kind of chrome dependency?
:)

Stolen from the GTK version of this code :) Will change.

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/render_widget_host_view_mac_delegate.h:19: 
Namespaces are a C++ feature. :( That's one area where C++ and ObjC don't mix
well.

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/web_contents_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:23: 
On 2012/02/03 00:13:33, jochen wrote:
> nit. no empty line?

Done.

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:38: virtual void
ShowContextMenu(const ContextMenuParams& params) = 0;
Look at the implementation of this. It's the TCVs who call
WebContentsDelegate::HandleContextMenu.

To be more precise, the flow is:

RenderViewHost::OnMsgContextMenu handles the IPC from the renderer. It says:

  RenderViewHostDelegate::View* view = delegate_->GetViewDelegate();
  [...]
  view->ShowContextMenu(validated_params);

The TabContentsViews _are_ the RVHD::Views.

If all the TCV::ShowContextMenu implementations are going to do is immediately
call into WebContentsDelegate::HandleContextMenu, sure. But we can't put that
into the RenderViewHost code, as that's a layering violation. And we have no
common base class for the TCVs.

I don't see how to do it otherwise.

[jam, 2012-02-03 07:59:17]

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
File content/public/browser/web_contents_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/3001/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:38: virtual void
ShowContextMenu(const ContextMenuParams& params) = 0;
On 2012/02/03 01:18:53, Avi wrote:
> Look at the implementation of this. It's the TCVs who call
> WebContentsDelegate::HandleContextMenu.
> 
> To be more precise, the flow is:
> 
> RenderViewHost::OnMsgContextMenu handles the IPC from the renderer. It says:
> 
>   RenderViewHostDelegate::View* view = delegate_->GetViewDelegate();
>   [...]
>   view->ShowContextMenu(validated_params);
> 
> The TabContentsViews _are_ the RVHD::Views.
> 
> If all the TCV::ShowContextMenu implementations are going to do is immediately
> call into WebContentsDelegate::HandleContextMenu, sure. But we can't put that
> into the RenderViewHost code, as that's a layering violation. And we have no
> common base class for the TCVs.
> 
> I don't see how to do it otherwise.

RenderViewHost will call TabContentsViewMac::ShowContextMenu. That
implementation calls TabContentsDelegate::ShowContextMenu. Am I missing
something/

[Avi (use Gerrit), 2012-02-03 15:09:14]

I think we have a bit of a misunderstanding.

As it exists _today_, what you describe is not what happens.
WebContentsDelegate::HandleContextMenu is called first, and if it returns false,
then "standard" contextual menu handling is done.

It certainly is possible to move the "standard" contextual menu handling to
Browser (the usual WCD) as you describe, and significantly simplify things. We
probably want to do this. But this is a different issue, as this is a refactor
that needs to happen across platforms.

Would you like me to do the refactor you describe? I can either get this CL
committed first with dual callbacks (please see the Gtk version as I'm doing
here exactly what Elliot did) and then do that refactor, or I can do that
refactor first and resume this change later.

Actually, now that I look at it, there's a difficulty to doing the refactor like
that. Let me email the list.

[jam, 2012-02-03 17:04:28]

On 2012/02/03 15:09:14, Avi wrote:
> I think we have a bit of a misunderstanding.
> 
> As it exists _today_, what you describe is not what happens.
> WebContentsDelegate::HandleContextMenu is called first, and if it returns
false,
> then "standard" contextual menu handling is done.
> 
> It certainly is possible to move the "standard" contextual menu handling to
> Browser (the usual WCD) as you describe, and significantly simplify things. We
> probably want to do this. But this is a different issue, as this is a refactor
> that needs to happen across platforms.
> 
> Would you like me to do the refactor you describe? I can either get this CL
> committed first with dual callbacks (please see the Gtk version as I'm doing
> here exactly what Elliot did) and then do that refactor, or I can do that
> refactor first and resume this change later.
> 
> Actually, now that I look at it, there's a difficulty to doing the refactor
like
> that. Let me email the list.

(for others reading)
ignore my comment for now, you've convinced me that it's better to do this after

[Avi (use Gerrit), 2012-02-03 17:30:15]

No problem. I like the refactor, as it'll be a lot cleaner. I'll do it right
after.

What are your thoughts about this CL, then?

[jam, 2012-02-03 18:15:41]

lgtm

https://chromiumcodereview.appspot.com/9307063/diff/1014/content/public/brows...
File content/public/browser/web_contents_view_mac_delegate.h (right):

https://chromiumcodereview.appspot.com/9307063/diff/1014/content/public/brows...
content/public/browser/web_contents_view_mac_delegate.h:23: class CONTENT_EXPORT
WebContentsViewMacDelegate {
nit: I don't think you need CONTENT_EXPORT since it's just an interface

[jochen (gone - plz use gerrit), 2012-02-03 19:36:43]

lgtm