Reland of Set user agent type of transient item the same as pending item

Reland of Set user agent type of transient item the same as pending item

The original CL assumes that a pending item should always exists when
adding a transient item, however this sometimes breaks because as
described in crbug.com/676129, NavigationManagerImpl or
CRWSessionController don't create a pending item if its URL is the same
as the last committed item.

This CL fixes the problem by relaxing the condition to the following:
if pending item exists, use pending item's user agent type, otherwise
use last committed non-native item's user agent type.

BUG=676129
Review-Url: https://codereview.chromium.org/2805453002
Cr-Commit-Position: refs/heads/master@{#462271}
Committed: https://chromium.googlesource.com/chromium/src/+/f2ccf5b200be3027dd7b67346574689fc483500f

[liaoyuke, 2017-04-05 17:52:53]

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

[liaoyuke, 2017-04-05 17:52:54]

PTAL.

Thank you very much!

[kkhorimoto, 2017-04-05 18:16:08]

lgtm

[Eugene But (OOO till 7-30), 2017-04-05 20:12:28]

https://codereview.chromium.org/2805453002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2805453002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:179: if (GetPendingItem()) {
Could you please avoid code duplication. Sothing like this:

NavigationItem* item = GetPendingItem();
// TODO: ...
if (!item) {
  item = GetLastCommittedNonAppSpecificItem();
}
GetTransientItem()->SetUserAgentType(item->GetUserAgentType());

[liaoyuke, 2017-04-05 20:28:02]

Patchset #3 (id:40001) has been deleted

[liaoyuke, 2017-04-05 20:29:40]

PTAL.

Thanks!

https://codereview.chromium.org/2805453002/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2805453002/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:179: if (GetPendingItem()) {
On 2017/04/05 20:12:28, Eugene But wrote:
> Could you please avoid code duplication. Sothing like this:
> 
> NavigationItem* item = GetPendingItem();
> // TODO: ...
> if (!item) {
>   item = GetLastCommittedNonAppSpecificItem();
> }
> GetTransientItem()->SetUserAgentType(item->GetUserAgentType());

Done.

[liaoyuke, 2017-04-05 22:03:29]

PTAL.

Thanks!

[Eugene But (OOO till 7-30), 2017-04-05 22:05:39]

lgtm!

https://codereview.chromium.org/2805453002/diff/60001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2805453002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:176: // TODO(crbug.com/676129):
Add a DCHECK that pending item should always exists
There is no need for DCHECK, because when crbug.com/676129 is fined then code
should look like this:

GetPendingItem()->GetUserAgentType() // no need to DCHECK something that will
crash anyway. In TODO comment, please explain that |item| can be null because of
the bug and the workaround should be removed.

https://codereview.chromium.org/2805453002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:183: if (item)
When can |item| be null?

[liaoyuke, 2017-04-05 22:20:13]

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

[commit-bot: I haz the power, 2017-04-05 22:21:13]

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-04-05 22:26:51]

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

[commit-bot: I haz the power, 2017-04-05 22:26:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[liaoyuke, 2017-04-05 22:44:35]

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

[commit-bot: I haz the power, 2017-04-05 22:45:23]

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-04-05 23:01:31]

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

[commit-bot: I haz the power, 2017-04-05 23:01:31]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-04-05 23:17:32]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-04-05 23:17:33]

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

[commit-bot: I haz the power, 2017-04-05 23:18:23]

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-04-05 23:30:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491434252731880,
"parent_rev": "369fddb8128986e11c9485342635999297d97e96", "commit_rev":
"f2ccf5b200be3027dd7b67346574689fc483500f"}

[commit-bot: I haz the power, 2017-04-05 23:31:50]

Description was changed from

==========
Reland of Set user agent type of transient item the same as pending item

The original CL assumes that a pending item should always exists when
adding a transient item, however this sometimes breaks because as
described in crbug.com/676129, NavigationManagerImpl or
CRWSessionController don't create a pending item if its URL is the same
as the last committed item.

This CL fixes the problem by relaxing the condition to the following:
if pending item exists, use pending item's user agent type, otherwise
use last committed non-native item's user agent type.

BUG=676129
==========

to

==========
Reland of Set user agent type of transient item the same as pending item

The original CL assumes that a pending item should always exists when
adding a transient item, however this sometimes breaks because as
described in crbug.com/676129, NavigationManagerImpl or
CRWSessionController don't create a pending item if its URL is the same
as the last committed item.

This CL fixes the problem by relaxing the condition to the following:
if pending item exists, use pending item's user agent type, otherwise
use last committed non-native item's user agent type.

BUG=676129

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

[commit-bot: I haz the power, 2017-04-05 23:31:51]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f2ccf5b200be3027dd7b67346574...