Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

Change just marks the method as deprecated, but doesn't change any actual code.


BUG=725241
Review-Url: https://codereview.chromium.org/2907723003
Cr-Commit-Position: refs/heads/master@{#476692}
Committed: https://chromium.googlesource.com/chromium/src/+/13b6a0f2fb8045004e2cd77c7e72947bae064d02

[Eugene But (OOO till 7-30), 2017-05-26 17:59:49]

Description was changed from

==========
WNativeContentProvider as line is 72 characters long--------------------
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

BUG=725241
==========

to

==========
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

BUG=725241
==========

[Eugene But (OOO till 7-30), 2017-05-26 18:00:30]

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

[Eugene But (OOO till 7-30), 2017-05-26 18:00:30]

PTAL. Not time sensitive. Can wait until marq@ is back.

[marq (ping after 24h), 2017-05-29 09:57:39]

The CQ bit was checked by marq@chromium.org

[marq (ping after 24h), 2017-05-29 09:57:40]

lgtm

[marq (ping after 24h), 2017-05-29 09:57:49]

The CQ bit was unchecked by marq@chromium.org

[commit-bot: I haz the power, 2017-05-29 09:57:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marq (ping after 24h), 2017-05-29 10:02:08]

Still LGTM, but please amend the CL description to clarify that it just marks
the method as deprecated, but doesn't change any actual code.

https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
File ios/web/public/web_state/ui/crw_native_content_provider.h (right):

https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
ios/web/public/web_state/ui/crw_native_content_provider.h:41:
isPost:(BOOL)isPost;
Add "__attribute__((deprecated))" or its macro equivalent?

[marq (ping after 24h), 2017-05-29 10:02:18]

The CQ bit was unchecked by marq@chromium.org

[Eugene But (OOO till 7-30), 2017-05-30 14:29:07]

Description was changed from

==========
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

BUG=725241
==========

to

==========
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

Change just marks the method as deprecated, but doesn't change any actual code.


BUG=725241
==========

[Eugene But (OOO till 7-30), 2017-05-30 17:49:45]

Thanks!

https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
File ios/web/public/web_state/ui/crw_native_content_provider.h (right):

https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
ios/web/public/web_state/ui/crw_native_content_provider.h:41:
isPost:(BOOL)isPost;
On 2017/05/29 10:02:07, marq (ping after 24h) wrote:
> Add "__attribute__((deprecated))" or its macro equivalent?
This throws "error: 'controllerForURL:withError:isPost:' is deprecated"
compilation error.

[sdefresne, 2017-05-31 08:19:57]

On 2017/05/30 17:49:45, Eugene But wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
> File ios/web/public/web_state/ui/crw_native_content_provider.h (right):
> 
>
https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
> ios/web/public/web_state/ui/crw_native_content_provider.h:41:
> isPost:(BOOL)isPost;
> On 2017/05/29 10:02:07, marq (ping after 24h) wrote:
> > Add "__attribute__((deprecated))" or its macro equivalent?
> This throws "error: 'controllerForURL:withError:isPost:' is deprecated"
> compilation error.

This is by design. Chrome is build with -Werror that treats all warning as
errors. As such Chrome should build without any warning.

The modus operandi for deprecation in Chrome is to just fix the clients and then
delete the old method. Usually a deprecated comment is added to the old method
with a TODO pointing to the tracking bug. That is exactly what this CL did.

[marq (ping after 24h), 2017-05-31 09:51:57]

On 2017/05/31 08:19:57, sdefresne wrote:
> On 2017/05/30 17:49:45, Eugene But wrote:
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
> > File ios/web/public/web_state/ui/crw_native_content_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/2907723003/diff/1/ios/web/public/web_state/ui...
> > ios/web/public/web_state/ui/crw_native_content_provider.h:41:
> > isPost:(BOOL)isPost;
> > On 2017/05/29 10:02:07, marq (ping after 24h) wrote:
> > > Add "__attribute__((deprecated))" or its macro equivalent?
> > This throws "error: 'controllerForURL:withError:isPost:' is deprecated"
> > compilation error.
> 
> This is by design. Chrome is build with -Werror that treats all warning as
> errors. As such Chrome should build without any warning.
> 
> The modus operandi for deprecation in Chrome is to just fix the clients and
then
> delete the old method. Usually a deprecated comment is added to the old method
> with a TODO pointing to the tracking bug. That is exactly what this CL did.

Fair enough; I know we see warnings for deprecated UIKit APIs. 

(I suspect it's not worth wiring in presubmit hooks to catch any new uses of
deprecated APIs).

[marq (ping after 24h), 2017-05-31 09:52:07]

Still LGTM!

[Eugene But (OOO till 7-30), 2017-05-31 14:44:32]

On 2017/05/31 09:52:07, marq (ping after 24h) wrote:
> Still LGTM!
Thanks for the explanation Sylvain. Rohit, Kurt, PTAL.

[kkhorimoto, 2017-05-31 21:16:55]

lgtm!

[rohitrao (ping after 24h), 2017-06-01 00:49:19]

lgtm

Is this the bug for removing native content from //web (based on the plan that
you and justin developed)?  Are we going to make this change in the old app as
well?

