Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415183008:
Use new Physical Web service on Google infrastructure (Closed)
|
Created:
5 years, 1 month ago by dvh Modified:
5 years, 1 month ago CC:
chromium-reviews, klundberg+watch_chromium.org, mikecase+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org, mmocny1, cco3 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse new Physical Web service on Google infrastructure
BUG=529962
Committed: https://crrev.com/00cf1f63a3aad6ab1ecf25640953b3b7a4a1b4bd
Cr-Commit-Position: refs/heads/master@{#358432}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== Use new Physical Web service on Google infrastructure BUG=529962 ========== to ========== Use new Physical Web service on Google infrastructure BUG=529962 ==========
dvh@chromium.org changed reviewers: + agrieve@chromium.org, newt@chromium.org, rogerta@chromium.org
newt@chromium.org: Please review changes in chrome/android/ agrieve@chromium.org: Please review changes in build/android/ rogerta@chromium.org: Please review changes in google_apis/ Thanks!
On 2015/11/05 19:54:16, dvh wrote: > mailto:newt@chromium.org: Please review changes in > chrome/android/ > > mailto:agrieve@chromium.org: Please review changes in > build/android/ > > mailto:rogerta@chromium.org: Please review changes in > google_apis/ > > Thanks! build/ lgtm
google_apis lgtm, with one comment below. https://codereview.chromium.org/1415183008/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): https://codereview.chromium.org/1415183008/diff/1/google_apis/google_api_keys... google_apis/google_api_keys.py:74: return _GetToken('GOOGLE_API_KEY_PHYSICAL_WEB_TEST') There should be two blank lines between functions. Please add one above and below.
dvh@chromium.org changed reviewers: + dtrainor@chromium.org
dtrainor@, could you please take a look at the change in chrome/android? Thanks!
newt@chromium.org changed reviewers: - newt@chromium.org
newt@chromium.org changed reviewers: + newt@chromium.org
I'm happy to rubberstamp PwsClient.java once someone else reviews it, but someone else needs to review it -- I don't have any background on this code. https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java (right): https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:57: return GoogleAPIKeys.GOOGLE_API_KEY_PHYSICAL_WEB_TEST; Do we really want to use GOOGLE_API_KEY_PHYSICAL_WEB_TEST on Beta builds and Dev? I'd recommend only using the test keys on local builds (or perhaps canary and dev) if that works for your purposes.
https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java (right): https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:53: private String apiKey() { nit: Should be called "getApiKey()" https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:140: String url = ENDPOINT_URL + "?key=" + apiKey(); variable unused?
https://codereview.chromium.org/1415183008/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): https://codereview.chromium.org/1415183008/diff/1/google_apis/google_api_keys... google_apis/google_api_keys.py:74: return _GetToken('GOOGLE_API_KEY_PHYSICAL_WEB_TEST') On 2015/11/06 13:45:55, Roger Tawa wrote: > There should be two blank lines between functions. Please add one above and > below. Done. https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java (right): https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:53: private String apiKey() { On 2015/11/06 19:17:53, agrieve wrote: > nit: Should be called "getApiKey()" Done. https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:57: return GoogleAPIKeys.GOOGLE_API_KEY_PHYSICAL_WEB_TEST; On 2015/11/06 19:02:47, newt wrote: > Do we really want to use GOOGLE_API_KEY_PHYSICAL_WEB_TEST on Beta builds and > Dev? I'd recommend only using the test keys on local builds (or perhaps canary > and dev) if that works for your purposes. Discussed about the plan offline. https://codereview.chromium.org/1415183008/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:140: String url = ENDPOINT_URL + "?key=" + apiKey(); On 2015/11/06 19:17:53, agrieve wrote: > variable unused? Acknowledged.
java lgtm https://codereview.chromium.org/1415183008/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java (right): https://codereview.chromium.org/1415183008/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:28: private static final String ENDPOINT_URL = "https://test-physicalweb.sandbox.googleapis.com/v1alpha1/urls:resolve"; nit: Please add a TODO() here explaining the scope of this endpoint (e.g. used only for GOOGLE_API_KEY_PHYSICAL_WEB_TEST api key. to be replaced by permanent endpoint at some point in the future.
rubberstamp lgtm based on agrieve's review
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1415183008/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java (right): https://codereview.chromium.org/1415183008/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:28: private static final String ENDPOINT_URL = "https://test-physicalweb.sandbox.googleapis.com/v1alpha1/urls:resolve"; On 2015/11/06 19:33:29, agrieve wrote: > nit: Please add a TODO() here explaining the scope of this endpoint (e.g. used > only for GOOGLE_API_KEY_PHYSICAL_WEB_TEST api key. to be replaced by permanent > endpoint at some point in the future. Done.
After discussions with mmocny@, we're going with the final URL here.
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, agrieve@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1415183008/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415183008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415183008/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/00cf1f63a3aad6ab1ecf25640953b3b7a4a1b4bd Cr-Commit-Position: refs/heads/master@{#358432} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
