change navigation initiation type default value to RENDERER_INITIATED

Change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
Review-Url: https://codereview.chromium.org/2706173003
Cr-Commit-Position: refs/heads/master@{#452140}
Committed: https://chromium.googlesource.com/chromium/src/+/01ce4225edd03c523175bb2c6b6cf0af8d5e9c86

[liaoyuke, 2017-02-21 18:28:19]

Description was changed from

==========
change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
==========

to

==========
change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
==========

[liaoyuke, 2017-02-21 18:28:19]

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

[liaoyuke, 2017-02-21 18:28:32]

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

[liaoyuke, 2017-02-21 18:29:11]

Hi Eugene, Kurt,

PTAL.

Thank you very much!

[commit-bot: I haz the power, 2017-02-21 18:29:25]

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-02-21 18:44:39]

lgtm

[kkhorimoto, 2017-02-21 18:53:19]

not lgtm

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

https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.mm:312:
web::NavigationInitiationType::RENDERER_INITIATED);
This should remain as USER_INITIATED.  Once a RENDERER_INITIATED navigation is
committed, we show its URL in the omnibox.  Any back/forward navigations to that
NavigationItem should treat it as USER_INITIATED because the user has already
seen the URL and is (presumably) consciously navigating back to it, so it should
be used as the visible item.

[Eugene But (OOO till 7-30), 2017-02-21 19:03:40]

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

https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.mm:312:
web::NavigationInitiationType::RENDERER_INITIATED);
On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> Any back/forward navigations to that NavigationItem should treat it as
USER_INITIATED because the user has already
> seen the URL and is (presumably) consciously navigating back to it, so it
should
> be used as the visible item.
Is this actually true? During back forward navigation chrome should not show
pending URL in the omnibox:
https://docs.google.com/a/google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XEr...

[kkhorimoto, 2017-02-21 19:28:11]

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

https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.mm:312:
web::NavigationInitiationType::RENDERER_INITIATED);
On 2017/02/21 19:03:40, Eugene But wrote:
> On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> > Any back/forward navigations to that NavigationItem should treat it as
> USER_INITIATED because the user has already
> > seen the URL and is (presumably) consciously navigating back to it, so it
> should
> > be used as the visible item.
> Is this actually true? During back forward navigation chrome should not show
> pending URL in the omnibox:
>
https://docs.google.com/a/google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XEr...

That was at least the intention of this function when it was created.  Maybe
after your refactor this function isn't required anymore?  It definitely seems
wrong to set every NavigationItem to RENDERER_INITIATED upon committing.

[Eugene But (OOO till 7-30), 2017-02-21 19:33:29]

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

https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.mm:312:
web::NavigationInitiationType::RENDERER_INITIATED);
On 2017/02/21 19:28:11, kkhorimoto_ wrote:
> On 2017/02/21 19:03:40, Eugene But wrote:
> > On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> > > Any back/forward navigations to that NavigationItem should treat it as
> > USER_INITIATED because the user has already
> > > seen the URL and is (presumably) consciously navigating back to it, so it
> > should
> > > be used as the visible item.
> > Is this actually true? During back forward navigation chrome should not show
> > pending URL in the omnibox:
> >
>
https://docs.google.com/a/google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XEr...
> 
> That was at least the intention of this function when it was created.  Maybe
> after your refactor this function isn't required anymore?  It definitely seems
> wrong to set every NavigationItem to RENDERER_INITIATED upon committing.
NavigationEntry also marks the entry as user-initiated:
void NavigationEntryImpl::ResetForCommit(FrameNavigationEntry* frame_entry) {
So seems like we want to keep current behavior (though I still not sure why :)).

Yuke, I think creis@ would be a good person to ask why ResetForCommit behaves
this way.

[commit-bot: I haz the power, 2017-02-21 20:26:32]

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

[commit-bot: I haz the power, 2017-02-21 20:26:33]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-02-21 22:21:16]

