Use bounds instead of size for prerender requests

Use bounds instead of size for prerender requests

This changes the pipeline that initializes prerender contents
with a size to use bounds instead. Note that not all addPrerenderX
calls are switched to use Rect since for everywhere except Android
this won't be needed. Android needs bounds instead of size to get
info about the Physical Backing Size, also the top container height
(to be used for toolbar hide on scroll). With a Rect, all these can
be initialized and we can avoid extra Resizes that happen when the
native View is properly tied to the Android View hierarcy on swap.

This change only updates the params and shouldn't be making any
functional changes. A follow up Android only patch will start setting
the value to the origin.

BUG=628302
Committed: https://crrev.com/7c8d964d95350a8a63ed75d761ae37a36ade3e85
Cr-Commit-Position: refs/heads/master@{#413469}

[Yusuf, 2016-08-18 18:40:51]

yusufo@chromium.org changed reviewers:
+ lizeb@chromium.org, pasko@chromium.org

[Yusuf, 2016-08-18 21:29:58]

yusufo@chromium.org changed reviewers:
+ mmenke@chromium.org

[Yusuf, 2016-08-18 21:29:59]

mmenke@ for prerender/
pasko@, lizeb@ for ExternalPrerenderHandler

[Yusuf, 2016-08-18 21:30:05]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 21:30:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 21:41:57]

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

[commit-bot: I haz the power, 2016-08-18 21:41:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuf, 2016-08-18 21:53:29]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 21:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 22:10:50]

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

[commit-bot: I haz the power, 2016-08-18 22:10:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yusuf, 2016-08-18 22:46:47]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 22:47:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 23:25:13]

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

[commit-bot: I haz the power, 2016-08-18 23:25:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yusuf, 2016-08-18 23:49:12]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 23:49:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 00:03:39]

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

[commit-bot: I haz the power, 2016-08-19 00:03:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuf, 2016-08-19 00:11:54]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 00:12:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 00:24:51]

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

[commit-bot: I haz the power, 2016-08-19 00:24:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Yusuf, 2016-08-19 00:37:07]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 00:37:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 00:48:39]

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

[commit-bot: I haz the power, 2016-08-19 00:48:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Benoit L, 2016-08-19 12:49:49]

Only 2 minor comments. But the bots still seem angry.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:241: DCHECK(!bounds.IsEmpty());
Isn't this going to be hit by the test in ExternalPrerenderHandlerTest.java:118?

(or at least create some flakiness if there is a race).

https://codereview.chromium.org/2259533003/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/PrerenderTestHelper.java
(right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/PrerenderTestHelper.java:100:
currentTab.getContentViewCore().getRenderCoordinates()
nit: the indentation looks funny here.

[mmenke, 2016-08-19 15:22:29]

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/external_prerender_handler_android.cc:96:
gfx::Rect(top, left, bottom, right));
include gfx::Rect header.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:26: #include
"ui/gfx/geometry/size.h"
Switch to Rect header.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:239: origin, url, referrer,
gfx::Rect(size), session_storage_namespace);
Need to include the header for gfx::Rect (Hrm...not sure we have the one for
gfx::size included, either)

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.h:53: class Size;
Need to add Rect here.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:37: #include
"ui/gfx/geometry/size.h"
Need rect.h, too.

[Yusuf, 2016-08-19 17:20:33]

Apologies folks, it seems I sent the review a bit preemptively after I built
chrome on android and also the test targets I thought would be effected. Looks
like ResizeWebContents touches a ton of files for targets that is not in All. I
will deal with bots and compile errors asap, apologies for the "dry run" pings

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:241: DCHECK(!bounds.IsEmpty());
On 2016/08/19 12:49:49, Benoit L wrote:
> Isn't this going to be hit by the test in
ExternalPrerenderHandlerTest.java:118?
> 
> (or at least create some flakiness if there is a race).

Actually, before prerender_manager calls this, we always check whether the one
sent to addPrerender is empty and set to default_bounds_ if it is. So by the
time we are here, it is always non-empty.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:26: #include
"ui/gfx/geometry/size.h"
On 2016/08/19 15:22:29, mmenke wrote:
> Switch to Rect header.

Done. Also switched to size in cc.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:239: origin, url, referrer,
gfx::Rect(size), session_storage_namespace);
On 2016/08/19 15:22:29, mmenke wrote:
> Need to include the header for gfx::Rect (Hrm...not sure we have the one for
> gfx::size included, either)

Done.

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:37: #include
"ui/gfx/geometry/size.h"
On 2016/08/19 15:22:29, mmenke wrote:
> Need rect.h, too.

Done.

https://codereview.chromium.org/2259533003/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/PrerenderTestHelper.java
(right):

https://codereview.chromium.org/2259533003/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/PrerenderTestHelper.java:100:
currentTab.getContentViewCore().getRenderCoordinates()
On 2016/08/19 12:49:49, Benoit L wrote:
> nit: the indentation looks funny here.

Done.

[Yusuf, 2016-08-19 17:20:55]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 17:21:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-08-19 17:40:50]

prerender/ LGTM, didn't look at the rest

[Yusuf, 2016-08-19 17:48:35]

yusufo@chromium.org changed reviewers:
+ sky@chromium.org

[Yusuf, 2016-08-19 17:48:35]

sky@ for chrome/browser/ui

sky@ the most important bit here is changing ResizeWebContents in
web_contents_sizer to take a Rect instead of Size. (This way we can initialize
Android prerenders much more reliably)

