Remove references to WebContentsView::SizeContents from chrome/ and app/

Remove references to WebContentsView::SizeContents from chrome/ and app/

WebContentsView::SizeContents was initially introduced as a hack to allow the
content module to resize a WebContentsView. This is odd because the embedder
should be in full control of the layout of the WebContentsView. Unfortunately,
SizeContents started being used as a platform-agnostic way for the embedder to
change the size of a WebContentsView.

The mac implementation of SizeContents has never been correct. The
implementation would work correctly when SizeContents was used by the embedder,
but it did not work when used by the content module. See
https://code.google.com/p/chromium/issues/detail?id=264207 for details. I
changed the mac implementation of SizeContents to be a no-op, which fixed the
use of SizeContents from the content module, and broke its usage from the
embedder.

This CL introduces a new platform agnostic utility method to resize a
WebContents. I've replaced all calls to SizeContents from chrome/ and app/ with
a call to the new utility method. There is no expected behavioral change on
aura, gtk, or android. This should fix the problems that have arisen from my
change to the mac implementation of SizeContents.

Ideally, the utility method would take a gfx::NativeView as its parameter.
Unfortunately, I was unable perform the resizing on a ui::AndroidView*, and I
was forced to pass in the whole WebContents to mimic the behavior of
WebContentsViewAndroid::SizeContents.

BUG=354769
TEST=Follow steps listed in bug.

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

[erikchen, 2014-03-21 23:38:50]

avi: Please take a look. If the general approach seems reasonable, I will pass
it on to an android person. (hopefully I can pass in a gfx::NativeView instead
of a WebContents).

I assume that we will also need to get some aura/gtk people to look at this?

[Avi (use Gerrit), 2014-03-22 04:48:06]

