Refactoring Reload in NavigationManager and CRWWebController.

Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also adds a DCHECK to CRWWebController's |-loadWithParams:|
to make sure that it won't be called with PAGE_TRANSITION_RELOAD.

BUG=700569
Review-Url: https://codereview.chromium.org/2741413007
Cr-Commit-Position: refs/heads/master@{#457825}
Committed: https://chromium.googlesource.com/chromium/src/+/563dc4a98290952cd8058a891574bfecce24c03c

[liaoyuke, 2017-03-13 23:07:24]

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

[commit-bot: I haz the power, 2017-03-13 23:08:16]

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

[liaoyuke, 2017-03-13 23:13:19]

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

[commit-bot: I haz the power, 2017-03-13 23:14:33]

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-03-13 23:52:14]

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

[commit-bot: I haz the power, 2017-03-13 23:52:15]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-14 22:34:05]

Patchset #1 (id:1) has been deleted

[liaoyuke, 2017-03-14 22:34:11]

Patchset #1 (id:20001) has been deleted

[liaoyuke, 2017-03-14 22:53:04]

Description was changed from

==========
Make |Reload| in NavigationManager call |Reload| in CRWWebController

BUG=
==========

to

==========
Make |Reload| in NavigationManager call |Reload| in CRWWebController.

Previously, |Reload| function in NavigationManager is not really
reloading, instead, it's calling |LoadURLWithParams|, which creates a
new pending item with the Url.

This CL wraps the |Reload| function in NavigationManager to call the
|Reload| function in CRWWebController, and updates callers outside web
to call the |Reload| function in NavigationManager instead of
CRWWebController.

BUG=700569
==========

[liaoyuke, 2017-03-14 23:01:22]

liaoyuke@chromium.org changed reviewers:
+ eugenebut@chromium.org, kkhorimoto@chromium.org

[liaoyuke, 2017-03-14 23:01:22]

Hey Eugene, Kurt

PTAL.

Thank you very much!

[liaoyuke, 2017-03-14 23:03:05]

CC Sylvain as Eugene told me you are also looking into cleaning up Reload.

[kkhorimoto, 2017-03-15 00:26:53]

The CL description here is not quite accurate, since a pending item is not
created for the reload transition type.  However, CRWWebController doesn't use
the transition type to call |-reload| when appropriate.  I think a better
solution would be to update CRWWebController's |-loadWithParams:| such that
|-reload| is called for PAGE_TRANSITION_RELOAD.  This would have to occur first
in |loadWithParams:| followed by an early return to match current behavior.

Additionally, instead of the excessive indirection caused by using the public
WebState::OpenURL() function that goes all the way up to BVC, we could just use
NavigationManager::LoadURLWithParams, with params with the reload transition
type.

[liaoyuke, 2017-03-15 04:50:06]

Patchset #3 (id:80001) has been deleted

[liaoyuke, 2017-03-15 05:06:43]

Description was changed from

==========
Make |Reload| in NavigationManager call |Reload| in CRWWebController.

Previously, |Reload| function in NavigationManager is not really
reloading, instead, it's calling |LoadURLWithParams|, which creates a
new pending item with the Url.

This CL wraps the |Reload| function in NavigationManager to call the
|Reload| function in CRWWebController, and updates callers outside web
to call the |Reload| function in NavigationManager instead of
CRWWebController.

BUG=700569
==========

to

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

BUG=700569
==========

[liaoyuke, 2017-03-15 16:16:41]

PTAL.

Thanks!

[kkhorimoto, 2017-03-15 17:16:13]

lgtm after you remove the unnecessary  GetWebController()

https://codereview.chromium.org/2741413007/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2741413007/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_delegate.h:46: virtual CRWWebController*
GetWebController() = 0;
This is no longer needed, right?

[liaoyuke, 2017-03-15 17:32:01]

Patchset #4 (id:120001) has been deleted

[liaoyuke, 2017-03-15 17:32:39]

Hey Eugene,

PTAL.

Thanks!

https://codereview.chromium.org/2741413007/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2741413007/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_delegate.h:46: virtual CRWWebController*
GetWebController() = 0;
On 2017/03/15 17:16:12, kkhorimoto_ wrote:
> This is no longer needed, right?

Done.

[kkhorimoto, 2017-03-15 17:34:24]

Could you also update the CL description with the changes you made to
CRWWebController? Thanks.

[liaoyuke, 2017-03-15 17:47:00]

Description was changed from

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

BUG=700569
==========

to

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also update CRWWebController's |-loadWithParams:| such that
|-reload| is called for PAGE_TRANSITION_RELOAD.

