Add way to reload the page, ignoring the cache

Add way to reload the page, ignoring the cache

Introduce a way to call NavigationControllerImpl::ReloadIgnoringCache()
from the Java-side TabBase.

Also, consolidate reload() functions.

BUG=307333
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233498

[gone, 2013-10-30 20:57:40]

Does this function look alright to you?  I'm not sure what, if any, hooks we
need to retain from the regular reload case.

[David Trainor- moved to gerrit, 2013-10-31 17:41:19]

lgtm.  Should we make this another version of reload like below?

reload() {
  reload(false);
}

reload(boolean force) {
...
}

[gone, 2013-10-31 18:12:39]

Combined the reload functions.

+tedchoc@chromium.org for OWNERS

[gone, 2013-11-05 22:13:14]

Ping, poke, prod

[Ted C, 2013-11-05 22:24:21]

https://chromiumcodereview.appspot.com/49353008/diff/110001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/49353008/diff/110001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1224:
public void reload() {
can we remove this method?  It's only used upstream so it should be possible to
update all references without doing a 18 part change.  (same for reload in
ContentView)

If that seems un-offensive to you, then can you add a TODO to remove reload on
TabBase as well as soon as the downstream versions switch to the other reload
method exclusively?

[gone, 2013-11-06 00:54:52]

https://chromiumcodereview.appspot.com/49353008/diff/110001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/49353008/diff/110001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1224:
public void reload() {
On 2013/11/05 22:24:21, Ted C wrote:
> can we remove this method?  It's only used upstream so it should be possible
to
> update all references without doing a 18 part change.  (same for reload in
> ContentView)
> 
> If that seems un-offensive to you, then can you add a TODO to remove reload on
> TabBase as well as soon as the downstream versions switch to the other reload
> method exclusively?

Removed this, added TODO.

[gone, 2013-11-06 19:45:33]

joth@chromium.org: Please review changes in 

android_webview

[joth, 2013-11-06 19:57:27]

https://codereview.chromium.org/49353008/diff/380001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/49353008/diff/380001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:32: import
android.webkit.WebView;
is this really needed? (I think it's just being brought in for some javadoc).
Generally trying not to import it here to avoid circular dependency. On balance
I rather fix or remove the link in javadoc than add an import purely for doc
sake.


(but thanks for the other fix ups. It's clear we've all given up on eclipse!)

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1208: else if
(web_contents_->GetController().NeedsReload())
nit: use {} on if else ladder

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1209:
web_contents_->GetController().LoadIfNecessary();
this is counter intuitive: if we "need a reload" we load, otherwise we reload.

add a comment to explain overall reasoning on this function?

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1233:
public void reload(boolean ignoreCache) {
There were plans afoot to make ContentViewCore be more like a java mirror of
WebContents etc. (Ted, Yaron, Alex may fill in more)

On that basis you may want to add reloadIgnoringCache() here and reserve passing
a single 'bool' parameter for the 'check for repost' toggle.
(that would also avoid need for many the edits in this CL....)

[joth, 2013-11-06 19:58:18]

+kristianm fyi

[gone, 2013-11-06 21:24:34]

https://codereview.chromium.org/49353008/diff/380001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/49353008/diff/380001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:32: import
android.webkit.WebView;
On 2013/11/06 19:57:28, joth wrote:
> is this really needed? (I think it's just being brought in for some javadoc).
> Generally trying not to import it here to avoid circular dependency. On
balance
> I rather fix or remove the link in javadoc than add an import purely for doc
> sake.
> 
> 
> (but thanks for the other fix ups. It's clear we've all given up on eclipse!)

Taking that import back out... there's a lot of places where @see *WebView# is
used in the javadocs.

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1208: else if
(web_contents_->GetController().NeedsReload())
On 2013/11/06 19:57:28, joth wrote:
> nit: use {} on if else ladder

Done.

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1209:
web_contents_->GetController().LoadIfNecessary();
On 2013/11/06 19:57:28, joth wrote:
> this is counter intuitive: if we "need a reload" we load, otherwise we reload.
> 
> add a comment to explain overall reasoning on this function?

Don't know the reasoning :/  I do know that the comment is out of date, though,
so that got taken out.

dtrainor/tedchoc: Should the ignore cache reload be part of the else clause
here, instead?  Seems like the NeedsReload() call indicates that the navigation
controller is about to restore a session, clone a tab, or had their renderers
killed.

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1233:
public void reload(boolean ignoreCache) {
On 2013/11/06 19:57:28, joth wrote:
> There were plans afoot to make ContentViewCore be more like a java mirror of
> WebContents etc. (Ted, Yaron, Alex may fill in more)
> 
> On that basis you may want to add reloadIgnoringCache() here and reserve
passing
> a single 'bool' parameter for the 'check for repost' toggle.
> (that would also avoid need for many the edits in this CL....)

I think that's how an older patch set was?  Ted asked me to remove the plain
reload() function.

[Ted C, 2013-11-06 21:38:13]

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1209:
web_contents_->GetController().LoadIfNecessary();
On 2013/11/06 21:24:35, dfalcantara wrote:
> On 2013/11/06 19:57:28, joth wrote:
> > this is counter intuitive: if we "need a reload" we load, otherwise we
reload.
> > 
> > add a comment to explain overall reasoning on this function?
> 
> Don't know the reasoning :/  I do know that the comment is out of date,
though,
> so that got taken out.
> 
> dtrainor/tedchoc: Should the ignore cache reload be part of the else clause
> here, instead?  Seems like the NeedsReload() call indicates that the
navigation
> controller is about to restore a session, clone a tab, or had their renderers
> killed.

At least in the past, Reload didn't work for session restored tabs, so we needed
to branch like this.  Here's the change where it was introduced (by me, but I
must admit I don't really recall more indepth reasoning behind that):
https://chromiumcodereview.appspot.com/11293025/

Now that you bring this up, I don't know if the NeedsReload/LoadIfNecessary will
do a cache ignoring load or not, but maybe this check should go first.

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1233:
public void reload(boolean ignoreCache) {
On 2013/11/06 21:24:35, dfalcantara wrote:
> On 2013/11/06 19:57:28, joth wrote:
> > There were plans afoot to make ContentViewCore be more like a java mirror of
> > WebContents etc. (Ted, Yaron, Alex may fill in more)
> > 
> > On that basis you may want to add reloadIgnoringCache() here and reserve
> passing
> > a single 'bool' parameter for the 'check for repost' toggle.
> > (that would also avoid need for many the edits in this CL....)
> 
> I think that's how an older patch set was?  Ted asked me to remove the plain
> reload() function.

WebContents doesn't actually export a reload function.  It's on the navigation
controller.  Personally, I prefer fewer methods that cascade into each other if
the usages are very small.

[joth, 2013-11-06 22:17:43]

On 2013/11/06 21:38:13, Ted C wrote:
>
https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:1209:
> web_contents_->GetController().LoadIfNecessary();
> On 2013/11/06 21:24:35, dfalcantara wrote:
> > On 2013/11/06 19:57:28, joth wrote:
> > > this is counter intuitive: if we "need a reload" we load, otherwise we
> reload.
> > > 
> > > add a comment to explain overall reasoning on this function?
> > 
> > Don't know the reasoning :/  I do know that the comment is out of date,
> though,
> > so that got taken out.
> > 
> > dtrainor/tedchoc: Should the ignore cache reload be part of the else clause
> > here, instead?  Seems like the NeedsReload() call indicates that the
> navigation
> > controller is about to restore a session, clone a tab, or had their
renderers
> > killed.
> 
> At least in the past, Reload didn't work for session restored tabs, so we
needed
> to branch like this.  Here's the change where it was introduced (by me, but I
> must admit I don't really recall more indepth reasoning behind that):
> https://chromiumcodereview.appspot.com/11293025/
> 
> Now that you bring this up, I don't know if the NeedsReload/LoadIfNecessary
will
> do a cache ignoring load or not, but maybe this check should go first.
> 
>
https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1233:
> public void reload(boolean ignoreCache) {
> On 2013/11/06 21:24:35, dfalcantara wrote:
> > On 2013/11/06 19:57:28, joth wrote:
> > > There were plans afoot to make ContentViewCore be more like a java mirror
of
> > > WebContents etc. (Ted, Yaron, Alex may fill in more)
> > > 
> > > On that basis you may want to add reloadIgnoringCache() here and reserve
> > passing
> > > a single 'bool' parameter for the 'check for repost' toggle.
> > > (that would also avoid need for many the edits in this CL....)
> > 
> > I think that's how an older patch set was?  Ted asked me to remove the plain
> > reload() function.
> 
> WebContents doesn't actually export a reload function.  It's on the navigation
> controller.

Right. I maybe wrong but I recalled the discussion back along was to eventually
extract the java analogs of such methods out of ContentViewCore into its own
NavigationController.java ?


> Personally, I prefer fewer methods that cascade into each other if
> the usages are very small.

Yeah I agree in general (i.e. if designing new API). I think here I value
consistency more. Just seems reload(true) in java has such a different meaning
to reload(true) in c++ this way, it will get difficult to remember which is
which.

Anyway.... don't let me hold this patch up more. PS9 a_w/ lgtm

[Ted C, 2013-11-06 22:36:35]

On 2013/11/06 22:17:43, joth wrote:
> On 2013/11/06 21:38:13, Ted C wrote:
> >
>
https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
> > File content/browser/android/content_view_core_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/49353008/diff/380001/content/browser/android/...
> > content/browser/android/content_view_core_impl.cc:1209:
> > web_contents_->GetController().LoadIfNecessary();
> > On 2013/11/06 21:24:35, dfalcantara wrote:
> > > On 2013/11/06 19:57:28, joth wrote:
> > > > this is counter intuitive: if we "need a reload" we load, otherwise we
> > reload.
> > > > 
> > > > add a comment to explain overall reasoning on this function?
> > > 
> > > Don't know the reasoning :/  I do know that the comment is out of date,
> > though,
> > > so that got taken out.
> > > 
> > > dtrainor/tedchoc: Should the ignore cache reload be part of the else
clause
> > > here, instead?  Seems like the NeedsReload() call indicates that the
> > navigation
> > > controller is about to restore a session, clone a tab, or had their
> renderers
> > > killed.
> > 
> > At least in the past, Reload didn't work for session restored tabs, so we
> needed
> > to branch like this.  Here's the change where it was introduced (by me, but
I
> > must admit I don't really recall more indepth reasoning behind that):
> > https://chromiumcodereview.appspot.com/11293025/
> > 
> > Now that you bring this up, I don't know if the NeedsReload/LoadIfNecessary
> will
> > do a cache ignoring load or not, but maybe this check should go first.
> > 
> >
>
https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/49353008/diff/380001/content/public/android/j...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1233:
> > public void reload(boolean ignoreCache) {
> > On 2013/11/06 21:24:35, dfalcantara wrote:
> > > On 2013/11/06 19:57:28, joth wrote:
> > > > There were plans afoot to make ContentViewCore be more like a java
mirror
> of
> > > > WebContents etc. (Ted, Yaron, Alex may fill in more)
> > > > 
> > > > On that basis you may want to add reloadIgnoringCache() here and reserve
> > > passing
> > > > a single 'bool' parameter for the 'check for repost' toggle.
> > > > (that would also avoid need for many the edits in this CL....)
> > > 
> > > I think that's how an older patch set was?  Ted asked me to remove the
plain
> > > reload() function.
> > 
> > WebContents doesn't actually export a reload function.  It's on the
navigation
> > controller.
> 
> Right. I maybe wrong but I recalled the discussion back along was to
eventually
> extract the java analogs of such methods out of ContentViewCore into its own
> NavigationController.java ?

My memory is getting vague, but I thought it was more along the lines of
extracting it
out to a proper WebContentsView.

That being said, having the java implementation more closely match the native
counter
part probably better outweighs my hope for a smaller number of exposed APIs in
java
land.

> 
> 
> > Personally, I prefer fewer methods that cascade into each other if
> > the usages are very small.
> 
> Yeah I agree in general (i.e. if designing new API). I think here I value
> consistency more. Just seems reload(true) in java has such a different meaning
> to reload(true) in c++ this way, it will get difficult to remember which is
> which.
> 
> Anyway.... don't let me hold this patch up more. PS9 a_w/ lgtm

Agreed.  I concede defeat and offer my apologies to Dan for sending him down
this wrong
path.  If you make the APIs match the navigation controller naming, then you win
(although
I do like removing it from ContentView though).

Sorry Dan!!!

[gone, 2013-11-06 23:31:11]

Alrighty, interpreted the comments and added back reloadIgnoringCache() as a
separate function, exposing the bool checkForRepost.  This required adding true
everywhere to match the current behavior.  PTAL

[gone, 2013-11-06 23:59:50]

On 2013/11/06 23:31:11, dfalcantara wrote:
> Alrighty, interpreted the comments and added back reloadIgnoringCache() as a
> separate function, exposing the bool checkForRepost.  This required adding
true
> everywhere to match the current behavior.  PTAL

Or not.  All target strikes again. :P

[joth__google, 2013-11-06 23:59:53]

thanks!

https://chromiumcodereview.appspot.com/49353008/diff/590001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/49353008/diff/590001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1242:
public void reloadIgnoringCache(boolean checkForRepost) {
ah I wasn't necessarily implying we needed to add this boolean in this patch (if
you don't actually need it) but as you've done it, then super!

[Ted C, 2013-11-07 00:15:25]

lgtm

https://chromiumcodereview.appspot.com/49353008/diff/800001/content/browser/a...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/49353008/diff/800001/content/browser/a...
content/browser/android/content_view_core_impl.cc:1213: void
ContentViewCoreImpl::ReloadIgnoringCache(JNIEnv* env, jobject obj,
jobject obj should go on it's own line

[commit-bot: I haz the power, 2013-11-07 00:51:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/49353008/850001

[commit-bot: I haz the power, 2013-11-07 01:40:19]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-07 02:54:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/49353008/850001

[commit-bot: I haz the power, 2013-11-07 06:16:23]

Change committed as 233498