Remove TabModel -replaceWebState: method.

Remove TabModel -replaceWebState: method.

In order to allow refactoring Tab and WebState ownership relationship,
remove the method allowing a test to replace the WebState owned by a
Tab.

The method was only used from two tests suites to allow using a mock
CRWWebController. As the mock is only implementing part of the class
API, registering the tab helpers with the WebState fails.

Instead introduce an additional parameter to the Tab initialiser to
control whether the tab helpers are registered or not. Call this new
initialiser from the two test suites to allow them to control which
tab helpers are initialised.

BUG=687207
Review-Url: https://codereview.chromium.org/2685653002
Cr-Commit-Position: refs/heads/master@{#448995}
Committed: https://chromium.googlesource.com/chromium/src/+/2176e15afc8a82813c303e3001a02aad930246a2

[sdefresne, 2017-02-07 17:37:14]

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

[commit-bot: I haz the power, 2017-02-07 17:37:28]

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

[sdefresne, 2017-02-07 17:38:18]

sdefresne@chromium.org changed reviewers:
+ rohitrao@chromium.org

[sdefresne, 2017-02-07 17:38:18]

Please take a look.

[sdefresne, 2017-02-07 17:42:00]

sdefresne@chromium.org changed reviewers:
+ marq@chromium.org

[sdefresne, 2017-02-07 17:42:01]

marq: FYI

[sdefresne, 2017-02-07 18:06:19]

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

[commit-bot: I haz the power, 2017-02-07 18:06: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, 2017-02-07 18:16:08]

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

[commit-bot: I haz the power, 2017-02-07 18:16:09]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-02-07 18:39:04]

https://codereview.chromium.org/2685653002/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model_unittest.mm (right):

https://codereview.chromium.org/2685653002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model_unittest.mm:84: if ((self = [super
initWithWebState:std::move(webStateImpl)
Prefer to keep the superclass init at the top of the method unless there's a
good reason not to.

If you don't, please fix the indent for the lines above.

[sdefresne, 2017-02-08 09:28:07]

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

[commit-bot: I haz the power, 2017-02-08 09:28:33]

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

[sdefresne, 2017-02-08 09:28:58]

https://codereview.chromium.org/2685653002/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model_unittest.mm (right):

https://codereview.chromium.org/2685653002/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model_unittest.mm:84: if ((self = [super
initWithWebState:std::move(webStateImpl)
On 2017/02/07 18:39:04, marq wrote:
> Prefer to keep the superclass init at the top of the method unless there's a
> good reason not to.
> 
> If you don't, please fix the indent for the lines above.

There's a good reason: I need to create and WebStateImpl and its mock
CRWWebController before I can invoke the super class initialiser.

Indentation fixed (I expected that "git cl format" would have reformatted them
but then it only reformat the modified code and "git diff" was over optimistic
in not marking the body of the "if" as modified).

[commit-bot: I haz the power, 2017-02-08 09:38:04]

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

[commit-bot: I haz the power, 2017-02-08 09:38:05]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-02-08 13:00:52]

Why does attaching tab helpers fail in the tests?  It looks like you have a real
WebStateImpl.  Do some of the TabHelpers try to call CRWWebController methods
that are not stubbed out?

[sdefresne, 2017-02-08 13:40:35]

On 2017/02/08 13:00:52, rohitrao wrote:
> Why does attaching tab helpers fail in the tests?  It looks like you have a
real
> WebStateImpl.  Do some of the TabHelpers try to call CRWWebController methods
> that are not stubbed out?

I have a real WebStateImpl but not a real CRWWebController and the tab helpers
try to access non-stubbed methods from CRWWebController (not directly).

I initially tried to stub those method and got to a point where attaching the
helpers worked. This required stubbing -[CRWWebController jsInjectionReceiver]
and then to stub -[CRWJSInjectionReceiver instanceOfClass:]. This is required
for the translation tab helper IOSTranslateDriver that access the receiver via
the public WebState::GetJSInjectionReceiver() method.

