Removed Form Resubmission callback from CRWWebDelegate.

Removed Form Resubmission callback from CRWWebUserInterfaceDelegate.

Use WebStateDelegate::ShowRepostFormWarningDialog callback instead to
mirror WebContentsDelegate API.

BUG=674991
TEST=Forms Resubmissions work correctly.

Review-Url: https://codereview.chromium.org/2642233004
Cr-Commit-Position: refs/heads/master@{#445971}
Committed: https://chromium.googlesource.com/chromium/src/+/b739bdcc12b1d4da4e614fe153cb05cd3b9e6104

[Eugene But (OOO till 7-30), 2017-01-20 23:07:14]

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

[commit-bot: I haz the power, 2017-01-20 23:07:44]

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-21 00:50:09]

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

[commit-bot: I haz the power, 2017-01-21 00:50:10]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-01-21 01:39:02]

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

[Eugene But (OOO till 7-30), 2017-01-21 01:39:02]

I will rename Coordinator, TabHelper and EG tests to follow content naming
pattern (RepostForm instead of FormResubmission) in a separate CL. PTAL.

[marq (ping after 24h), 2017-01-23 08:18:28]

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:42: // Requests the repost form
confirmation dialog. Clients must call callback
nit: |callback|.

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate_bridge.h:34: // handler with YES to
allow repost and with NO to cancel the repost.
nit "completion handler" -> |handler|.

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge.mm:50: }
Should there be a default behavior if |delegate_| doesn't implement the method?

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_stub.mm (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_stub.mm:46: _repostFormWarningRequested =
YES;
super nit: It seems like this class is functioning more like a mock than a stub.

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

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:461: }
Default?

[Eugene But (OOO till 7-30), 2017-01-23 18:08:20]

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

[commit-bot: I haz the power, 2017-01-23 18:08:43]

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-23 19:35:29]

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

[commit-bot: I haz the power, 2017-01-23 19:35:31]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-01-23 19:48:33]

Thanks! PTAL

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:42: // Requests the repost form
confirmation dialog. Clients must call callback
On 2017/01/23 08:18:28, marq wrote:
> nit: |callback|.

Done.

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate_bridge.h (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate_bridge.h:34: // handler with YES to
allow repost and with NO to cancel the repost.
On 2017/01/23 08:18:28, marq wrote:
> nit "completion handler" -> |handler|.

Done.

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge.mm:50: }
On 2017/01/23 08:18:28, marq wrote:
> Should there be a default behavior if |delegate_| doesn't implement the
method?

Good catch. Added default behavior and updated the comment.

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_stub.mm (right):

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_stub.mm:46: _repostFormWarningRequested =
YES;
On 2017/01/23 08:18:28, marq wrote:
> super nit: It seems like this class is functioning more like a mock than a
stub.
Good point. This is a subclass of OCMockComplexTypeHelper. Filed a
crbug.com/683983 to fix this

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

https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:461: }
On 2017/01/23 08:18:28, marq wrote:
> Default? 

Done.

[Eugene But (OOO till 7-30), 2017-01-24 01:00:48]

On 2017/01/23 19:48:33, Eugene But wrote:
> Thanks! PTAL
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
> File ios/web/public/web_state/web_state_delegate.h (right):
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
> ios/web/public/web_state/web_state_delegate.h:42: // Requests the repost form
> confirmation dialog. Clients must call callback
> On 2017/01/23 08:18:28, marq wrote:
> > nit: |callback|.
> 
> Done.
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
> File ios/web/public/web_state/web_state_delegate_bridge.h (right):
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/public/web_stat...
> ios/web/public/web_state/web_state_delegate_bridge.h:34: // handler with YES
to
> allow repost and with NO to cancel the repost.
> On 2017/01/23 08:18:28, marq wrote:
> > nit "completion handler" -> |handler|.
> 
> Done.
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> File ios/web/web_state/web_state_delegate_bridge.mm (right):
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> ios/web/web_state/web_state_delegate_bridge.mm:50: }
> On 2017/01/23 08:18:28, marq wrote:
> > Should there be a default behavior if |delegate_| doesn't implement the
> method?
> 
> Good catch. Added default behavior and updated the comment.
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> File ios/web/web_state/web_state_delegate_stub.mm (right):
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> ios/web/web_state/web_state_delegate_stub.mm:46: _repostFormWarningRequested =
> YES;
> On 2017/01/23 08:18:28, marq wrote:
> > super nit: It seems like this class is functioning more like a mock than a
> stub.
> Good point. This is a subclass of OCMockComplexTypeHelper. Filed a
> crbug.com/683983 to fix this
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> File ios/web/web_state/web_state_impl.mm (right):
> 
>
https://codereview.chromium.org/2642233004/diff/20001/ios/web/web_state/web_s...
> ios/web/web_state/web_state_impl.mm:461: }
> On 2017/01/23 08:18:28, marq wrote:
> > Default? 
> 
> Done.
Rohit?

