created web::ReloadType

created web::ReloadType

This CL introduces web::ReloadType, which can be used to specify
detailed behavior on requesting reload via navigation manager.

BUG=

Review-Url: https://codereview.chromium.org/2742953003
Cr-Commit-Position: refs/heads/master@{#456433}
Committed: https://chromium.googlesource.com/chromium/src/+/935ed42ba809c4982618c170c62f269e7cdbc95c

[liaoyuke, 2017-03-10 17:20:55]

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

[liaoyuke, 2017-03-10 17:20:56]

PTAL.

Thank you very much!

[Eugene But (OOO till 7-30), 2017-03-10 18:37:32]

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:158: virtual void Reload(ReloadType
reload_type, bool check_for_repost) = 0;
Content reload does not have this argument. Can we keep web API aligned with
content?

[kkhorimoto, 2017-03-10 18:47:47]

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:158: virtual void Reload(ReloadType
reload_type, bool check_for_repost) = 0;
On 2017/03/10 18:37:32, Eugene But wrote:
> Content reload does not have this argument. Can we keep web API aligned with
> content?

This is because content// stores the ReloadType in the NavigationEntry, which in
our discussions yesterday we decided was not the approach we wanted to go in, as
the reload behavior is transient and doesn't need to be stored with the
navigations in our history stack.  If we wanted to go with their approach, we
would need to add ReloadType::NONE, which is something we agreed that wouldn't
really be necessary if we went with this approach

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

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:158: virtual void Reload(ReloadType
reload_type, bool check_for_repost) = 0;
On 2017/03/10 18:47:46, kkhorimoto_ wrote:
> On 2017/03/10 18:37:32, Eugene But wrote:
> > Content reload does not have this argument. Can we keep web API aligned with
> > content?
> 
> This is because content// stores the ReloadType in the NavigationEntry, which
in
> our discussions yesterday we decided was not the approach we wanted to go in,
as
> the reload behavior is transient and doesn't need to be stored with the
> navigations in our history stack.  If we wanted to go with their approach, we
> would need to add ReloadType::NONE, which is something we agreed that wouldn't
> really be necessary if we went with this approach
Sorry, I did not realize that proposed approach diverge us from content API. The
end goal is to have common API for content and web. Having less API in web is
fine, adding API which is not compatible with content should be the last resort.


Can we talk to content folks and ask if they would be fine with adding
|ReloadType| to their Reload API? It would be very unfortunate if we change
NavigationManager::Reload now and be forced to change it again later.

[liaoyuke, 2017-03-10 21:16:23]

Comments addressed offline. PTAL.

Thank you very much!

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:158: virtual void Reload(ReloadType
reload_type, bool check_for_repost) = 0;
On 2017/03/10 18:58:04, Eugene But wrote:
> On 2017/03/10 18:47:46, kkhorimoto_ wrote:
> > On 2017/03/10 18:37:32, Eugene But wrote:
> > > Content reload does not have this argument. Can we keep web API aligned
with
> > > content?
> > 
> > This is because content// stores the ReloadType in the NavigationEntry,
which
> in
> > our discussions yesterday we decided was not the approach we wanted to go
in,
> as
> > the reload behavior is transient and doesn't need to be stored with the
> > navigations in our history stack.  If we wanted to go with their approach,
we
> > would need to add ReloadType::NONE, which is something we agreed that
wouldn't
> > really be necessary if we went with this approach
> Sorry, I did not realize that proposed approach diverge us from content API.
The
> end goal is to have common API for content and web. Having less API in web is
> fine, adding API which is not compatible with content should be the last
resort.
> 
> 
> Can we talk to content folks and ask if they would be fine with adding
> |ReloadType| to their Reload API? It would be very unfortunate if we change
> NavigationManager::Reload now and be forced to change it again later.

Acknowledged.

[kkhorimoto, 2017-03-10 21:26:45]

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.h:29: enum class ReloadType : short;
optional nit: Generally for public web// interfaces that are simple typedefs or
enums, we just include the header directly above, like with
navigation_item_list.h and page_transition_types.h

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.mm:346: GURL url = item ?
item->GetURL() : GURL(url::kAboutBlankURL);
Do you want to just update the logic here in this CL?  It should be as simple as
adding an extra condition here and using the original request URL depending on
the reload type.

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:23: enum class ReloadType : short;
same comment about includes

[liaoyuke, 2017-03-10 21:42:35]

PTAL

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.h:29: enum class ReloadType : short;
On 2017/03/10 21:26:45, kkhorimoto_ wrote:
> optional nit: Generally for public web// interfaces that are simple typedefs
or
> enums, we just include the header directly above, like with
> navigation_item_list.h and page_transition_types.h

Done. For consistency.

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.mm:346: GURL url = item ?
item->GetURL() : GURL(url::kAboutBlankURL);
On 2017/03/10 21:26:45, kkhorimoto_ wrote:
> Do you want to just update the logic here in this CL?  It should be as simple
as
> adding an extra condition here and using the original request URL depending on
> the reload type.

The |Reload| function in navigation manager is not doing reload, instead, it
calls LoadURLWithParams, which is wrong. I want to fix the problem first before
adding the user agent logic to avoid any surprising behaviors.

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:23: enum class ReloadType : short;
On 2017/03/10 21:26:45, kkhorimoto_ wrote:
> same comment about includes

Done.

[kkhorimoto, 2017-03-10 22:24:59]

lgtm.

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2742953003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.mm:346: GURL url = item ?
item->GetURL() : GURL(url::kAboutBlankURL);
On 2017/03/10 21:42:34, liaoyuke wrote:
> On 2017/03/10 21:26:45, kkhorimoto_ wrote:
> > Do you want to just update the logic here in this CL?  It should be as
simple
> as
> > adding an extra condition here and using the original request URL depending
on
> > the reload type.
> 
> The |Reload| function in navigation manager is not doing reload, instead, it
> calls LoadURLWithParams, which is wrong. I want to fix the problem first
before
> adding the user agent logic to avoid any surprising behaviors.

Okay that's fine.  Can you add the details of how you're going to refactor
Reload() to your design doc + add a TODO to fix the reload type?

[liaoyuke, 2017-03-10 23:34:20]

Updated design doc and added TODO. PTAL.

Thanks!

[Eugene But (OOO till 7-30), 2017-03-11 00:33:36]

lgtm

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/inte...
File ios/chrome/browser/interstitials/ios_chrome_controller_client.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/inte...
ios/chrome/browser/interstitials/ios_chrome_controller_client.mm:58:
web_state_->GetNavigationManager()->Reload(web::ReloadType::NORMAL, true);
Could you please add |true /*check_for_repost*/| inline comment, since you
touching this

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298:
navigationManager->Reload(web::ReloadType::NORMAL, false);
ditto

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:92:
_webState->GetNavigationManager()->Reload(web::ReloadType::NORMAL, false);
ditto

https://codereview.chromium.org/2742953003/diff/40001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:159: // ORIGINAL_REQUEST_URL.
Since you touching this, could you please add a bug and TODO to actually
implement or remove |check_for_repost|.

[liaoyuke, 2017-03-13 16:40:06]

Thank you very much for reviewing!

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/inte...
File ios/chrome/browser/interstitials/ios_chrome_controller_client.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/inte...
ios/chrome/browser/interstitials/ios_chrome_controller_client.mm:58:
web_state_->GetNavigationManager()->Reload(web::ReloadType::NORMAL, true);
On 2017/03/11 00:33:36, Eugene But wrote:
> Could you please add |true /*check_for_repost*/| inline comment, since you
> touching this

Done.

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:298:
navigationManager->Reload(web::ReloadType::NORMAL, false);
On 2017/03/11 00:33:36, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:92:
_webState->GetNavigationManager()->Reload(web::ReloadType::NORMAL, false);
On 2017/03/11 00:33:36, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2742953003/diff/40001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2742953003/diff/40001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:159: // ORIGINAL_REQUEST_URL.
On 2017/03/11 00:33:36, Eugene But wrote:
> Since you touching this, could you please add a bug and TODO to actually
> implement or remove |check_for_repost|.

Done.

[liaoyuke, 2017-03-13 16:43:27]

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

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

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 18:14:31]

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

[commit-bot: I haz the power, 2017-03-13 18:14:32]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-13 18:15:24]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-03-13 18:15:24]

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/2742953003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-13 18:16:14]

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 18:21:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489428924462170,
"parent_rev": "5dec7437ab22a52aa3dcab8e3415142b6db89040", "commit_rev":
"935ed42ba809c4982618c170c62f269e7cdbc95c"}

[commit-bot: I haz the power, 2017-03-13 18:22:52]

Description was changed from

==========
created web::ReloadType

This CL introduces web::ReloadType, which can be used to specify
detailed behavior on requesting reload via navigation manager.

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

to

==========
created web::ReloadType

This CL introduces web::ReloadType, which can be used to specify
detailed behavior on requesting reload via navigation manager.

BUG=

Review-Url: https://codereview.chromium.org/2742953003
Cr-Commit-Position: refs/heads/master@{#456433}
Committed:
https://chromium.googlesource.com/chromium/src/+/935ed42ba809c4982618c170c62f...
==========

[commit-bot: I haz the power, 2017-03-13 18:22:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/935ed42ba809c4982618c170c62f...