Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2138973002:
Initial CL for talking to the WebAPK server to generate WebAPK (Closed)
|
Created:
4 years, 5 months ago by pkotwicz Modified:
4 years, 4 months ago Reviewers:
dominickn, mlamouri (slow - plz ping), ScottK, scottkirkwood, Yaron, no sievers, Robert Sesek, Xi Han, gone CC:
chromium-reviews, dominickn+watch_chromium.org, hartmanng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL changes the behavior of
ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap()
- To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto)
to generate WebAPK
- To download WebAPK from URL returned by WebAPK server
This CL also:
- Adds stub for talking to Google Play client to install WebAPK
- Introduces manifest_util.h with methods for converting blink::WebDisplayMode
and blink::WebScreenOrientationLockType to and from string
BUG=619739
TEST=WebApkInstallerTest.*
R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara
TBR=brettw (for third_party/smhasher/BUILD.gn)
Committed: https://crrev.com/879b1ed437bd6e1d535d0dfa0c3c16f79e0b55d2
Cr-Commit-Position: refs/heads/master@{#410233}
Patch Set 1 #
Total comments: 45
Patch Set 2 : Merge branch 'webapk_builder_impl0' into webapk_builder_impl2 #Patch Set 3 : Merge branch 'webapk_builder_impl0' into webapk_builder_impl2 #
Total comments: 19
Patch Set 4 : Merge branch 'webapk_builder_impl0' into webapk_builder_impl2 #
Total comments: 4
Patch Set 5 : Merge branch 'master' into webapk_builder_impl2 #Patch Set 6 : Merge branch 'master' into webapk_builder_impl2 #Patch Set 7 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 35
Patch Set 8 : Merge branch 'master' into webapk_builder_impl2 #Patch Set 9 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 2
Patch Set 10 : Merge branch 'master' into webapk_builder_impl2 #Patch Set 11 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 12
Patch Set 12 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 4
Patch Set 13 : Merge branch 'master' into webapk_builder_impl2 #Patch Set 14 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 16
Patch Set 15 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 2
Patch Set 16 : Merge branch 'master' into webapk_builder_impl2 #
Total comments: 7
Patch Set 17 : Merge branch 'master' into webapk_builder_impl22_no_content #Patch Set 18 : Merge branch 'master' into webapk_builder_impl22_no_content #Patch Set 19 : Merge branch 'master' into webapk_builder_impl22_no_content #Messages
Total messages: 95 (31 generated)
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org, scottkirkwood@google.com, yfriedman@chromium.org
Scott, Yaron, and Xi can you please take a look? Scott I think I need your feedback first because I modified the proto slightly from what you have in the server code This CL does not deal with OAuth This CL is blocked on having a way of testing it. I have tested that the CreateWebApkRequest is sent using Charles Proxy but nothing more https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:18: message CreateWebApkResponse { This differs from the previously agreed version of the protos. It does not makes sense for the WebAPK server to send to the client the bitmaps for all of the icons in the Web manifest as a response. The client cares about just one field the market_url. The pattern of the request and the response being different is used in several places in Chrome. One example is in components/safe_browsing_db/safebrowsing.proto FindThreatMatchesRequest & FindThreatMatchesResponse https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; The server proto says that "requester_application_version" is required. I am excluding it because we do not need to send the field yet (We may need to in the future) https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:40: repeated Image icon = 6; The names of the repeated fields are "scope" instead of "scopes" and "icon" instead of "icons" because these names make the generated code make more sense. webapk::Image* webapk::WebAppManifest::add_icon() makes more sense than webapk::Image* webapk::WebAppManifest::add_icons() https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:45: // the manifest. The colors are ints instead of CSS strings as previously discussed. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:55: // applied to the icon's bytes prior to taking the MD5 hash. I excluded the "hash_method" field in the server proto. If no "hash_method" is specified the server should assume MD5.
https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:45: // the manifest. The default looks wrong but that's what content::ManifestParser uses. I will investigate why it uses such a weird value.
first pass - looks like a great start https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:14: * Java counterpart to webapk_builder.h Contains functionality to download and install WebAPKs. missing "." (or newline) https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:16: public class WebApkBuilder { add a private constructor since this is just a util class https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.cc:94: new WebApkBuilder(browser_context, info, icon_bitmap); brief comment on ownership please https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.cc:168: void ShortcutHelper::OnBuiltWebApk(bool success) { without any state on what was successful (ShortcutInfo is static) this will be hard to deal with. Perhaps it should retain ownership of the ShortcutInfo? https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.h:63: // the Google Play server. this is an implementation detail https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/DEPS (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/DEPS:3: "+content/renderer/manifest/manifest_parser.h", does this need to be moved to content/public or are there examples of unittests which reach into content (I'm unsure) https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; On 2016/07/11 20:43:52, pkotwicz wrote: > The server proto says that "requester_application_version" is required. I am > excluding it because we do not need to send the field yet (We may need to in the > future) It should be required though. Isn't the host application package one of the template fields in the manifest? https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:123: timer_.Stop(); DCHECK ui thread (can you add remaining DCHECKs on non-trivial fn's so it's easier to see) https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:189: WebApkBuilder::MakeCreateWebApkRequest() { Nit: WebApkBuilder::BuildCreate.. ? (since it's a WebApkBuilder?) https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:224: OnFailure(); what if we have an in-flight request? shouldn't it be cancelled or you're relying on the delet? https://codereview.chromium.org/2138973002/diff/1/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/2138973002/diff/1/chrome/chrome_tests_unit.gy... chrome/chrome_tests_unit.gypi:35: 'browser/android/webapk/webapk_builder_unittest.cc', no need to maintain this file
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:14: * Java counterpart to webapk_builder.h Contains functionality to download and install WebAPKs. I do not understand. The comment has a "." at the end. The comment does not exceed 100 chars. https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:16: public class WebApkBuilder { On 2016/07/13 00:24:00, Yaron wrote: > add a private constructor since this is just a util class Done. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.cc:94: new WebApkBuilder(browser_context, info, icon_bitmap); On 2016/07/13 00:24:00, Yaron wrote: > brief comment on ownership please Done. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.cc:168: void ShortcutHelper::OnBuiltWebApk(bool success) { Yes ShortcutHelper::OnBuiltWebApk() should take more parameters once it has an actual implementation. I suspect that we will want to generate a non-WebAPK web app if the WebAPK generation fails so we will need to pass all the parameters passed to AddWebappInBackgroundWithSkBitmap() to OnBuiltWebApk() We also need to figure out when the WebAPK will get registered with WebappDataStorage so we will need to pass parameters in order to do that too https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.h:63: // the Google Play server. How do you suggest that I improve this comment? https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; Sorry for the confusion. I am purposefully excluding the "requester_application_version" field. I am including the "requester_application_package". The "requester_application_package" is used to populate the host application package field in the Android Manifest The server proto requires both "requester_application_version" and "requester_application_package" https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:123: timer_.Stop(); On 2016/07/13 00:24:01, Yaron wrote: > DCHECK ui thread (can you add remaining DCHECKs on non-trivial fn's so it's > easier to see) Done. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:189: WebApkBuilder::MakeCreateWebApkRequest() { BuildCreate it is https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_builder.cc:224: OnFailure(); Yes, I am relying on the destructor to cancel the request https://codereview.chromium.org/2138973002/diff/1/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/2138973002/diff/1/chrome/chrome_tests_unit.gy... chrome/chrome_tests_unit.gypi:35: 'browser/android/webapk/webapk_builder_unittest.cc', GN gets the list of files to include from the gypi file
Sorry for the delay, codereview.chromium.org is not part of my flow, currently. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:18: message CreateWebApkResponse { On 2016/07/11 20:43:52, pkotwicz wrote: > This differs from the previously agreed version of the protos. It does not makes > sense for the WebAPK server to send to the client the bitmaps for all of the > icons in the Web manifest as a response. > > The client cares about just one field the market_url. > > The pattern of the request and the response being different is used in several > places in Chrome. One example is in > components/safe_browsing_db/safebrowsing.proto FindThreatMatchesRequest & > FindThreatMatchesResponse You need to follow the proto at http://cs/piper///depot/google3/google/chrome/webapk/v1alpha/webapk_service.p... There is no CreateWebApkResponse message, anymore. If you don't care about some fields you can put them as reserved = 1, 2, 3, etc. and your code will ignore it. Most of the fields in WebAPK response would be emptyfits in with how the API is designed. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; On 2016/07/11 20:43:52, pkotwicz wrote: > The server proto says that "requester_application_version" is required. I am > excluding it because we do not need to send the field yet (We may need to in the > future) You really should send it as we use this as part of the row key. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:40: repeated Image icon = 6; On 2016/07/11 20:43:52, pkotwicz wrote: > The names of the repeated fields are "scope" instead of "scopes" and "icon" > instead of "icons" because these names make the generated code make more sense. > > webapk::Image* webapk::WebAppManifest::add_icon() > makes more sense than > webapk::Image* webapk::WebAppManifest::add_icons() That was the old recommendation, now the new recommendation is to pluralize: Please see http://shortn/_nk2AGQpWvJ https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:45: // the manifest. On 2016/07/11 20:43:52, pkotwicz wrote: > The colors are ints instead of CSS strings as previously discussed. Right, and we agreed you should pass strings in the format "#FEFEFE". https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:55: // applied to the icon's bytes prior to taking the MD5 hash. On 2016/07/11 20:43:52, pkotwicz wrote: > I excluded the "hash_method" field in the server proto. If no "hash_method" is > specified the server should assume MD5. fine, please just "reserve" all fields you are not using.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:18: message CreateWebApkResponse { Based on offline discussion, scottkirkwood@ will start a conversation with the server API folks to see if we can bring back the CreateWebApkResponse message. There does not seem to be any precedent in Chrome for sending the same proto in the server request and recieving the same proto back from the server in the server response. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; Based on offline discussion we do not use "requester_application_version" as part of the row key but we use "requester_application_package" https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:40: repeated Image icon = 6; On 2016/07/13 14:41:45, scottkirkwood wrote: > On 2016/07/11 20:43:52, pkotwicz wrote: > > The names of the repeated fields are "scope" instead of "scopes" and "icon" > > instead of "icons" because these names make the generated code make more > sense. > > > > webapk::Image* webapk::WebAppManifest::add_icon() > > makes more sense than > > webapk::Image* webapk::WebAppManifest::add_icons() > > That was the old recommendation, now the new recommendation is to pluralize: > Please see http://shortn/_nk2AGQpWvJ Done. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:45: // the manifest. On 2016/07/13 14:41:45, scottkirkwood wrote: > On 2016/07/11 20:43:52, pkotwicz wrote: > > The colors are ints instead of CSS strings as previously discussed. > > Right, and we agreed you should pass strings in the format "#FEFEFE". Done. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:55: // applied to the icon's bytes prior to taking the MD5 hash. On 2016/07/13 14:41:45, scottkirkwood wrote: > On 2016/07/11 20:43:52, pkotwicz wrote: > > I excluded the "hash_method" field in the server proto. If no "hash_method" is > > specified the server should assume MD5. > > fine, please just "reserve" all fields you are not using. Done.
Looks good, just left some comments. Thank you for taking this task! https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:133: base::Unretained(this))); Please document here why base::Unretatined(this) would be safe. Otherwise, it is always safe to use base::WeakPtrFactory. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:140: if (!source->GetStatus().is_success() || source->GetResponseCode() != 200) { net::HTTP_OK? https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:171: base::Unretained(this))); Same here. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.h (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:60: // request to download the WebAPK is sent tot he Google Play server. "sent tot he" -> "send to the". The description is a little confusing. From my understanding, chrome client asks Play to install the WebAPK, and the installation process on Play side is a combination of "download" + "install". How about: Talks to Chrome WebAPK server to generate a WebAPK on the server, and asks Google Play servers to install it. Calls |callback| after the request to install the WebAPK is sent to the Google Play server. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:72: // download the generated WebAPK. to download and install the generated WebAPK. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:75: // Called with the URL to send to the Google Play server to download the "to download" -> "to install". https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:85: // Called when the request to download the WebAPK is sent to the Google Play Maybe "download and install"? https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:1061: const char kWebApkServerUrl[] = "webapk-server-url"; Move it to the block with "#if defined(OS_ANDROID)" from line 1080. https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.h:299: extern const char kWebApkServerUrl[]; Please place it within the block with "#if defined(OS_ANDROID)". See line 306.
https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:14: * Java counterpart to webapk_builder.h Contains functionality to download and install WebAPKs. On 2016/07/13 02:15:30, pkotwicz wrote: > I do not understand. The comment has a "." at the end. The comment does not > exceed 100 chars. The capitalized "C" is threw me off. Seems like this should be: Java counterpart to webapk_builder.h Contains functionality to download and install WebAPKs. or Java counterpart to webapk_builder.h. Contains functionality to download and install WebAPKs. or Java counterpart to webapk_builder.h which contains functionality to download and install WebAPKs. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/DEPS (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/DEPS:3: "+content/renderer/manifest/manifest_parser.h", On 2016/07/13 00:24:01, Yaron wrote: > does this need to be moved to content/public or are there examples of unittests > which reach into content (I'm unsure) bump. looking at the test, I don't think it's testing the right thing. content::ManifestParser isn't even going to really parse the manifest (the server will) and is orthogonal to your functionality. https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; On 2016/07/13 19:49:35, pkotwicz wrote: > Based on offline discussion we do not use "requester_application_version" as > part of the row key but we use "requester_application_package" Actually, please add the version number now. The number of bytes we're talking about is trivial and the flexibility it offers in the server is not to be underestimated. A simple example is being able to protect the server from a buggy client release. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:62: // - A request to download and install the WebAPK has been sent to .. has been sent. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/shortcut_helper.h:66: // |success| indicates whether the request was sent to the Google Play server. |success| indicates whether the request was issued to the server.
https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:24: reserved 1, 3, 5, 7; I usually put the 'reserved' line at the end of the message https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:124: return base::StringPrintf("rgba(%d,%d,%d,%.2f)", r, g, b, a); Glenn's parser supports rgb(), not rgba()
Description was changed from ========== Initial CL for talking to the WebAPK server to generate WebAPK BUG=619739 ========== to ========== Initial CL for talking to the WebAPK server to generate WebAPK BUG=619739 ==========
Patchset #4 (id:120001) has been deleted
Scott, can you please take another look? https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2138973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:14: * Java counterpart to webapk_builder.h Contains functionality to download and install WebAPKs. Ok, I added a new line https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/DEPS (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/DEPS:3: "+content/renderer/manifest/manifest_parser.h", You are right using content/renderer/manifest/manifest_parser.h in the unit test is wrong. The spirit of the test is correct. The server API is defined in terms of the W3C Web Manifest spec. Testing WebApkBuilder::OrientationToString() against ManifestParser is valid. Perhaps I should introduce ManifestUtil in content/public/manifest_util.h which converts between blink::WebScreenOrientationLockType <-> std::string. What do you think? https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:28: optional string requester_application_package = 4; On 2016/07/19 17:31:13, Yaron wrote: > On 2016/07/13 19:49:35, pkotwicz wrote: > > Based on offline discussion we do not use "requester_application_version" as > > part of the row key but we use "requester_application_package" > > Actually, please add the version number now. The number of bytes we're talking > about is trivial and the flexibility it offers in the server is not to be > underestimated. A simple example is being able to protect the server from a > buggy client release. Done. https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:124: return base::StringPrintf("rgba(%d,%d,%d,%.2f)", r, g, b, a); I looked at the source code to Glenn's parser and it would be trivial to make it support rgba(). I am pushing back because sending rgb() makes the client side update logic in https://codereview.chromium.org/2124513002/ more complicated. Each piece of extra complexity that we put into Chrome equals a bigger Chrome APK https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:133: base::Unretained(this))); base::Unretained() is safe wherever it is used in this class. But that's not obvious. If I started the timer in a different place than I do, using base::Unretained() could be unsafe. Switching to base::WeakPtrFactory https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.h (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:60: // request to download the WebAPK is sent tot he Google Play server. You are right that the comment was a little confusing. I changed the comment to "Talks to the Chrome WebAPK server to generate a WebAPK on the server and to the Google Play server to download and install the generated WebAPK. Calls |callback| after the request to download and install the WebAPK is sent to the Google Play server." https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:72: // download the generated WebAPK. I changed "to download" to "to download and install" everywhere. Download and install everything! https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/common/chrome_s... chrome/common/chrome_switches.h:299: extern const char kWebApkServerUrl[]; Done! Thank you for catching this
lgtm once those two issues are addressed (you'll need a content owner for the dependency change regardless) https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/DEPS (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/DEPS:3: "+content/renderer/manifest/manifest_parser.h", On 2016/07/20 05:13:41, pkotwicz wrote: > You are right using content/renderer/manifest/manifest_parser.h in the unit test > is wrong. The spirit of the test is correct. The server API is defined in terms > of the W3C Web Manifest spec. Testing WebApkBuilder::OrientationToString() > against ManifestParser is valid. Perhaps I should introduce ManifestUtil in > content/public/manifest_util.h which converts between > blink::WebScreenOrientationLockType <-> std::string. What do you think? That would be fine with me. Another option would be to move the enum conversion code down to content (with a public accessor) and test it there. It's nice in that it localizes it with the relevant code as opposed to the mapping being in a consumer and it's not dependent on our specific feature. https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:247: OnFailure(); should this be posted to ui thread? I thought success/failure are expected on ui thread?
https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:124: return base::StringPrintf("rgba(%d,%d,%d,%.2f)", r, g, b, a); On 2016/07/20 05:13:41, pkotwicz wrote: > I looked at the source code to Glenn's parser and it would be trivial to make it > support rgba(). > > I am pushing back because sending rgb() makes the client side update logic in > https://codereview.chromium.org/2124513002/ more complicated. Each piece of > extra complexity that we put into Chrome equals a bigger Chrome APK Yes, would be easy for Glenn to add and would make the code more general, at least for these reasons we should support it.
scottkirkwood@chromium.org changed reviewers: + scottkirkwood@chromium.org
lgtm https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:18: message CreateWebApkResponse { On 2016/07/13 19:49:35, pkotwicz wrote: > Based on offline discussion, scottkirkwood@ will start a conversation with the > server API folks to see if we can bring back the CreateWebApkResponse message. > > There does not seem to be any precedent in Chrome for sending the same proto in > the server request and recieving the same proto back from the server in the > server response. Also there are only 3 chrome APIs that I could find, so not a large set of precedents using our API system. Once we get the green light from the API folks we should have the ability to change the messages the way we like.
lgtm
https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:30: const char kDefaultWebApkServerUrl[] = "http://www.awesomeserver.appspot.com"; this is going to be "https://webapk.googleapis.com" Also to create a new WebAPK; it would be the query path "/v1alpha/webApks?alt=proto" the ?alt=proto is to indicate (as well as the content-type=application/x-protobuf) that we are sending a proto. https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:33: const char kProtoMimeType[] = "application/octet-stream"; Turns out this needs to be: "application/x-protobuf"
Xi, Yaron, and Scott can you please take another look? I have changed this CL to download the WebAPK from the server as well https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/140001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:247: OnFailure(); WebApkBuilder is created and started on the IO thread actually
lgtm
(pls update CL description with new changes)
lgtm including the download part.
Description was changed from ========== Initial CL for talking to the WebAPK server to generate WebAPK BUG=619739 ========== to ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 ==========
pkotwicz@chromium.org changed reviewers: + rsesek@chromium.org
Robert, can you please take a look at this CL from a security standpoint Scott, can you please look at the proto changes in this CL?
https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. What about unittests for this file, to cover the various timeout/error scenarios? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:34: const char kDefaultWebApkServerUrl[] = "https://webapk.googleapis.com/v1alpha/webApks?alt=proto"; nit: 80 cols, break after = https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; Why MD5? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:63: std::string ColorToString(int64_t color) { I'm surprised there isn't a function already to do this in the codebase. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); Why reinterpret this as a reference? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.h (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:39: class WebApkBuilder : public net::URLFetcherDelegate { Why is this not WebApkInstaller if it also installs the APK? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:41: typedef base::Callback<void(bool)> FinishCallback; using FinishCallback = base::Callback<void(bool); is the preferred C++11 syntax. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:51: // Talks to the Chrome WebAPK server to generate a WebAPK on the server and to Can this be called more than once per object? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:54: void BuildAsync(const FinishCallback& callback); Similarly, shouldn't this be "Install" ? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:72: // Called once the WebAPK has been downloaded. Mention that this installs the APK.
Patchset #8 (id:220001) has been deleted
Robert, can you please take another look? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. I am confused as to what you want unittested. Do you want a test testing that the callback is called in timeout/error scenarios? Something like this? std::unique_ptr<WebApkBuilder> webapk_builder(new WebApkBuilder(...)); webapk_builder->BuildAsync(...); // For the sake of the test do not do any of the intermediate processing webapk_builder->OnWebApkDownloaded(...); // Call OnWebApkDownloaded with invalid parameters EXPECT_TRUE(CallbackCalled()); // Check that the callback passed in BuildAsync() has been called https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:34: const char kDefaultWebApkServerUrl[] = "https://webapk.googleapis.com/v1alpha/webApks?alt=proto"; On 2016/07/25 23:01:33, Robert Sesek wrote: > nit: 80 cols, break after = Done. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; I chose MD5 because we use the hash in many places in Chrome. Is there a different hash that we should be using? The server will compare the hash of the app icon sent by the client to the hash of the app icon that the server has fetched to determine whether the images are the same. If the hashes are the same, the WebAPK server can send a WebAPK that it has previously generated back to the client (CertifiedWebAPK). If the hashes are different, the WebAPK server needs to send back a WebAPK which has been personalized to the one user & device (OneOffWebAPK). Here is an (old) design docs for OneOffWebAPKs https://docs.google.com/document/d/1PMwF8r0oruUcyW8zkfHAzyKxGKrBk6fGkFW88T3zN... https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:63: std::string ColorToString(int64_t color) { I don't see anything in ui/gfx/color_utils.h or SkColor.h The int64_t color format using by content::Manifest is pretty special. It is different than SkColor. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); I believe this the only way of using reinterpret cast to convert signed 64 bit to unsigned 32 bit. This is reversing the logic in ManifestParser::ParseColor() and matches the logic in ExtractColor() in manifest_parser_unittest.cc Is there a better way of doing the conversion? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.h (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:39: class WebApkBuilder : public net::URLFetcherDelegate { On 2016/07/25 23:01:33, Robert Sesek wrote: > Why is this not WebApkInstaller if it also installs the APK? Done. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:41: typedef base::Callback<void(bool)> FinishCallback; On 2016/07/25 23:01:33, Robert Sesek wrote: > using FinishCallback = base::Callback<void(bool); is the preferred C++11 syntax. Done. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:51: // Talks to the Chrome WebAPK server to generate a WebAPK on the server and to No it cannot https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:54: void BuildAsync(const FinishCallback& callback); On 2016/07/25 23:01:33, Robert Sesek wrote: > Similarly, shouldn't this be "Install" ? Done. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.h:72: // Called once the WebAPK has been downloaded. On 2016/07/25 23:01:34, Robert Sesek wrote: > Mention that this installs the APK. Done.
https://codereview.chromium.org/2138973002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:23: optional string webapk_package_name = 2; I think you mentioned previously that you needed the manifest_url because it is asynchronous and needed to know which message you are getting a response to, is that not true anymore? This will be = 1 in the response and signed_download_url = 2.
https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/26 17:54:46, pkotwicz wrote: > I am confused as to what you want unittested. This class should have tests was the larger point I was making. > Do you want a test testing that the callback is called in timeout/error > scenarios? > > Something like this? > std::unique_ptr<WebApkBuilder> webapk_builder(new WebApkBuilder(...)); > webapk_builder->BuildAsync(...); > // For the sake of the test do not do any of the intermediate processing > webapk_builder->OnWebApkDownloaded(...); // Call OnWebApkDownloaded with invalid > parameters > EXPECT_TRUE(CallbackCalled()); // Check that the callback passed in BuildAsync() > has been called Right. And what happens if the download is partial and times out? Is the download file left around? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); On 2016/07/26 17:54:46, pkotwicz wrote: > I believe this the only way of using reinterpret cast to convert signed 64 bit > to unsigned 32 bit. > This is reversing the logic in ManifestParser::ParseColor() and matches the > logic in ExtractColor() in manifest_parser_unittest.cc > > Is there a better way of doing the conversion? You can just mask the lower 32 bits if you want a 32-bit quantity.
Missed one, sorry. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; On 2016/07/26 17:54:46, pkotwicz wrote: > I chose MD5 because we use the hash in many places in Chrome. Is there a > different hash that we should be using? MD5 is compromised and there are several attacks against it, so I don't think any new code should be using it. > The server will compare the hash of the app icon sent by the client to the hash > of the app icon that the server has fetched to determine whether the images are > the same. If the hashes are the same, the WebAPK server can send a WebAPK that > it has previously generated back to the client (CertifiedWebAPK). If the hashes > are different, the WebAPK server needs to send back a WebAPK which has been > personalized to the one user & device (OneOffWebAPK). Here is an (old) design > docs for OneOffWebAPKs > https://docs.google.com/document/d/1PMwF8r0oruUcyW8zkfHAzyKxGKrBk6fGkFW88T3zN... The server is making a trust decision based off this hash, which indicates that it should be cryptographically secure. MD5 is broken (as noted above), and SHA1 is at risk. I'd recommend SHA256.
https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; On 2016/07/26 22:33:26, Robert Sesek wrote: > On 2016/07/26 17:54:46, pkotwicz wrote: > > I chose MD5 because we use the hash in many places in Chrome. Is there a > > different hash that we should be using? > > MD5 is compromised and there are several attacks against it, so I don't think > any new code should be using it. > > > The server will compare the hash of the app icon sent by the client to the > hash > > of the app icon that the server has fetched to determine whether the images > are > > the same. If the hashes are the same, the WebAPK server can send a WebAPK that > > it has previously generated back to the client (CertifiedWebAPK). If the > hashes > > are different, the WebAPK server needs to send back a WebAPK which has been > > personalized to the one user & device (OneOffWebAPK). Here is an (old) design > > docs for OneOffWebAPKs > > > https://docs.google.com/document/d/1PMwF8r0oruUcyW8zkfHAzyKxGKrBk6fGkFW88T3zN... > > The server is making a trust decision based off this hash, which indicates that > it should be cryptographically secure. MD5 is broken (as noted above), and SHA1 > is at risk. I'd recommend SHA256. More background: https://goto.google.com/rtmye
Robert, can you please take another look? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. The bigger issue is: "After a WebAPK is installed, when will the downloaded APK be deleted?" I have added a TODO to address this issue. You are right that once I implement deleting the downloaded file that I should unit test the logic. I am not sure what I should unit test out of the current implementation (I honestly do not know). This file used to have unit tests but the unit tested code was moved to manifest_util.cc https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; Sorry, my answer was incomplete The WebAPK server compares: - The icon URL (I missed this part in my previous answer) - The bitmap hash against the icon URL and bitmap hash that the WebAPK server retrieved for a given Web Manifest while crawling the web The goal of the hash is to enable the server to detect whether the page generates a different app icon for each user agent. For instance www.funny.com could dynamically generate a different looking icon at www.funny.com/appicon.png for English speaking users and for French speaking users based on the User Agent If the WebAPK server incorrectly thinks that two bitmaps are identical, the user will get the WebAPK that the server generated while it was crawling the web (not another's users WebAPK) I don't mind using SHA256. However, I am still confused why a cryptographically secure hash is needed https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); How would I convert from a signed 32 bit integer to an unsigned 32 bit integer though? I would rather not change the implementation of ManifestParser::ParseColor() because the parsed value is cached in persistent storage. ManifestParser::ParseColor() currently uses reinterpret_cast. Apparently reinterpret_cast<> and static_cast<> from signed to unsigned produces the same result if the cpu architecture uses 2's complement to represent negative numbers. https://codereview.chromium.org/2138973002/diff/260001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2138973002/diff/260001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:23: optional string webapk_package_name = 2; We know which request we are getting the response to. It would be nice if the client can verify that the server did not send the download URL for a completely unrelated WebAPK. (Bug in the server code). Unfortunately, I do not think that the server sending the manifest_url back to the client helps to verify that the server did not send a completely unrelated WebAPK.
https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/27 02:50:22, pkotwicz wrote: > I am not sure what I should unit test out of the current implementation (I > honestly do not know). This file used to have unit tests but the unit tested > code was moved to manifest_util.cc This class can finish its operation in four unique ways: - Success - Download failure - Request timeout - Download timeout There should be a test for each one of these. Right now the test may just be validating the bool in the callback. When adding new classes, I find it's always good to add the basic test support as well, since that decreases friction to adding new tests in the future. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; On 2016/07/27 02:50:22, pkotwicz wrote: > Sorry, my answer was incomplete > > The WebAPK server compares: > - The icon URL (I missed this part in my previous answer) > - The bitmap hash > against the icon URL and bitmap hash that the WebAPK server retrieved for a > given Web Manifest while crawling the web > > The goal of the hash is to enable the server to detect whether the page > generates a different app icon for each user agent. For instance http://www.funny.com > could dynamically generate a different looking icon at http://www.funny.com/appicon.png > for English speaking users and for French speaking users based on the User Agent > > If the WebAPK server incorrectly thinks that two bitmaps are identical, the user > will get the WebAPK that the server generated while it was crawling the web (not > another's users WebAPK) > > I don't mind using SHA256. However, I am still confused why a cryptographically > secure hash is needed With MD5 in this code, you're currently using a cryptographic hash function, just a bad and broken one. If you're going to use a cryptographic hash function, it needs to be SHA256. If you can tolerate collisions in these hash values, then you can use a faster, smaller, non-cryptographic hash function. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); On 2016/07/27 02:50:22, pkotwicz wrote: > How would I convert from a signed 32 bit integer to an unsigned 32 bit integer > though? > > I would rather not change the implementation of ManifestParser::ParseColor() > because the parsed value is cached in persistent storage. > ManifestParser::ParseColor() currently uses reinterpret_cast. > > Apparently reinterpret_cast<> and static_cast<> from signed to unsigned produces > the same result if the cpu architecture uses 2's complement to represent > negative numbers. I see. Why put this in a signed int32 at all then? Java can read this as a long and only access the lower 32 bits, and then the comment about negative alphas in manifest_parser.cc would no longer apply.
Patchset #11 (id:300001) has been deleted
Robert, can you please take another look? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. I have added integration tests in webapk_installer_unittest.cc The tests check that the WebApkInstaller installation process finishes. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:56: base::MD5Digest digest; Switched to Murmur2 instead https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); I am unsure what you are suggesting. Are you suggesting changing the implementation of ManifestParser::ParseColor() line 149 in particular?
LGTM. We don't need to gate this CL on the color encoding discussion. https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); On 2016/07/28 18:29:51, pkotwicz wrote: > I am unsure what you are suggesting. Are you suggesting changing the > implementation of ManifestParser::ParseColor() line 149 in particular? I'm trying to understand why a signed int32 was used when it can lose alpha precision, when an int64/java long could have been used instead. https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:240: response->set_content("😀"); :)
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
dominickn@ and dfalcantara@ can you please take a look? https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_builder.cc (right): https://codereview.chromium.org/2138973002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_builder.cc:67: SkColor sk_color = reinterpret_cast<uint32_t&>(color); I am not sure why this decision was made. I suspect that part of the reason is compatibility with the Android int color format specified in https://developer.android.com/reference/android/graphics/Color.html
Don't know what I'm supposed to be looking at, in particular. https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:16: * install succeeds. indent so that "install" lines up with "True" https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java (right): https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java:14: * Java counterpart to webapk_installer.h It's weird that you have a webapk_installer.h that hooks to a WebApkInstallerBridge.java but you also have a WebApkInstaller.java that doesn't actually have a native side. Introduces more confusion than it really should. Why the difference? https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... File chrome/browser/android/shortcut_helper.cc (right): https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... chrome/browser/android/shortcut_helper.cc:170: LOG(ERROR) << "Sent request to install WebAPK. Seems to have worked."; ERROR doesn't seem like the right LOG level to use here. https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... chrome/browser/android/webapk/webapk_installer.cc:41: const uint32_t kMurmur2HashSeed = 0; The seed is unimportant, I guess? https://chromiumcodereview.appspot.com/2138973002/diff/320001/chrome/browser/... chrome/browser/android/webapk/webapk_installer.cc:161: // Must be called on UI thread. Not sure if you get anything by having this comment here, give nthat the function says that it has to be on the UI thread and it DCHECKs.
dfalcantara@ and dominickn@ can you please take another look? I am asking for a code review because you are the real owners of: - chrome/browser/android/webapps - chrome/android/java/src/org/chromium/chrome/browser/webapps - ShortcutHelper.java https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:16: * install succeeds. On 2016/07/29 21:20:28, dfalcantara wrote: > indent so that "install" lines up with "True" Done. https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java:14: * Java counterpart to webapk_installer.h The Java WebApkInstaller implements the calls to the private Google Play Api (or it will in the future) chrome_apk uses WebApkInstallerImpl chrome_public_apk does not use a WebApkInstaller at all I can merge WebApkInstaller and WebApkInstallerBridge if you want. That would make WebApkInstaller an abstract class and no longer an interface. Do you prefer this? https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:170: LOG(ERROR) << "Sent request to install WebAPK. Seems to have worked."; Changed to LOG(INFO). The reason for the log is so that this function is non-empty. Once this function is non-empty I will move the logging to webapk_installer.cc https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:41: const uint32_t kMurmur2HashSeed = 0; The seed has to be the same as the one used on the server. Other than that I do not think it matters which seed we use. https://codereview.chromium.org/2138973002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:161: // Must be called on UI thread. I have changed the comment to say that Profile::GetRequestContext() must be called on the UI thread. This is non-obvious I believe that this is due to BrowserContext::GetStoragePartitionMap() calling BrowserContext::GetUserData().
dfalcantara@ and dominickn@ can you please take another look? I am asking for a code review because you are the real owners of: - chrome/browser/android/webapps - chrome/android/java/src/org/chromium/chrome/browser/webapps - ShortcutHelper.java
dfalcantara@ and dominickn@ can you please take another look? I am asking for a code review because you are the real owners of: - chrome/browser/android/webapps - chrome/android/java/src/org/chromium/chrome/browser/webapps - ShortcutHelper.java
Patchset #12 (id:340001) has been deleted
https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right): https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:48: browser_context_(web_contents->GetBrowserContext()) { Instead of adding this extra member, can you just call data_fetcher_->web_contents()->GetBrowserContext() in AddShortcut?
https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right): https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:48: browser_context_(web_contents->GetBrowserContext()) { It is possible for the tab that a user requested be added to the homescreen to be closed prior to the user selecting "ADD" in the "Add to Home screen" dialog. I have a test page which closes itself at https://jsfiddle.net/nbq637k8/5/ This means that calling data_fetcher_->web_contents()->GetBrowserContext() in AddToHomescreenDialogHelper#AddShortcut() may cause a Segfault. I am unsure what the correct solution is. Should we not create a shortcut in this scenario? (And display some error message?)
https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right): https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:48: browser_context_(web_contents->GetBrowserContext()) { On 2016/08/02 20:49:29, pkotwicz wrote: > It is possible for the tab that a user requested be added to the homescreen to > be closed prior to the user selecting "ADD" in the "Add to Home screen" dialog. > I have a test page which closes itself at https://jsfiddle.net/nbq637k8/5/ > > This means that calling data_fetcher_->web_contents()->GetBrowserContext() in > AddToHomescreenDialogHelper#AddShortcut() may cause a Segfault. I am unsure what > the correct solution is. Should we not create a shortcut in this scenario? (And > display some error message?) If a tab is closed before add to homescreen finishes, I think the appropriate thing to do is to abort the procedure without an error message. That's what app banners do if the WebContents unexpectedly closes during the pipeline. The tab closing should take everything that's attached to it away - e.g. the add to homescreen dialog. Concretely, I think that just means null-checking data_fetcher_->web_contents() in AddToHomescreenDialogHelper#AddShortcut(), and just returning if it's null. Otherwise, pass GetBrowserContext() to ShortcutHelper. WDYT?
dominickn@ can you please take another look? https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right): https://codereview.chromium.org/2138973002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:48: browser_context_(web_contents->GetBrowserContext()) { I have added the early exit to AddToHomescreenDialogHelper#AddShortcut() Currently, the "Add to homescreen" dialog does not close when the tab is closed but I don't think that it is worth fixing
lgtm thanks!
dfalcantara@ can you please take a look?
On 2016/08/03 03:32:55, pkotwicz wrote: > dfalcantara@ can you please take a look? I'm running through my exploding queue.
https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java (right): https://codereview.chromium.org/2138973002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallerBridge.java:14: * Java counterpart to webapk_installer.h On 2016/08/02 04:09:37, pkotwicz wrote: > The Java WebApkInstaller implements the calls to the private Google Play Api (or > it will in the future) > > chrome_apk uses WebApkInstallerImpl > chrome_public_apk does not use a WebApkInstaller at all > > I can merge WebApkInstaller and WebApkInstallerBridge if you want. That would > make WebApkInstaller an abstract class and no longer an interface. Do you prefer > this? Yeah. Thought you'd already done this for the latest patch but was mistaken. It makes tracking JNI calls a lot easier.
Patchset #14 (id:400001) has been deleted
Patchset #14 (id:420001) has been deleted
dfalcantara@ can you please take another look? I have merged WebApkInstaller and WebApkInstallerBridge as you requested
lgtm
pkotwicz@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri@ can you please take a look at the changes in content/ ?
https://codereview.chromium.org/2138973002/diff/440001/content/common/manifes... File content/common/manifest_util_unittest.cc (right): https://codereview.chromium.org/2138973002/diff/440001/content/common/manifes... content/common/manifest_util_unittest.cc:16: WebDisplayModeFromString(display_string); I don't think this test is working. If you wrote your code such as `WebDisplayBrowser` returns "foobar" and "foobar" is interpreted as `WebDisplayBrowser`, the test would pass but I don't think it would mean the implementation is correct. Maybe you should test one method at a time? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... File content/public/common/manifest_util.cc (right): https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:7: #include "base/strings/string_util.h" Why do you need this? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:27: if (display == "browser") Maybe add a DCHECK() enforcing that |display| is lower case? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:35: else scratch the `else` here https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:65: if (orientation == "portrait-primary") As above, maybe add a DCHECK? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... File content/public/common/manifest_util.h (right): https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:17: // https://www.w3.org/TR/appmanifest/#dfn-fallback-display-mode You probably meant: https://www.w3.org/TR/appmanifest/#dfn-display-modes-values https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:23: // https://www.w3.org/TR/appmanifest/#dfn-fallback-display-mode. Returns ditto https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:24: // blink::WebDisplayModeUndefined if there is no match. Add that |display_string| must be lower case? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:26: const std::string& display_string); nit: you use `display` in the implementation. https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:29: // https://www.w3.org/TR/screen-orientation/#orientationlocktype-enum Maybe note that WebScreenOrientationLockDefault will return the empty string? https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:31: blink::WebScreenOrientationLockType); style: add parameter name. https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:34: // |orientation_string|. |orientation_string| should be one of see below https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.h:38: WebScreenOrientationLockTypeFromString(const std::string& orientation_string); s/orientation_string/orientation/ to match the implementation?
mlamouri: I will respond to your other comments but I wanted to reply to the "test comment" before you got off work https://codereview.chromium.org/2138973002/diff/440001/content/common/manifes... File content/common/manifest_util_unittest.cc (right): https://codereview.chromium.org/2138973002/diff/440001/content/common/manifes... content/common/manifest_util_unittest.cc:16: WebDisplayModeFromString(display_string); I am banking on ManifestParserTest#OrientationParserRules() testing content::WebScreenOrientationLockTypeToString() - Should I add a comment to make this assumption more explicit - Should I move ManifestParserTest#OrientationParserRules() to manifest_util_unittest.cc ?
mlamouri@ can you please take another look? I felt uncomfortable removing the test so I made the test test all of the ifs() in each of the functions in manifest_util.cc I think that the testing that WebDisplayModeFromString() and WebScreenOrientationLockTypeFromString() are reversible is important https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... File content/public/common/manifest_util.cc (right): https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:27: if (display == "browser") I have changed this function to be case insensitive https://codereview.chromium.org/2138973002/diff/440001/content/public/common/... content/public/common/manifest_util.cc:65: if (orientation == "portrait-primary") I have changed this function to be case insensitive
lgtm Looks great! Thanks :) https://codereview.chromium.org/2138973002/diff/460001/content/public/common/... File content/public/common/manifest_util.h (right): https://codereview.chromium.org/2138973002/diff/460001/content/public/common/... content/public/common/manifest_util.h:25: // |display| does not matter. Returns blink::WebDisplayModeUndefined if there is case doesn't matter => case insensitive https://codereview.chromium.org/2138973002/diff/460001/content/public/common/... content/public/common/manifest_util.h:40: // The case of |orientation| does not matter. Returns ditto
pkotwicz@chromium.org changed reviewers: + brettw@chromium.org, sievers@chromium.org
sievers@ for content/ OWNERS
pkotwicz@chromium.org changed reviewers: - brettw@chromium.org
seems fine, just a few nit questions/comments https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... File content/public/common/manifest_util.h (right): https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... content/public/common/manifest_util.h:15: Feels every so slightly odd that the content public api exposes these blink enum<->string helper (and not for example Blink itself). But then again, the types are exposed a lot in the content-public api here, so also not totally unreasonable. But then it's probably also not a huge deal for it to exist in the context of the manifest api here. https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:228: AddErrorInfo("unknown 'display' value ignored."); why is this an error? seems to be allowed by the spec not to override the display mode. https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:244: AddErrorInfo("unknown 'orientation' value ignored."); why is 'default' orientation an error?
https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... File content/public/common/manifest_util.h (right): https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... content/public/common/manifest_util.h:15: On 2016/08/05 17:53:08, sievers wrote: > Feels every so slightly odd that the content public api exposes these blink > enum<->string helper (and not for example Blink itself). But then again, the > types are exposed a lot in the content-public api here, so also not totally > unreasonable. > > But then it's probably also not a huge deal for it to exist in the context of > the manifest api here. I should also point out this from the content-api guide (https://www.chromium.org/developers/content-module/content-api): "content/public should contain only interfaces, structs and (rarely) static functions". Which is of course in this case not a definite recommendation or even rule :)
https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... File content/public/common/manifest_util.h (right): https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... content/public/common/manifest_util.h:15: sievers: Do you think that I should move the functions to third_party/WebKit/public/platform/WebDisplayModeUtil.h and third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationUtil.h ? https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:228: AddErrorInfo("unknown 'display' value ignored."); The case of "display mode not in Web Manifest" is handled by line 223 https://codereview.chromium.org/2138973002/diff/480001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:244: AddErrorInfo("unknown 'orientation' value ignored."); According to the spec (https://www.w3.org/TR/appmanifest/#orientation-member): "If value is not one of the OrientationLockType enum values, or value is unsupported by the user agent, or the value cannot be used together with display mode: - Issue a developer warning. - Return undefined." 'default' is not an OrientationLockType enum value. See https://w3c.github.io/screen-orientation/#orientationlocktype-enum
On 2016/08/05 18:16:22, pkotwicz wrote: > https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... > File content/public/common/manifest_util.h (right): > > https://codereview.chromium.org/2138973002/diff/480001/content/public/common/... > content/public/common/manifest_util.h:15: > sievers: Do you think that I should move the functions to > third_party/WebKit/public/platform/WebDisplayModeUtil.h and > third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationUtil.h > ? > Maybe somebody from Blink can comment. If it's not wanted there, then let's just put it in content like you suggest.
It looks like mlamouri@ is the OWNER for third_party/WebKit/public/platform/modules/screen_orientation/ I will remove the content/ changes from this CL and continue discussion as part of a follow up CL
Patchset #17 (id:500001) has been deleted
Description was changed from ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 ========== to ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ==========
Description was changed from ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ========== to ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 TEST=WebApkInstallerTest.* R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ==========
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottkirkwood@chromium.org, hanxi@chromium.org, yfriedman@chromium.org, rsesek@chromium.org, dominickn@chromium.org, dfalcantara@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2138973002/#ps520001 (title: "Merge branch 'master' into webapk_builder_impl22_no_content")
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, mlamouri@chromium.org, scottkirkwood@chromium.org, yfriedman@chromium.org, rsesek@chromium.org, hanxi@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2138973002/#ps540001 (title: "Merge branch 'master' into webapk_builder_impl22_no_content")
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, mlamouri@chromium.org, scottkirkwood@chromium.org, yfriedman@chromium.org, rsesek@chromium.org, hanxi@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2138973002/#ps560001 (title: "Merge branch 'master' into webapk_builder_impl22_no_content")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 TEST=WebApkInstallerTest.* R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ========== to ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 TEST=WebApkInstallerTest.* R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ==========
Message was sent while issue was closed.
Committed patchset #19 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 TEST=WebApkInstallerTest.* R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) ========== to ========== This CL changes the behavior of ShortcutHelper::InstallWebApkInBackgroundWithSkBitmap() - To talk to WebAPK server (https://webapk.googleapis.com/v1alpha/webApks?alt=proto) to generate WebAPK - To download WebAPK from URL returned by WebAPK server This CL also: - Adds stub for talking to Google Play client to install WebAPK - Introduces manifest_util.h with methods for converting blink::WebDisplayMode and blink::WebScreenOrientationLockType to and from string BUG=619739 TEST=WebApkInstallerTest.* R=dominickn,scottkirkwood,yfriedman,rsesek,hanxi,dfalcantara TBR=brettw (for third_party/smhasher/BUILD.gn) Committed: https://crrev.com/879b1ed437bd6e1d535d0dfa0c3c16f79e0b55d2 Cr-Commit-Position: refs/heads/master@{#410233} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/879b1ed437bd6e1d535d0dfa0c3c16f79e0b55d2 Cr-Commit-Position: refs/heads/master@{#410233} |