On 2017/02/21 19:33:29, Eugene But wrote:
>
https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
> File ios/web/navigation/navigation_item_impl.mm (right):
> 
>
https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
> ios/web/navigation/navigation_item_impl.mm:312:
> web::NavigationInitiationType::RENDERER_INITIATED);
> On 2017/02/21 19:28:11, kkhorimoto_ wrote:
> > On 2017/02/21 19:03:40, Eugene But wrote:
> > > On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> > > > Any back/forward navigations to that NavigationItem should treat it as
> > > USER_INITIATED because the user has already
> > > > seen the URL and is (presumably) consciously navigating back to it, so
it
> > > should
> > > > be used as the visible item.
> > > Is this actually true? During back forward navigation chrome should not
show
> > > pending URL in the omnibox:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XEr...
> > 
> > That was at least the intention of this function when it was created.  Maybe
> > after your refactor this function isn't required anymore?  It definitely
seems
> > wrong to set every NavigationItem to RENDERER_INITIATED upon committing.
> NavigationEntry also marks the entry as user-initiated:
> void NavigationEntryImpl::ResetForCommit(FrameNavigationEntry* frame_entry) {
> So seems like we want to keep current behavior (though I still not sure why
:)).
> 
> Yuke, I think creis@ would be a good person to ask why ResetForCommit behaves
> this way.

hmm, this is tricky. I just talked to creis@, and he says when
"is_renderer_initiated" is set to false, it means that "is_renderer_initiated"
is cleared, and does not support querying anymore.

Personally, I think this is super confusing as it is more intuitive to
understand the negate of "is renderer initiated" as "is user initiated" instead
of "cleared".

Should we add a NONE to NavigationInitiationType to avoid confusion?

[Eugene But (OOO till 7-30), 2017-02-21 22:31:11]

On 2017/02/21 22:21:16, liaoyuke wrote:
> On 2017/02/21 19:33:29, Eugene But wrote:
> >
>
https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
> > File ios/web/navigation/navigation_item_impl.mm (right):
> > 
> >
>
https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
> > ios/web/navigation/navigation_item_impl.mm:312:
> > web::NavigationInitiationType::RENDERER_INITIATED);
> > On 2017/02/21 19:28:11, kkhorimoto_ wrote:
> > > On 2017/02/21 19:03:40, Eugene But wrote:
> > > > On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> > > > > Any back/forward navigations to that NavigationItem should treat it as
> > > > USER_INITIATED because the user has already
> > > > > seen the URL and is (presumably) consciously navigating back to it, so
> it
> > > > should
> > > > > be used as the visible item.
> > > > Is this actually true? During back forward navigation chrome should not
> show
> > > > pending URL in the omnibox:
> > > >
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XEr...
> > > 
> > > That was at least the intention of this function when it was created. 
Maybe
> > > after your refactor this function isn't required anymore?  It definitely
> seems
> > > wrong to set every NavigationItem to RENDERER_INITIATED upon committing.
> > NavigationEntry also marks the entry as user-initiated:
> > void NavigationEntryImpl::ResetForCommit(FrameNavigationEntry* frame_entry)
{
> > So seems like we want to keep current behavior (though I still not sure why
> :)).
> > 
> > Yuke, I think creis@ would be a good person to ask why ResetForCommit
behaves
> > this way.
> 
> hmm, this is tricky. I just talked to creis@, and he says when
> "is_renderer_initiated" is set to false, it means that "is_renderer_initiated"
> is cleared, and does not support querying anymore.
> 
> Personally, I think this is super confusing as it is more intuitive to
> understand the negate of "is renderer initiated" as "is user initiated"
instead
> of "cleared".
> 
> Should we add a NONE to NavigationInitiationType to avoid confusion?
Adding NONE and updating navigation_item_impl comments sounds like the right
direction to me.

[liaoyuke, 2017-02-21 22:55:18]

Patchset #2 (id:20001) has been deleted

[liaoyuke, 2017-02-21 22:56:17]

PTAL.

Thank you very much!

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

