Don't create a new entry in the Nav stack when reloading a page.

Don't create a new entry in the Nav stack when reloading a page.

When we have an offline page, and we refresh it to an online page,
it would be better not to create a fresh entry in the nav stack, the
user likely has no interest in having multiple entries in the nav stack
with similar copies of the same page.

BUG=584419
Committed: https://crrev.com/4b74b06a8ba2dd9c9c410c4e310cd49ab4152f33
Cr-Commit-Position: refs/heads/master@{#378469}

[Pete Williamson, 2016-02-29 21:32:03]

petewil@chromium.org changed reviewers:
+ lizeb@chromium.org, nasko@chromium.org

[Pete Williamson, 2016-02-29 21:32:04]

LizeB, where would be a good place to move the comment that I am removing in
LoadUrlParams.java (see mail I sent you for context).

[Benoit L, 2016-02-29 22:11:46]

On 2016/02/29 21:32:04, Pete Williamson wrote:
> LizeB, where would be a good place to move the comment that I am removing in
> LoadUrlParams.java (see mail I sent you for context).

Sorry for not getting back to you earlier.
You can remove the comment, there is already a matching TODO in
CustomTabsConnection. It was added because the only use of this is a hack.
However, I think that should_replace_current_entry is not widely used in
Chromium currently, and only lightly tested (that is, I could only find one test
referring it,
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
). If this gains more users, perhaps we should look into adding tests.

[Benoit L, 2016-02-29 22:48:01]

On 2016/02/29 22:11:46, Benoit L wrote:
> On 2016/02/29 21:32:04, Pete Williamson wrote:
> > LizeB, where would be a good place to move the comment that I am removing in
> > LoadUrlParams.java (see mail I sent you for context).
> 
> Sorry for not getting back to you earlier.
> You can remove the comment, there is already a matching TODO in
> CustomTabsConnection. It was added because the only use of this is a hack.
> However, I think that should_replace_current_entry is not widely used in
> Chromium currently, and only lightly tested (that is, I could only find one
test
> referring it,
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> ). If this gains more users, perhaps we should look into adding tests.

Forgot, but non-owner lgtm FWIW.

Also, the way we test that in Custom Tabs is by checking Tab.canGoBack() in
instrumentation tests. May be worth adding one here as well.

[Pete Williamson, 2016-02-29 22:55:56]

petewil@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Pete Williamson, 2016-02-29 22:55:57]

tedchoc@chromium.org: Please review changes in LoadURLParams.java

[Ted C, 2016-02-29 23:01:21]

On 2016/02/29 22:55:57, Pete Williamson wrote:
> mailto:tedchoc@chromium.org: Please review changes in LoadURLParams.java

LoadUrlParams.java - lgtm

[nasko, 2016-02-29 23:22:59]

Just a drive by question from someone clueless about the Java side of things.

https://codereview.chromium.org/1742763002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1742763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:226:
params.setShouldReplaceCurrentEntry(true);
Where does params here come from?

[Pete Williamson, 2016-03-01 00:55:01]

Fix per Nasko.

https://codereview.chromium.org/1742763002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1742763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:226:
params.setShouldReplaceCurrentEntry(true);
On 2016/02/29 23:22:59, nasko wrote:
> Where does params here come from?

Good catch, missing part of change put back in.  This now looks like the other
change at line 134.

[Pete Williamson, 2016-03-01 00:56:56]

petewil@chromium.org changed reviewers:
+ dimich@chromium.org

[Pete Williamson, 2016-03-01 00:56:56]

Oops, I should add an owner/reviewer for OfflinePageUtils.java too.

[Dmitry Titov, 2016-03-01 01:12:59]

OfflinePageUtils.java lgtm
Not necessarily in this CL, but it needs a test.

[Pete Williamson, 2016-03-01 01:18:02]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-03-01 01:18:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1742763002/#ps60001
(title: "Add back missing part of change")

[commit-bot: I haz the power, 2016-03-01 01:18:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742763002/60001

[Pete Williamson, 2016-03-01 01:22:20]

On 2016/03/01 01:12:59, Dmitry Titov wrote:
> OfflinePageUtils.java lgtm
> Not necessarily in this CL, but it needs a test.

crbug.com/590932 added to write a test.

[commit-bot: I haz the power, 2016-03-01 01:58:42]

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

[commit-bot: I haz the power, 2016-03-01 01:58:44]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Pete Williamson, 2016-03-01 17:52:41]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-03-01 17:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742763002/60001

[commit-bot: I haz the power, 2016-03-01 18:00:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-01 18:01:12]

Description was changed from

==========
Don't create a new entry in the Nav stack when reloading a page.

When we have an offline page, and we refresh it to an online page,
it would be better not to create a fresh entry in the nav stack, the
user likely has no interest in having multiple entries in the nav stack
with similar copies of the same page.

BUG=584419
==========

to

==========
Don't create a new entry in the Nav stack when reloading a page.

When we have an offline page, and we refresh it to an online page,
it would be better not to create a fresh entry in the nav stack, the
user likely has no interest in having multiple entries in the nav stack
with similar copies of the same page.

BUG=584419

Committed: https://crrev.com/4b74b06a8ba2dd9c9c410c4e310cd49ab4152f33
Cr-Commit-Position: refs/heads/master@{#378469}
==========

[commit-bot: I haz the power, 2016-03-01 18:01:13]

Patchset 4 (id:??) landed as
https://crrev.com/4b74b06a8ba2dd9c9c410c4e310cd49ab4152f33
Cr-Commit-Position: refs/heads/master@{#378469}