BUG=700569
==========

[liaoyuke, 2017-03-15 17:48:16]

Done. Thank you for catching this!

On Wed, Mar 15, 2017 at 10:34 AM <kkhorimoto@chromium.org> wrote:

> Could you also update the CL description with the changes you made to
> CRWWebController? Thanks.
>
> https://codereview.chromium.org/2741413007/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Eugene But (OOO till 7-30), 2017-03-15 17:53:32]

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/geo...
File ios/chrome/browser/geolocation/omnibox_geolocation_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/geo...
ios/chrome/browser/geolocation/omnibox_geolocation_controller.mm:460:
navigationManager->Reload(web::ReloadType::NORMAL,
What is this called? check_for_repost means that FormResubmission dialog should
be presented. Is this a good place to present the dialog?

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1463: true /* check_for_repost */);
ditto

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:2002: [strongSelf
navigationManager]->Reload(web::ReloadType::NORMAL,
ditto

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/browser_view_controller.mm:3853:
webState->GetNavigationManager()->Reload(web::ReloadType::NORMAL,
ditto

https://codereview.chromium.org/2741413007/diff/140001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:600: 
Can we add more tests? Specifically tests which would catch bugs in
NavigationManagerImpl::Reload

https://codereview.chromium.org/2741413007/diff/140001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:610:
navigation_manager()->Reload(web::ReloadType::NORMAL, true);
nit: /*check_for_repost*/

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1848: ui::PAGE_TRANSITION_RELOAD)) {
This changes the way how NavigationManager::LoadURLWithParams (public web API)
works by ignoring all WebLoadParams except originalParams.transition_type. Is
this new behavior the same as content behavior?

[kkhorimoto, 2017-03-15 18:04:11]

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1848: ui::PAGE_TRANSITION_RELOAD)) {
On 2017/03/15 17:53:32, Eugene But wrote:
> This changes the way how NavigationManager::LoadURLWithParams (public web API)
> works by ignoring all WebLoadParams except originalParams.transition_type. Is
> this new behavior the same as content behavior?

This doesn't actually change how we handle |originalParams|, since the RELOAD
page transition type goes through the "if (forwardBack)" condition below, where
we aren't actually adding a new NavigationItem.  Going through reload instead of
the old flow goes through |-reloadInternal|, allowing us to A) properly record
the reload UMA, B) behave correctly for reloads during interstitials, and C)
call the native controller's |-reload| instead of creating a new native
controller.  I'm not sure exactly if using the transition type and ignoring the
URLs in the params is correct, but this seems like a step in the right
direction.

[Eugene But (OOO till 7-30), 2017-03-15 18:07:11]

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1848: ui::PAGE_TRANSITION_RELOAD)) {
On 2017/03/15 18:04:11, kkhorimoto_ wrote:
> On 2017/03/15 17:53:32, Eugene But wrote:
> > This changes the way how NavigationManager::LoadURLWithParams (public web
API)
> > works by ignoring all WebLoadParams except originalParams.transition_type.
Is
> > this new behavior the same as content behavior?
> 
> This doesn't actually change how we handle |originalParams|, since the RELOAD
> page transition type goes through the "if (forwardBack)" condition below,
where
> we aren't actually adding a new NavigationItem.  Going through reload instead
of
> the old flow goes through |-reloadInternal|, allowing us to A) properly record
> the reload UMA, B) behave correctly for reloads during interstitials, and C)
> call the native controller's |-reload| instead of creating a new native
> controller.  I'm not sure exactly if using the transition type and ignoring
the
> URLs in the params is correct, but this seems like a step in the right
> direction.
Thanks! If current behavior is preserved then we don't need to look into
content.

[liaoyuke, 2017-03-16 16:22:20]

Actually, the fact that we are ignoring all WebLoadParams except
originalParams.transition_type is making me quite uncomfortable, which sounds
more like a hacking instead of a solution.

How about we add a |Reload| to NavigationManagerDelegate, which calls the
|-reload| of WebController, and have |Reload| in NavigationManager directly call
|Reload| in NavigationManagerDelegate?

This way, the new flow also goes through |-reloadInternal|, and also avoids
exposing WebController to code outside web.

I uploaded a new tentative patch that details the change.

Let me know how you think about this approach?

