Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629573004:
Add a chrome://webapks page. (Closed)
|
Created:
3 years, 11 months ago by gonzalon Modified:
3 years, 10 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, oshima+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a chrome://webapks page.
Adds an about:webapks page with information about all installed Web APKs
on the device
Design doc: go/about-webapks
BUG=624111
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2629573004
Cr-Commit-Position: refs/heads/master@{#448747}
Committed: https://chromium.googlesource.com/chromium/src/+/87192d7728d4f0829ba7eba2c49bb61810b0ea1a
Patch Set 1 #Patch Set 2 : Adds an about:webapks page with information about all installed Web APKs on the device #
Total comments: 74
Patch Set 3 : Fixes code review style issues #Patch Set 4 : Adds comment to explain the reason of an empty param #
Total comments: 6
Patch Set 5 : Some style fixes #Patch Set 6 : Changes the arguments of the WebAPKs list callback to return a struct #Patch Set 7 : Makes the listWebApks method receive a callback on the Java side #
Total comments: 48
Patch Set 8 : Fixes some more style issues #
Total comments: 13
Patch Set 9 : Removes strings from i18n file #
Total comments: 48
Patch Set 10 : Adds an about:webapks page with information about all installed Web APKs on the device #
Total comments: 14
Patch Set 11 : Adds an about:webapks page with information about all installed Web APKs on the device #
Total comments: 33
Patch Set 12 : Add a chrome://webapks page. #
Total comments: 2
Patch Set 13 : Add a chrome://webapks page. #
Total comments: 30
Patch Set 14 : Add a chrome://webapks page. #Patch Set 15 : Add a chrome://webapks page. #
Total comments: 4
Patch Set 16 : Add a chrome://webapks page. #
Total comments: 6
Patch Set 17 : Add a chrome://webapks page. #Patch Set 18 : Add a chrome://webapks page. #
Total comments: 10
Patch Set 19 : Add a chrome://webapks page. #Patch Set 20 : Add a chrome://webapks page. #
Total comments: 2
Patch Set 21 : Add a chrome://webapks page. #
Total comments: 11
Patch Set 22 : Add a chrome://webapks page. #Patch Set 23 : Add a chrome://webapks page. #Patch Set 24 : Add a chrome://webapks page. #Patch Set 25 : Add a chrome://webapks page. #Messages
Total messages: 128 (56 generated)
The CQ bit was checked by gonzalon@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by gonzalon@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...
Description was changed from ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: https://docs.google.com/a/chromium.org/document/d/16fm7kirRh8fVnCjSLtgJcmnf6i... BUG=624111 ========== to ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: https://docs.google.com/a/chromium.org/document/d/16fm7kirRh8fVnCjSLtgJcmnf6i... BUG=624111 ==========
gonzalon@chromium.org changed reviewers: + hanxi@chromium.org, hartmanng@chromium.org, pkotwicz@chromium.org
Hi! Would you mind taking a look at this CL before I send it to the owners of each section? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. Update the year - it's 2017 now :) https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:14: * Called with information of one Web APK. When information is requested, it will be called once nit: for consistency, we generally spell it "WebAPK" without the space. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:188: intent.putExtra(ShortcutHelper.EXTRA_URL, "https://www.google.com/"); I don't understand why a URL is required at all, could you explain this a bit more? https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapk_info_callback.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapk_info_callback.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. s/2016/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.h:12: // A callback wrapper for fetching information regarding the Web APKs installed s/Web APKs/WebAPKs/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/(c) 2012/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:9: #include "base/callback_forward.h" this seems like a lot of includes for a small file - are all these actually necessary? https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/(c) 2012/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/(c) 2012/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.cc:7: #include "base/android/build_info.h" lots of includes again, you might be able to remove some https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_ui.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/(c) 2012/2017/ https://codereview.chromium.org/2629573004/diff/20001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/common/url_const... chrome/common/url_constants.cc:273: const char kChromeUIWebAPKsHost[] = "webapks"; this should maybe be wrapped in an #if defined(OS_ANDROID) like the rest of the new code in this file https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/B... File components/webapks_ui/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/B... components/webapks_ui/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. s/2015/2017/ https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. s/(c) 2012/2017/ https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. s/(c) 2011/2017/ https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... File components/webapks_ui/webapks_ui_constants.cc (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... components/webapks_ui/webapks_ui_constants.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. s/2015/2017/ https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... File components/webapks_ui/webapks_ui_constants.h (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... components/webapks_ui/webapks_ui_constants.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. s/2015/2017/
Overall looks good on the first round. Thank you gonzalon@ for taking care of this! https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:10: public class WebapkInfoCallback { Anyone has suggestion about the naming of this class? https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:48: static final String WEB_APK_PACKAGE_NAME_PREFIX = "org.chromium.webapk"; You can use WebApkConstants#WEBAPK_PACKAGE_PREFIX https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:181: static void listWebAPKs(WebapkInfoCallback webapkInfoCallback) { Usually we put helper functions in ShorcutHelper. WebAppRegister is used to register WebApp or WebAPK in the shared preference, it might not be the right place. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:189: WebApkInfo webApkInfo = WebApkInfo.create(intent); I understand you want to reuse the code, but it is weird to create a WebApkInfo which is barely used. My suggestion is: 1) Make WebApkInfo#extractWebApkMetaData() public, so call it to get the metadata info. 2) Use IntentUtils.safeGet...() to get the meta data needed: String shortName = IntentUtils.safeGetString(bundle, WebApkMetaDataKeys.SHORT_NAME); 3) I remembered that we also want to have the icon, don't we? You can follow this example to get the icon: int iconId = IntentUtils.safeGetInt(bundle, WebApkMetaDataKeys.ICON_ID, 0); Bitmap icon = decodeImageResource(webApkPackageName, iconId); Note: make decodeImageResource as public too. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/webapp_registry.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:35: WebapkInfoCallback* webapk_info_callback = new WebapkInfoCallback(callback); Please add a comment that the WebApkInfoCallback will delete itself after it is done. "WebapkInfoCallback" -> "WebApkInfoCallback" with capital A. We have naming conventions discussions for WebAPK when we began to checkin code to Chromium, and now we all follow the following pattern: 1) in comments (c++/Java/documentation): use WebAPK 2) in code like function/parameter name, use WebApk 3) in c++ file name: webapk_installer.h (.cc) My apology that I didn't mention it before:( https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:39: webapk_info_callback->j_webapk_info_callback()); It is better to move this JNI call into WebApkInfoCallback, so we don't need to expose the Java pointer to the caller. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... File components/webapks_ui_strings.grdp (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:9: <message name="IDS_WEBAPKS_UI_PACKAGE_NAME" desc="Label for the package name of a Web APK"> WebAPK https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:12: <message name="IDS_WEBAPKS_UI_SHELL_APK_VERSION" desc="Label for the shell APK version of a Web APK"> WebAPK https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:15: <message name="IDS_WEBAPKS_UI_VERSION_CODE" desc="Label for the version code of a Web APK"> WebAPK
Still doing the code review. Some initial comments https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:184: if (packageInfo.packageName.startsWith(WEB_APK_PACKAGE_NAME_PREFIX)) { Perhaps, you can call WebApkValidator#isValidWebApk() here. Note that WebApkValidator#isValidWebApk() is currently private https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:189: WebApkInfo webApkInfo = WebApkInfo.create(intent); Can you create a new version of WebApkInfo#create() which takes in just: - The WebAPK package name - The URL - The source I would use an empty URL instead of "https://www.google.com/" (and as Glenn mentioned add a comment why the URL is empty instead of null)
https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:189: WebApkInfo webApkInfo = WebApkInfo.create(intent); I had code elsewhere which looked like what you are suggesting and Dominick did not like it. I am not very concerned about not really using WebApkInfo because this is a function which will not be called often https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:196: appName, packageName, shellApkVersion, versionCode); Nit: Inline lines 191 - 194
Still reviewing but need to go for lunch https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:29: public void destroy() { I must be missing something. Who calls destroy()? https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/webapp_registry.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:34: callback) { I think that it would be clearer if this function returned an array. We do something similar in WebApkUpdateManager#updateAsync() Yes converting to an array is suboptimal, but: - There is likely going to be 1 or 2 installed WebAPKs - PackageManager#getInstalledPackages() is slow. - The chrome://webapk page should be rarely shown. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.html:36: <script src="chrome://resources/js/i18n_template.js"></script> Nit: You did not close the outer <div> https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:9: function boldedTextNode(text, className) { Can you instead: Call createElementWithClassName() with <span> Bold the text via CSS https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:11: node.appendChild(document.createTextNode(text)); Can you set the text by setting node.textContent ? https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:18: * This will be called once for each web APK available on the device and each "web APK" -> WebAPK https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:21: function returnWebAPKsInfo(appName, packageName, shellApkVersion, versionCode) { Can this function be called just once with the info for all of the WebAPKs. The history page does this with history entries I think. BrowsingHistoryHandler::OnQueryComplete()
https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:189: WebApkInfo webApkInfo = WebApkInfo.create(intent); On 2017/01/13 16:23:19, pkotwicz wrote: > I had code elsewhere which looked like what you are suggesting and Dominick did > not like it. I am not very concerned about not really using WebApkInfo because > this is a function which will not be called often That is fine, we can defer it to Dominick:)
The CQ bit was checked by gonzalon@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...
A lot of lines were added to the BUILD file, but I didn't add them, and I don't think I rebased either. Maybe I did something wrong elsewhere, is there an easy way to fix it? https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > Update the year - it's 2017 now :) I copy pasted this everywhere without checking the year, sorry :( https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:14: * Called with information of one Web APK. When information is requested, it will be called once On 2017/01/13 15:40:21, hartmanng wrote: > nit: for consistency, we generally spell it "WebAPK" without the space. Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebapkInfoCallback.java:29: public void destroy() { On 2017/01/13 17:00:58, pkotwicz wrote: > I must be missing something. Who calls destroy()? I forgot to add the call, I added it on the listWebApks method. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:48: static final String WEB_APK_PACKAGE_NAME_PREFIX = "org.chromium.webapk"; On 2017/01/13 15:50:15, Xi Han wrote: > You can use WebApkConstants#WEBAPK_PACKAGE_PREFIX Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:181: static void listWebAPKs(WebapkInfoCallback webapkInfoCallback) { On 2017/01/13 15:50:15, Xi Han wrote: > Usually we put helper functions in ShorcutHelper. WebAppRegister is used to > register WebApp or WebAPK in the shared preference, it might not be the right > place. Sounds good! Sorry, I just didn't know where to put it, there are a lot of classes! Moved it there. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:184: if (packageInfo.packageName.startsWith(WEB_APK_PACKAGE_NAME_PREFIX)) { On 2017/01/13 16:00:54, pkotwicz wrote: > Perhaps, you can call WebApkValidator#isValidWebApk() here. Note that > WebApkValidator#isValidWebApk() is currently private SG, done https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:188: intent.putExtra(ShortcutHelper.EXTRA_URL, "https://www.google.com/"); On 2017/01/13 15:40:21, hartmanng wrote: > I don't understand why a URL is required at all, could you explain this a bit > more? As Xi mentioned in the other comment, it was an attempt to reuse the code that was already there, but I'll change it to the approach she described, which will make this code clearer and won't require this URL anymore. https://codereview.chromium.org/2629573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:196: appName, packageName, shellApkVersion, versionCode); On 2017/01/13 16:23:19, pkotwicz wrote: > Nit: Inline lines 191 - 194 Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/webapp_registry.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:34: callback) { On 2017/01/13 17:00:58, pkotwicz wrote: > I think that it would be clearer if this function returned an array. > We do something similar in WebApkUpdateManager#updateAsync() > > Yes converting to an array is suboptimal, but: > - There is likely going to be 1 or 2 installed WebAPKs > - PackageManager#getInstalledPackages() is slow. > - The chrome://webapk page should be rarely shown. Returning an array was my first approach too, it would make the whole thing much clearer, but I don't know how to do that here. I looked at WebApkUpdateManager#updateAsync() but that method only returns one WebAPK, not many. I'd have to return a list of objects that have several fields, but when I asked I was told that's not possible with JNI as we use it here, maybe I'm mistaken. Could you clarify how to do this? https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:35: WebapkInfoCallback* webapk_info_callback = new WebapkInfoCallback(callback); On 2017/01/13 15:50:15, Xi Han wrote: > Please add a comment that the WebApkInfoCallback will delete itself after it is > done. > > "WebapkInfoCallback" -> "WebApkInfoCallback" with capital A. > > We have naming conventions discussions for WebAPK when we began to checkin code > to Chromium, and now we all follow the following pattern: > 1) in comments (c++/Java/documentation): use WebAPK > 2) in code like function/parameter name, use WebApk > 3) in c++ file name: webapk_installer.h (.cc) > > My apology that I didn't mention it before:( Good to know! Thanks for explaining. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapk_info_callback.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/2016/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapk_info_callback.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/2016/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapk_info_callback.h:12: // A callback wrapper for fetching information regarding the Web APKs installed On 2017/01/13 15:40:21, hartmanng wrote: > s/Web APKs/WebAPKs/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/(c) 2012/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:9: #include "base/callback_forward.h" On 2017/01/13 15:40:21, hartmanng wrote: > this seems like a lot of includes for a small file - are all these actually > necessary? Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/(c) 2012/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/(c) 2012/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.cc:7: #include "base/android/build_info.h" On 2017/01/13 15:40:21, hartmanng wrote: > lots of includes again, you might be able to remove some Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_ui.h (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_ui.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/(c) 2012/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/common/url_const... chrome/common/url_constants.cc:273: const char kChromeUIWebAPKsHost[] = "webapks"; On 2017/01/13 15:40:21, hartmanng wrote: > this should maybe be wrapped in an #if defined(OS_ANDROID) like the rest of the > new code in this file Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/B... File components/webapks_ui/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/B... components/webapks_ui/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/2015/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:21, hartmanng wrote: > s/(c) 2012/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.html:36: <script src="chrome://resources/js/i18n_template.js"></script> On 2017/01/13 17:00:58, pkotwicz wrote: > Nit: You did not close the outer <div> Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:22, hartmanng wrote: > s/(c) 2011/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:9: function boldedTextNode(text, className) { On 2017/01/13 17:00:59, pkotwicz wrote: > Can you instead: > Call createElementWithClassName() with <span> > Bold the text via CSS Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:11: node.appendChild(document.createTextNode(text)); On 2017/01/13 17:00:59, pkotwicz wrote: > Can you set the text by setting node.textContent ? Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:18: * This will be called once for each web APK available on the device and each On 2017/01/13 17:00:59, pkotwicz wrote: > "web APK" -> WebAPK Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/r... components/webapks_ui/resources/about_webapks.js:21: function returnWebAPKsInfo(appName, packageName, shellApkVersion, versionCode) { On 2017/01/13 17:00:59, pkotwicz wrote: > Can this function be called just once with the info for all of the WebAPKs. > > The history page does this with history entries I think. > BrowsingHistoryHandler::OnQueryComplete() From what I understood, the history page works a bit differently in the C++ <-> Java communication. The reason I did it this way is because I can't return `WebApkInfo[]` in the call from C++ to Java (because of JNI limitations, correct me if I'm wrong). The only way to make this receive a list I can think of would be to store the results of the callback somewhere (maybe the WebApkInfoCallback?) and when all of the information is received, put in on a list and call a second callback that calls this JS code with the list of information. So, to summarize, WebApkInfoCallback (C++) calls ShortcutHelper (Java) and stores all results on a list. When it finishes, it calls a callback to return the whole list at once to WebApksHandler (C++) which just gives the list to the Javascript code. Does that sound good or did you have a different implementation in mind? https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... File components/webapks_ui/webapks_ui_constants.cc (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... components/webapks_ui/webapks_ui_constants.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:22, hartmanng wrote: > s/2015/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... File components/webapks_ui/webapks_ui_constants.h (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui/w... components/webapks_ui/webapks_ui_constants.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/01/13 15:40:22, hartmanng wrote: > s/2015/2017/ Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... File components/webapks_ui_strings.grdp (right): https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:9: <message name="IDS_WEBAPKS_UI_PACKAGE_NAME" desc="Label for the package name of a Web APK"> On 2017/01/13 15:50:15, Xi Han wrote: > WebAPK Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:12: <message name="IDS_WEBAPKS_UI_SHELL_APK_VERSION" desc="Label for the shell APK version of a Web APK"> On 2017/01/13 15:50:15, Xi Han wrote: > WebAPK Done. https://codereview.chromium.org/2629573004/diff/20001/components/webapks_ui_s... components/webapks_ui_strings.grdp:15: <message name="IDS_WEBAPKS_UI_VERSION_CODE" desc="Label for the version code of a Web APK"> On 2017/01/13 15:50:15, Xi Han wrote: > WebAPK Done.
Which BUILD file? They all look good to me. https://codereview.chromium.org/2629573004/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:16: import org.chromium.webapk.lib.common.WebApkConstants; Is this change necessary? https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/android/... chrome/browser/android/shortcut_helper.h:8: #include <string> Revert this? https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:30: weak_ptr_factory_.GetWeakPtr()); Please add a commend here: "WebApkInfoCallback will delete itself after it is done."
Fixed the corrections, thanks a lot for the review! https://codereview.chromium.org/2629573004/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:16: import org.chromium.webapk.lib.common.WebApkConstants; On 2017/01/16 16:56:19, Xi Han wrote: > Is this change necessary? I could finally figure out what was happening. The auto importer puts the static import after all of the other imports, but when that happens I can't push the code, because I get an error. I manually put the static import on top and now it works, it was just a matter of setting up android studio to change the import order to fit Google's guidelines. https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/android/... chrome/browser/android/shortcut_helper.h:8: #include <string> On 2017/01/16 16:56:19, Xi Han wrote: > Revert this? I deleted this, but the reason I added it in the first place is that the linter throws a warning otherwise. This file depends on the <string> include, it's just that we can get it indirectly from other files, which I'm not sure it's the right way to do it. Anyways, this is like this all over the codebase, so I guess it's fine. https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:30: weak_ptr_factory_.GetWeakPtr()); On 2017/01/16 16:56:19, Xi Han wrote: > Please add a commend here: "WebApkInfoCallback will delete itself after it is > done." Done.
https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/webapp_registry.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:34: callback) { I recommend doing this by making WebappRegistry::ListWebApks() call WebappRegistry::ListWebAPKsInternal(const base::Callback<void(std::vector<std::string>, std::vector<std::string>, std::vector<int>, std::vector<int>)>& callback) To get an array of objects from C++ to Java you can use the technique used by BookmarkBridge::GetBookmarksForFolder(). I don't recommend using this technique to get an array of objects from Java to C++ because it would require way too many JNI calls
W.r.t to the BUILD.gn files, when you diff the BUILD.gn file of one patch set with another you see the difference between the BUILD.gn file at the time that you uploaded the initial patch set to the BUILD.gn file now (e.g. one week later) after having rebased to master many times. The "View" link is much more helpful for BUILD.gn files. The Chromium code base changes really really quickly
On 2017/01/17 03:40:13, pkotwicz wrote: > W.r.t to the BUILD.gn files, when you diff the BUILD.gn file of one patch set > with another you see the difference between the BUILD.gn file at the time that > you uploaded the initial patch set to the BUILD.gn file now (e.g. one week > later) after having rebased to master many times. The "View" link is much more > helpful for BUILD.gn files. The Chromium code base changes really really quickly Thanks for the explanation. I thought something like this might be happening but it's much clearer now :)
https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/webapp_registry.cc (right): https://codereview.chromium.org/2629573004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/webapp_registry.cc:34: callback) { On 2017/01/17 03:20:16, pkotwicz wrote: > I recommend doing this by making WebappRegistry::ListWebApks() call > WebappRegistry::ListWebAPKsInternal(const > base::Callback<void(std::vector<std::string>, > > std::vector<std::string>, > std::vector<int>, > > std::vector<int>)>& callback) > > To get an array of objects from C++ to Java you can use the technique used by > BookmarkBridge::GetBookmarksForFolder(). I don't recommend using this technique > to get an array of objects from Java to C++ because it would require way too > many JNI calls I did something very similar to what you recommended, only instead of returning separate lists for each field I just created a wrapper struct with all of them. I think this makes it less error prone, but let me know what you think.
As discussed online I think that the solution that I suggested using multiple vectors is cleaner. I think you can delete webapk_info_callback.cc I think that you can pass a base::Callback to Java the way that ShortcutHelper::AddWebappWithSkBitmap() does (A base::Closure is a base::Callback with void return type) The base::Closure is passed back from Java to C++ in OnWebappDataStored()
As discussed online I think that the solution that I suggested using multiple vectors is cleaner. I think you can delete webapk_info_callback.cc I think that you can pass a base::Callback to Java the way that ShortcutHelper::AddWebappWithSkBitmap() does (A base::Closure is a base::Callback with void return type) The base::Closure is passed back from Java to C++ in OnWebappDataStored()
As discussed online I think that the solution that I suggested using multiple vectors is cleaner. I think you can delete webapk_info_callback.cc I think that you can pass a base::Callback to Java the way that ShortcutHelper::AddWebappWithSkBitmap() does (A base::Closure is a base::Callback with void return type) The base::Closure is passed back from Java to C++ in OnWebappDataStored()
Implemented the changes we discussed offline.
Looks really good. Once you address my comments I need to double check your comments and then l-g-t-m https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:663: List<String> shortNames = new LinkedList<>(); Can you explain why you chose a LinkedList instead of an ArrayList? Miscellaneous trivia: The initial capacity of a java.util.ArrayList is 10 https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:674: WebApkInfo webApkInfo = WebApkInfo.create(packageInfo.packageName, "", 0); Nit: 0 -> ShortcutSource.UNKNOWN https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:682: } Nit: For the sake of clarity I would do: String[] shortNamesArray = shortNames.toArray(new String[shortNames.size()]); nativeOnWebApksFound(callbackPointer, shortNamesArray, ...); This is equally correct and shorter: String[] shortNamesArray = shortNames.toArray(new String[0]); https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:688: private static int[] integerListToIntArray(@NonNull List<Integer> list) { +agrieve@ Cool! I did not know about this annotation. Andrew is our local expert on all of the build stuff https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:691: for (Integer integer : list) { Nit: integer -> value I'm not a fan of naming a variable |integer| https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:62: * manifest. Nit: "passed as parameter ..." -> "passed in parameters ..." https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:71: if (info.activityInfo != null Can you move line 72 to the isValidWebApk() function? https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:285: void ShortcutHelper::ListWebApks(JNIEnv* env, Nit: Remove the JNIEnv* parameter and call AttachCurrentThread() inside this function https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<WebApkInfo*> webapks_list; Can this be std::vector<WebApkInfo> instead? https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:8: #include <string> Nit: This include is unnecessary https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:124: const base::Callback<void(std::vector<WebApkInfo*>)>& callback); You should typedef to make your life easier typedeffing callbacks is super common in Chromium https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:12: WebApkInfo(std::string short_name, Nit: These should be const references (const std::string& short_name) and so forth https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:16: WebApkInfo(const WebApkInfo& other); You can remove the copy constructor. The compiler adds a copy constructor to structs by default. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:21: webapks_ui::kRequestWebApksInfo, Nit: You can inline the JS method name here the way that browsing_history_handler.cc does https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:35: base::ListValue* list = new base::ListValue(); Can you create the value on the stack instead? In Chromium, we generally avoid raw pointers. We often use std::unique_ptr<> to hold pointers. In this case, it looks like you can do: base::ListValue list; Note that the second parameter in WebUI::CallJavascriptFunctionUnsafe() is a const reference https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:38: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()); Awesome! Creating base::DictionaryValue on the heap to avoid a copy on line 44 makes perfect sense https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:17: // Handler class for WebAPKs page operations. Nit: WebAPKs -> WebAPK https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:29: <br><br> Nit: I think that <p> is the same as two <br>s https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:10: var node = document.createElement('span'); You should use createElementWithClassName() to make your code slightly shorter https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:30: webApksList.appendChild(document.createElement("BR")); Nit: It looks like most of the other places in the code use lowercase for tags https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui_... File components/webapks_ui_strings.grdp (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui_... components/webapks_ui_strings.grdp:3: <message name="IDS_WEBAPKS_UI_TITLE" desc="Title on the about:webapks page"> chrome://webapks is an internal only debugging page like chrome://sync-internals and does not need to be translated (Btw chrome://sync-internals is super cool) That means that you don't need any of the fancy grd stuff and you can put the strings right into the html file
Thanks for you time reviewing! https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:663: List<String> shortNames = new LinkedList<>(); On 2017/01/18 02:08:20, pkotwicz wrote: > Can you explain why you chose a LinkedList instead of an ArrayList? > > Miscellaneous trivia: The initial capacity of a java.util.ArrayList is 10 I only use ArrayList when I'm sure of the final size of the array or if I'm going to be querying by index a lot. In this case, since there probably aren't going to be many elements anyways, it's the same performance-wise to use one or the other, so I can use ArrayList if you prefer. https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:674: WebApkInfo webApkInfo = WebApkInfo.create(packageInfo.packageName, "", 0); On 2017/01/18 02:08:20, pkotwicz wrote: > Nit: 0 -> ShortcutSource.UNKNOWN Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:682: } On 2017/01/18 02:08:20, pkotwicz wrote: > Nit: For the sake of clarity I would do: > String[] shortNamesArray = shortNames.toArray(new String[shortNames.size()]); > > nativeOnWebApksFound(callbackPointer, shortNamesArray, ...); > > This is equally correct and shorter: > String[] shortNamesArray = shortNames.toArray(new String[0]); It's not exactly the same, if the size of the array is too small, a new one with the correct size is instantiated. I guess the hit of wasting an array of size 1 is negligible. Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:688: private static int[] integerListToIntArray(@NonNull List<Integer> list) { On 2017/01/18 02:08:20, pkotwicz wrote: > +agrieve@ > > Cool! I did not know about this annotation. Andrew is our local expert on all of > the build stuff This annotation doesn't actually enforce the argument not to be null, but if you try to pass a null value (or a value that might be null) to this function, Android Studio will highlight it with a warning. https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:691: for (Integer integer : list) { On 2017/01/18 02:08:20, pkotwicz wrote: > Nit: integer -> value > > I'm not a fan of naming a variable |integer| Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:285: void ShortcutHelper::ListWebApks(JNIEnv* env, On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: Remove the JNIEnv* parameter and call AttachCurrentThread() inside this > function Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<WebApkInfo*> webapks_list; On 2017/01/18 02:08:21, pkotwicz wrote: > Can this be std::vector<WebApkInfo> instead? Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:8: #include <string> On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: This include is unnecessary I think that's because it's being indirectly included from the other imports, unless there's some Google-specific thing I'm not accounting for. This class makes use of std::string directly, so I think it should have the corresponding include. Anyways, I removed it since I may not be aware of some Google convention. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:124: const base::Callback<void(std::vector<WebApkInfo*>)>& callback); On 2017/01/18 02:08:21, pkotwicz wrote: > You should typedef to make your life easier > > typedeffing callbacks is super common in Chromium Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:21: webapks_ui::kRequestWebApksInfo, On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: You can inline the JS method name here the way that > browsing_history_handler.cc does Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:35: base::ListValue* list = new base::ListValue(); On 2017/01/18 02:08:21, pkotwicz wrote: > Can you create the value on the stack instead? In Chromium, we generally avoid > raw pointers. We often use std::unique_ptr<> to hold pointers. > > In this case, it looks like you can do: > base::ListValue list; > > Note that the second parameter in WebUI::CallJavascriptFunctionUnsafe() is a > const reference Yeah, not sure why I used so many pointers, it's better this way, thanks for pointing it out!
Looking at your CL now. Sorry about the delay
@pkotwicz Removed the strings file as we talked offline.
pkotwicz@chromium.org changed reviewers: + agrieve@chromium.org
Mostly comment nits. There are a few comments from the last round of review that you missed as well. +agrieve@ for: @NonNull annotation need compress="gzip" in webapks_ui_resources.grdp https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:71: if (info.activityInfo != null ^^^ I think you missed this comment https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:8: #include <string> Yes, you are right. I does use std::string so it should have the include https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:12: WebApkInfo(std::string short_name, ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:16: WebApkInfo(const WebApkInfo& other); ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:29: <br><br> ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:10: var node = document.createElement('span'); ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:30: webApksList.appendChild(document.createElement("BR")); ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:657: * caller using a callback. Nit: "installed on the device" -> "installed" I think that "on the device" is redundant. Shorter comments are easier to read :) https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:659: * @param callbackPointer Callback to be called with the information on the WebAPKs found. Nit: "to be called" -> "to call" https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:673: // information about the WebAPK. How about: "Pass non-null URL parameter so that {@link WebApkInfo#create()} return value is non-null." https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:678: packageNames.add(packageInfo.packageName); Nit: packageInfo.packageName -> webApkInfo.webApkPackageName() https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:62: * manifest. Nit: "the passed as parameter" -> "the passed in parameters" https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<WebApkInfo> webapks_list; Nit: webapks_list -> webapk_list https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:336: ShortcutHelper::WebApkInfoCallback* webapks_list_callback = Nit: webapks_list_callback -> webapk_list_callback https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<WebApkInfo>)> WebApkInfoCallback; Nit: The argument should be a const reference base::Callback<void(std::vector<const WebApkInfo&>)> WebApkInfoCallback; https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:569: "//components/webapks_ui", When we remove the dedicated constants file we should be able to remove this line too https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:9: #include "chrome/browser/profiles/profile.h" I don't think that this include is needed https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:10: #include "chrome/grit/generated_resources.h" I don't think that this include is needed https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:11: #include "components/strings/grit/components_strings.h" I don't think that this include is needed https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:29: ShortcutHelper::ListWebApks(callback); Nit: Inline |callback|. I think that it is cleaner. ShortcutHelper::ListWebApks(base::Bind(&WebApksHandler::OnWebApkInfoReceived, weak_ptr_factory_.GetWeakPtr()) https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:8: #include <string> I don't think this include is needed https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:13: #include "base/values.h" Nit: You can forward declare base::ListValue and move the include to the .cc file https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:14: #include "chrome/browser/android/webapk/webapk_info.h" I think you can also forward declare WebApkInfo https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:17: // Handler class for WebAPKs page operations. How about: "Handles JavaScript messages from the chrome://webapks page." https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:28: // OnWebApkInfoReceived How about: "Handler for the "requestWebApksInfo" message. This requests information for the installed WebAPKs and returns it to JS using OnWebApkInfoReceived()." - I think that putting the () makes it obvious that a method is a method - I think that "from the device" is redundant. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:33: // front end. This comment looks out of date. How about: "Sends information for the installed WebAPKs to JS." https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:14: #include "components/strings/grit/components_strings.h" I don't think that you need any of the includes on line 10,11,13,14 https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:15: #include "components/webapks_ui/webapks_ui_constants.h" We are getting rid of the constants file so we should be able to remove this too https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:16: #include "content/public/browser/url_data_source.h" I don't think that you need this include https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:36: } // namespace Nit "namespace" -> "anonymous namespace" https://codereview.chromium.org/2629573004/diff/160001/components/resources/w... File components/resources/webapks_ui_resources.grdp (right): https://codereview.chromium.org/2629573004/diff/160001/components/resources/w... components/resources/webapks_ui_resources.grdp:3: <include name="IDR_WEBAPKS_UI_CSS" file="../webapks_ui/resources/about_webapks.css" type="BINDATA" /> I think you need compress="gzip" as well. You can double check with Andrew. He's the expert. https://codereview.chromium.org/1968993002 https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.css:44: vertical-align: top; Stupid question: Do you need vertical-align: top ? https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:31: <span class="label">WebAPKs</span> Nit: I think you can make this WebAPKs: (with the colon) and get rid of the special CSS which adds the colon. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:31: webApksList.appendChild(document.createElement("BR")); Two <br>s are one <p> https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/webapks_ui_constants.cc (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:11: const char kWebApksJS[] = "webapks.js"; I think that these constants can be put in the anonymous namespace in chrome/browser/ui/webui/webapks_ui.cc (Though you can inline these constants too) https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:14: const char kRequestWebApksInfo[] = "requestWebApksInfo"; I think that this constant can be inlined. e.g. https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_internals_ui.cc?... https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:15: const char kReturnWebApksInfo[] = "returnWebApksInfo"; I think that this constant can be inlined. e.g. https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_internals_ui.cc?...
https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:688: private static int[] integerListToIntArray(@NonNull List<Integer> list) { On 2017/01/18 15:29:32, gonzalon wrote: > On 2017/01/18 02:08:20, pkotwicz wrote: > > +agrieve@ > > > > Cool! I did not know about this annotation. Andrew is our local expert on all > of > > the build stuff > > This annotation doesn't actually enforce the argument not to be null, but if you > try to pass a null value (or a value that might be null) to this function, > Android Studio will highlight it with a warning. Yep, would love to see more support annotations being added!
I think I addressed all of your comments. I'm not very familiar with the chromium code review tool's UI yet, so I sometimes seem to miss comments, they don't always show up. Please let me know if you see that I missed any others. https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:71: if (info.activityInfo != null On 2017/01/18 02:08:21, pkotwicz wrote: > Can you move line 72 to the isValidWebApk() function? Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:12: WebApkInfo(std::string short_name, On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: These should be const references (const std::string& short_name) and so > forth Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:16: WebApkInfo(const WebApkInfo& other); On 2017/01/18 02:08:21, pkotwicz wrote: > You can remove the copy constructor. The compiler adds a copy constructor to > structs by default. Done. https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:17: // Handler class for WebAPKs page operations. On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: WebAPKs -> WebAPK Done. https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:29: <br><br> On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: I think that <p> is the same as two <br>s Done. https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/120001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:30: webApksList.appendChild(document.createElement("BR")); On 2017/01/18 02:08:21, pkotwicz wrote: > Nit: It looks like most of the other places in the code use lowercase for tags Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:657: * caller using a callback. On 2017/01/19 19:37:16, pkotwicz wrote: > Nit: "installed on the device" -> "installed" > > I think that "on the device" is redundant. Shorter comments are easier to read > :) Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:659: * @param callbackPointer Callback to be called with the information on the WebAPKs found. On 2017/01/19 19:37:16, pkotwicz wrote: > Nit: "to be called" -> "to call" Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:673: // information about the WebAPK. On 2017/01/19 19:37:16, pkotwicz wrote: > How about: "Pass non-null URL parameter so that {@link WebApkInfo#create()} > return value is non-null." Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:678: packageNames.add(packageInfo.packageName); On 2017/01/19 19:37:16, pkotwicz wrote: > Nit: packageInfo.packageName -> webApkInfo.webApkPackageName() Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<WebApkInfo> webapks_list; On 2017/01/19 19:37:17, pkotwicz wrote: > Nit: webapks_list -> webapk_list Done. https://codereview.chromium.org/2629573004/diff/140001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:336: ShortcutHelper::WebApkInfoCallback* webapks_list_callback = On 2017/01/19 19:37:17, pkotwicz wrote: > Nit: webapks_list_callback -> webapk_list_callback Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<WebApkInfo>)> WebApkInfoCallback; On 2017/01/19 19:37:17, pkotwicz wrote: > Nit: The argument should be a const reference > > base::Callback<void(std::vector<const WebApkInfo&>)> WebApkInfoCallback; Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/BUIL... chrome/browser/ui/BUILD.gn:569: "//components/webapks_ui", On 2017/01/19 19:37:17, pkotwicz wrote: > When we remove the dedicated constants file we should be able to remove this > line too Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:9: #include "chrome/browser/profiles/profile.h" On 2017/01/19 19:37:17, pkotwicz wrote: > I don't think that this include is needed Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:10: #include "chrome/grit/generated_resources.h" On 2017/01/19 19:37:17, pkotwicz wrote: > I don't think that this include is needed Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:11: #include "components/strings/grit/components_strings.h" On 2017/01/19 19:37:17, pkotwicz wrote: > I don't think that this include is needed Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:29: ShortcutHelper::ListWebApks(callback); On 2017/01/19 19:37:17, pkotwicz wrote: > Nit: Inline |callback|. I think that it is cleaner. > > ShortcutHelper::ListWebApks(base::Bind(&WebApksHandler::OnWebApkInfoReceived, > weak_ptr_factory_.GetWeakPtr()) Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:8: #include <string> On 2017/01/19 19:37:17, pkotwicz wrote: > I don't think this include is needed Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:17: // Handler class for WebAPKs page operations. On 2017/01/19 19:37:17, pkotwicz wrote: > How about: "Handles JavaScript messages from the chrome://webapks page." Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:28: // OnWebApkInfoReceived On 2017/01/19 19:37:17, pkotwicz wrote: > How about: "Handler for the "requestWebApksInfo" message. This requests > information for the installed WebAPKs and returns it to JS using > OnWebApkInfoReceived()." > > - I think that putting the () makes it obvious that a method is a method > - I think that "from the device" is redundant. Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:33: // front end. On 2017/01/19 19:37:17, pkotwicz wrote: > This comment looks out of date. How about: > "Sends information for the installed WebAPKs to JS." Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:14: #include "components/strings/grit/components_strings.h" On 2017/01/19 19:37:18, pkotwicz wrote: > I don't think that you need any of the includes on line 10,11,13,14 Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:15: #include "components/webapks_ui/webapks_ui_constants.h" On 2017/01/19 19:37:18, pkotwicz wrote: > We are getting rid of the constants file so we should be able to remove this too Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:16: #include "content/public/browser/url_data_source.h" On 2017/01/19 19:37:17, pkotwicz wrote: > I don't think that you need this include Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:36: } // namespace On 2017/01/19 19:37:18, pkotwicz wrote: > Nit "namespace" -> "anonymous namespace" Done. https://codereview.chromium.org/2629573004/diff/160001/components/resources/w... File components/resources/webapks_ui_resources.grdp (right): https://codereview.chromium.org/2629573004/diff/160001/components/resources/w... components/resources/webapks_ui_resources.grdp:3: <include name="IDR_WEBAPKS_UI_CSS" file="../webapks_ui/resources/about_webapks.css" type="BINDATA" /> On 2017/01/19 19:37:18, pkotwicz wrote: > I think you need compress="gzip" as well. > > You can double check with Andrew. He's the expert. > https://codereview.chromium.org/1968993002 Done. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:31: <span class="label">WebAPKs</span> On 2017/01/19 19:37:18, pkotwicz wrote: > Nit: I think you can make this WebAPKs: (with the colon) and get rid of the > special CSS which adds the colon. Done. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:31: webApksList.appendChild(document.createElement("BR")); On 2017/01/19 19:37:18, pkotwicz wrote: > Two <br>s are one <p> Didn't know about that :P https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:31: webApksList.appendChild(document.createElement("BR")); On 2017/01/19 19:37:18, pkotwicz wrote: > Two <br>s are one <p> I made this change, but it didn't work. I tried it on a sandbox and it's not the same unfortunately, the <p> tag doesn't leave a space like the double <br> does. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... File components/webapks_ui/webapks_ui_constants.cc (right): https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:11: const char kWebApksJS[] = "webapks.js"; On 2017/01/19 19:37:18, pkotwicz wrote: > I think that these constants can be put in the anonymous namespace in > chrome/browser/ui/webui/webapks_ui.cc (Though you can inline these constants > too) Done. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:14: const char kRequestWebApksInfo[] = "requestWebApksInfo"; On 2017/01/19 19:37:18, pkotwicz wrote: > I think that this constant can be inlined. e.g. > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_internals_ui.cc?... Done. https://codereview.chromium.org/2629573004/diff/160001/components/webapks_ui/... components/webapks_ui/webapks_ui_constants.cc:15: const char kReturnWebApksInfo[] = "returnWebApksInfo"; On 2017/01/19 19:37:18, pkotwicz wrote: > I think that this constant can be inlined. e.g. > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_internals_ui.cc?... Done.
pkotwicz@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@ for code review of HTML/JS/CSS and chrome/browser/ui/webui dominickn@ for webapp and WebAPK code The best way of seeing all of the comments is by looking at the messages in the code review. For instance, all of the comments for all of the patch sets that I made during the previous code review are aggregated in message #36 (https://codereview.chromium.org/2629573004/#msg36) LGTM with comments https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:62: * manifest. ^^^ I think you missed this comment https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<WebApkInfo>)> WebApkInfoCallback; Sorry for the incorrect advice. The argument should be base::Callback<void(const std::vector<WebApkInfo>&)> WebApkInfoCallback; https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:13: #include "base/values.h" ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:14: #include "chrome/browser/android/webapk/webapk_info.h" ^^^ I think that you missed this comment https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:7: WebApkInfo::WebApkInfo(const std::string short_name, This should be: const std::string& short_name, Passing the parameter as a reference avoids a copy https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:8: const std::string package_name, This should be: const std::string& package_name, https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:9: const int shell_apk_version, This does not need to be const. Passing ints by copy is fine https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:10: const int version_code) This does not need to be const. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:22: // The WebApkInfoCallback will delete itself after it is done. Nit: Delete the comment. I think that commenting about the lifetime of an object is only useful if the object is created via new. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:22: // content::WebUIMessageHandler implementation. Nit: "content::WebUIMessageHandler implementation." -> "content::WebUIMessageHandler:" This is the new way of doing these comments e.g. chrome/browser/ui/webui/policy_material_design_ui.cc https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:7: #include <string> Nit: You should also include unordered_set
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:62: * manifest. On 2017/01/18 02:08:20, pkotwicz wrote: > Nit: "passed as parameter ..." -> "passed in parameters ..." Done. https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/160001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<WebApkInfo>)> WebApkInfoCallback; On 2017/01/20 16:57:17, pkotwicz wrote: > Sorry for the incorrect advice. > > The argument should be > base::Callback<void(const std::vector<WebApkInfo>&)> WebApkInfoCallback; Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:7: WebApkInfo::WebApkInfo(const std::string short_name, On 2017/01/20 16:57:17, pkotwicz wrote: > This should be: > const std::string& short_name, > > Passing the parameter as a reference avoids a copy Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:8: const std::string package_name, On 2017/01/20 16:57:17, pkotwicz wrote: > This should be: > const std::string& package_name, Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:9: const int shell_apk_version, On 2017/01/20 16:57:17, pkotwicz wrote: > This does not need to be const. Passing ints by copy is fine Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.cc:10: const int version_code) On 2017/01/20 16:57:17, pkotwicz wrote: > This does not need to be const. Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:22: // The WebApkInfoCallback will delete itself after it is done. On 2017/01/20 16:57:17, pkotwicz wrote: > Nit: Delete the comment. > > I think that commenting about the lifetime of an object is only useful if the > object is created via new. Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.h (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.h:22: // content::WebUIMessageHandler implementation. On 2017/01/20 16:57:17, pkotwicz wrote: > Nit: "content::WebUIMessageHandler implementation." -> > "content::WebUIMessageHandler:" > This is the new way of doing these comments > > e.g. chrome/browser/ui/webui/policy_material_design_ui.cc Done. https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_ui.cc (right): https://codereview.chromium.org/2629573004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_ui.cc:7: #include <string> On 2017/01/20 16:57:17, pkotwicz wrote: > Nit: You should also include unordered_set Done.
https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<const WebApkInfo>&)> This should be: typedef base::Callback<void(const std::vector<WebApkInfo>&)> WebApkInfoCallback;
Please wrap the description to 80 characters, and change the title to be: Add a chrome://webapks page. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:656: * Fetches the information of all WebAPKs installed and returns them to the caller using a "Calls the native |callbackPointer| with lists of information on all installed WebAPKs." https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:662: public static void listWebApks(long callbackPointer) { Call this method "retrieveWebApks" - "listWebApks" makes this sound like it's printing something to console. It also makes for awkward native method naming ("onWebApksListed" isn't great) https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:668: PackageManager packageManager = ContextUtils.getApplicationContext().getPackageManager(); Since you're reusing the context a lot, stick it in a variable instead of calling the method over and over. Context context = ContextUtils.getApplicationContext(); PackageManager pm = context.getPackageManager(); if (WebApkValidator.isValidWebApk(context, packageInfo.packageName)) etc. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:692: for (Integer value : list) { Just do: for (int i = 0; i < list.size(); ++i) { array[i] = list.get(i); } https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:699: private static native void nativeOnWebApksFound(long callbackPointer, String[] shortNames, Call this nativeOnWebApksRetrieved to match retrieveWebApks. This method might not find any WebAPKs. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:54: } Nit: don't remove this newline https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:309: void OnWebApksFound(JNIEnv* env, OnWebApksRetrieved (you might not find any WebAPKs when this method is called. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:327: base::android::JavaIntArrayToIntVector(env, jversion_codes, &version_codes); You should DCHECK that all of the vectors are the same length. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<const WebApkInfo> webapk_list; Since you know how long this vector will be, you can reserve() the right size. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:331: const WebApkInfo webapk_info(short_names[i], package_names[i], Fix indentation here. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<const WebApkInfo>&)> On 2017/01/20 20:33:33, pkotwicz wrote: > This should be: > typedef base::Callback<void(const std::vector<WebApkInfo>&)> WebApkInfoCallback; Agreed: const std::vector here https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:126: static void ListWebApks(const WebApkInfoCallback& callback); Call this RetrieveWebApks https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:28: const int version_code; #include "base/macros.h" private: DISALLOW_COPY_AND_ASSIGN(WebApkInfo); https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:27: void WebApksHandler::OnWebApkInfoReceived( Retrieved instead of Received? https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:28: std::vector<const WebApkInfo>& webapks_list) { const std::vector<const WebApkInfo>& webapk_list
Description was changed from ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: https://docs.google.com/a/chromium.org/document/d/16fm7kirRh8fVnCjSLtgJcmnf6i... BUG=624111 ========== to ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: https://docs.google.com/a/chromium.org/document/d/16fm7kirRh8fVnCjSLtgJcmnf6i... BUG=624111 ==========
https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:28: const int version_code; I know that it is normal to DISALLOW_COPY_AND_ASSIGN classes but I didn't know this was also the case with structs. e.g. shortcut_info.h
Description was changed from ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: https://docs.google.com/a/chromium.org/document/d/16fm7kirRh8fVnCjSLtgJcmnf6i... BUG=624111 ========== to ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 ==========
I have a couple questions with two of the comments, thanks for your time reviewing! https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:656: * Fetches the information of all WebAPKs installed and returns them to the caller using a On 2017/01/23 00:48:07, dominickn wrote: > "Calls the native |callbackPointer| with lists of information on all installed > WebAPKs." Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:662: public static void listWebApks(long callbackPointer) { On 2017/01/23 00:48:07, dominickn wrote: > Call this method "retrieveWebApks" - "listWebApks" makes this sound like it's > printing something to console. It also makes for awkward native method naming > ("onWebApksListed" isn't great) Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:668: PackageManager packageManager = ContextUtils.getApplicationContext().getPackageManager(); On 2017/01/23 00:48:07, dominickn wrote: > Since you're reusing the context a lot, stick it in a variable instead of > calling the method over and over. > > Context context = ContextUtils.getApplicationContext(); > PackageManager pm = context.getPackageManager(); > > if (WebApkValidator.isValidWebApk(context, packageInfo.packageName)) > > etc. Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:692: for (Integer value : list) { On 2017/01/23 00:48:07, dominickn wrote: > Just do: > > for (int i = 0; i < list.size(); ++i) { > array[i] = list.get(i); > } Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:699: private static native void nativeOnWebApksFound(long callbackPointer, String[] shortNames, On 2017/01/23 00:48:07, dominickn wrote: > Call this nativeOnWebApksRetrieved to match retrieveWebApks. This method might > not find any WebAPKs. Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:54: } On 2017/01/23 00:48:07, dominickn wrote: > Nit: don't remove this newline Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:309: void OnWebApksFound(JNIEnv* env, On 2017/01/23 00:48:07, dominickn wrote: > OnWebApksRetrieved (you might not find any WebAPKs when this method is called. Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:327: base::android::JavaIntArrayToIntVector(env, jversion_codes, &version_codes); On 2017/01/23 00:48:07, dominickn wrote: > You should DCHECK that all of the vectors are the same length. Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:329: std::vector<const WebApkInfo> webapk_list; On 2017/01/23 00:48:07, dominickn wrote: > Since you know how long this vector will be, you can reserve() the right size. Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:331: const WebApkInfo webapk_info(short_names[i], package_names[i], On 2017/01/23 00:48:07, dominickn wrote: > Fix indentation here. I'm sorry, I'm not sure what you mean. I tried running git cl format, but it leaves it this way. I didn't find anything about this on the guidelines, how should it be indented? https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:29: typedef base::Callback<void(std::vector<const WebApkInfo>&)> On 2017/01/23 00:48:07, dominickn wrote: > On 2017/01/20 20:33:33, pkotwicz wrote: > > This should be: > > typedef base::Callback<void(const std::vector<WebApkInfo>&)> > WebApkInfoCallback; > > Agreed: const std::vector here Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:126: static void ListWebApks(const WebApkInfoCallback& callback); On 2017/01/23 00:48:07, dominickn wrote: > Call this RetrieveWebApks Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:28: const int version_code; On 2017/01/23 00:48:07, dominickn wrote: > #include "base/macros.h" > > private: > DISALLOW_COPY_AND_ASSIGN(WebApkInfo); I'm having some trouble with this. I updated the rest of the comments, but when adding this line I get some strange errors. When looking for a solution I found that using DISALLOW_COPY_AND_ASSIGN is apparently discouraged? Maybe it doesn't apply for Chromium. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... I got some errors that were not very descriptive when using this struct if I add this line, I was wondering if there's anything in particular I need to keep in mind when adding it? https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:27: void WebApksHandler::OnWebApkInfoReceived( On 2017/01/23 00:48:07, dominickn wrote: > Retrieved instead of Received? Done. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:28: std::vector<const WebApkInfo>& webapks_list) { On 2017/01/23 00:48:07, dominickn wrote: > const std::vector<const WebApkInfo>& webapk_list Done.
Please make "Add a chrome://webapks page." as the first line of the description (with a newline underneath) (sorry, missed that first time round). When it's committed, all that is seen in the commit message is the description, so you need to have the subject there by itself on the first line. https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:28: const int version_code; On 2017/01/23 16:58:51, gonzalon wrote: > On 2017/01/23 00:48:07, dominickn wrote: > > #include "base/macros.h" > > > > private: > > DISALLOW_COPY_AND_ASSIGN(WebApkInfo); > > I'm having some trouble with this. I updated the rest of the comments, but when > adding this line I get some strange errors. When looking for a solution I found > that using DISALLOW_COPY_AND_ASSIGN is apparently discouraged? Maybe it doesn't > apply for Chromium. > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > I got some errors that were not very descriptive when using this struct if I add > this line, I was wondering if there's anything in particular I need to keep in > mind when adding it? For now, Chromium's style guide is to use DISALLOW_COPY_AND_ASSIGN (we differ from the google style guide in some places). There are definitely structs which use this (e.g. ContextualSearchContext). The errors are exposing the fact that you're copying this struct in multiple places. There's three copies in ShortcutHelper::OnWebApksRetrieved when filling a std::vector - converting the string from Java to C++ (1x copy), then each WebApkInfo is constructed (2x copy), then copied into the vector (3x copy). There's also another copy in WebApksHandler (4x copy). That's a lot of calls to malloc. IMO, reducing the number of copies is highly desirable. To do so, you should make this class movable. Then you can construct it by std::move'ing the strings from the std::vector<std::string> in OnWebApksRetrieved, and then move it into the std::vector<WebApkInfo>. That way, we have 1x copy per string (the conversion from Java string to C++ string), and no more. 1. defining the move constructor/operator as default WebApkInfo(WebApkInfo&& other) = default; WebApkInfo& operator=(WebApkInfo&& other) = default; 2. removing const from the WebApkInfo members (can't move when members are const; the whole thing is passed in a const vector so that stops modification) 3. in-place constructing the WebApkInfo in the push_back call, with a slightly modification to the constructor: WebApkInfo(std::string&& short_name, std::string&& package_name, int shell_apk_version, int version_code); std::vector<WebApkInfo> webapk_list; webapk_list.reserve(short_names.size()); for (...) { webapk_list.push_back(WebApkInfo(std::move(short_names[i], package_names[i], shell_apk_versions[i], version_codes[i]); } 4. Make WebApksHandler using a reference, not value. 5. using DISALLOW_COPY_AND_ASSIGN What do you think? https://codereview.chromium.org/2629573004/diff/220001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/220001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:338: webapk_list.push_back(webapk_info); Inline the construction of WebApkInfo inside push_back (so it's a temporary object). https://codereview.chromium.org/2629573004/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:32: const WebApkInfo webapk_info = webapks_list[i]; This copies the object. Use const WebApkInfo& webapk_info to avoid the copy. Better yet, just use for (const auto& webapk_info : webapks_list) { }
Had a really bad time typing all that: the WebApkInfo constructor call should be: webapk_list.push_back(WebApkInfo(std::move(short_names[i]), std::move(package_names[i]), shell_apk_versions[i], version_codes[i]);
Description was changed from ========== Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 ========== to ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 ==========
Thanks a lot for the detailed explanation. I see I still have a lot to learn about C++, but your explanation really helped :) Updated.
i'd prefer if we didn't use Unsafe() methods https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:23: void WebApksHandler::HandleRequestWebApksInfo(const base::ListValue* args) { AllowJavascript(); https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:29: const std::vector<WebApkInfo>& webapks_list) { if (!IsJavascriptAllowed()) return; https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:40: web_ui()->CallJavascriptFunctionUnsafe("returnWebApksInfo", list); CallJavascriptFunction("returnWebApksInfo", list);
https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.css:16: text-align: left; how does this work in RTL? https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:7: <html id="t" i18n-values="dir:textdirection;lang:language"> where are you copying this from? can we do <html dir="$i18n{textdirection}" lang="$i18n{language}"> instead? does $i18n{} substitution run on your page? https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:11: <title>About WebAPKs</title> why are you varying direction and language if you're embedding an LTR/English string here? https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:29: <br><br> you should not need <br> tags, you can likely just add more margin-bottom on #logo https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:9: function createTextElementWithClassName(text, className) { createSpanWithTextAndClass? https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:18: * one will be appended at the end of the other. @param {...some type goes here...} webApksList ... some description goes here ... https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:20: function returnWebApksInfo(webApksList) { can this just be webApkList? https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:25: @param for webApkInfo https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:30: webApksList.appendChild(document.createElement('br')); don't use br https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:55: document.addEventListener('DOMContentLoaded', onLoadWork); can we just inline this function? document.addEventListener('DOMContentLoaded', function() { chrome.send('requestWebApksInfo'); });
Thanks for reviewing! https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/24 18:35:32, Dan Beam wrote: > no (c) Done. https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:23: void WebApksHandler::HandleRequestWebApksInfo(const base::ListValue* args) { On 2017/01/24 18:35:32, Dan Beam wrote: > AllowJavascript(); Done. https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:29: const std::vector<WebApkInfo>& webapks_list) { On 2017/01/24 18:35:32, Dan Beam wrote: > if (!IsJavascriptAllowed()) > return; Done. https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/webapks_handler.cc:40: web_ui()->CallJavascriptFunctionUnsafe("returnWebApksInfo", list); On 2017/01/24 18:35:32, Dan Beam wrote: > CallJavascriptFunction("returnWebApksInfo", list); Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.css:16: text-align: left; On 2017/01/24 18:41:50, Dan Beam wrote: > how does this work in RTL? Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:7: <html id="t" i18n-values="dir:textdirection;lang:language"> On 2017/01/24 18:41:50, Dan Beam wrote: > where are you copying this from? > > can we do > > <html dir="$i18n{textdirection}" lang="$i18n{language}"> > > instead? does $i18n{} substitution run on your page? I think I based this from https://cs.chromium.org/chromium/src/components/flags_ui/resources/flags.html... I was going to i18n at first, but I was then told it's better not to, forgot to remove the i18n references. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:11: <title>About WebAPKs</title> On 2017/01/24 18:41:50, Dan Beam wrote: > why are you varying direction and language if you're embedding an LTR/English > string here? I was going to i18n at first, but I was then told it's better not to, forgot to remove the i18n references. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:29: <br><br> On 2017/01/24 18:41:50, Dan Beam wrote: > you should not need <br> tags, you can likely just add more margin-bottom on > #logo Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:9: function createTextElementWithClassName(text, className) { On 2017/01/24 18:41:50, Dan Beam wrote: > createSpanWithTextAndClass? Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:18: * one will be appended at the end of the other. On 2017/01/24 18:41:50, Dan Beam wrote: > @param {...some type goes here...} webApksList ... some description goes here > ... Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:20: function returnWebApksInfo(webApksList) { On 2017/01/24 18:41:50, Dan Beam wrote: > can this just be webApkList? Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:25: On 2017/01/24 18:41:50, Dan Beam wrote: > @param for webApkInfo Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:30: webApksList.appendChild(document.createElement('br')); On 2017/01/24 18:41:50, Dan Beam wrote: > don't use br Done. https://codereview.chromium.org/2629573004/diff/240001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.js:55: document.addEventListener('DOMContentLoaded', onLoadWork); On 2017/01/24 18:41:50, Dan Beam wrote: > can we just inline this function? > > document.addEventListener('DOMContentLoaded', function() { > chrome.send('requestWebApksInfo'); > }); Done.
lgtm with nits. https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:30: WebApkInfoCallback; The new style is: using WebApkInfoCallback = base::Callback<void(const std::vector<WebApkInfo>&)>;
https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2629573004/diff/240001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:30: WebApkInfoCallback; On 2017/01/24 20:44:46, Xi Han wrote: > The new style is: > using WebApkInfoCallback = base::Callback<void(const std::vector<WebApkInfo>&)>; Done.
lgtm https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.css:1: /* Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" in any of your new code https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:33: <div class="content" id="webapks-list"></div> nit: webapk-list
https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.css (right): https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.css:1: /* Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/24 22:30:28, Dan Beam wrote: > no "(c)" in any of your new code Done. https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... File components/webapks_ui/resources/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/280001/components/webapks_ui/... components/webapks_ui/resources/about_webapks.html:33: <div class="content" id="webapks-list"></div> On 2017/01/24 22:30:28, Dan Beam wrote: > nit: webapk-list Done.
gonzalon@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Hi! I added a new directory on the components folder for Web APKs. dbean@ has reviewed that code but I'm missing OWNER approval. Would you mind taking a look?
The CQ bit was checked by gonzalon@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...
webapps / webapks lgtm % nits. Thanks for making the move-only changes: this code is now much, much more efficient wrt string copying. https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:7: #include <jni.h> Nit: #include <utility> for std::move https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:12: // Structure with information about a WebAPK. Nit: add a comment like: "This class is passed around in a std::vector to generate the chrome://webapks page. To reduce copying overhead, this class is move-only, and move-constructs its string arguments (which are copied from Java to C++ into a temporary prior to construction)". https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:14: WebApkInfo(const std::string&& short_name, Remove the const from the two string arguments. The main reason for taking rvalue references (&&) as arguments is so they can be moved into this object (i.e. the original strings are dead after the constructor is run because this class has stolen their internal buffers). This is nice because the argument strings are in temporary vectors anyway that disappear after ShortcutHelper::OnWebApksRetrieved finishes running. Enforcing that the reference is const is a little beside the point.
https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:7: #include <jni.h> On 2017/01/24 23:15:20, dominickn wrote: > Nit: > > #include <utility> for std::move Done. https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:12: // Structure with information about a WebAPK. On 2017/01/24 23:15:20, dominickn wrote: > Nit: add a comment like: > > "This class is passed around in a std::vector to generate the chrome://webapks > page. To reduce copying overhead, this class is move-only, and move-constructs > its string arguments (which are copied from Java to C++ into a temporary prior > to construction)". Done. https://codereview.chromium.org/2629573004/diff/300001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:14: WebApkInfo(const std::string&& short_name, On 2017/01/24 23:15:20, dominickn wrote: > Remove the const from the two string arguments. The main reason for taking > rvalue references (&&) as arguments is so they can be moved into this object > (i.e. the original strings are dead after the constructor is run because this > class has stolen their internal buffers). This is nice because the argument > strings are in temporary vectors anyway that disappear after > ShortcutHelper::OnWebApksRetrieved finishes running. Enforcing that the > reference is const is a little beside the point. Makes sense, removed!
On 2017/01/24 23:03:05, gonzalon wrote: > mailto:jam@chromium.org: > > Hi! > > I added a new directory on the components folder for Web APKs. dbean@ has > reviewed that code but I'm missing OWNER approval. > > Would you mind taking a look? Why is it in components/ ? If it's only used by src/chrome, then leave it in src/chrome.
Description was changed from ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 ========== to ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Moved the html, css and js to src/chrome/browser/resources
I'm also an owner of chrome/browser/resources/ so jam@ may not need to review the files at their new location (up to y'all). the code still generally lgtm but i have a few comments. by the way, I highly recommend using closure compiler to typecheck your code.[1] you can do this by making a file named: chrome/browser/resources/webapks/compiled_resources2.gyp in a similar style of: https://cs.chromium.org/chromium/src/chrome/browser/resources/vr_shell/compil... and add your newly created file to the list in: third_party/closure_compiler/compiled_resources2.gyp to typecheck the code locally you can run: third_party/closure_compiler/run_compiler <optional_name_of_your_target_to_speed_things_up> it'll be continuously typechecked by our try bots (checks code before committing): https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil... and build bots (after committing): https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux [1] https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.html:7: <html> can you hardcode a lang="en" and maybe a dir="ltr" here if we're hardcoding English? having a page-wide language is helpful for a11y https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:7: * |className| class. @return {HTMLElement} https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:10: var node = createElementWithClassName('span', className); nit: name this el? Node and Element are different (Node is a parent class), and you're calling create*Element* https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:20: * @param webApkList List of objects with information about WebAPKs installed. you need a type name in this @param. an example: /** * @typedef {{ * shortName: string, * packageName: string, * shellApkVersion: number, * versionCode: number * }} */ var WebApkInfo; @param {!Array<WebApkInfo>} webApkList https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:31: * @param webApkInfo Information about an installed WebAPK. @param {WebApkInfo} webApkInfo
On 2017/01/25 16:49:42, gonzalon wrote: > Moved the html, css and js to src/chrome/browser/resources thanks, removing myself as reviewer
Description was changed from ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jam@chromium.org changed reviewers: - jam@chromium.org
The CQ bit was checked by gonzalon@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...
Fixed the comments and added the closure compiler file. https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.html:7: <html> On 2017/01/25 23:27:33, Dan Beam wrote: > can you hardcode a lang="en" and maybe a dir="ltr" here if we're hardcoding > English? > > having a page-wide language is helpful for a11y Done. https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:7: * |className| class. On 2017/01/25 23:27:33, Dan Beam wrote: > @return {HTMLElement} Done. https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:10: var node = createElementWithClassName('span', className); On 2017/01/25 23:27:33, Dan Beam wrote: > nit: name this el? Node and Element are different (Node is a parent class), and > you're calling create*Element* Done. https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:20: * @param webApkList List of objects with information about WebAPKs installed. On 2017/01/25 23:27:33, Dan Beam wrote: > you need a type name in this @param. an example: > > /** > * @typedef {{ > * shortName: string, > * packageName: string, > * shellApkVersion: number, > * versionCode: number > * }} > */ > var WebApkInfo; > > @param {!Array<WebApkInfo>} webApkList Done. https://codereview.chromium.org/2629573004/diff/340001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.js:31: * @param webApkInfo Information about an installed WebAPK. On 2017/01/25 23:27:33, Dan Beam wrote: > @param {WebApkInfo} webApkInfo Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2629573004/diff/380001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/380001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:19: WebApkInfo(std::string&& short_name, Sorry! One last thing: remove && from the two string arguments here so it's pass by value. Reason: I've just realised that rvalue reference arguments are only permitted for move constructors and move operators by the style guide (https://chromium-cpp.appspot.com/). Removing && here will preserve the move semantics and be compliant with the style guide (calling std::move() on the string argument when constructing this will move the string into the value, so it won't copy/construct a new string).
In retrospect the failure that you are seeing on the bots was a simple one (after I spent > 30 minutes investigating) The bots failed in the "gn gen" step. The bots complained that //out/Release/gen/chrome/webapks_ui_resources.pak was not being generated by a dependency of //chrome:packed_resources_extra The //chrome:packed_resources template expands into multiple targets. One of these is chrome:packed_resources_extra (https://cs.chromium.org/chromium/src/chrome/chrome_paks.gni?rcl=0&l=221) In the "chrome_extra_paks" template definition "webapks_ui_resources.pak" is listed as a source but //chrome/browser/resources:webapks_ui_resources is not listed as a dependency I figured this out mostly by luck. "gn desc" helped me figure out that //chrome:extra_resources and //chrome:packed_extra_resources were not related
On 2017/01/27 16:59:24, pkotwicz wrote: > In retrospect the failure that you are seeing on the bots was a simple one > (after I spent > 30 minutes investigating) > > The bots failed in the "gn gen" step. > The bots complained that //out/Release/gen/chrome/webapks_ui_resources.pak was > not being generated by a dependency of //chrome:packed_resources_extra > The //chrome:packed_resources template expands into multiple targets. One of > these is chrome:packed_resources_extra > (https://cs.chromium.org/chromium/src/chrome/chrome_paks.gni?rcl=0&l=221) > In the "chrome_extra_paks" template definition "webapks_ui_resources.pak" is > listed as a source but //chrome/browser/resources:webapks_ui_resources is not > listed as a dependency > > I figured this out mostly by luck. "gn desc" helped me figure out that > //chrome:extra_resources and //chrome:packed_extra_resources were not related Thanks a lot for the help Peter, that worked!
https://codereview.chromium.org/2629573004/diff/380001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/380001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:19: WebApkInfo(std::string&& short_name, On 2017/01/27 05:42:33, dominickn wrote: > Sorry! One last thing: remove && from the two string arguments here so it's pass > by value. > > Reason: I've just realised that rvalue reference arguments are only permitted > for move constructors and move operators by the style guide > (https://chromium-cpp.appspot.com/). > > Removing && here will preserve the move semantics and be compliant with the > style guide (calling std::move() on the string argument when constructing this > will move the string into the value, so it won't copy/construct a new string). Sure! Done.
The CQ bit was checked by gonzalon@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...
https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:19: WebApkInfo(std::string short_name, don't we want these to be const &, i.e. const std::string& short_name? https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/BUILD.gn:154: grit("webapks_ui_resources") { don't we want to only pack these resources on android? https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.html:7: <html lang:"en" dir="ltr"> lang="en" https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/webapks/compiled_resources2.gyp (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/webapks/compiled_resources2.gyp:10: '../../../../ui/webui/resources/js/compiled_resources2.gyp:util', nit: <(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:util https://codereview.chromium.org/2629573004/diff/400001/chrome/chrome_paks.gni File chrome/chrome_paks.gni (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/chrome_paks.gni... chrome/chrome_paks.gni:101: "$root_gen_dir/chrome/webapks_ui_resources.pak", i think you only want to add these on android, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:19: WebApkInfo(std::string short_name, On 2017/01/27 21:52:14, Dan Beam wrote: > don't we want these to be const &, i.e. const std::string& short_name? No. Const ref arguments means the strings get copied into this object, losing the move semantics. The arguments to this method come from a temporary vector that's already been copied from Java and would be discarded after use anyway.
https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_info.h:19: WebApkInfo(std::string short_name, On 2017/01/27 22:31:05, dominickn wrote: > On 2017/01/27 21:52:14, Dan Beam wrote: > > don't we want these to be const &, i.e. const std::string& short_name? > > No. Const ref arguments means the strings get copied into this object, losing > the move semantics. The arguments to this method come from a temporary vector > that's already been copied from Java and would be discarded after use anyway. aight, feel free to ignore this then :)
https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/BUILD.gn (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/BUILD.gn:154: grit("webapks_ui_resources") { On 2017/01/27 21:52:14, Dan Beam wrote: > don't we want to only pack these resources on android? Yup, Done, thanks https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/webapks/about_webapks.html (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/webapks/about_webapks.html:7: <html lang:"en" dir="ltr"> On 2017/01/27 21:52:15, Dan Beam wrote: > lang="en" Done. https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... File chrome/browser/resources/webapks/compiled_resources2.gyp (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/browser/resourc... chrome/browser/resources/webapks/compiled_resources2.gyp:10: '../../../../ui/webui/resources/js/compiled_resources2.gyp:util', On 2017/01/27 21:52:15, Dan Beam wrote: > nit: <(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:util Done. https://codereview.chromium.org/2629573004/diff/400001/chrome/chrome_paks.gni File chrome/chrome_paks.gni (right): https://codereview.chromium.org/2629573004/diff/400001/chrome/chrome_paks.gni... chrome/chrome_paks.gni:101: "$root_gen_dir/chrome/webapks_ui_resources.pak", On 2017/01/27 21:52:15, Dan Beam wrote: > i think you only want to add these on android, right? Done.
lgtm
The CQ bit was checked by gonzalon@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by gonzalon@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: This issue passed the CQ dry run.
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, hanxi@chromium.org, dominickn@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2629573004/#ps440001 (title: "Add a chrome://webapks page.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gonzalon@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Hi again! I'm missing OWNER approval for some build files, would you mind taking a look? Thanks!
jam@: Ping!
lgtm
The CQ bit was checked by gonzalon@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by gonzalon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, dbeam@chromium.org, pkotwicz@chromium.org, jam@chromium.org, hanxi@chromium.org Link to the patchset: https://codereview.chromium.org/2629573004/#ps460001 (title: "Add a chrome://webapks page.")
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
Failed to apply patch for chrome/common/url_constants.h: While running git apply --index -p1; error: patch failed: chrome/common/url_constants.h:101 error: chrome/common/url_constants.h: patch does not apply Patch: chrome/common/url_constants.h Index: chrome/common/url_constants.h diff --git a/chrome/common/url_constants.h b/chrome/common/url_constants.h index d5d54031b0cf88d148a5f78496d4689d51248599..1cbaf2fc68dd1c7fd399508919fd850835c7c6bf 100644 --- a/chrome/common/url_constants.h +++ b/chrome/common/url_constants.h @@ -101,6 +101,7 @@ extern const char kChromeUINativeBookmarksURL[]; extern const char kChromeUINativeHistoryURL[]; extern const char kChromeUINativePhysicalWebDiagnosticsURL[]; extern const char kChromeUINativeRecentTabsURL[]; +extern const char kChromeUIWebApksURL[]; #endif // defined(OS_ANDROID) #if defined(OS_CHROMEOS) @@ -266,6 +267,7 @@ extern const char kChromeUIOfflineInternalsURL[]; extern const char kChromeUIPhysicalWebDiagnosticsHost[]; extern const char kChromeUIPopularSitesInternalsHost[]; extern const char kChromeUISnippetsInternalsHost[]; +extern const char kChromeUIWebApksHost[]; #endif #if defined(ENABLE_VR_SHELL) || defined(ENABLE_WEBVR)
The CQ bit was checked by gonzalon@chromium.org
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
Failed to apply patch for chrome/common/url_constants.h: While running git apply --index -p1; error: patch failed: chrome/common/url_constants.h:101 error: chrome/common/url_constants.h: patch does not apply Patch: chrome/common/url_constants.h Index: chrome/common/url_constants.h diff --git a/chrome/common/url_constants.h b/chrome/common/url_constants.h index d5d54031b0cf88d148a5f78496d4689d51248599..1cbaf2fc68dd1c7fd399508919fd850835c7c6bf 100644 --- a/chrome/common/url_constants.h +++ b/chrome/common/url_constants.h @@ -101,6 +101,7 @@ extern const char kChromeUINativeBookmarksURL[]; extern const char kChromeUINativeHistoryURL[]; extern const char kChromeUINativePhysicalWebDiagnosticsURL[]; extern const char kChromeUINativeRecentTabsURL[]; +extern const char kChromeUIWebApksURL[]; #endif // defined(OS_ANDROID) #if defined(OS_CHROMEOS) @@ -266,6 +267,7 @@ extern const char kChromeUIOfflineInternalsURL[]; extern const char kChromeUIPhysicalWebDiagnosticsHost[]; extern const char kChromeUIPopularSitesInternalsHost[]; extern const char kChromeUISnippetsInternalsHost[]; +extern const char kChromeUIWebApksHost[]; #endif #if defined(ENABLE_VR_SHELL) || defined(ENABLE_WEBVR)
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, dbeam@chromium.org, pkotwicz@chromium.org, jam@chromium.org, hanxi@chromium.org Link to the patchset: https://codereview.chromium.org/2629573004/#ps480001 (title: "Add a chrome://webapks page.")
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": 480001, "attempt_start_ts": 1486496204118920,
"parent_rev": "0ad079ddaea6dedf8656bf374b5c0cd8a601da98", "commit_rev":
"87192d7728d4f0829ba7eba2c49bb61810b0ea1a"}
Message was sent while issue was closed.
Description was changed from ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add a chrome://webapks page. Adds an about:webapks page with information about all installed Web APKs on the device Design doc: go/about-webapks BUG=624111 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2629573004 Cr-Commit-Position: refs/heads/master@{#448747} Committed: https://chromium.googlesource.com/chromium/src/+/87192d7728d4f0829ba7eba2c49b... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/87192d7728d4f0829ba7eba2c49b... |