The Mac code LGTM. I'm not sure about the DEPS, and the fact that all the
questions arise because of the Android code :(

https://codereview.chromium.org/209023003/diff/50001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_tabrestore.cc (right):

https://codereview.chromium.org/209023003/diff/50001/chrome/browser/ui/browse...
chrome/browser/ui/browser_tabrestore.cc:117: // We set the size of the view
here, before WebKit does its initial
While you're in this file, Blink. Heh.

https://codereview.chromium.org/209023003/diff/50001/ui/gfx/DEPS
File ui/gfx/DEPS (right):

https://codereview.chromium.org/209023003/diff/50001/ui/gfx/DEPS#newcode3
ui/gfx/DEPS:3: "+content/public/browser",
I'm not sure if this is OK. :\ Does apps/ and chrome/ have anything else in
common?

[erikchen, 2014-03-24 16:43:39]

tedchoc: Looking for a review of the android code in layout_util.cc, which
copies the code from WebContentsViewAndroid::SizeContents, but ideally the
resizing would be performed directly on a NativeView (ui::ViewAndroid*).

[erikchen, 2014-03-26 21:33:51]

tedchoc: ping

[Ted C, 2014-03-27 00:58:53]

+sievers to keep me honest

lgtm

SetSize triggered from native does nothing in android as size is entirely driven
by the backing android widgets.  

This just sets the default size to be used for cases like prerendering.  We
could make NativeView in android actually do this plumbing itself, but then we'd
need two paths where we get a size value from Android that we must respect and a
different one from native that we are most likely going to ignore in most
circumstances.

I'm all for having more unified code paths though, so I wouldn't object if we
want to move this into ViewAndroid (just wanted you to know how wonky android
is).

[no sievers, 2014-03-27 01:29:12]

On 2014/03/27 00:58:53, Ted C wrote:
> +sievers to keep me honest
> 
> lgtm
> 
> SetSize triggered from native does nothing in android as size is entirely
driven
> by the backing android widgets.  
> 
> This just sets the default size to be used for cases like prerendering.  We
> could make NativeView in android actually do this plumbing itself, but then
we'd
> need two paths where we get a size value from Android that we must respect and
a
> different one from native that we are most likely going to ignore in most
> circumstances.
> 
> I'm all for having more unified code paths though, so I wouldn't object if we
> want to move this into ViewAndroid (just wanted you to know how wonky android
> is).

Yes, that sounds all very reasonable to me. I'm not entirely sure about
prerendering, and whether the call site in
chrome/browser/prerender/prerender_contents.cc affects Android. But it wouldn't
change any behavior since WebContentsViewAndroid::SizeContents() actually just
calls rwhv->SetSize() also.

[erikchen, 2014-03-27 17:51:27]

ben: Looking for an overall review. The logic in layout_util is likely correct,
but it's unclear where the logic belongs.

https://codereview.chromium.org/209023003/diff/50001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_tabrestore.cc (right):

https://codereview.chromium.org/209023003/diff/50001/chrome/browser/ui/browse...
chrome/browser/ui/browser_tabrestore.cc:117: // We set the size of the view
here, before WebKit does its initial
On 2014/03/22 04:48:06, Avi wrote:
> While you're in this file, Blink. Heh.

Done.

https://codereview.chromium.org/209023003/diff/50001/ui/gfx/DEPS
File ui/gfx/DEPS (right):

https://codereview.chromium.org/209023003/diff/50001/ui/gfx/DEPS#newcode3
ui/gfx/DEPS:3: "+content/public/browser",
On 2014/03/22 04:48:06, Avi wrote:
> I'm not sure if this is OK. :\ Does apps/ and chrome/ have anything else in
> common?

uhh...I'm not sure. Ben?

[erikchen, 2014-04-02 01:23:39]

-ben 
sky: Looking for an overall review. The logic in layout_util is likely correct,
but it's unclear where the logic belongs.

[sky, 2014-04-02 14:39:42]

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc
File ui/gfx/layout_util.cc (right):

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc#ne...
ui/gfx/layout_util.cc:30: content::RenderWidgetHostView* view =
web_contents->GetRenderWidgetHostView();
Is it possible to use the android implementation every where?

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.h
File ui/gfx/layout_util.h (right):

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.h#new...
ui/gfx/layout_util.h:20: GFX_EXPORT void ResizeWebContents(content::WebContents*
web_contents,
We don't want ui/gfx to depend upon content.

[erikchen, 2014-04-02 17:10:41]

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc
File ui/gfx/layout_util.cc (right):

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc#ne...
ui/gfx/layout_util.cc:30: content::RenderWidgetHostView* view =
web_contents->GetRenderWidgetHostView();
On 2014/04/02 14:39:42, sky wrote:
> Is it possible to use the android implementation every where?

The android implementation is different from the mac implementation, and cannot
be reconciled. My naive glance at the aura implementation suggests that it is
also different from the android implementation. I did not look at the gtk
implementation.

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.h
File ui/gfx/layout_util.h (right):

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.h#new...
ui/gfx/layout_util.h:20: GFX_EXPORT void ResizeWebContents(content::WebContents*
web_contents,
On 2014/04/02 14:39:42, sky wrote:
> We don't want ui/gfx to depend upon content.

Okay! Can you please tell me where you want this logic to live? The only
alternative I see is for the logic to live in content, e.g. by making a new
method on WebContentsView called ResizeView (which will only be used by the
embedder to resize the view, whereas SizeContents will remain a hack, and only
be used by the content module.

[sky, 2014-04-02 19:55:45]

Maybe some where in ui/base?

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc
File ui/gfx/layout_util.cc (right):

https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc#ne...
ui/gfx/layout_util.cc:30: content::RenderWidgetHostView* view =
web_contents->GetRenderWidgetHostView();
On 2014/04/02 17:10:41, erikchen1 wrote:
> On 2014/04/02 14:39:42, sky wrote:
> > Is it possible to use the android implementation every where?
> 
> The android implementation is different from the mac implementation, and
cannot
> be reconciled. My naive glance at the aura implementation suggests that it is
> also different from the android implementation. I did not look at the gtk
> implementation.

Actually, why is android resizing the RWHV but the others are resizing the
WebContentsView?

[sky, 2014-04-02 19:56:13]

Also, layout_util is an incredibly generic name. Maybe web_contents_sizer.

[erikchen, 2014-04-02 20:04:31]

On 2014/04/02 19:55:45, sky wrote:
> Maybe some where in ui/base?
> 
> https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc
> File ui/gfx/layout_util.cc (right):
> 
>
https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc#ne...
> ui/gfx/layout_util.cc:30: content::RenderWidgetHostView* view =
> web_contents->GetRenderWidgetHostView();
> On 2014/04/02 17:10:41, erikchen1 wrote:
> > On 2014/04/02 14:39:42, sky wrote:
> > > Is it possible to use the android implementation every where?
> > 
> > The android implementation is different from the mac implementation, and
> cannot
> > be reconciled. My naive glance at the aura implementation suggests that it
is
> > also different from the android implementation. I did not look at the gtk
> > implementation.
> 
> Actually, why is android resizing the RWHV but the others are resizing the
> WebContentsView?

This is because the WebContentsViewAndroid cannot be resized directly. See
earlier comments from tedchoc and sievers. Ideally, the interface would use a
gfx::NativeView, but I had to pass in the whole WebContents just to get the
android case to work. I'm open to suggestions if you have ideas.

[sky, 2014-04-02 22:22:59]

Ok, I get it. Everyone just wants to be different:)

  -Scott

On Wed, Apr 2, 2014 at 1:04 PM,  <erikchen@chromium.org> wrote:
> On 2014/04/02 19:55:45, sky wrote:
>>
>> Maybe some where in ui/base?
>
>
>> https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc
>> File ui/gfx/layout_util.cc (right):
>
>
>
>
https://codereview.chromium.org/209023003/diff/80001/ui/gfx/layout_util.cc#ne...
>>
>> ui/gfx/layout_util.cc:30: content::RenderWidgetHostView* view =
>> web_contents->GetRenderWidgetHostView();
>> On 2014/04/02 17:10:41, erikchen1 wrote:
>> > On 2014/04/02 14:39:42, sky wrote:
>> > > Is it possible to use the android implementation every where?
>> >
>> > The android implementation is different from the mac implementation, and
>> cannot
>> > be reconciled. My naive glance at the aura implementation suggests that
>> > it
>
> is
>>
>> > also different from the android implementation. I did not look at the
>> > gtk
>> > implementation.
>
>
>> Actually, why is android resizing the RWHV but the others are resizing the
>> WebContentsView?
>
>
> This is because the WebContentsViewAndroid cannot be resized directly. See
> earlier comments from tedchoc and sievers. Ideally, the interface would use
> a
> gfx::NativeView, but I had to pass in the whole WebContents just to get the
> android case to work. I'm open to suggestions if you have ideas.
>
> https://codereview.chromium.org/209023003/

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

[erikchen, 2014-04-03 01:29:17]

sky: PTAL

I've done as you've suggested and moved the class to ui/base, and renamed it to
web_contents_sizer. win8_aura try-bot passes, but win fails. This is expected,
since I didn't write a windows, non-aura implementation. Do I need to?

[sky, 2014-04-03 15:05:50]

Unfortunately I was wrong, ui/base can't depend upon aura. Which is why you're
seeing the compile error. The only thing I can suggest is moving the code to
apps since chrome can depend upon that. Not particularly satisfying though:(

https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
File ui/base/web_contents_sizer.cc (right):

https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
ui/base/web_contents_sizer.cc:14: #elif defined(OS_ANDROID)
Why the empty elif?

https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
ui/base/web_contents_sizer.cc:22: gfx::NativeView view =
web_contents->GetView()->GetNativeView();
gfx::NativeView->aura::Window

https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
ui/base/web_contents_sizer.cc:25: view->SetBounds(rect);
window->SetBounds(gfx::Rect(window->bounds().size()));

[sky, 2014-04-03 15:06:12]

Also, there is no longer such a build as non-aura windows.

On Thu, Apr 3, 2014 at 8:05 AM,  <sky@chromium.org> wrote:
> Unfortunately I was wrong, ui/base can't depend upon aura. Which is why
> you're
> seeing the compile error. The only thing I can suggest is moving the code to
> apps since chrome can depend upon that. Not particularly satisfying though:(
>
>
>
https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
> File ui/base/web_contents_sizer.cc (right):
>
>
https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
> ui/base/web_contents_sizer.cc:14: #elif defined(OS_ANDROID)
> Why the empty elif?
>
>
https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
> ui/base/web_contents_sizer.cc:22: gfx::NativeView view =
> web_contents->GetView()->GetNativeView();
> gfx::NativeView->aura::Window
>
>
https://codereview.chromium.org/209023003/diff/120001/ui/base/web_contents_si...
> ui/base/web_contents_sizer.cc:25: view->SetBounds(rect);
> window->SetBounds(gfx::Rect(window->bounds().size()));
>
> https://codereview.chromium.org/209023003/

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

[erikchen, 2014-04-03 17:49:07]

sky: PTAL

[sky, 2014-04-03 19:46:15]

You're going to need a new owner for apps.

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
File apps/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:15: #endif
no reason for empty if here.

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:23: gfx::Rect rect = view->bounds();
Simplify this to:
window->SetBounds(gfx::Rect(window->bounds().size()));

[erikchen, 2014-04-04 01:06:23]

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
File apps/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:15: #endif
On 2014/04/03 19:46:16, sky wrote:
> no reason for empty if here.

gladly, I was just trying to maintain consistency with the later #if blocks.

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:23: gfx::Rect rect = view->bounds();
On 2014/04/03 19:46:16, sky wrote:
> Simplify this to:
> window->SetBounds(gfx::Rect(window->bounds().size()));

I'm afraid I don't understand your comment. To the best of my knowledge, the
origin of the gfx::Rect passed into window->SetBounds is not discarded. In my
code, I am maintaining the origin, and changing the size of the gfx::rect. In
your suggestion, the gfx::rect is given a new size, but its origin defaults to
(0,0). Is there an implicit guarantee in aura::Window that the origin of the
bounds is always (0,0)?

[erikchen, 2014-04-04 01:07:10]

yoz: Looking for an OWNER review of apps/

[sky, 2014-04-04 14:18:02]

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
File apps/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:23: gfx::Rect rect = view->bounds();
On 2014/04/04 01:06:23, erikchen1 wrote:
> On 2014/04/03 19:46:16, sky wrote:
> > Simplify this to:
> > window->SetBounds(gfx::Rect(window->bounds().size()));
> 
> I'm afraid I don't understand your comment. To the best of my knowledge, the
> origin of the gfx::Rect passed into window->SetBounds is not discarded. In my
> code, I am maintaining the origin, and changing the size of the gfx::rect. In
> your suggestion, the gfx::rect is given a new size, but its origin defaults to
> (0,0). Is there an implicit guarantee in aura::Window that the origin of the
> bounds is always (0,0)?

My mistake. I was suggesting two things here:
1. Not point in NativeView in platform specific code, use aura::Window*
2. change to:
window->SetBounds(gfx::Rect(window->bounds().origin(), new_size));

[erikchen, 2014-04-04 20:47:52]

sky: PTAL

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
File apps/ui/web_contents_sizer.cc (right):

https://codereview.chromium.org/209023003/diff/160001/apps/ui/web_contents_si...
apps/ui/web_contents_sizer.cc:23: gfx::Rect rect = view->bounds();
On 2014/04/04 14:18:03, sky wrote:
> On 2014/04/04 01:06:23, erikchen1 wrote:
> > On 2014/04/03 19:46:16, sky wrote:
> > > Simplify this to:
> > > window->SetBounds(gfx::Rect(window->bounds().size()));
> > 
> > I'm afraid I don't understand your comment. To the best of my knowledge, the
> > origin of the gfx::Rect passed into window->SetBounds is not discarded. In
my
> > code, I am maintaining the origin, and changing the size of the gfx::rect.
In
> > your suggestion, the gfx::rect is given a new size, but its origin defaults
to
> > (0,0). Is there an implicit guarantee in aura::Window that the origin of the
> > bounds is always (0,0)?
> 
> My mistake. I was suggesting two things here:
> 1. Not point in NativeView in platform specific code, use aura::Window*
> 2. change to:
> window->SetBounds(gfx::Rect(window->bounds().origin(), new_size));

Done.

[sky, 2014-04-04 20:51:44]

LGTM

[Yoyo Zhou, 2014-04-04 21:02:02]

apps LGTM

[erikchen, 2014-04-04 21:03:46]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-04-04 21:04:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/209023003/200001

[commit-bot: I haz the power, 2014-04-04 21:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/209023003/200001

[commit-bot: I haz the power, 2014-04-04 22:32:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:32:45]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[erikchen, 2014-04-04 22:46:44]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:48:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/209023003/200001

[commit-bot: I haz the power, 2014-04-04 23:44:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 23:44:22]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[erikchen, 2014-04-05 01:08:12]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-04-05 01:08:36]

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

[commit-bot: I haz the power, 2014-04-05 08:26:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-05 08:26:06]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[erikchen, 2014-04-07 17:58:35]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-04-07 17:58:48]

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

[erikchen, 2014-04-07 22:25:10]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-04-07 22:25:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/209023003/260001

[commit-bot: I haz the power, 2014-04-08 01:09:34]

Change committed as 262276