Reversed WebState <-> CRWWebController ownership.

Reversed WebState <-> CRWWebController ownership.

WebState is a main public class in web//, while CRWWebController is an
implementation class, so WebState should own CRWWebController.

BUG=597997
Committed: https://crrev.com/b70db6e5b1700b230792342da63343b580f0bbf4
Cr-Commit-Position: refs/heads/master@{#387052}

[Eugene But (OOO till 7-30), 2016-04-10 01:55:07]

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

[commit-bot: I haz the power, 2016-04-10 01:55:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874903003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874903003/1

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

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

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

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2016-04-11 14:59:48]

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

[commit-bot: I haz the power, 2016-04-11 15:00:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874903003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874903003/20001

[Eugene But (OOO till 7-30), 2016-04-11 15:04:11]

eugenebut@chromium.org changed reviewers:
+ jyquinn@chromium.org, kkhorimoto@chromium.org, rohitrao@chromium.org

[commit-bot: I haz the power, 2016-04-11 15:40:10]

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

[commit-bot: I haz the power, 2016-04-11 15:40:11]

Dry run: This issue passed the CQ dry run.

[Jackie Quinn, 2016-04-12 20:03:46]

lgtm with comment nit.

https://codereview.chromium.org/1874903003/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1874903003/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:289: // The CRWWebController that backs and
owns this object.
Comment needs update.

[Eugene But (OOO till 7-30), 2016-04-12 20:47:39]

https://codereview.chromium.org/1874903003/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1874903003/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.h:289: // The CRWWebController that backs and
owns this object.
On 2016/04/12 20:03:46, Jackie Quinn wrote:
> Comment needs update.

Done.

[kkhorimoto, 2016-04-12 21:36:13]

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller+protected.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller+protected.h:109: // Designated
initializer. Initializes web controller with unowned |webState|.
"Initializes a web controller owned by |webState|"?

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.h:64: // This is an abstract class which
must not be instantiated directly.
Are we going to keep CRWWKWebViewWebController around once we're finished
cleaning up all the UIWV stuff?

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:281: // Unowned WebStateImpl instance
associated with this CRWWebController.
"Unowned" seems kinda strangely worded.  It would be clearer if you instead
mentioned that |_webStateImpl| owns |self|.  Also, do you think it'd be worth it
to have a WeakPtrFactor owned by WebStateImpl so we can mitigate the possibility
of classes holding onto pointers to deleted objects?

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.h:15: // Designated
initializer. Initializes web controller with unowned |webState|.
Same here.  Specifying which object owns what is clearer than just mentioning
that |webState| is "unowned".

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:105: [web_controller_ close];
DCHECK_EQ(this, web_controller.webState)?  We might get into a bad state if this
is not the case.

[Eugene But (OOO till 7-30), 2016-04-13 02:51:26]

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller+protected.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller+protected.h:109: // Designated
initializer. Initializes web controller with unowned |webState|.
On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> "Initializes a web controller owned by |webState|"?
Owner of WC should not be mentioned here (see my other comments, where I explain
why). But it is important to note that WC does not own |webState|.

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.h:64: // This is an abstract class which
must not be instantiated directly.
On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> Are we going to keep CRWWKWebViewWebController around once we're finished
> cleaning up all the UIWV stuff? 
No. If you are worrying that this comment will rot after cleanup, I can remove
it now.

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:281: // Unowned WebStateImpl instance
associated with this CRWWebController.
On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> "Unowned" seems kinda strangely worded.  It would be clearer if you instead
> mentioned that |_webStateImpl| owns |self|.  Also, do you think it'd be worth
it
> to have a WeakPtrFactor owned by WebStateImpl so we can mitigate the
possibility
> of classes holding onto pointers to deleted objects?
WC should not make any assumptions about it's owner. So I don't want to write
that WC is owned by WS. However I'm very open for replacing "Unowned" with
something else, so let me know if you have suggestions.

As for WeakFactory, I don't think it make sense to pay performance cost here. WC
clears reference to WS inside close and if WS had died before |close|, I think
we have a bigger problem than crashes :)

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.h:15: // Designated
initializer. Initializes web controller with unowned |webState|.
On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> Same here.  Specifying which object owns what is clearer than just mentioning
> that |webState| is "unowned".
Specifying the owner is a layering violation or even misleading information
(f.e. in unit test WC can be owned by test).

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:105: [web_controller_ close];
On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> DCHECK_EQ(this, web_controller.webState)?  We might get into a bad state if
this
> is not the case.
SetWebController is used only in unit tests and in all places web_controller is
mock and does not return webState. So this DCHECK will fail for every call. It
is possible to change the test and make mock return webState, but it will not
make the code more robust. The right direction would be removing
GetWebController and SetWebController once Embedder stop using WC.

[rohitrao (ping after 24h), 2016-04-13 11:52:27]

LGTM but please make sure that Kurt signs off as well.

Maybe instead of "unowned pointer" say something like "the calling code retains
ownership of |web_state|"?

[Eugene But (OOO till 7-30), 2016-04-13 13:57:58]

Thanks everyone. Kurt, I removed "unowned" and using similar wording to Rohit's
suggestion. PTAL.

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller+protected.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller+protected.h:109: // Designated
initializer. Initializes web controller with unowned |webState|.
On 2016/04/13 02:51:26, eugenebut wrote:
> On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> > "Initializes a web controller owned by |webState|"?
> Owner of WC should not be mentioned here (see my other comments, where I
explain
> why). But it is important to note that WC does not own |webState|.
Updated the comments according to Rohit's suggestion.

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:281: // Unowned WebStateImpl instance
associated with this CRWWebController.
On 2016/04/13 02:51:26, eugenebut wrote:
> On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> > "Unowned" seems kinda strangely worded.  It would be clearer if you instead
> > mentioned that |_webStateImpl| owns |self|.  Also, do you think it'd be
worth
> it
> > to have a WeakPtrFactor owned by WebStateImpl so we can mitigate the
> possibility
> > of classes holding onto pointers to deleted objects?
> WC should not make any assumptions about it's owner. So I don't want to write
> that WC is owned by WS. However I'm very open for replacing "Unowned" with
> something else, so let me know if you have suggestions.
> 
> As for WeakFactory, I don't think it make sense to pay performance cost here.
WC
> clears reference to WS inside close and if WS had died before |close|, I think
> we have a bigger problem than crashes :)
Eliminated "unowned" word.

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_web_view_web_controller.h (right):