[commit-bot: I haz the power, 2016-08-19 17:52:17]

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

[commit-bot: I haz the power, 2016-08-19 17:52:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-08-19 19:36:19]

LGTM

[sky, 2016-08-19 19:36:55]

And thanks for a great description that clearly indicates why this is needed.

[Yusuf, 2016-08-19 23:32:22]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 23:33:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 00:22:28]

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

[commit-bot: I haz the power, 2016-08-20 00:22:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Benoit L, 2016-08-22 07:43:28]

LGTM

[pasko, 2016-08-22 15:17:58]

lgtm, thank you

https://codereview.chromium.org/2259533003/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2259533003/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:114: // indicates the rectangular
dimensions that the prerendered page should have.
it is not only the dimensions, but the 4 values defining the rectangle, right?

Sounds more like:

s/rectangular dimensions/rectangle/

[mmenke, 2016-08-22 15:19:01]

Also, should we have a browser test for this?  Make sure the prerender isn't
resized when we swap it in?

[pasko, 2016-08-22 15:23:59]

On 2016/08/22 15:19:01, mmenke wrote:
> Also, should we have a browser test for this?  Make sure the prerender isn't
> resized when we swap it in?

I am guessing the Yusuf wants to introduce it with the change that actually
prevents the resize ..

[Yusuf, 2016-08-22 16:29:58]

On 2016/08/22 15:23:59, pasko wrote:
> On 2016/08/22 15:19:01, mmenke wrote:
> > Also, should we have a browser test for this?  Make sure the prerender isn't
> > resized when we swap it in?
> 
> I am guessing the Yusuf wants to introduce it with the change that actually
> prevents the resize ..

Yeah, right now, we still get a resize because of the PhysicalBackingSize not
being initialized properly. I was hoping to add tests with the last patch that
fixes 628302.

[mmenke, 2016-08-22 16:32:29]

On 2016/08/22 16:29:58, Yusuf wrote:
> On 2016/08/22 15:23:59, pasko wrote:
> > On 2016/08/22 15:19:01, mmenke wrote:
> > > Also, should we have a browser test for this?  Make sure the prerender
isn't
> > > resized when we swap it in?
> > 
> > I am guessing the Yusuf wants to introduce it with the change that actually
> > prevents the resize ..
> 
> Yeah, right now, we still get a resize because of the PhysicalBackingSize not
> being initialized properly. I was hoping to add tests with the last patch that
> fixes 628302.

Sure, if this isn't the final CL in the chain needed to fix the issue, SGTM.

[Yusuf, 2016-08-22 16:38:48]

yusufo@chromium.org changed reviewers:
+ hubbe@chromium.org, mariakhomenko@chromium.org

[Yusuf, 2016-08-22 16:38:49]

hubbe@ for /extensions/api/tab_capture/offline_tab
mariakhomenko@ for chrome/android

https://codereview.chromium.org/2259533003/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/2259533003/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:114: // indicates the rectangular
dimensions that the prerendered page should have.
On 2016/08/22 15:17:58, pasko wrote:
> it is not only the dimensions, but the 4 values defining the rectangle, right?
> 
> Sounds more like:
> 
> s/rectangular dimensions/rectangle/

Done.

[Yusuf, 2016-08-22 16:39:02]

The CQ bit was checked by yusufo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 16:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maria, 2016-08-22 17:06:46]

lgtm

[hubbe, 2016-08-22 17:13:22]

lgtm

[commit-bot: I haz the power, 2016-08-22 17:24:13]

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

[commit-bot: I haz the power, 2016-08-22 17:24:14]

Dry run: This issue passed the CQ dry run.

[Yusuf, 2016-08-22 17:35:15]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2016-08-22 17:35:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
mmenke@chromium.org, pasko@chromium.org, lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2259533003/#ps160001
(title: "comments nit")

[commit-bot: I haz the power, 2016-08-22 17:36:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 17:41:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-22 17:43:11]

Description was changed from

==========
Use bounds instead of size for prerender requests

This changes the pipeline that initializes prerender contents
with a size to use bounds instead. Note that not all addPrerenderX
calls are switched to use Rect since for everywhere except Android
this won't be needed. Android needs bounds instead of size to get
info about the Physical Backing Size, also the top container height
(to be used for toolbar hide on scroll). With a Rect, all these can
be initialized and we can avoid extra Resizes that happen when the
native View is properly tied to the Android View hierarcy on swap.

This change only updates the params and shouldn't be making any
functional changes. A follow up Android only patch will start setting
the value to the origin.

BUG=628302
==========

to

==========
Use bounds instead of size for prerender requests

This changes the pipeline that initializes prerender contents
with a size to use bounds instead. Note that not all addPrerenderX
calls are switched to use Rect since for everywhere except Android
this won't be needed. Android needs bounds instead of size to get
info about the Physical Backing Size, also the top container height
(to be used for toolbar hide on scroll). With a Rect, all these can
be initialized and we can avoid extra Resizes that happen when the
native View is properly tied to the Android View hierarcy on swap.

This change only updates the params and shouldn't be making any
functional changes. A follow up Android only patch will start setting
the value to the origin.

BUG=628302

Committed: https://crrev.com/7c8d964d95350a8a63ed75d761ae37a36ade3e85
Cr-Commit-Position: refs/heads/master@{#413469}
==========

[commit-bot: I haz the power, 2016-08-22 17:43:13]

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