https://codereview.chromium.org/2706173003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.mm:312:
web::NavigationInitiationType::RENDERER_INITIATED);
On 2017/02/21 18:53:19, kkhorimoto_ wrote:
> This should remain as USER_INITIATED.  Once a RENDERER_INITIATED navigation is
> committed, we show its URL in the omnibox.  Any back/forward navigations to
that
> NavigationItem should treat it as USER_INITIATED because the user has already
> seen the URL and is (presumably) consciously navigating back to it, so it
should
> be used as the visible item.

Got it! Thank you for explaining!

[Eugene But (OOO till 7-30), 2017-02-21 23:00:40]

Please update SetNavigationInitiationType/NavigationInitiationType comments

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
This can be 0, because it's a good default value.

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
Needs comments

[liaoyuke, 2017-02-21 23:07:47]

PTAL.

Thanks!

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
On 2017/02/21 23:00:40, Eugene But wrote:
> This can be 0, because it's a good default value.

Done.

https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
On 2017/02/21 23:00:40, Eugene But wrote:
> Needs comments

Done.

[kkhorimoto, 2017-02-21 23:08:41]

lgtm, but please capitalize "Change" in your CL description :)

[liaoyuke, 2017-02-21 23:09:12]

Description was changed from

==========
change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
==========

to

==========
Change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
==========

[Eugene But (OOO till 7-30), 2017-02-21 23:09:54]

On 2017/02/21 23:07:47, liaoyuke wrote:
> PTAL.
> 
> Thanks!
> 
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_manager_impl.h (right):
> 
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
> On 2017/02/21 23:00:40, Eugene But wrote:
> > This can be 0, because it's a good default value.
> 
> Done.
> 
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
> On 2017/02/21 23:00:40, Eugene But wrote:
> > Needs comments
> 
> Done.
Please address my previous comment which was:
"Please update SetNavigationInitiationType/NavigationInitiationType comments"

[liaoyuke, 2017-02-21 23:17:14]

On 2017/02/21 23:09:54, Eugene But wrote:
> On 2017/02/21 23:07:47, liaoyuke wrote:
> > PTAL.
> > 
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> > File ios/web/navigation/navigation_manager_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> > ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
> > On 2017/02/21 23:00:40, Eugene But wrote:
> > > This can be 0, because it's a good default value.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2706173003/diff/40001/ios/web/navigation/navi...
> > ios/web/navigation/navigation_manager_impl.h:31: NONE = 1,
> > On 2017/02/21 23:00:40, Eugene But wrote:
> > > Needs comments
> > 
> > Done.
> Please address my previous comment which was:
> "Please update SetNavigationInitiationType/NavigationInitiationType comments"

Done. Thank you for catching it!

PTAL.

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

lgtm

[liaoyuke, 2017-02-21 23:26:21]

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

[commit-bot: I haz the power, 2017-02-21 23:28:19]

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-02-22 01:35:48]

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

[commit-bot: I haz the power, 2017-02-22 01:35:49]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[liaoyuke, 2017-02-22 18:01:39]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-02-22 18:01:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2706173003/#ps80001
(title: "Update comments again")

[commit-bot: I haz the power, 2017-02-22 18:03:09]

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-02-22 18:27:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487786499908800,
"parent_rev": "a4571ccd2f0151685e7653c2ae5fda4b859f00c3", "commit_rev":
"01ce4225edd03c523175bb2c6b6cf0af8d5e9c86"}

[commit-bot: I haz the power, 2017-02-22 18:28:06]

Description was changed from

==========
Change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789
==========

to

==========
Change navigation initiation type default value to RENDERER_INITIATED

This CL changes the default value of navigation initiation type to
RENDERER_INITIATED because USER_INITIATED has higher permissions than
RENDERER_INITIATED.

BUG=693789

Review-Url: https://codereview.chromium.org/2706173003
Cr-Commit-Position: refs/heads/master@{#452140}
Committed:
https://chromium.googlesource.com/chromium/src/+/01ce4225edd03c523175bb2c6b6c...
==========

[commit-bot: I haz the power, 2017-02-22 18:28:08]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/01ce4225edd03c523175bb2c6b6c...