Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303003002:
Restrict Google Now URL Permissions (Closed)
|
Created:
6 years, 6 months ago by robliao Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRestrict Google Now URL Permissions
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273638
Patch Set 1 #
Total comments: 4
Messages
Total messages: 12 (0 generated)
xiyuan: Please provide owner approval for this CL. Thanks!
lgtm
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/303003002/1
https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/manifest.json:24: "https://*.googleusercontent.com/*" Did you find something that suggested that restaurant/hotel/etc cards would only be from one of those listed? What's the failure mode if we get this wrong? Will we get a jserror report?
https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/manifest.json:24: "https://*.googleusercontent.com/*" Nope. The images I got back from my tests are either direct data or links to googleusercontent (G+, Frequent Places, Maps). The current failure mode is to log an error to the console. Once we do a test pass on this, it should be clear what works and doesn't work. On 2014/05/29 17:30:01, rgustafson wrote: > Did you find something that suggested that restaurant/hotel/etc cards would only > be from one of those listed? > > What's the failure mode if we get this wrong? Will we get a jserror report?
https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/manifest.json:21: "*://*.google.com/*", overall I'd be more worried about blocking something with this CL, but we have a few weeks for bugs to pop up -- so on the other hand, could this line be made more narrow?
lgtm
https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/303003002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/manifest.json:21: "*://*.google.com/*", Issue discussed. On 2014/05/29 18:28:53, Travis Skare wrote: > overall I'd be more worried about blocking something with this CL, but we have a > few weeks for bugs to pop up -- so on the other hand, could this line be made > more narrow?
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
Message was sent while issue was closed.
Change committed as 273638 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
