Make pull to refresh not perform regular reload (with cache revalidation)

Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh)
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
Committed: https://crrev.com/1be5023c6d5cefd95ba73830f506f00a436c1316
Cr-Commit-Position: refs/heads/master@{#365413}

[kinuko, 2015-12-07 09:03:18]

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

[commit-bot: I haz the power, 2015-12-07 09:04:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493403002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493403002/20001

[commit-bot: I haz the power, 2015-12-07 09:39:29]

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

[commit-bot: I haz the power, 2015-12-07 09:39:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[kinuko, 2015-12-09 06:21:21]

Description was changed from

==========
pull to refresh

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

to

==========
pull to refresh

BUG=558829
==========

[kinuko, 2015-12-10 08:19:29]

Description was changed from

==========
pull to refresh

BUG=558829
==========

to

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

[kinuko, 2015-12-10 08:19:43]

Patchset #2 (id:20001) has been deleted

[kinuko, 2015-12-10 08:57:14]

kinuko@chromium.org changed reviewers:
+ nasko@chromium.org

[kinuko, 2015-12-11 04:33:25]

Description was changed from

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

to

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

[kinuko, 2015-12-11 04:35:21]

Hi Nasko, do you think you could review this?  The main change is in
navigation_controller_impl.cc and other changes are just for plumbing. If the
issue description doesn't give enough context please feel free to ping. Thanks!

[nasko, 2015-12-14 23:28:54]

nasko@chromium.org changed reviewers:
+ avi@chromium.org

[nasko, 2015-12-14 23:28:54]

Hey Avi,
Can you help looking at this one? I want to make sure we are not blocking
kinuko@ and I have a big backlog of reviews :(.
Thanks!
Nasko

[Avi (use Gerrit), 2015-12-15 21:34:19]

lgtm

Looks reasonable. You're looking to remove the flag once things settle down?

[kinuko, 2015-12-15 22:06:11]

On 2015/12/15 21:34:19, Avi wrote:
> lgtm
> 
> Looks reasonable. You're looking to remove the flag once things settle down?

Thanks! Yes I'll be removing the flag once settled.

[kinuko, 2015-12-15 22:08:31]

kinuko@chromium.org changed reviewers:
+ bauerb@chromium.org

[kinuko, 2015-12-15 22:10:46]

kinuko@chromium.org changed reviewers:
+ newt@chromium.org
- bauerb@chromium.org

[kinuko, 2015-12-15 22:11:35]

newt@: would you be able to review changes in chrome/android?  Thanks,

[newt (away), 2015-12-15 22:40:22]

https://codereview.chromium.org/1493403002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java
(right):

https://codereview.chromium.org/1493403002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:95:
contentViewCore.getWebContents().getNavigationController().reloadToRefreshContent(
Instead of a reload, could we simply load the current URL again? Then we
wouldn't need a special new method. Or is that a terrible idea for some reason?

https://codereview.chromium.org/1493403002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1493403002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:297:
ReloadInternal(check_for_repost, RELOAD_IGNORING_CACHE);
Why RELOAD_IGNORING_CACHE? Seems like we want "NO_RELOAD" here, and "RELOAD"
below. Am I misunderstanding the meaning of the
kEnableNonValidatingReloadOnRefreshContent flag?

https://codereview.chromium.org/1493403002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1493403002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:358: virtual void Reload(bool
check_for_repost) = 0;
Random side question: why not have a single Reload() method that takes a
ReloadType as a parameter? Seems like that would reduce proliferation of
ReloadXyz() methods everywhere.

[kinuko, 2015-12-15 22:54:38]

The reload part had remnants of my experiments, thanks for catching it.  PTAL

https://codereview.chromium.org/1493403002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java
(right):

https://codereview.chromium.org/1493403002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:95:
contentViewCore.getWebContents().getNavigationController().reloadToRefreshContent(
On 2015/12/15 22:40:21, newt wrote:
> Instead of a reload, could we simply load the current URL again? Then we
> wouldn't need a special new method. Or is that a terrible idea for some
reason?

That'd probably work, but I thought we might still want to have logic for
avoiding repost etc.

https://codereview.chromium.org/1493403002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1493403002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:297:
ReloadInternal(check_for_repost, RELOAD_IGNORING_CACHE);
On 2015/12/15 22:40:21, newt wrote:
> Why RELOAD_IGNORING_CACHE? Seems like we want "NO_RELOAD" here, and "RELOAD"
> below. Am I misunderstanding the meaning of the
> kEnableNonValidatingReloadOnRefreshContent flag?

Oops my bad, I was experimenting multiple flags and uploaded a mess here. 
Updated.

https://codereview.chromium.org/1493403002/diff/60001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/1493403002/diff/60001/content/public/browser/...
content/public/browser/navigation_controller.h:358: virtual void Reload(bool
check_for_repost) = 0;
On 2015/12/15 22:40:21, newt wrote:
> Random side question: why not have a single Reload() method that takes a
> ReloadType as a parameter? Seems like that would reduce proliferation of
> ReloadXyz() methods everywhere.

That's what ReloadInternal does.  I can kinda imagine that we didn't want to
sprinkle the various loading enums or we probably didn't have too many when we
started adding these.  Maybe we can revisit cleaning up this

[newt (away), 2015-12-15 23:50:42]

lgtm

[kinuko, 2015-12-16 01:37:29]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-12-16 01:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1493403002/#ps80001
(title: "fix")

[commit-bot: I haz the power, 2015-12-16 01:40:36]

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

[commit-bot: I haz the power, 2015-12-16 02:32:03]

Description was changed from

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

to

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

[commit-bot: I haz the power, 2015-12-16 02:32:04]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-12-16 02:33:08]

Description was changed from

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh) 
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829
==========

to

==========
Make pull to refresh not perform regular reload (with cache revalidation)

Pull-to-refresh action on Android is currently handled as a
regular reload, which causes revalidation of cached contents,
however the action would usually imply that the user wants to
refresh the 'contents' rather than reloading everything (e.g.
to reset the page contents when something's screwed up).

This change adds new a reload method, i.e. reloadToRefreshContent,
to the navigation controller so that we can distinguish the two
reload cases.

The new behavior (i.e. non-validating reload on pull-to-refresh)
will be only enabled if the following command-line flag is given:

"--enable-non-validating-reload-on-refresh-content"

(I plan to add chrome://flags once I can land this)

BUG=558829

Committed: https://crrev.com/1be5023c6d5cefd95ba73830f506f00a436c1316
Cr-Commit-Position: refs/heads/master@{#365413}
==========

[commit-bot: I haz the power, 2015-12-16 02:33:09]

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