Set CRWWebViewContentView's size before resetting scroll proxy.

Set CRWWebViewContentView's size before resetting scroll proxy.

Because CRWWebViewContentView's frame was CGRectZero when the scroll
proxy's backing UIScrollView was reset,
CRWWebViewScrollViewProxyObservers were mistakenly resetting the web
view's frame to an invalid frame (negative values for size dimensions).
This was causing various rendering issues, including an incorrect
scroll offset for links opened in new tabs and broken tap handling for
"Read More" links on cnn.com.

BUG=577793, 574996
Committed: https://crrev.com/c47dcc4e47d42932d126d80d5aa740cf8470280a
Cr-Commit-Position: refs/heads/master@{#374581}

[kkhorimoto, 2016-02-09 19:04:55]

kkhorimoto@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[kkhorimoto, 2016-02-09 19:04:56]

+ Stuart for OWNER

cc Shreyas for "click more" bug

[stuartmorgan, 2016-02-09 21:27:50]

Good find! LGTM with one suggestion.

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: self.frame = kDummyRect;
Would it be worth adding a DCHECK here that the frame isn't zero/invalid, with a
brief comment explaining why, just to be sure we never regress this on accident?

[shreyasv1, 2016-02-09 21:58:28]

shreyasv@chromium.org changed reviewers:
+ shreyasv@chromium.org

[shreyasv1, 2016-02-09 21:58:29]

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: self.frame = kDummyRect;
On 2016/02/09 21:27:50, stuartmorgan wrote:
> Would it be worth adding a DCHECK here that the frame isn't zero/invalid, with
a
> brief comment explaining why, just to be sure we never regress this on
accident?

The fact that it's non zero is a coincidence from the embedder's side.
When a UIView is instantiated it's bounds is CGRectZero. 
Whenever a WebController is alloc, inited the CRWWebControllerContainerView has
a bounds of CGRectZero.

Given that, adding a DCHECK is the wrong thing here.

[stuartmorgan, 2016-02-09 22:15:19]

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: self.frame = kDummyRect;
On 2016/02/09 21:58:29, shreyasv1 wrote:
> The fact that it's non zero is a coincidence from the embedder's side.

If having it be non-zero avoids critical bugs, then it's not a coincidence, it's
something embedders need to do.

[kkhorimoto, 2016-02-09 22:17:11]

Turns out that this fixed the scrolling on iPads, but broke it again on iPhones.
 Similarly, the "Read More" button works only on iPad with this approach; I'm
investigating more on a solution that works for both (though it'll likely be
hacky)

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_view_content_view.mm (left):

https://codereview.chromium.org/1682873002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: self.frame = kDummyRect;
On 2016/02/09 22:15:19, stuartmorgan wrote:
> On 2016/02/09 21:58:29, shreyasv1 wrote:
> > The fact that it's non zero is a coincidence from the embedder's side.
> 
> If having it be non-zero avoids critical bugs, then it's not a coincidence,
it's
> something embedders need to do.

I think it'd be better to add a DCHECK in
|-loadRequestForCurrentNavigationItem|, since all that really matters is that it
has a valid size before attempting to load/render anything.

[kkhorimoto, 2016-02-09 22:54:39]

Turns out the DCHECK was helpful; the web view was not getting properly resized
on iPhones due to the codepaths for fullscreen controller differing for each
device.  I updated CRWWebViewContentView to resize the web view immediately when
its frame or bounds changes so the web view can be an appropriate size when we
start loading items (instead of being resized later in the layout run).

[shreyasv1, 2016-02-09 22:58:18]

lgtm

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_view_content_view.mm (right):

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: if
(!CGSizeEqualToSize(oldSize, self.frame.size))
NIT: How about early returning in the method by comparing it with |frame|?

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right):

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:591:
DCHECK(CGRectGetWidth(self.webView.frame) > 0.0);
DCHECK_GE?

[stuartmorgan, 2016-02-09 23:00:51]

lgtm

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right):

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:592:
DCHECK(CGRectGetHeight(self.webView.frame) > 0.0);
CGRectIsNull?

Also, a brief comment here explaining why the check, maybe referencing the bug,
would be helpful, so that if someone hits the DCHECK in a year they'll
understand why it's there and not just remove it.

[kkhorimoto, 2016-02-10 00:08:36]

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_view_content_view.mm (right):

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_view_content_view.mm:92: if
(!CGSizeEqualToSize(oldSize, self.frame.size))
On 2016/02/09 22:58:18, shreyasv1 wrote:
> NIT: How about early returning in the method by comparing it with |frame|?

Done.

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right):

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:591:
DCHECK(CGRectGetWidth(self.webView.frame) > 0.0);
On 2016/02/09 22:58:18, shreyasv1 wrote:
> DCHECK_GE?

Done.

https://codereview.chromium.org/1682873002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:592:
DCHECK(CGRectGetHeight(self.webView.frame) > 0.0);
On 2016/02/09 23:00:51, stuartmorgan wrote:
> CGRectIsNull?
> 
> Also, a brief comment here explaining why the check, maybe referencing the
bug,
> would be helpful, so that if someone hits the DCHECK in a year they'll
> understand why it's there and not just remove it.

CGRectIsNull specifically checks against {{Inf, Inf}, {0, 0}}, so it can't be
used here.  I also checked whether CGRectIsEmpty would work, but it returns NO
if both size dimensions are negative, so I just used DCHECK_GT as Shreyas
suggested.  I've also added a comment.

[kkhorimoto, 2016-02-10 00:08:43]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2016-02-10 00:08:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
shreyasv@chromium.org, stuartmorgan@chromium.org
Link to the patchset: https://codereview.chromium.org/1682873002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2016-02-10 00:09:04]

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

[commit-bot: I haz the power, 2016-02-10 00:43:55]

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

[commit-bot: I haz the power, 2016-02-10 00:43:56]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[kkhorimoto, 2016-02-10 02:14:14]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2016-02-10 02:14:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
shreyasv@chromium.org, stuartmorgan@chromium.org
Link to the patchset: https://codereview.chromium.org/1682873002/#ps60001
(title: "Fix tests")

[commit-bot: I haz the power, 2016-02-10 02:15:34]

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

[commit-bot: I haz the power, 2016-02-10 02:36:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-10 02:38:38]

Description was changed from

==========
Set CRWWebViewContentView's size before resetting scroll proxy.

Because CRWWebViewContentView's frame was CGRectZero when the scroll
proxy's backing UIScrollView was reset,
CRWWebViewScrollViewProxyObservers were mistakenly resetting the web
view's frame to an invalid frame (negative values for size dimensions).
This was causing various rendering issues, including an incorrect
scroll offset for links opened in new tabs and broken tap handling for
"Read More" links on cnn.com.

BUG=577793, 574996
==========

to

==========
Set CRWWebViewContentView's size before resetting scroll proxy.

Because CRWWebViewContentView's frame was CGRectZero when the scroll
proxy's backing UIScrollView was reset,
CRWWebViewScrollViewProxyObservers were mistakenly resetting the web
view's frame to an invalid frame (negative values for size dimensions).
This was causing various rendering issues, including an incorrect
scroll offset for links opened in new tabs and broken tap handling for
"Read More" links on cnn.com.

BUG=577793, 574996

Committed: https://crrev.com/c47dcc4e47d42932d126d80d5aa740cf8470280a
Cr-Commit-Position: refs/heads/master@{#374581}
==========

[commit-bot: I haz the power, 2016-02-10 02:38:39]

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