However, I then add crashed during the tests because tab helper reacted to the
navigation events and called method on WebState that are implemented by
CRWWebController. As my stub was incomplete, the value returned by those method
was default value, which does not work correctly when the returned value is
supposed to be a C++ object. One such occurrence is in password_manager.mm
GetPageURLAndCheckTrustLevel free function. It call WebState::GetCurrentURL()
that return a GURL by value and a web::URLVerificationTrustLevel by out
parameter (i.e. pointer). The implementation of WebStateImpl::GetCurrentURL()
invokes -[CRWWebController currentURLWithTrustLevel:] that returns a GURL. Since
the method is not stubbed, the returned value is undefined (I think it is
whatever garbage was on the stack) and GURL copy constructor crashes.

I think that trying to stub each and every methods to be able to attach the tab
helper is hard and will put extra effort on everyone making changes in any of
the tab helpers.

Please note that -[Tab replaceWebState:] was not attaching the tab helper to the
new WebState (well it was only attaching two of them, and they are not really
necessary for the tests). As the old code was resetting the old webStateImpl_
smart pointer, all the attached tab helpers where destroyed and none were
attached (since they are only attached in the initializer). So my change is the
simplest one.

[rohitrao (ping after 24h), 2017-02-08 13:46:33]

lgtm, thanks for the explanation

Longer term, we'll want to move helper creation out of Tab, and probably move it
to the point where WebStates are added to the TabModel (rather than at the time
of construction).  I don't think that will cause any issues with these tests.

[sdefresne, 2017-02-08 13:52:07]

On 2017/02/08 13:46:33, rohitrao wrote:
> lgtm, thanks for the explanation
> 
> Longer term, we'll want to move helper creation out of Tab, and probably move
it
> to the point where WebStates are added to the TabModel (rather than at the
time
> of construction).  I don't think that will cause any issues with these tests.

I think that long term those tests want to use TestWebState. They cannot
currently because they use API that expect Tab. At that point, it will make
sense for the TestWebState to not have any tab helper by default and to let the
test attach those it requires.

[sdefresne, 2017-02-08 14:00:05]

Patchset #4 (id:60001) has been deleted

[sdefresne, 2017-02-08 14:54:30]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2017-02-08 14:54:50]

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, 2017-02-08 14:58:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486565670907590,
"parent_rev": "d3173fb3e213e658abf34667f130b55b6baac395", "commit_rev":
"2176e15afc8a82813c303e3001a02aad930246a2"}

[commit-bot: I haz the power, 2017-02-08 14:58:58]

Description was changed from

==========
Remove TabModel -replaceWebState: method.

In order to allow refactoring Tab and WebState ownership relationship,
remove the method allowing a test to replace the WebState owned by a
Tab.

The method was only used from two tests suites to allow using a mock
CRWWebController. As the mock is only implementing part of the class
API, registering the tab helpers with the WebState fails.

Instead introduce an additional parameter to the Tab initialiser to
control whether the tab helpers are registered or not. Call this new
initialiser from the two test suites to allow them to control which
tab helpers are initialised.

BUG=687207
==========

to

==========
Remove TabModel -replaceWebState: method.

In order to allow refactoring Tab and WebState ownership relationship,
remove the method allowing a test to replace the WebState owned by a
Tab.

The method was only used from two tests suites to allow using a mock
CRWWebController. As the mock is only implementing part of the class
API, registering the tab helpers with the WebState fails.

Instead introduce an additional parameter to the Tab initialiser to
control whether the tab helpers are registered or not. Call this new
initialiser from the two test suites to allow them to control which
tab helpers are initialised.

BUG=687207

Review-Url: https://codereview.chromium.org/2685653002
Cr-Commit-Position: refs/heads/master@{#448995}
Committed:
https://chromium.googlesource.com/chromium/src/+/2176e15afc8a82813c303e3001a0...
==========

[commit-bot: I haz the power, 2017-02-08 14:58:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2176e15afc8a82813c303e3001a0...