Thanks!

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/geo...
File ios/chrome/browser/geolocation/omnibox_geolocation_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/geo...
ios/chrome/browser/geolocation/omnibox_geolocation_controller.mm:460:
navigationManager->Reload(web::ReloadType::NORMAL,
On 2017/03/15 17:53:32, Eugene But wrote:
> What is this called? check_for_repost means that FormResubmission dialog
should
> be presented. Is this a good place to present the dialog?

Done.

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1463: true /* check_for_repost */);
On 2017/03/15 17:53:32, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:2002: [strongSelf
navigationManager]->Reload(web::ReloadType::NORMAL,
On 2017/03/15 17:53:32, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/browser_view_controller.mm:3853:
webState->GetNavigationManager()->Reload(web::ReloadType::NORMAL,
On 2017/03/15 17:53:32, Eugene But wrote:
> ditto

Done.

[Eugene But (OOO till 7-30), 2017-03-16 17:17:47]

https://codereview.chromium.org/2741413007/diff/180001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:2003: web::ReloadType::NORMAL, false /*
check_for_repost */);
This is called from SadTab and explicitly initiated by the user, so I think
check_for_repost should be true

https://codereview.chromium.org/2741413007/diff/180001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:612: true /*
check_for_repost */);
s/true/false so this test does not fail when form repost dialog is implemented
for reloads

https://codereview.chromium.org/2741413007/diff/180001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1847: if
(PageTransitionCoreTypeIs(originalParams.transition_type,
Do you still need this? Having NavigationDelegate::Reload is reasonable to me,
but then we should not change this code. Kurt, WDYT?

https://codereview.chromium.org/2741413007/diff/180001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:136: true /* check_for_repost */);
s/true/false CWVWebView does not support form resubmission dialogs

[liaoyuke, 2017-03-16 17:58:13]

Patchset #7 (id:200001) has been deleted

[liaoyuke, 2017-03-16 18:09:40]

Patchset #7 (id:220001) has been deleted

[liaoyuke, 2017-03-16 18:10:42]

https://codereview.chromium.org/2741413007/diff/180001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:2003: web::ReloadType::NORMAL, false /*
check_for_repost */);
On 2017/03/16 17:17:47, Eugene But wrote:
> This is called from SadTab and explicitly initiated by the user, so I think
> check_for_repost should be true

Done.

https://codereview.chromium.org/2741413007/diff/180001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl_unittest.mm:612: true /*
check_for_repost */);
On 2017/03/16 17:17:47, Eugene But wrote:
> s/true/false so this test does not fail when form repost dialog is implemented
> for reloads

Done.

https://codereview.chromium.org/2741413007/diff/180001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1847: if
(PageTransitionCoreTypeIs(originalParams.transition_type,
On 2017/03/16 17:17:47, Eugene But wrote:
> Do you still need this? Having NavigationDelegate::Reload is reasonable to me,
> but then we should not change this code. Kurt, WDYT?

This might be still useful there are other callers use WebState's
|LoadURLWithParams| with a reload page transition type, and in that case, we
want them to go through CRWWebController's |-reload| as well, and I should
create a bug to clean this up once all callers are converted.

Let's wait for Kurt to weigh in.

https://codereview.chromium.org/2741413007/diff/180001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2741413007/diff/180001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:136: true /* check_for_repost */);
On 2017/03/16 17:17:47, Eugene But wrote:
> s/true/false CWVWebView does not support form resubmission dialogs

Done.

[kkhorimoto, 2017-03-16 18:13:05]

I generally like this approach, as Eugene is doing something similar for
back/forward navigations in that function.  Reloads should go through |-reload|,
and back/forward should go through |-goToItemAtIndex:|

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:351: delegate_->Reload();
We can handle |reload_type| here by updating the visible NavigaitonItem's URL to
its original URL.  However, I think we're going to need to handle the
|check_for_repost| parameter in WebStateImpl or CRWWebController, so we should
pass |check_for_repost| in this delegate callback.

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1851: }
This is no longer needed right?  How about a DCHECK to ensure that reloads don't
go through this API.

[Eugene But (OOO till 7-30), 2017-03-16 18:39:04]

lgtm

Thanks!

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_delegate.h:30: // Informs that delegate to
reload.
Instructs the delegate to reload.

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1851: }
On 2017/03/16 18:13:04, kkhorimoto_ wrote:
> This is no longer needed right?  How about a DCHECK to ensure that reloads
don't
> go through this API.
+1

https://codereview.chromium.org/2741413007/diff/240001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:135: // |check_for_repost| is false
because CWVWebView does not support form
s/form resubmission/repost form

