Adding CRWWebViewContentView as a subview when it is set

Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubviews/34301753)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.

Committed: https://crrev.com/f5406c826d16602ce699bdf7f7da5cc5461be7f0
Cr-Commit-Position: refs/heads/master@{#365691}

[shreyasv1, 2015-12-16 00:24:10]

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

[shreyasv1, 2015-12-16 00:26:50]

As a (intended) side effect this CL fixes a bug in UIWebView where adding it as
part of layoutSubViews would lead to issues in scrolling of certain pages.

[kkhorimoto, 2015-12-16 18:54:17]

lgtm with nit.  Also, can you add a sentence to the CL description that explains
that this fixes the UIWebView bug (for whatever reason)?  At a glance, the CL
and the bug don't seem correlated, and someone would have to look at the review
comments to see that this fixes the UIWebView issue.

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

https://codereview.chromium.org/1531593002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller_container_view.mm:222:
[_webViewContentView setFrame:self.bounds];
Can you use property notation for consistency?  The ivars backing properties
should only really be accessed in accessors (or initializers when setting
default values).

[shreyasv1, 2015-12-16 22:58:57]

Description was changed from

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

BUG=568097
TEST=
Illustrated in the bug.
==========

to

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView where adding it as
part of layoutSubViews would lead to issues in scrolling 
of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

[shreyasv1, 2015-12-16 23:02:05]

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

https://codereview.chromium.org/1531593002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller_container_view.mm:222:
[_webViewContentView setFrame:self.bounds];
On 2015/12/16 18:54:17, kkhorimoto_ wrote:
> Can you use property notation for consistency?  The ivars backing properties
> should only really be accessed in accessors (or initializers when setting
> default values).

Done.

[shreyasv1, 2015-12-16 23:02:20]

The CQ bit was checked by shreyasv@chromium.org

[shreyasv1, 2015-12-16 23:02:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1531593002/#ps40001
(title: "y")

[commit-bot: I haz the power, 2015-12-16 23:03:49]

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

[kkhorimoto, 2015-12-16 23:10:04]

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

[kkhorimoto, 2015-12-16 23:10:05]

I'm adding Eugene as a review since I'm not a web/ OWNER.  Also, can you limit
the line width for your description?

[shreyasv1, 2015-12-16 23:10:56]

The CQ bit was unchecked by shreyasv@chromium.org

[shreyasv1, 2015-12-16 23:11:08]

Description was changed from

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView where adding it as
part of layoutSubViews would lead to issues in scrolling 
of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

to

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues 
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

[Eugene But (OOO till 7-30), 2015-12-16 23:21:09]

lgtm

[shreyasv1, 2015-12-16 23:21:17]

The CQ bit was checked by shreyasv@chromium.org

[commit-bot: I haz the power, 2015-12-16 23:23:49]

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

[commit-bot: I haz the power, 2015-12-16 23:52:40]

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

[commit-bot: I haz the power, 2015-12-16 23:52:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[shreyasv1, 2015-12-16 23:53:07]

The CQ bit was checked by shreyasv@chromium.org

[shreyasv1, 2015-12-16 23:54:02]

The CQ bit was unchecked by shreyasv@chromium.org

[shreyasv1, 2015-12-17 00:00:59]

The CQ bit was checked by shreyasv@chromium.org

[shreyasv1, 2015-12-17 00:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1531593002/#ps60001
(title: "y")

[commit-bot: I haz the power, 2015-12-17 00:03:22]

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

[commit-bot: I haz the power, 2015-12-17 02:48:39]

Description was changed from

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues 
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

to

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues 
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

[commit-bot: I haz the power, 2015-12-17 02:48:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-17 02:50:02]

Description was changed from

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues 
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.
==========

to

==========
Adding CRWWebViewContentView as a subview when it is set

Adding subviews in layoutSubviews is problematic
(see
http://stackoverflow.com/questions/34301366/call-addsubview-within-layoutsubv...)

This CL moves it out of |layoutSubviews| and
into when the CRWWebViewContentView is actually
set on the ContainerView.

The existing logic will take care of removing
the old ContentView from the superview
when there is a need to replace the
ContentView.

As a (intended) side effect this CL fixes a bug in UIWebView
 where adding it as part of layoutSubViews would lead to issues
in scrolling of certain pages.

BUG=568097
TEST=
Illustrated in the bug.

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

[commit-bot: I haz the power, 2015-12-17 02:50:03]

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