Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912393005:
Add support for webapk without runtimeHost (part 2) (Closed)
DescriptionAdd support for webapk without runtimeHost (part 2)
Add a dialog to ask user to install a host browser for WebAPK when there isn't
any installed browser supporting WebAPKs.
The screenshots are:
https://drive.google.com/open?id=0B7zEF5GgyYmpZEdmY1h4LXg5WlU
BUG=714737
Review-Url: https://codereview.chromium.org/2912393005
Cr-Commit-Position: refs/heads/master@{#478278}
Committed: https://chromium.googlesource.com/chromium/src/+/dded0412e190a6f4857d10d7f9d865c4ab700f56
Patch Set 1 #
Total comments: 20
Patch Set 2 : MainActivity no longer implements InstallHostBrowserDialog.DialogListener. #
Total comments: 22
Patch Set 3 : pkotwicz@'s comments. #
Total comments: 22
Patch Set 4 : Nits. #Patch Set 5 : Fix lint and text color. #
Total comments: 2
Patch Set 6 : Nits. #Patch Set 7 : Rebase. #Patch Set 8 : Clean up. #
Total comments: 2
Patch Set 9 : Nits. #Patch Set 10 : Adopt hdpi logo image. #Messages
Total messages: 53 (24 generated)
Description was changed from ========== Add support for webapk without runtimeHost (part 2 Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. BUG=714737 ========== to ========== Add support for webapk without runtimeHost (part 2) Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. BUG=714737 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Add support for webapk without runtimeHost (part 2) Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. BUG=714737 ========== to ========== Add support for webapk without runtimeHost (part 2) Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. The screenshots are: https://drive.google.com/open?id=0B7zEF5GgyYmpZEdmY1h4LXg5WlU BUG=714737 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:28: public static final String DEFAULT_HOST_BROWSER_APPLICATION_NAME = "Google Chrome"; This file is shared with the ShellAPK and with Chrome. We should not put ShellAPK specific constants in here. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: T activity, List<ResolveInfo> infos, String url) { Can this function be static? If you make |activity| static you don't need the |mListener| member variable https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:40: public <T extends Activity & DialogListener> void show(T activity, String url) { Nits: - Can you pass as an argument the desired host package, host title, and host icon resource id? - Can this function be static? This class does not need to store any data if you make |activity| final. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:41: if (!(activity instanceof DialogListener)) { Given the method signature, isn't this impossible? https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:57: applicationName = metadata.getString(WebApkMetaDataKeys.RUNTIME_HOST_APPLICATION_NAME); |RUNTIME_HOST_APPLICATION_NAME| should be called LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME For Play WebAPKs, LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME should be set to "Chrome" In my opinion, WebApkConstants.DEFAULT_HOST_BROWSER_APPLICATION_NAME should not exist https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:66: icon.setImageResource(R.drawable.runtime_host_logo); The resource id should be called |last_resort_runtime_host_logo| https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:74: intent.addFlags(Intent.FLAG_ACTIVITY_NEW_DOCUMENT); Can you document why setting the Intent.FLAG_ACTIVITY_NEW_DOCUMENT flag is necessary? https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:124: dialog.show(this, hostUrl); Can you instantiate an anonymous class which implements InstallHostBrowserDialog.DialogListener InstallHostBrowserDialog.DialogListener listener = new InstallHostBrowserDialog.DialogListener() { ... } dialog.show(this, listener, hostUrl); Alternatively can you name rename the interface methods in InstallHostBrowserDialog.DialogListener to onConfirmInstallDialogInstall(String packageName) {} onCofnirmInstallDialogQuit() {} https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:200: PackageManager packageManager, List<ResolveInfo> resolveInfos) { I think that this function along with the definition of the BrowserItem class is better suited in ChooseHostBrowserDialog
Patchset #2 (id:80001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:28: public static final String DEFAULT_HOST_BROWSER_APPLICATION_NAME = "Google Chrome"; On 2017/06/02 00:54:57, pkotwicz wrote: > This file is shared with the ShellAPK and with Chrome. We should not put > ShellAPK specific constants in here. Make senses. Moved. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: T activity, List<ResolveInfo> infos, String url) { On 2017/06/02 00:54:57, pkotwicz wrote: > Can this function be static? If you make |activity| static you don't need the > |mListener| member variable That is a good idea, change it to static and remove the member variable. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:40: public <T extends Activity & DialogListener> void show(T activity, String url) { On 2017/06/02 00:54:57, pkotwicz wrote: > Nits: > - Can you pass as an argument the desired host package, host title, and host > icon resource id? Yes, pass these params from the MainActivity. > - Can this function be static? This class does not need to store any data if you > make |activity| final. Make this function be static. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:41: if (!(activity instanceof DialogListener)) { On 2017/06/02 00:54:57, pkotwicz wrote: > Given the method signature, isn't this impossible? Forgot to update it here. Removed the template for now. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:57: applicationName = metadata.getString(WebApkMetaDataKeys.RUNTIME_HOST_APPLICATION_NAME); On 2017/06/02 00:54:57, pkotwicz wrote: > |RUNTIME_HOST_APPLICATION_NAME| should be called > LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME I add it because we can't get the application name once the "RUNTIME_HOST" isn't installed. Since we already have |RUNTIME_HOST|, |RUNTIME_HOST_APPLICATION_NAME| sounds more suitable for this purpose. > For Play WebAPKs, LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME should be set to > "Chrome" Yes, I use "Google Chrome" which matches the UI review. > In my opinion, WebApkConstants.DEFAULT_HOST_BROWSER_APPLICATION_NAME should not > exist Removed, and make it a local string in MainActivity. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:66: icon.setImageResource(R.drawable.runtime_host_logo); On 2017/06/02 00:54:57, pkotwicz wrote: > The resource id should be called |last_resort_runtime_host_logo| I still think it is better to keep consistent with |runtime_host|. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:74: intent.addFlags(Intent.FLAG_ACTIVITY_NEW_DOCUMENT); On 2017/06/02 00:54:57, pkotwicz wrote: > Can you document why setting the Intent.FLAG_ACTIVITY_NEW_DOCUMENT flag is > necessary? I thought it is better to have both new_task and new_document flags set for post-L, but yes, it isn't mandatory. Removed for now. https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:124: dialog.show(this, hostUrl); On 2017/06/02 00:54:57, pkotwicz wrote: > Can you instantiate an anonymous class which implements > InstallHostBrowserDialog.DialogListener > > InstallHostBrowserDialog.DialogListener listener = new > InstallHostBrowserDialog.DialogListener() { > ... > } > dialog.show(this, listener, hostUrl); > > Alternatively can you name rename the interface methods in > InstallHostBrowserDialog.DialogListener to > onConfirmInstallDialogInstall(String packageName) {} > onCofnirmInstallDialogQuit() {} > Done. Is it preferable than implementing the interface by the MainActivity? Or is it just for avoiding the cast from Activity to DialogListener? https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:200: PackageManager packageManager, List<ResolveInfo> resolveInfos) { On 2017/06/02 00:54:57, pkotwicz wrote: > I think that this function along with the definition of the BrowserItem class is > better suited in ChooseHostBrowserDialog Done.
Looking at your CL! (< 5 minutes since you posted it)
https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:124: dialog.show(this, hostUrl); My goal was clarity. It is confusing that MainActivity has onQuit() and onConfirmQuit() https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: */ Might as well make this method static https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:96: /** Returns a list of BrowserItem for all of the installed browsers. */ It probably makes sense to move the BrowserItem class into this class https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:31: * @param url URL of the WebAPK that is shown on the dialog. How about: "URL of the WebAPK for which the dialog is shown." I think that your version mentions the function's implementation details (more so than the other parameters) https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:77: super.onCreate(savedInstanceState); Can you do this here: Bundle metadata = WebApkUtils.readMetaData(this); and simplify WebApkUtils.readMetaDataFromManifest(this, WebApkMetaDataKeys.START_URL) and pass the bundle to WebApkUtils#getInstalledBrowserResolveInfos() and showInstallHostBrowserDialog() This would enable you to delete the WebApkUtils#readMetaDataFromManifest() function https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:237: if (metadata != null) { If you pass |metadata| as an argument you can assume that it is non-null https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:241: if (TextUtils.isEmpty(hostBrowserPackageName)) { The reason that I was suggesting naming RUNTIME_HOST_APPLICATION_NAME LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME Is that I think that it is confusing that R.string.runtime_host_logo is used even when |hostBrowserPackageName| is empty. For this reason, I think that the Android Manifest has to different types of runtime hosts: "preferred runtime host package" to use whenever the package is installed "last resort runtime host package" to install from the play store if no WebAPK compatible browser is installed The last resort meta data attributes are guaranteed to be set and non empty. In theory, we could have a LAST_RESORT_RUNTIME_HOST property for the package name too. I think that using WebApkConstants.DEFAULT_HOST_BROWSER is fine https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:262: this, listener, hostUrl, hostBrowserPackageName, applicationName); Please pass in the icon resource id to InstallHostBrowserDialog#show(). Although the id is always the same, the icon looks different based on what the host browser in AndroidManifest.xml is https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:93: * @return This comment looks incomplete https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:239: browserIntent, PackageManager.MATCH_DEFAULT_ONLY); Can this function make use of getInstalledBrowserResolveInfos()
Patchset #3 (id:120001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:124: dialog.show(this, hostUrl); On 2017/06/02 16:53:05, pkotwicz wrote: > My goal was clarity. It is confusing that MainActivity has onQuit() and > onConfirmQuit() Acknowledged. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: */ On 2017/06/02 16:53:05, pkotwicz wrote: > Might as well make this method static I planed to change it in the first CL, but it is ok to do the refactory here:) https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:96: /** Returns a list of BrowserItem for all of the installed browsers. */ On 2017/06/02 16:53:05, pkotwicz wrote: > It probably makes sense to move the BrowserItem class into this class SGTM, moved. Thanks! https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:31: * @param url URL of the WebAPK that is shown on the dialog. On 2017/06/02 16:53:05, pkotwicz wrote: > How about: "URL of the WebAPK for which the dialog is shown." > > I think that your version mentions the function's implementation details (more > so than the other parameters) Done. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:77: super.onCreate(savedInstanceState); On 2017/06/02 16:53:05, pkotwicz wrote: > Can you do this here: > > Bundle metadata = WebApkUtils.readMetaData(this); > > and simplify > > WebApkUtils.readMetaDataFromManifest(this, WebApkMetaDataKeys.START_URL) Done. > and pass the bundle to > WebApkUtils#getInstalledBrowserResolveInfos() and showInstallHostBrowserDialog() > > This would enable you to delete the WebApkUtils#readMetaDataFromManifest() > function The WebApkUtils#readMetaDataFromManifest() is also used in WebApkUtils#getHostBrowserPackageNameInternal(), but the plumbing doesn't look neet. Honestly, I would like to keep the WebApkUtils#readMetaDataFromManifest(), since I expect the WebApkUtils#readMetaData() is only used when more than one meta data values are needed. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:237: if (metadata != null) { On 2017/06/02 16:53:05, pkotwicz wrote: > If you pass |metadata| as an argument you can assume that it is non-null I would like to, but I don't think that is true. In MainActivity, it is possible that |metadata| be null. You actually remind me to add a null check there. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:241: if (TextUtils.isEmpty(hostBrowserPackageName)) { On 2017/06/02 16:53:05, pkotwicz wrote: > The reason that I was suggesting naming RUNTIME_HOST_APPLICATION_NAME > LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME > > Is that I think that it is confusing that R.string.runtime_host_logo is used > even when |hostBrowserPackageName| is empty. Make sense. > For this reason, I think that the Android Manifest has to different types of > runtime hosts: > "preferred runtime host package" to use whenever the package is installed > "last resort runtime host package" to install from the play store if no WebAPK > compatible browser is installed It makes sense, but I would prefer not changing the Android Manifest too much. How about still use |RUNTIME_HOST_APPLICATION_NAME| in the Android Manifest for Browser-WebApks? > The last resort meta data attributes are guaranteed to be set and non empty. In > theory, we could have a LAST_RESORT_RUNTIME_HOST property for the package name > too. I think that using WebApkConstants.DEFAULT_HOST_BROWSER is fine I added |LAST_RESORT_RUNTIME_HOST| in the MainActivity to make it and the |LAST_RESORT_RUNTIME_HOST_APPLICATION_NAME| a pair for Play-WebAPKs. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:262: this, listener, hostUrl, hostBrowserPackageName, applicationName); On 2017/06/02 16:53:05, pkotwicz wrote: > Please pass in the icon resource id to InstallHostBrowserDialog#show(). Although > the id is always the same, the icon looks different based on what the host > browser in AndroidManifest.xml is Done. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:93: * @return On 2017/06/02 16:53:05, pkotwicz wrote: > This comment looks incomplete Sorry, removed. https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:239: browserIntent, PackageManager.MATCH_DEFAULT_ONLY); On 2017/06/02 16:53:05, pkotwicz wrote: > Can this function make use of getInstalledBrowserResolveInfos() Done.
LGTM with nits https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: */ You meant refactoring? :) Thank you for making the change. The code looks much cleaner now https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:77: super.onCreate(savedInstanceState); Fair enough https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:241: if (TextUtils.isEmpty(hostBrowserPackageName)) { Fair enough https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:84: * @param infos The list of resolved infos of the browsers that is shown on the dialog. Nit: "resolved infos" -> ResolveInfos https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:139: return browsers; A friendly reminder to add the Comparable logic from your other CL here too https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:20: * dialog. Nit: "A listener to receive updates" -> "A listener which is notified" "receive updates" implies that the listener's methods will be called several times. In our case in the listener is called exactly oncce. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:84: if (mOverrideUrl == null && metadata == null) { Can this be simplified to: if (metadata == null) { } If the meta data is null, something has gone very very wrong https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:248: } else { Is it possible to go through this branch now? https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:272: String applicationName = null; Nit: Rename the variables to lastResortHostBrowserPackageName and lastResortHostBrowserApplicationName https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:273: if (metadata != null) { Isn't it impossible for |metadata| to be null given the early return on line 86? (with my suggested modification) https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:279: // Google as the default host browser. You mean "Chrome" or "Google Chrome" https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:54: * Returns a list of browsers that supports WebAPKs. TODO(hanxi): Replace this function once we Nit: 'supports' -> 'support' (because browsers is pluralised) https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:110: INSTALL Is it necessary to make these uppercase? Will Android make the button text uppercase automatically? For instance, R.string.login_dialog_ok_button_label in LoginPrompt.java is not uppercase.
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:170012) has been deleted
hanxi@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@. Hi Ted: could you please do a UI review for the InstallHostBrowserDialog? Thanks! https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:48: */ On 2017/06/05 19:18:19, pkotwicz wrote: > You meant refactoring? :) > > Thank you for making the change. The code looks much cleaner now Acknowledged. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:84: * @param infos The list of resolved infos of the browsers that is shown on the dialog. On 2017/06/05 19:18:20, pkotwicz wrote: > Nit: "resolved infos" -> ResolveInfos Done. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:139: return browsers; On 2017/06/05 19:18:19, pkotwicz wrote: > A friendly reminder to add the Comparable logic from your other CL here too Thanks! https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:20: * dialog. On 2017/06/05 19:18:20, pkotwicz wrote: > Nit: "A listener to receive updates" -> "A listener which is notified" > > "receive updates" implies that the listener's methods will be called several > times. In our case in the listener is called exactly oncce. Done. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:84: if (mOverrideUrl == null && metadata == null) { On 2017/06/05 19:18:20, pkotwicz wrote: > Can this be simplified to: > > if (metadata == null) { > } > > If the meta data is null, something has gone very very wrong sg, updated. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:248: } else { On 2017/06/05 19:18:20, pkotwicz wrote: > Is it possible to go through this branch now? Good catch! Now we only need to call |launchInHostBrowser()|:) https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:272: String applicationName = null; On 2017/06/05 19:18:20, pkotwicz wrote: > Nit: Rename the variables to lastResortHostBrowserPackageName and > lastResortHostBrowserApplicationName Done. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:273: if (metadata != null) { On 2017/06/05 19:18:20, pkotwicz wrote: > Isn't it impossible for |metadata| to be null given the early return on line 86? > (with my suggested modification) Done. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:279: // Google as the default host browser. On 2017/06/05 19:18:20, pkotwicz wrote: > You mean "Chrome" or "Google Chrome" Done. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:54: * Returns a list of browsers that supports WebAPKs. TODO(hanxi): Replace this function once we On 2017/06/05 19:18:20, pkotwicz wrote: > Nit: 'supports' -> 'support' (because browsers is pluralised) Ya, sorry for the typo. https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:110: INSTALL On 2017/06/05 19:18:20, pkotwicz wrote: > Is it necessary to make these uppercase? Will Android make the button text > uppercase automatically? For instance, R.string.login_dialog_ok_button_label in > LoginPrompt.java is not uppercase. In the Play Store, the green button of "INSTALL" is uppercase, and I guess this is where Sam's UI design initially comes from?
https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:110: INSTALL I meant: Won't the Android framework make the word uppercase on the button even if the word is lowercase here?
https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:110: INSTALL On 2017/06/06 15:21:47, pkotwicz wrote: > I meant: Won't the Android framework make the word uppercase on the button even > if the word is lowercase here? Yes, it is auto converted to uppercase. I still think it is safer to use the uppercase here to avoid any unexpected things during translation.
lgtm https://codereview.chromium.org/2912393005/diff/210001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/210001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:41: R.layout.choose_host_browser_dialog_list, null); we should rename choose_host_browser_dialog_list to something more generic host_browser_list_item or "something"?
hanxi@chromium.org changed reviewers: + agrieve@chromium.org
Hi agrieve@: Please review changes in - build/android/lint/suppressions.xml. Thanks! https://codereview.chromium.org/2912393005/diff/210001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java (right): https://codereview.chromium.org/2912393005/diff/210001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/InstallHostBrowserDialog.java:41: R.layout.choose_host_browser_dialog_list, null); On 2017/06/06 16:25:46, Ted C wrote: > we should rename choose_host_browser_dialog_list to something more generic > > host_browser_list_item or "something"? Done.
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lint lgtm https://codereview.chromium.org/2912393005/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2912393005/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:107: <ignore regexp="chrome/android/webapk/shell_apk/res/drawable-*"/> nit: Can you add a comment saying that this is intentional to save on apk size?
Thanks! https://codereview.chromium.org/2912393005/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2912393005/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:107: <ignore regexp="chrome/android/webapk/shell_apk/res/drawable-*"/> On 2017/06/07 13:43:00, agrieve wrote: > nit: Can you add a comment saying that this is intentional to save on apk size? Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, tedchoc@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2912393005/#ps290001 (title: "Nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... how big is the image resource?
On 2017/06/07 14:41:59, Yaron wrote: > On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > > CQ is trying da patch. > > > > Follow status at: > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > how big is the image resource? The image is 5.5KB.
On 2017/06/07 14:46:34, Xi Han wrote: > On 2017/06/07 14:41:59, Yaron wrote: > > On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > > > CQ is trying da patch. > > > > > > Follow status at: > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > how big is the image resource? > > The image is 5.5KB. Yikes! That's over a 15% increase just for this asset. How bad does mdpi version look scaled up vs size savings? I think we may have to make this a little less pretty given the increased size for arguably an edge case
The CQ bit was unchecked by hanxi@chromium.org
On 2017/06/07 14:52:20, Yaron wrote: > On 2017/06/07 14:46:34, Xi Han wrote: > > On 2017/06/07 14:41:59, Yaron wrote: > > > On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. > > > > > > > > Follow status at: > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > how big is the image resource? > > > > The image is 5.5KB. > > Yikes! That's over a 15% increase just for this asset. How bad does mdpi version > look scaled up vs size savings? I think we may have to make this a little less > pretty given the increased size for arguably an edge case The image sizes are: 1. mdpi: 1.1KB 2. hdpi: 1.8KB 3. xhdpi: 2.4KB The screenshots on nexus5x are: https://drive.google.com/corp/drive/folders/0B7zEF5GgyYmpZEdmY1h4LXg5WlU I think the mdpi is a little bit blur, we could choose between hdpi and xhdpi.
Cache runtime host icon in hdpi now to save on APK size.
On 2017/06/07 14:52:20, Yaron wrote: > On 2017/06/07 14:46:34, Xi Han wrote: > > On 2017/06/07 14:41:59, Yaron wrote: > > > On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. > > > > > > > > Follow status at: > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > how big is the image resource? > > > > The image is 5.5KB. > > Yikes! That's over a 15% increase just for this asset. How bad does mdpi version > look scaled up vs size savings? I think we may have to make this a little less > pretty given the increased size for arguably an edge case Complete tangent. Any thoughts on fetching the image dynamically if you really expect this to be an edge case?
On 2017/06/07 17:05:32, Ted C wrote: > On 2017/06/07 14:52:20, Yaron wrote: > > On 2017/06/07 14:46:34, Xi Han wrote: > > > On 2017/06/07 14:41:59, Yaron wrote: > > > > On 2017/06/07 14:24:31, commit-bot: I haz the power wrote: > > > > > CQ is trying da patch. > > > > > > > > > > Follow status at: > > > > > > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > > > how big is the image resource? > > > > > > The image is 5.5KB. > > > > Yikes! That's over a 15% increase just for this asset. How bad does mdpi > version > > look scaled up vs size savings? I think we may have to make this a little less > > pretty given the increased size for arguably an edge case > > Complete tangent. > > Any thoughts on fetching the image dynamically if you really expect this > to be an edge case? It is doable, but could be slow. So I still prefer to have them in the APK, and we control the icon density to make its size reasonable. Does Chrome do similar things?
hanxi@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, I use the hdpi icon instead. PTAL, thanks!
I dunno, I think I agree with Ted that it's ok to be slow in this case. If we're offline, user won't be able to install the browser anyway On Wednesday, June 7, 2017, <hanxi@chromium.org> wrote: > Hi Yaron, I use the hdpi icon instead. PTAL, thanks! > > https://codereview.chromium.org/2912393005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/06/07 19:42:22, chromium-reviews wrote: > I dunno, I think I agree with Ted that it's ok to be slow in this case. If > we're offline, user won't be able to install the browser anyway > > On Wednesday, June 7, 2017, <mailto:hanxi@chromium.org> wrote: > > > Hi Yaron, I use the hdpi icon instead. PTAL, thanks! > > > > https://codereview.chromium.org/2912393005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. This definitely isn't a trivial fix. File a bug crbug.com/730780 to track it. For the current CL, I would prefer to keep the cached icon, since it is only < 2KB.
On 2017/06/07 20:54:29, Xi Han wrote: > On 2017/06/07 19:42:22, chromium-reviews wrote: > > I dunno, I think I agree with Ted that it's ok to be slow in this case. If > > we're offline, user won't be able to install the browser anyway > > > > On Wednesday, June 7, 2017, <mailto:hanxi@chromium.org> wrote: > > > > > Hi Yaron, I use the hdpi icon instead. PTAL, thanks! > > > > > > https://codereview.chromium.org/2912393005/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > This definitely isn't a trivial fix. File a bug crbug.com/730780 to track it. > For the current CL, I would prefer to keep the cached icon, since it is only < > 2KB. ok, I guess we can come back to recoup this later so lgtm I do think it's worth keeping these as small as possible so we should be actively monitoring size on any non-trivial change to the shell apk
On 2017/06/07 20:54:29, Xi Han wrote: > On 2017/06/07 19:42:22, chromium-reviews wrote: > > I dunno, I think I agree with Ted that it's ok to be slow in this case. If > > we're offline, user won't be able to install the browser anyway > > > > On Wednesday, June 7, 2017, <mailto:hanxi@chromium.org> wrote: > > > > > Hi Yaron, I use the hdpi icon instead. PTAL, thanks! > > > > > > https://codereview.chromium.org/2912393005/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > This definitely isn't a trivial fix. File a bug crbug.com/730780 to track it. > For the current CL, I would prefer to keep the cached icon, since it is only < > 2KB. ok, I guess we can come back to recoup this later so lgtm I do think it's worth keeping these as small as possible so we should be actively monitoring size on any non-trivial change to the shell apk
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pkotwicz@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2912393005/#ps310001 (title: "Adopt hdpi logo image.")
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": 310001, "attempt_start_ts": 1497015799204000,
"parent_rev": "357c5990c47bb65c2022fe47ee6308b1e7aaf3fc", "commit_rev":
"dded0412e190a6f4857d10d7f9d865c4ab700f56"}
Message was sent while issue was closed.
Description was changed from ========== Add support for webapk without runtimeHost (part 2) Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. The screenshots are: https://drive.google.com/open?id=0B7zEF5GgyYmpZEdmY1h4LXg5WlU BUG=714737 ========== to ========== Add support for webapk without runtimeHost (part 2) Add a dialog to ask user to install a host browser for WebAPK when there isn't any installed browser supporting WebAPKs. The screenshots are: https://drive.google.com/open?id=0B7zEF5GgyYmpZEdmY1h4LXg5WlU BUG=714737 Review-Url: https://codereview.chromium.org/2912393005 Cr-Commit-Position: refs/heads/master@{#478278} Committed: https://chromium.googlesource.com/chromium/src/+/dded0412e190a6f4857d10d7f9d8... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:310001) as https://chromium.googlesource.com/chromium/src/+/dded0412e190a6f4857d10d7f9d8... |
