Load live version when pulling down to reload an offline page on connected network

Load live version when pulling down to reload an offline page on connected network

BUG=653713
Committed: https://crrev.com/7befde4e348af0a68997619a2d4a2f635a32dafd
Cr-Commit-Position: refs/heads/master@{#425451}

[jianli, 2016-10-13 00:17:32]

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

[jianli, 2016-10-13 00:17:37]

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

[commit-bot: I haz the power, 2016-10-13 00:17:46]

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, 2016-10-13 00:24:05]

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

[commit-bot: I haz the power, 2016-10-13 00:24:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Ted C, 2016-10-13 21:24:26]

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

https://codereview.chromium.org/2416723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:70:
public SwipeRefreshHandler(Context context, SwipeActionDelegate delegate) {
Welcome to long winded answer time...

tldr; just pass Tab in the constructor.

I think we shouldn't pass this delegate but instead we should clean up/refactor
this class (if this out of scope for you, I can take a quick stab at it).

Here's what I think we should do:
1.) Pass Tab in the constructor.
2.) Remove public setContentViewCore method (it is only called when it is
initially created).
3.) Expose a public destroy method that does the cleanup that is currently done
in setContentViewCore when null is passed.
4.) Just call reload directly on the tab.

Alternatively, we could make this class a tab helper that observes the tab and
particularly watches onContentChanged.  In that case, we could not recreate the
instance every time and actually do the disconnect and connect based on tab
states.

Also, remove the @JNINamespace("content") at the top while you're here :-)

[jianli, 2016-10-13 22:38:32]

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

[jianli, 2016-10-13 22:38:35]

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

https://codereview.chromium.org/2416723002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:70:
public SwipeRefreshHandler(Context context, SwipeActionDelegate delegate) {
On 2016/10/13 21:24:26, Ted C wrote:
> Welcome to long winded answer time...
> 
> tldr; just pass Tab in the constructor.
> 
> I think we shouldn't pass this delegate but instead we should clean
up/refactor
> this class (if this out of scope for you, I can take a quick stab at it).
> 
> Here's what I think we should do:
> 1.) Pass Tab in the constructor.
> 2.) Remove public setContentViewCore method (it is only called when it is
> initially created).
> 3.) Expose a public destroy method that does the cleanup that is currently
done
> in setContentViewCore when null is passed.
> 4.) Just call reload directly on the tab.
> 
> Alternatively, we could make this class a tab helper that observes the tab and
> particularly watches onContentChanged.  In that case, we could not recreate
the
> instance every time and actually do the disconnect and connect based on tab
> states.
> 
> Also, remove the @JNINamespace("content") at the top while you're here :-)

Done.

[commit-bot: I haz the power, 2016-10-13 22:38:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2016-10-13 22:49:56]

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

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:75:
mTab.getContentViewCore().getContext().getResources().getString(resId);
I would cache the contentview core as a class member variable.  It shouldn't
change for the lifetime of this object, but I think it's better to be safe than
sorry.

Then we can also get rid of the null check you have below in destroy.

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1982:
mSwipeRefreshHandler.destroy();
I think you still want to do this in destroyContentViewCore.  A tab can have
multiple CVCs in it's lifetime and each one has a different SwipeRefreshHandler
associated with it since it is created each time in setContentViewCore

[jianli, 2016-10-13 23:05:47]

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

[jianli, 2016-10-13 23:05:56]

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

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SwipeRefreshHandler.java:75:
mTab.getContentViewCore().getContext().getResources().getString(resId);
On 2016/10/13 22:49:56, Ted C wrote:
> I would cache the contentview core as a class member variable.  It shouldn't
> change for the lifetime of this object, but I think it's better to be safe
than
> sorry.
> 
> Then we can also get rid of the null check you have below in destroy.

Done.

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2416723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1982:
mSwipeRefreshHandler.destroy();
On 2016/10/13 22:49:56, Ted C wrote:
> I think you still want to do this in destroyContentViewCore.  A tab can have
> multiple CVCs in it's lifetime and each one has a different
SwipeRefreshHandler
> associated with it since it is created each time in setContentViewCore

Done.

[commit-bot: I haz the power, 2016-10-13 23:06:06]

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, 2016-10-13 23:43:44]

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

[commit-bot: I haz the power, 2016-10-13 23:43:45]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-14 17:30:47]

lgtm

https://codereview.chromium.org/2416723002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2416723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2363:
mSwipeRefreshHandler.destroy();
I'd still have this where it was.  CVC is destroyed above and this does depend
on it, so I think's it is better to do it before.

[jianli, 2016-10-14 19:41:36]

https://codereview.chromium.org/2416723002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2416723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2363:
mSwipeRefreshHandler.destroy();
On 2016/10/14 17:30:47, Ted C wrote:
> I'd still have this where it was.  CVC is destroyed above and this does depend
> on it, so I think's it is better to do it before.

Done.

[jianli, 2016-10-14 19:42:49]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-10-14 19:42:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2416723002/#ps60001
(title: "Address one more feedback")

[commit-bot: I haz the power, 2016-10-14 19:43:34]

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, 2016-10-14 20:20:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-14 20:22:56]

Description was changed from

==========
Load live version when pulling down to reload an offline page on connected
network

BUG=653713
==========

to

==========
Load live version when pulling down to reload an offline page on connected
network

BUG=653713

Committed: https://crrev.com/7befde4e348af0a68997619a2d4a2f635a32dafd
Cr-Commit-Position: refs/heads/master@{#425451}
==========

[commit-bot: I haz the power, 2016-10-14 20:22:57]

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