Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1430993002:
Introduce isSandboxedForTesting. (Closed)
DescriptionIntroduce isSandboxedForTesting.
security_StatusSandboxStatus can use this method and not scrape the page
going forward.
BUG=549681
TEST=security_SandboxStatus
Committed: https://crrev.com/ca1790aff1ba6f498652fc03a84f356e11104531
Cr-Commit-Position: refs/heads/master@{#358137}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update comment #Messages
Total messages: 16 (6 generated)
achuith@chromium.org changed reviewers: + jorgelo@chromium.org, kbr@chromium.org
These lines are fragile, and we'd like to replace them with isSandboxedForTesting: https://cs.corp.google.com/#chromeos_public/src/third_party/autotest/files/cl... Please take a look, Ken and Jorge.
lgtm https://codereview.chromium.org/1430993002/diff/1/content/browser/resources/g... File content/browser/resources/gpu/browser_bridge.js (right): https://codereview.chromium.org/1430993002/diff/1/content/browser/resources/g... content/browser/resources/gpu/browser_bridge.js:160: * Returns the value of the sandboxed row. Nit: I'd make it even more evident: Returns the value of the "Sandboxed" row.
Ken, are there any tests for BrowserBridge? I looked at: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... But it looks like these aren't actually used anywhere. https://codereview.chromium.org/1430993002/diff/1/content/browser/resources/g... File content/browser/resources/gpu/browser_bridge.js (right): https://codereview.chromium.org/1430993002/diff/1/content/browser/resources/g... content/browser/resources/gpu/browser_bridge.js:160: * Returns the value of the sandboxed row. On 2015/11/04 18:04:44, Jorge Lucangeli Obes wrote: > Nit: I'd make it even more evident: > > Returns the value of the "Sandboxed" row. Done.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430993002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce isSandboxedForTesting. security_StatusSandboxStatus can use this method and not scrape the page going forward. BUG=549681 TEST=security_SandboxStatus ========== to ========== Introduce isSandboxedForTesting. security_StatusSandboxStatus can use this method and not scrape the page going forward. BUG=549681 TEST=security_SandboxStatus ==========
On 2015/11/04 18:10:52, achuithb wrote: > Ken, are there any tests for BrowserBridge? I looked at: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > But it looks like these aren't actually used anywhere. Sorry, I don't know off the top of my head how these tests are supposed to be invoked. I studied the original CL https://codereview.chromium.org/6712048 which introduced them and it's not clear they were hooked up. CC'ing nduca. It looks like src/chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc is a pattern for how these should be tested. Can help study this more next week if necessary. If this has been tested with the associated change to security_SandboxStatus.py then lgtm.
On 2015/11/05 08:43:31, Ken Russell wrote: > On 2015/11/04 18:10:52, achuithb wrote: > > Ken, are there any tests for BrowserBridge? I looked at: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > > > But it looks like these aren't actually used anywhere. > > Sorry, I don't know off the top of my head how these tests are supposed to be > invoked. I studied the original CL https://codereview.chromium.org/6712048 which > introduced them and it's not clear they were hooked up. CC'ing nduca. It looks > like src/chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc > is a pattern for how these should be tested. > > Can help study this more next week if necessary. > > If this has been tested with the associated change to security_SandboxStatus.py > then lgtm. Yup, I have this CL: https://chromium-review.googlesource.com/#/c/310875/ I've verified that it works correctly. I'll land this change and we can figure out how to add either a browsertest or a more explicit dependency so this doesn't accidentally break.
The CQ bit was checked by achuith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jorgelo@chromium.org Link to the patchset: https://codereview.chromium.org/1430993002/#ps20001 (title: "update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430993002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ca1790aff1ba6f498652fc03a84f356e11104531 Cr-Commit-Position: refs/heads/master@{#358137} |