[rohitrao (ping after 24h), 2017-01-24 14:15:13]

lgtm

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge.mm:47:
runRepostFormDialogWithCompletionHandler:^(BOOL should_continue) {
This is a block that runs a callback that runs a block that runs a block? =)  I
don't have any better suggestions, it just seems unfortunate.

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge_unittest.mm (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge_unittest.mm:100: // Tests
|ShowRepostFormWarningDialog| forwarding.
Is it worth also testing that the default implementation calls callback(true),
if the delegate does not implement this method?

[marq (ping after 24h), 2017-01-24 14:46:46]

https://codereview.chromium.org/2642233004/diff/40001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:45: virtual void
ShowRepostFormWarningDialog(
Naming consistency: Should this be OnFormRepostWarning() to match
OnAuthRequired()?

[Eugene But (OOO till 7-30), 2017-01-24 21:28:40]

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

[commit-bot: I haz the power, 2017-01-24 21:29:05]

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

[Eugene But (OOO till 7-30), 2017-01-24 21:33:11]

Thanks!

https://codereview.chromium.org/2642233004/diff/40001/ios/web/public/web_stat...
File ios/web/public/web_state/web_state_delegate.h (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/public/web_stat...
ios/web/public/web_state/web_state_delegate.h:45: virtual void
ShowRepostFormWarningDialog(
On 2017/01/24 14:46:46, marq wrote:
> Naming consistency: Should this be OnFormRepostWarning() to match
> OnAuthRequired()?
ShowRepostFormWarningDialog name matches //content. I think //ios/web should be
consistent with //content, so people who know //content API can understand how
//ios/web works and vice versa.

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge.mm (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge.mm:47:
runRepostFormDialogWithCompletionHandler:^(BOOL should_continue) {
On 2017/01/24 14:15:13, rohitrao wrote:
> This is a block that runs a callback that runs a block that runs a block? =) 
I
> don't have any better suggestions, it just seems unfortunate.
Well... Objective-C and C++ callback patterns are different. There should be an
ugly place where C++ API is adapted for Objective-C language.

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_delegate_bridge_unittest.mm (right):

https://codereview.chromium.org/2642233004/diff/40001/ios/web/web_state/web_s...
ios/web/web_state/web_state_delegate_bridge_unittest.mm:100: // Tests
|ShowRepostFormWarningDialog| forwarding.
On 2017/01/24 14:15:13, rohitrao wrote:
> Is it worth also testing that the default implementation calls callback(true),
> if the delegate does not implement this method?
Yep. Added the test.

[marq (ping after 24h), 2017-01-24 21:39:25]

lgtm

[commit-bot: I haz the power, 2017-01-25 00:41:31]

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

[commit-bot: I haz the power, 2017-01-25 00:41:33]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-01-25 06:26:28]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-01-25 06:26:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2642233004/#ps60001
(title: "Added ShowRepostFormWarningWithNoDelegateMethod test")

[commit-bot: I haz the power, 2017-01-25 06:26:43]

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-25 06:32:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485325588049510,
"parent_rev": "492b051065545ec01b91f9ea0034e8563201a937", "commit_rev":
"b739bdcc12b1d4da4e614fe153cb05cd3b9e6104"}

[commit-bot: I haz the power, 2017-01-25 06:33:22]

Description was changed from

==========
Removed Form Resubmission callback from CRWWebUserInterfaceDelegate.

Use WebStateDelegate::ShowRepostFormWarningDialog callback instead to
mirror WebContentsDelegate API.

BUG=674991
TEST=Forms Resubmissions work correctly.
==========

to

==========
Removed Form Resubmission callback from CRWWebUserInterfaceDelegate.

Use WebStateDelegate::ShowRepostFormWarningDialog callback instead to
mirror WebContentsDelegate API.

BUG=674991
TEST=Forms Resubmissions work correctly.

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

[commit-bot: I haz the power, 2017-01-25 06:33:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b739bdcc12b1d4da4e614fe153cb...