Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(95)

Issue 2937973002: Add an offline dialog when launching the WebAPK needs network connection. (Closed)

Created:
3 years, 6 months ago by Xi Han
Modified:
3 years, 5 months ago
Reviewers:
dominickn, pkotwicz, Yaron
CC:
chromium-reviews, dominickn+watch_chromium.org, srahim+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add an offline dialog when launching the WebAPK needs network connection. When launching WebAPK for the first time or user cleans Chrome's data, the WebAPK can't be properly launched when the device is offline. This CL adds a offline dialog to tell the user to connect to the internet if they want to use the WebAPK. Once the network connection is back, the offline dialog will disappear and the WebAPK loads. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/view?usp=sharing Steps: 1) launch the WebAPK when the device is offline, and a dialog is shown. 2) turn off the air plane mode. Observe: The dialog disappears, and the url is loaded in the WebAPK. BUG=733241 Review-Url: https://codereview.chromium.org/2937973002 Cr-Commit-Position: refs/heads/master@{#483061} Committed: https://chromium.googlesource.com/chromium/src/+/373c86f2c5469d8d8de08437e5bcdf377ccd4a92

Patch Set 1 #

Total comments: 4

Patch Set 2 : Retry if we see the ERR_NETWORK_CHANGED. #

Total comments: 26

Patch Set 3 : pkotwicz@'s comments. #

Total comments: 16

Patch Set 4 : Nits. #

Patch Set 5 : Don't show the offline dialog when the splash screen has gone. #

Total comments: 6

Patch Set 6 : Nits. #

Total comments: 4

Patch Set 7 : Nits. #

Total comments: 16

Patch Set 8 : Nits. #

Total comments: 2

Patch Set 9 : Nits. #

Total comments: 4

Patch Set 10 : dominickn@'s comments. #

Patch Set 11 : Rebase on Peter's CL that introduces WebappSplashScreenController. #

Total comments: 4

Patch Set 12 : Nits. #

Total comments: 2

Patch Set 13 : Nits. #

Patch Set 14 : Update strings and show App name on the offline dialog instead of url. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -4 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +90 lines, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +53 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +13 lines, -3 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 62 (32 generated)
Xi Han
Hi Peter, could you please take a look? Thanks!
3 years, 6 months ago (2017-06-14 14:11:25 UTC) #8
pkotwicz
https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode91 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:91: tab.loadUrl(new LoadUrlParams(url, PageTransition.AUTO_TOPLEVEL)); On my device, when the dialog ...
3 years, 6 months ago (2017-06-14 19:12:41 UTC) #11
Xi Han
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode91 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:91: tab.loadUrl(new LoadUrlParams(url, PageTransition.AUTO_TOPLEVEL)); On 2017/06/14 ...
3 years, 6 months ago (2017-06-16 14:52:15 UTC) #16
pkotwicz
Mostly nits I did some investigation. I got lucky and noticed a few things 1) ...
3 years, 6 months ago (2017-06-16 22:37:44 UTC) #19
Xi Han
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:62: private class WebApkTabObserver extends WebappTabObserver ...
3 years, 6 months ago (2017-06-19 17:37:31 UTC) #20
pkotwicz
https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode86 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); I don't understand. Isn't the errorCode passed to ...
3 years, 6 months ago (2017-06-21 05:18:44 UTC) #21
Xi Han
+yfriedman@. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode86 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); On 2017/06/21 05:18:43, pkotwicz wrote: > I ...
3 years, 6 months ago (2017-06-21 18:39:42 UTC) #23
Yaron
https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode93 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; On 2017/06/21 ...
3 years, 6 months ago (2017-06-21 19:53:15 UTC) #28
Xi Han
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode93 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != ...
3 years, 6 months ago (2017-06-21 20:56:17 UTC) #29
Yaron
https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode84 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean hasNetowrkChanged = mErrorCode == NetError.ERR_NETWORK_CHANGED; hasNetworkChanged https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode109 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:109: ...
3 years, 6 months ago (2017-06-21 21:05:21 UTC) #30
Xi Han
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode84 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean hasNetowrkChanged = mErrorCode == ...
3 years, 6 months ago (2017-06-22 14:54:00 UTC) #31
Yaron
sorry, I missed this on the prevoius round but otherwise this lg to me https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java ...
3 years, 6 months ago (2017-06-22 21:40:57 UTC) #32
Xi Han
HI Yaron, PTAL, thanks! I am also watching the progress of CL (https://codereview.chromium.org/2946213002/), and will ...
3 years, 6 months ago (2017-06-23 15:21:45 UTC) #35
Yaron
lgtm
3 years, 6 months ago (2017-06-23 17:26:12 UTC) #38
pkotwicz
Just nits! https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode139 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; I did some ...
3 years, 6 months ago (2017-06-23 17:28:24 UTC) #39
Xi Han
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode139 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; On ...
3 years, 6 months ago (2017-06-23 19:57:26 UTC) #41
pkotwicz
LGTM! Thank you for bearing with me! https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java#newcode54 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:54: if (mDialog ...
3 years, 6 months ago (2017-06-23 20:25:19 UTC) #42
Xi Han
Hi Dom, could you please take a look? Thanks! I am watching the progress of ...
3 years, 6 months ago (2017-06-23 20:53:45 UTC) #44
dominickn
https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode84 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean isDisconnected = mErrorCode == NetError.ERR_INTERNET_DISCONNECTED; Can this be ...
3 years, 5 months ago (2017-06-26 03:34:00 UTC) #45
Xi Han
Hi Dom, I rebased this CL on Peter's CL that introduces the WebappSplashScreenController (https://codereview.chromium.org/2946213002/). PTAL, ...
3 years, 5 months ago (2017-06-26 17:54:04 UTC) #46
pkotwicz
https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode80 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:80: if (mSplashScreen == null) return; WebappSplashScreenController should no longer ...
3 years, 5 months ago (2017-06-27 01:32:07 UTC) #47
Xi Han
Hi Peter and Dom, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode80 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:80: if (mSplashScreen == ...
3 years, 5 months ago (2017-06-27 14:41:21 UTC) #48
pkotwicz
Just one comment https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java (right): https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java#newcode36 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java:36: protected ViewGroup mSplashScreen; This can stay ...
3 years, 5 months ago (2017-06-27 15:13:01 UTC) #49
Xi Han
PTAL, thanks! https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java (right): https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java#newcode36 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java:36: protected ViewGroup mSplashScreen; On 2017/06/27 15:13:01, pkotwicz ...
3 years, 5 months ago (2017-06-27 17:11:27 UTC) #50
Xi Han
I updated the strings, and mocks are: https://drive.google.com/drive/folders/0B7zEF5GgyYmpSF9hRXI5U3ZWUVE?usp=sharing. PTAL, thanks!
3 years, 5 months ago (2017-06-27 21:14:20 UTC) #54
dominickn
Thanks for following up on the strings. lgtm :)
3 years, 5 months ago (2017-06-27 23:54:21 UTC) #55
Yaron
lgtm!
3 years, 5 months ago (2017-06-28 13:45:03 UTC) #56
pkotwicz
Still LGTM
3 years, 5 months ago (2017-06-28 16:43:36 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2937973002/360001
3 years, 5 months ago (2017-06-28 16:47:12 UTC) #59
commit-bot: I haz the power
3 years, 5 months ago (2017-06-28 18:09:03 UTC) #62
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/373c86f2c5469d8d8de08437e5bc...

Powered by Google App Engine
This is Rietveld 408576698