https://codereview.chromium.org/1874903003/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_web_view_web_controller.h:15: // Designated
initializer. Initializes web controller with unowned |webState|.
On 2016/04/13 02:51:26, eugenebut wrote:
> On 2016/04/12 21:36:13, kkhorimoto_ wrote:
> > Same here.  Specifying which object owns what is clearer than just
mentioning
> > that |webState| is "unowned".
> Specifying the owner is a layering violation or even misleading information
> (f.e. in unit test WC can be owned by test).
Updated the comments according to Rohit's suggestion.

[kkhorimoto, 2016-04-13 16:59:22]

lgtm!

[Eugene But (OOO till 7-30), 2016-04-13 17:47:23]

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

[commit-bot: I haz the power, 2016-04-13 17:48:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874903003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874903003/100001

[commit-bot: I haz the power, 2016-04-13 17:52:37]

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

[commit-bot: I haz the power, 2016-04-13 17:52:38]

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

[Eugene But (OOO till 7-30), 2016-04-13 18:00:34]

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

[commit-bot: I haz the power, 2016-04-13 18:01:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874903003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874903003/120001

[commit-bot: I haz the power, 2016-04-13 18:04:32]

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

[commit-bot: I haz the power, 2016-04-13 18:04:33]

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

[Eugene But (OOO till 7-30), 2016-04-13 18:14:16]

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

[commit-bot: I haz the power, 2016-04-13 18:14:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874903003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874903003/140001

[commit-bot: I haz the power, 2016-04-13 18:27:59]

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

[commit-bot: I haz the power, 2016-04-13 18:28:00]

Dry run: 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...)

[Eugene But (OOO till 7-30), 2016-04-13 18:30:07]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-04-13 18:30:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyquinn@chromium.org, rohitrao@chromium.org, kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/1874903003/#ps140001
(title: "Merged with origin/master")

[commit-bot: I haz the power, 2016-04-13 18:30:51]

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

[commit-bot: I haz the power, 2016-04-13 18:53:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-13 18:54:40]

Description was changed from

==========
Reversed WebState <-> CRWWebController ownership.

WebState is a main public class in web//, while CRWWebController is an
implementation class, so WebState should own CRWWebController.

BUG=597997
==========

to

==========
Reversed WebState <-> CRWWebController ownership.

WebState is a main public class in web//, while CRWWebController is an
implementation class, so WebState should own CRWWebController.

BUG=597997

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

[commit-bot: I haz the power, 2016-04-13 18:54:41]

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