Pass the correct webState to the nativeContent.

Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the webState from the webController instead.

BUG=685969
Review-Url: https://codereview.chromium.org/2655463014
Cr-Commit-Position: refs/heads/master@{#446753}
Committed: https://chromium.googlesource.com/chromium/src/+/d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a

[Olivier, 2017-01-27 10:32:51]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, marq@chromium.org

[Olivier, 2017-01-27 10:32:51]

I think there are 3 way to fix this issue.
1. Add a webState method to nativeContentDelegate (which is the web_controller),
but delegate should not be a provider.
2. Add webState as an argument of nativeContentProvider's methods, but
offlinePageNativeContent is the only native content needing the webState.

I chose to add an optional setWebState to CRWNativeContent.
Please tell me if one of the other two methods (or a 4th one) is better.

[Olivier, 2017-01-27 10:33:21]

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

[commit-bot: I haz the power, 2017-01-27 10:33:46]

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

[gambard, 2017-01-27 10:39:31]

I am not sure which method would be best for architecture, waiting for marq
answer.

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:105:
_webState->GetNavigationManager()->GetLastCommittedItem();
If there is no _webState, you probably crash here. Add a guard to prevent it.

[commit-bot: I haz the power, 2017-01-27 10:43:52]

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

[commit-bot: I haz the power, 2017-01-27 10:43:53]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-27 10:46:32]

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:105:
_webState->GetNavigationManager()->GetLastCommittedItem();
On 2017/01/27 10:39:30, gambard wrote:
> If there is no _webState, you probably crash here. Add a guard to prevent it.

You should not recover from DCHECK, so I guess we just let it crash...

[marq (ping after 24h), 2017-01-27 11:56:57]

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

[marq (ping after 24h), 2017-01-27 11:56:57]

Honestly I'm not sure. +eugenebut@

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2655463014/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:105:
_webState->GetNavigationManager()->GetLastCommittedItem();
On 2017/01/27 10:46:32, Olivier Robin wrote:
> On 2017/01/27 10:39:30, gambard wrote:
> > If there is no _webState, you probably crash here. Add a guard to prevent
it.
> 
> You should not recover from DCHECK, so I guess we just let it crash...

Yep, the DCHECK on line 99 means that the rest of the method can assume a
nonnull pointer.

[Eugene But (OOO till 7-30), 2017-01-27 16:02:58]

I would extend -[CRWNativeContentProvider controllerForURL:] to pass web state.
I think it is fair to pass more information to CRWNativeContentProvider. WDYT?

Please note that we will rethink the whole concept of NativeContent, so I would
not spend too much time on finding a perfect solution.

[Olivier, 2017-01-27 16:28:16]

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

[commit-bot: I haz the power, 2017-01-27 16:28:48]

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-01-27 16:39:21]

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

[commit-bot: I haz the power, 2017-01-27 16:39:22]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-27 16:39:52]

I think extending CRWNativeContentProvider is a good solution.
I hesitated because OfflinePageNativeContent is the only content using it.

Done.

PTAL.

[Olivier, 2017-01-27 16:43:27]

Description was changed from

==========
Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the tab from the webController instead.

BUG=685969
==========

to

==========
Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the webState from the webController instead.

BUG=685969
==========

[Eugene But (OOO till 7-30), 2017-01-27 17:31:54]

lgtm

https://codereview.chromium.org/2655463014/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/ui/crw_native_content_provider.h (right):

https://codereview.chromium.org/2655463014/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/ui/crw_native_content_provider.h:28:
withWebState:(web::WebState*)webState;
Should this be s/withWebState/webState? From interface perspective this returns
native controller "For" given web state. From implementation perspective it can
be "WithWebState", but it's not quite important here.

[marq (ping after 24h), 2017-01-27 17:39:15]

LGTM for ui/

[Olivier, 2017-01-27 20:17:27]

https://codereview.chromium.org/2655463014/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/ui/crw_native_content_provider.h (right):

https://codereview.chromium.org/2655463014/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/ui/crw_native_content_provider.h:28:
withWebState:(web::WebState*)webState;
On 2017/01/27 17:31:54, Eugene But wrote:
> Should this be s/withWebState/webState? From interface perspective this
returns
> native controller "For" given web state. From implementation perspective it
can
> be "WithWebState", but it's not quite important here.

|controllerForURL:webState:| it is then.
Done.

[Olivier, 2017-01-27 20:17:33]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-27 20:17:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2655463014/#ps40001
(title: "s/ithW//")

[commit-bot: I haz the power, 2017-01-27 20:18:17]

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-01-27 20:35:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485548253096700,
"parent_rev": "66660640fd0ed0b483330345072245a8d5a83a06", "commit_rev":
"d43eecb7ed3d688f344cc64bc4d09b7fb7e88d7a"}

[commit-bot: I haz the power, 2017-01-27 20:36:03]

Description was changed from

==========
Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the webState from the webController instead.

BUG=685969
==========

to

==========
Pass the correct webState to the nativeContent.

If an offline content is created in a new tab, currentWebState in the
BVC will return the webState from the original tab (created tab is in
background).

Pass the webState from the webController instead.

BUG=685969

Review-Url: https://codereview.chromium.org/2655463014
Cr-Commit-Position: refs/heads/master@{#446753}
Committed:
https://chromium.googlesource.com/chromium/src/+/d43eecb7ed3d688f344cc64bc4d0...
==========

[commit-bot: I haz the power, 2017-01-27 20:36:04]

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