Sorry we have specific terminology in Chrome for this, which I'm always
forgetting :(

[liaoyuke, 2017-03-17 15:32:31]

Patchset #8 (id:260001) has been deleted

[liaoyuke, 2017-03-17 15:34:08]

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

[commit-bot: I haz the power, 2017-03-17 15:34:42]

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

[liaoyuke, 2017-03-17 15:42:47]

Description was changed from

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also update CRWWebController's |-loadWithParams:| such that
|-reload| is called for PAGE_TRANSITION_RELOAD.

BUG=700569
==========

to

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also adds a DCHECK to CRWWebController's |-loadWithParams:|
to make sure that it won't be called with PAGE_TRANSITION_RELOAD.

BUG=700569
==========

[commit-bot: I haz the power, 2017-03-17 15:51:52]

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

[commit-bot: I haz the power, 2017-03-17 15:51:53]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-17 17:56:40]

Patchset #9 (id:300001) has been deleted

[liaoyuke, 2017-03-17 17:59:21]

Thank you very much for reviewing! :)

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_delegate.h:30: // Informs that delegate to
reload.
On 2017/03/16 18:39:04, Eugene But wrote:
> Instructs the delegate to reload.

Done.

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/navigation/nav...
ios/web/navigation/navigation_manager_impl.mm:351: delegate_->Reload();
On 2017/03/16 18:13:04, kkhorimoto_ wrote:
> We can handle |reload_type| here by updating the visible NavigaitonItem's URL
to
> its original URL.  However, I think we're going to need to handle the
> |check_for_repost| parameter in WebStateImpl or CRWWebController, so we should
> pass |check_for_repost| in this delegate callback.

Since it's still unclear where we want to handle |check_for_repost|, I think we
should keep the APIs simple for now to avoid potential confusions, and add
necessary parameters later when actually implement it.

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1851: }
On 2017/03/16 18:39:04, Eugene But wrote:
> On 2017/03/16 18:13:04, kkhorimoto_ wrote:
> > This is no longer needed right?  How about a DCHECK to ensure that reloads
> don't
> > go through this API.
> +1

Done.

https://codereview.chromium.org/2741413007/diff/240001/ios/web_view/internal/...
File ios/web_view/internal/cwv_web_view.mm (right):

https://codereview.chromium.org/2741413007/diff/240001/ios/web_view/internal/...
ios/web_view/internal/cwv_web_view.mm:135: // |check_for_repost| is false
because CWVWebView does not support form
On 2017/03/16 18:39:04, Eugene But wrote:
> s/form resubmission/repost form
> 
> Sorry we have specific terminology in Chrome for this, which I'm always
> forgetting :(

Done.

[liaoyuke, 2017-03-17 18:08:44]

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

[commit-bot: I haz the power, 2017-03-17 18:09:14]

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-03-17 18:24:33]

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

[commit-bot: I haz the power, 2017-03-17 18:24:34]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-17 18:26:40]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-03-17 18:26:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2741413007/#ps320001
(title: "Rebase and Remove DCHECK in |loadWithParams| as it has already been
taken care of")

[commit-bot: I haz the power, 2017-03-17 18:27:24]

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-03-17 18:36:36]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1489775200074000,
"parent_rev": "10c77a53e77e2ae475a00cbddb5da81f0f2965c4", "commit_rev":
"563dc4a98290952cd8058a891574bfecce24c03c"}

[commit-bot: I haz the power, 2017-03-17 18:37:20]

Description was changed from

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also adds a DCHECK to CRWWebController's |-loadWithParams:|
to make sure that it won't be called with PAGE_TRANSITION_RELOAD.

BUG=700569
==========

to

==========
Refactoring Reload in NavigationManager and CRWWebController.

Previously, the |Reload| function in NavigationManager goes all the way
up to BrowserViewController to call web state's |openURLWithParams|, 
which is completely unnecessary. Additionally, CRWWebController doesn't
use the transition type to call |-reload| when appropriate.

This CL first eliminates the excessive indirections by changing the 
implementations of |Reload| function in NavigationManager to call 
|LoadURLWithParams| directly and then updates callers of 
CRWWebController's |Reload| to call NavigationManager's |Reload| to 
correct layer violations.

This CL also adds a DCHECK to CRWWebController's |-loadWithParams:|
to make sure that it won't be called with PAGE_TRANSITION_RELOAD.

BUG=700569

Review-Url: https://codereview.chromium.org/2741413007
Cr-Commit-Position: refs/heads/master@{#457825}
Committed:
https://chromium.googlesource.com/chromium/src/+/563dc4a98290952cd8058a891574...
==========

[commit-bot: I haz the power, 2017-03-17 18:37:22]

Committed patchset #9 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/563dc4a98290952cd8058a891574...