[Eugene But (OOO till 7-30), 2017-06-01 03:34:06]

On 2017/06/01 00:49:19, rohitrao (ping after 24h) wrote:
> lgtm
> 
> Is this the bug for removing native content from //web (based on the plan that
> you and justin developed)?  Are we going to make this change in the old app as
> well?
Thanks! Justin planned to stop using NativeContent only for NTP, and only for
the new architecture. This sounds reasonable to me and I think it's totally ok
to keep NativeContent support for the old code.

Although NTP is the most complex example of NativeContent, there are a few
others:
 - chrome://terms (which I think has already been moved to WebUI)
 - Error Pages (which can be supported by new DidFinishNavigation API)
 - Offline Mode For Reading list (which will be supported via file:// URLs;
Olivier is on board with that approach and I plan to prototype and send a doc by
the end of Q2)
 - Bookmarks/External File (which can already be supported via
WebStatePolicyDecider)

Removing NativeContent from old architecture is just too much work, and we
probably should not do that. What I'd like to do is to make sure that ios/web
provides enough API to support all these features in the new architecture.

[Eugene But (OOO till 7-30), 2017-06-01 03:34:19]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-06-01 03:34:32]

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-06-01 03:34:34]

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

[commit-bot: I haz the power, 2017-06-01 03:34:35]

CQ cannot get SignCLA result. Please try later.

[Eugene But (OOO till 7-30), 2017-06-02 16:37:18]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:37:41]

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-06-02 16:49:56]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496421438399350, "parent_rev":
"b87d80f6485276511b3c543e90cde926eed2838d", "commit_rev":
"13b6a0f2fb8045004e2cd77c7e72947bae064d02"}

[commit-bot: I haz the power, 2017-06-02 16:50:06]

Description was changed from

==========
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

Change just marks the method as deprecated, but doesn't change any actual code.


BUG=725241
==========

to

==========
Deprecated -[CRWNativeContentProvider controllerForURL:withError:isPost:]

Clients must use WebStateObserver::DidFinishNavigation and check for
NavigationContext::GetError() and NavigationContext::IsPost().

Change just marks the method as deprecated, but doesn't change any actual code.


BUG=725241

Review-Url: https://codereview.chromium.org/2907723003
Cr-Commit-Position: refs/heads/master@{#476692}
Committed:
https://chromium.googlesource.com/chromium/src/+/13b6a0f2fb8045004e2cd77c7e72...
==========

[commit-bot: I haz the power, 2017-06-02 16:50:08]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/13b6a0f2fb8045004e2cd77c7e72...