|
|
Created:
3 years, 6 months ago by Xi Han Modified:
3 years, 5 months ago 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. |
DescriptionAdd 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. #Messages
Total messages: 62 (32 generated)
Description was changed from ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. BUG=733241 ========== to ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... Steps: 1) launch the WebAPK when the device is offline, and a dialog is shown. 2) turn off the air plate mode Observe: The dialog disappears, and the url is loaded in the WebAPK. BUG=733241 ==========
Description was changed from ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... Steps: 1) launch the WebAPK when the device is offline, and a dialog is shown. 2) turn off the air plate mode Observe: The dialog disappears, and the url is loaded in the WebAPK. BUG=733241 ========== to ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... Steps: 1) launch the WebAPK when the device is offline, and a dialog is shown. 2) turn off the air plate mode Observe: The dialog disappears, and the url is loaded in the WebAPK. BUG=733241 ==========
Description was changed from ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... Steps: 1) launch the WebAPK when the device is offline, and a dialog is shown. 2) turn off the air plate mode Observe: The dialog disappears, and the url is loaded in the WebAPK. BUG=733241 ========== to ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ==========
Description was changed from ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ========== to ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ==========
Description was changed from ========== Add an offline dialog when launching the WebAPK and the device is offline. 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ========== to ========== 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ==========
Patchset #1 (id:1) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Description was changed from ========== 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. Records a video: https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpaTRRdWFoRkNhdnc/vie... 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 ========== to ========== 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/vie... 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 ==========
Description was changed from ========== 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/vie... 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 ========== to ========== 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/vie... 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 ==========
https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... 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 goes away I get an interstitial saying "This site cannot be reached" which shows for about ~10 seconds https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:134: super.onPageLoadFailed(tab, errorCode); This is pretty hacky. Perhaps WebappTabObserver should have a method canHideSplashScreen() that WebApkActivity overrides
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:91: tab.loadUrl(new LoadUrlParams(url, PageTransition.AUTO_TOPLEVEL)); On 2017/06/14 19:12:40, pkotwicz wrote: > On my device, when the dialog goes away I get an interstitial saying "This site > cannot be reached" which shows for about ~10 seconds I checked the timer of url request reload (https://cs.chromium.org/chromium/src/components/error_page/renderer/net_error...): [0, 5000, 30000, 60000, 300000, 600000, 1800000] milliseconds depends on the times of reload. I think the offline page you saw is caused by a race conditions that the native side the network haven't ready yet when we call the reload explicitly. For now, we could still show the splash screen and try to reload again (maximum 3 times) to reduce the waiting time. If it is still failed, we then hand it over to auto reload fired by NetErrorHelperCore. https://codereview.chromium.org/2937973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:134: super.onPageLoadFailed(tab, errorCode); On 2017/06/14 19:12:40, pkotwicz wrote: > This is pretty hacky. Perhaps WebappTabObserver should have a method > canHideSplashScreen() that WebApkActivity overrides Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Mostly nits I did some investigation. I got lucky and noticed a few things 1) Doing the TODO in the NetErrorHelperCore contstructor // TODO(ellyjones): Make online_ accurate at object creation. would solve the issue of the reload being delayed a long time after the user is reconnected to the internet. We should see if we can convince the //net team to do the TODO. The TODO is likely contentious. 2) NetErrorHelperCore::ShouldSuppressErrorPage() prevents different error page from showing if the reload fails 3) NetErrorHelperCore::ShouldSuppressErrorPage() supresses error pages for many errors not just NetError.ERR_INTERNET_DISCONNECTED and NetError.ERR_NETWORK_CHANGED. I think that handling just these two errors is ok because of (1) https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:62: private class WebApkTabObserver extends WebappTabObserver { This should be a static class https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:67: private boolean mHasNetowrkChangedError; Can you store the errorCode instead of |mHasOfflineError| and |mHasNetowrkChangedError| https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:71: /** The number of times that reloading the tab is tried. */ Nit: is -> was Because the variable contains reloads which have occurred in the past https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); Can the super call be the first line in the function? https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:88: mReloadCount = 0; Why are you clearing |mReloadCount|? https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; If a WebAPK no longer has a service worker (and hence no fetch handler) and: 1) the user was connected to the internet when they launched the WebAPK 2) the user turns off their internet (The user keeps the WebAPK open) Won't ERR_NETWORK_CHANGED cause reloads even though the WebAPK has been open for a long time. I suggest closing the dialog once a navigation finishes without any errors (mHasOfflineError == false && mHasNetowrkChangedError == false) https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:115: mOfflineDialog.cancel(); I don't think that you should close the dialog now. You should close the dialog as a result of onDidFinishNavigation() https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:126: Log.e(TAG, "Invalid URL of the WebApk."); I don't think that this log message is necessary. This situation should never occur https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:496: /** A {@link TabObserver} that also handles Web apps logic. */ How about: "A {@link TabObserver} which handles Web app logic. */ Can you please make this a static class? It is confusing to think about non-static nested classes Can you please move the class declaration to the top of the file. It is kind of weird to have the class declaration in the middle of the file. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:580: if (canHideSplashScreen()) { Can you rename hideSplashScreen() to maybeHideSplashScreen() and have maybeHideSplashScreen() call canHideSplashScreen() I don't see any issues with moving hideSplashScreen() to WebappTabObserver. We'll probably need to pass a pointer to the splash screen FrameLayout to the WebappTabObserver constructor
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:62: private class WebApkTabObserver extends WebappTabObserver { On 2017/06/16 22:37:43, pkotwicz wrote: > This should be a static class Since WebappTabObserver is non-static nested class, I will keep this one as well. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:67: private boolean mHasNetowrkChangedError; On 2017/06/16 22:37:43, pkotwicz wrote: > Can you store the errorCode instead of |mHasOfflineError| and > |mHasNetowrkChangedError| Done. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:71: /** The number of times that reloading the tab is tried. */ On 2017/06/16 22:37:43, pkotwicz wrote: > Nit: is -> was > > Because the variable contains reloads which have occurred in the past Done. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); On 2017/06/16 22:37:43, pkotwicz wrote: > Can the super call be the first line in the function? Agree it looks wired, but I want to store the error code ASAP, since whether hiding the splash screen in the other three override functions depends on it. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:88: mReloadCount = 0; On 2017/06/16 22:37:43, pkotwicz wrote: > Why are you clearing |mReloadCount|? Ya, it seems not necessary. Removed. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; On 2017/06/16 22:37:43, pkotwicz wrote: > If a WebAPK no longer has a service worker (and hence no fetch handler) and: > 1) the user was connected to the internet when they launched the WebAPK > 2) the user turns off their internet (The user keeps the WebAPK open) That is a good point, and I haven't thought about it before. > Won't ERR_NETWORK_CHANGED cause reloads even though the WebAPK has been open for > a long time. I guess the confusing thing in the code is the if check of !mHasOfflineError (line 93), remove it and update the checks in the new patch. > I suggest closing the dialog once a navigation finishes without any errors > (mHasOfflineError == false && mHasNetowrkChangedError == false) I am ok to close the dialog a little bit later. The ERR_NETWORK_CHANGED happens in a very short of time, so it is only the time to close the dialog sooner or later. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:115: mOfflineDialog.cancel(); On 2017/06/16 22:37:43, pkotwicz wrote: > I don't think that you should close the dialog now. You should close the dialog > as a result of onDidFinishNavigation() Done. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:126: Log.e(TAG, "Invalid URL of the WebApk."); On 2017/06/16 22:37:43, pkotwicz wrote: > I don't think that this log message is necessary. This situation should never > occur Done. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:496: /** A {@link TabObserver} that also handles Web apps logic. */ On 2017/06/16 22:37:43, pkotwicz wrote: > How about: "A {@link TabObserver} which handles Web app logic. */ Done. > Can you please make this a static class? It is confusing to think about > non-static nested classes This class needs to access the member variables/functions of the WebappActivity, so it is natural to make it non-static. We could pass the WebappActivity via constructor, but the code doesn't look neat. I checked google java style, and there isn't any suggestion that we can't use non-static nested classes, so personally I would prefer to keep it as a non-staic nested class. > Can you please move the class declaration to the top of the file. It is kind of > weird to have the class declaration in the middle of the file. Done. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:580: if (canHideSplashScreen()) { On 2017/06/16 22:37:44, pkotwicz wrote: > Can you rename hideSplashScreen() to maybeHideSplashScreen() and have > maybeHideSplashScreen() call canHideSplashScreen() > > I don't see any issues with moving hideSplashScreen() to WebappTabObserver. > We'll probably need to pass a pointer to the splash screen FrameLayout to the > WebappTabObserver constructor Hmm, I would like to keep it as it is now, since it is more clear to leave the logic to where it belongs to. WebappActivity is the one who knows how to hide the splash screen, and the WebappTabObserver knows when to hide the splash screen. It isn't ideal to mix these logic together.
https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); I don't understand. Isn't the errorCode passed to super.onDidFinishNavigation() by copy (so it does not matter if super.onDidFinishNavigation() modifies the error code)? I don't think that it is possible for TabObserver#onPageLoadFinished() to be called in the middle of TabObserver#onDidFinishNavigation()'s processing https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; In the scenario in my previous comment, won't the dialog show even though the WebAPK has been open for a long time? I think that this problem will go away when you make this class inherit from SplashScreenController that I introduce in https://codereview.chromium.org/2946213002/ https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:70: /** The maximum times to reload the tab after the device is online. */ Nit: 'maximum times' -> 'maximum number of times' https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean isNetowrkChanged = mErrorCode == NetError.ERR_NETWORK_CHANGED; Nit: |isNetowrkChanged| -> |hasNetworkChanged| https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:85: if (errorCode == 0 || (!isDisconnected && !isNetowrkChanged)) { Can't this be simplified to: if (!isDisconnected && !isNetowrkChanged) {} https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: // It is possible that we gets {@link NetError.ERR_NETWORK_CHANGED} during the first Nit: 'gets' -> 'get' https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:94: // reload after the device is online. The navigation will still fail until the next auto Nit: 'still fail until' -> 'fail until' ('still' is unnecessary) https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:103: // If the offline dialog is showing, we stops here. You can probably remove the comment. The comment does not say anything which is not obvious from the code https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; Over the weekend I got a DNS_PROBE_FINISHED_NO_INTERNET error at home (I did not do anything special to trigger the error). We should probably consider DNS_PROBE_FINISHED_NO_INTERNET as offline
hanxi@chromium.org changed reviewers: + yfriedman@chromium.org
+yfriedman@. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:86: httpStatusCode); On 2017/06/21 05:18:43, pkotwicz wrote: > I don't understand. Isn't the errorCode passed to super.onDidFinishNavigation() > by copy (so it does not matter if super.onDidFinishNavigation() modifies the > error code)? > > I don't think that it is possible for TabObserver#onPageLoadFinished() to be > called in the middle of TabObserver#onDidFinishNavigation()'s processing This is my concern. Ok, I won't worry about this any more. https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; On 2017/06/21 05:18:43, pkotwicz wrote: > In the scenario in my previous comment, > > won't the dialog show even though the WebAPK has been open for a long time? > > I think that this problem will go away when you make this class inherit from > SplashScreenController that I introduce in > https://codereview.chromium.org/2946213002/ As discussed offline, let Yaron take a look at this patch:) https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:70: /** The maximum times to reload the tab after the device is online. */ On 2017/06/21 05:18:44, pkotwicz wrote: > Nit: 'maximum times' -> 'maximum number of times' Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean isNetowrkChanged = mErrorCode == NetError.ERR_NETWORK_CHANGED; On 2017/06/21 05:18:44, pkotwicz wrote: > Nit: |isNetowrkChanged| -> |hasNetworkChanged| Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:85: if (errorCode == 0 || (!isDisconnected && !isNetowrkChanged)) { On 2017/06/21 05:18:44, pkotwicz wrote: > Can't this be simplified to: > if (!isDisconnected && !isNetowrkChanged) {} Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: // It is possible that we gets {@link NetError.ERR_NETWORK_CHANGED} during the first On 2017/06/21 05:18:44, pkotwicz wrote: > Nit: 'gets' -> 'get' Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:94: // reload after the device is online. The navigation will still fail until the next auto On 2017/06/21 05:18:44, pkotwicz wrote: > Nit: 'still fail until' -> 'fail until' ('still' is unnecessary) Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:103: // If the offline dialog is showing, we stops here. On 2017/06/21 05:18:44, pkotwicz wrote: > You can probably remove the comment. The comment does not say anything which is > not obvious from the code Done. https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; On 2017/06/21 05:18:44, pkotwicz wrote: > Over the weekend I got a DNS_PROBE_FINISHED_NO_INTERNET error at home (I did not > do anything special to trigger the error). We should probably consider > DNS_PROBE_FINISHED_NO_INTERNET as offline Can we hold off handling the dns error? The DNS_PROBE_FINISHED_NO_INTERNET isn't a error in NetError.java. It is easier for the renderer to show the error message on the error page according to the error domain( https://cs.chromium.org/chromium/src/components/error_page/common/localized_e... ), but I didn't find the associated error code there.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; On 2017/06/21 18:39:41, Xi Han wrote: > On 2017/06/21 05:18:43, pkotwicz wrote: > > In the scenario in my previous comment, > > > > won't the dialog show even though the WebAPK has been open for a long time? > > > > I think that this problem will go away when you make this class inherit from > > SplashScreenController that I introduce in > > https://codereview.chromium.org/2946213002/ > > As discussed offline, let Yaron take a look at this patch:) Ok, having seen the context, I don't think we should show the prompt in all cases. (especially given the current wording). It's meant to be user-friendly and patch over the case where app hasn't yet installed a SW. With SW installed, only edge cases will trigger this and we can just get chrome error pages. If we later decide to hide all errors in some way we can do that but it's out of scope. So I'm on-board with Peter's suggestion of only using this while splashscreen is visible. Doing this with the SplashScreenController is orthogonal to this question though (although that looks like a nice clean-up)
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:93: if (!mHasOfflineError || mOfflineDialog != null) return; On 2017/06/21 19:53:15, Yaron wrote: > On 2017/06/21 18:39:41, Xi Han wrote: > > On 2017/06/21 05:18:43, pkotwicz wrote: > > > In the scenario in my previous comment, > > > > > > won't the dialog show even though the WebAPK has been open for a long time? > > > > > > I think that this problem will go away when you make this class inherit from > > > SplashScreenController that I introduce in > > > https://codereview.chromium.org/2946213002/ > > > > As discussed offline, let Yaron take a look at this patch:) > > Ok, having seen the context, I don't think we should show the prompt in all > cases. (especially given the current wording). It's meant to be user-friendly > and patch over the case where app hasn't yet installed a SW. With SW installed, > only edge cases will trigger this and we can just get chrome error pages. If we > later decide to hide all errors in some way we can do that but it's out of > scope. So I'm on-board with Peter's suggestion of only using this while > splashscreen is visible. Doing this with the SplashScreenController is > orthogonal to this question though (although that looks like a nice clean-up) All right. Only shows the dialog when the splash screen exists.
https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... 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/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:109: mOfflineDialog = new WebApkOfflineDialog(); nit: i think this logically goes with the next block (vs the code block here that's seetting about NCN) https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:118: mReloadCount = 0; why does this 0-out reload count? Is the idea that for each time we get a NCN notificatoin, we get 3 tries to reload? It's just a little hard to grok why in this case of reloading, we set it to 0 but in the othe rcase (L102), we increment the count.
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean hasNetowrkChanged = mErrorCode == NetError.ERR_NETWORK_CHANGED; On 2017/06/21 21:05:21, Yaron wrote: > hasNetworkChanged Done. https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:109: mOfflineDialog = new WebApkOfflineDialog(); On 2017/06/21 21:05:21, Yaron wrote: > nit: i think this logically goes with the next block (vs the code block here > that's seetting about NCN) Done. https://codereview.chromium.org/2937973002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:118: mReloadCount = 0; On 2017/06/21 21:05:21, Yaron wrote: > why does this 0-out reload count? Is the idea that for each time we get a NCN > notificatoin, we get 3 tries to reload? It's just a little hard to grok why in > this case of reloading, we set it to 0 but in the othe rcase (L102), we > increment the count. I updated this part to: allow one more reloading after the network connection is back. Therefore, we can force a reloading if the NCN notification is got after the first loading.
sorry, I missed this on the prevoius round but otherwise this lg to me https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:194: hideSplashScreen(WebappUma.SPLASHSCREEN_HIDES_REASON_CRASH); Hmm - if we crash, do we want to hide the splashscreen? The user will think the page is loading forever and take a while to realize that chrome crashed and they can try reloading https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2964: QUIT is there not an existing QUIT that could be re-used?
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
HI Yaron, PTAL, thanks! I am also watching the progress of CL (https://codereview.chromium.org/2946213002/), and will rebase on it as long as everyone is happy with the refactoring. https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:194: hideSplashScreen(WebappUma.SPLASHSCREEN_HIDES_REASON_CRASH); On 2017/06/22 21:40:57, Yaron wrote: > Hmm - if we crash, do we want to hide the splashscreen? The user will think the > page is loading forever and take a while to realize that chrome crashed and they > can try reloading Done. https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2964: QUIT On 2017/06/22 21:40:57, Yaron wrote: > is there not an existing QUIT that could be re-used? There isn't any in this file. In WebAPK strings, we do reuse the existing one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Just nits! https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; I did some more digging. DNS_PROBE_FINISHED_NO_INTERNET corresponds to one of ERR_NAME_NOT_RESOLVED and ERR_NAME_RESOLUTION_FAILED. (See https://cs.chromium.org/chromium/src/chrome/browser/net/net_error_tab_helper....) net_error_tab_helper.cc does additional processing to determine the type of DNS error https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:92: if (isDisconnected && mSplashScreen == null) return; Nit: Can you move this to the first line of this function (right after the super call)? Can this be if (mSplashScreen == null) return instead? https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:104: if (mOfflineDialog != null) return; Should this be: if (mOfflineDialog != null || !isDisconnected) return https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:114: // One more reloading is allowed after the network connection is back. Nit: 'reloading' -> 'reload' https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:16: * A dialog to notify users that the launch of a WebAPK needs network connection. How about: "that the launch of a WebAPK needs network connection" -> "that WebAPKs need a network connection to launch." I think that my version is slightly clearer https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:22: */ Nit: Make this comment one line https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:54: if (mDialog == null) return; Can this function simply do mDialog.cancel() and not null out |mDialog| ? https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2964: QUIT Android will automatically uppercase the text. This should be lowercase to match the text for other buttons in this file
Patchset #8 (id:200001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:139: && mErrorCode != NetError.ERR_NETWORK_CHANGED; On 2017/06/23 17:28:24, pkotwicz wrote: > I did some more digging. DNS_PROBE_FINISHED_NO_INTERNET corresponds to one of > ERR_NAME_NOT_RESOLVED and ERR_NAME_RESOLUTION_FAILED. (See > https://cs.chromium.org/chromium/src/chrome/browser/net/net_error_tab_helper....) > net_error_tab_helper.cc does additional processing to determine the type of DNS > error Thanks for the investigation! I have gained more knowledge about how the navigation work:) It isn't possible to know which dns error (DNS_PROBE_FINISHED_NO_INTERNET or other dns error) when this function is called. So the offline dialog will not help for other dns errors. As discussed offline, we don't handle the dns error for now, but would be if users report bugs and we know the usage better. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:92: if (isDisconnected && mSplashScreen == null) return; On 2017/06/23 17:28:24, pkotwicz wrote: > Nit: Can you move this to the first line of this function (right after the super > call)? > > Can this be > if (mSplashScreen == null) return > > instead? Done. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:104: if (mOfflineDialog != null) return; On 2017/06/23 17:28:24, pkotwicz wrote: > Should this be: > if (mOfflineDialog != null || !isDisconnected) return Good catch, thanks! Actually it is for |hasNetworkChanged| == true, right? I updated the block of line 98 to 102 to make it easier to understand. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:114: // One more reloading is allowed after the network connection is back. On 2017/06/23 17:28:24, pkotwicz wrote: > Nit: 'reloading' -> 'reload' Done. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:16: * A dialog to notify users that the launch of a WebAPK needs network connection. On 2017/06/23 17:28:24, pkotwicz wrote: > How about: "that the launch of a WebAPK needs network connection" -> "that > WebAPKs need a network connection to launch." > > I think that my version is slightly clearer Done. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:22: */ On 2017/06/23 17:28:24, pkotwicz wrote: > Nit: Make this comment one line Done. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:54: if (mDialog == null) return; On 2017/06/23 17:28:24, pkotwicz wrote: > Can this function simply do > > mDialog.cancel() > > and not null out |mDialog| ? I don't have a strong preference, but I thought set to null explicitly can result to garbage collection faster. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2964: QUIT On 2017/06/23 17:28:24, pkotwicz wrote: > Android will automatically uppercase the text. This should be lowercase to match > the text for other buttons in this file Done.
LGTM! Thank you for bearing with me! https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:54: if (mDialog == null) return; Less code is better :) (It also decreases Chrome.apk size) https://codereview.chromium.org/2937973002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:79: // We don't show the offline dialog if the splash screen has gone. Instead, shows Nit: 'shows' -> 'show' (Because this is the 'imperative' mood)
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dom, could you please take a look? Thanks! I am watching the progress of Peter's CL (https://codereview.chromium.org/2946213002/), will rebase on it if everyone is happy with the new SplashScreenHandler. https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java (right): https://codereview.chromium.org/2937973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkOfflineDialog.java:54: if (mDialog == null) return; On 2017/06/23 20:25:19, pkotwicz wrote: > Less code is better :) (It also decreases Chrome.apk size) Acknowledged. https://codereview.chromium.org/2937973002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:79: // We don't show the offline dialog if the splash screen has gone. Instead, shows On 2017/06/23 20:25:19, pkotwicz wrote: > Nit: 'shows' -> 'show' (Because this is the 'imperative' mood) Done.
https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean isDisconnected = mErrorCode == NetError.ERR_INTERNET_DISCONNECTED; Can this be written as: if (mErrorCode == NetError.ERR_NETWORK_CHANGED) { // lines 99 - 103 and the comment from line 94 return; } else if (mErrorCode != NetError.ERR_INTERNET_DISCONNECTED) { // comment why all other network errors don't need to have the offline dialog shown // lines 87 - 90 return; } if (mOfflineDialog != null) return; Then you can eliminate the two booleans and unify some of this logic https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2961: <ph name="WEB_SITE">%1$s<ex>Housing.com</ex></ph> must be online the first time it is run to start properly. Have we run these strings past UX, specifically srahim@?
Hi Dom, I rebased this CL on Peter's CL that introduces the WebappSplashScreenController (https://codereview.chromium.org/2946213002/). PTAL, thanks! https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:84: boolean isDisconnected = mErrorCode == NetError.ERR_INTERNET_DISCONNECTED; On 2017/06/26 03:34:00, dominickn wrote: > Can this be written as: > > if (mErrorCode == NetError.ERR_NETWORK_CHANGED) { > // lines 99 - 103 and the comment from line 94 > return; > } else if (mErrorCode != NetError.ERR_INTERNET_DISCONNECTED) { > // comment why all other network errors don't need to have the offline dialog > shown > // lines 87 - 90 > return; > } > > if (mOfflineDialog != null) return; > > Then you can eliminate the two booleans and unify some of this logic Thanks for the suggestions. I remove the |isDisconnected| to eliminate one boolean. The reason that I put |if (!isDisconnected && !hasNetworkChanged)| first is: in most cases, the navigation should just success or fail according to other reason. Therefore, we can return early. Also, the |NetError.ERR_NETWORK_CHANGED| error is very rare, and the block between line 99 and line 103 works like a bug fix. Even though the current login isn't as unified as you suggested, but personally I still prefer it which is a little bit easier for me to understand what happened. https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2937973002/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2961: <ph name="WEB_SITE">%1$s<ex>Housing.com</ex></ph> must be online the first time it is run to start properly. On 2017/06/26 03:34:00, dominickn wrote: > Have we run these strings past UX, specifically srahim@? I'm not sure, but the strings are coming from the "WebAPK in Play Store" preview (https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F...).
https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:80: if (mSplashScreen == null) return; WebappSplashScreenController should no longer be observing the tab when |mSplashScreen| == null https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:96: if (hasNetworkChanged) { Optional: I think that the code would be clearer if you had separate functions to handle: - ERR_NETWORK_CHANGED - ERR_INTERNET_DISCONNECTED This would enable onDidFinishNavigation() to be like this onDidFinishNavigation() { super.onDidFinishNavigation() mErrorCode = errorCode; switch (mErrorCode) { case NetError.ERR_INTERNET_DISCONNECTED: onInternetDisconnected(); break; case NetError.ERR_NETWORK_CHANGED: OnNetworkChanged(); break; default: if (mOfflineDialog != null) { ... } break; } }
Hi Peter and Dom, PTAL, thanks! https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:80: if (mSplashScreen == null) return; On 2017/06/27 01:32:07, pkotwicz wrote: > WebappSplashScreenController should no longer be observing the tab when > |mSplashScreen| == null Good catch. https://codereview.chromium.org/2937973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:96: if (hasNetworkChanged) { On 2017/06/27 01:32:07, pkotwicz wrote: > Optional: I think that the code would be clearer if you had separate functions > to handle: > > - ERR_NETWORK_CHANGED > - ERR_INTERNET_DISCONNECTED > > This would enable onDidFinishNavigation() to be like this > > onDidFinishNavigation() { > super.onDidFinishNavigation() > mErrorCode = errorCode; > > switch (mErrorCode) { > case NetError.ERR_INTERNET_DISCONNECTED: > onInternetDisconnected(); > break; > case NetError.ERR_NETWORK_CHANGED: > OnNetworkChanged(); > break; > default: > if (mOfflineDialog != null) { > ... > } > break; > } > } Thanks, updated since both of you want the unified code:)
Just one comment https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java (right): https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java:36: protected ViewGroup mSplashScreen; This can stay private?
PTAL, thanks! https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java (right): https://codereview.chromium.org/2937973002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappSplashScreenController.java:36: protected ViewGroup mSplashScreen; On 2017/06/27 15:13:01, pkotwicz wrote: > This can stay private? Thanks for catching it!
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #14 (id:340001) has been deleted
I updated the strings, and mocks are: https://drive.google.com/drive/folders/0B7zEF5GgyYmpSF9hRXI5U3ZWUVE?usp=sharing. PTAL, thanks!
Thanks for following up on the strings. lgtm :)
lgtm!
Still LGTM
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1498668415890820, "parent_rev": "0a29cb4bf60a59b0f52da3eb4ef88eec5b371d25", "commit_rev": "373c86f2c5469d8d8de08437e5bcdf377ccd4a92"}
Message was sent while issue was closed.
Description was changed from ========== 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/vie... 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 ========== to ========== 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/vie... 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/+/373c86f2c5469d8d8de08437e5bc... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as https://chromium.googlesource.com/chromium/src/+/373c86f2c5469d8d8de08437e5bc... |