Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1167693010:
Add a DevTools Security domain to protocol.json. (Closed)
|
Created:
5 years, 6 months ago by lgarron Modified:
5 years, 6 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd a DevTools Security domain to protocol.json.
BUG=445359, 484398
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196883
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address Pavel's comments. #Patch Set 3 : Add "unknown" as a security state, and reorder to match the SecurityStyle enum (also followed in SecurityStyleToProtocolSecurityState()). #
Total comments: 1
Patch Set 4 : Exclude Security from the agents-enable-disable test (like ServiceWorker). #Messages
Total messages: 27 (11 generated)
lgarron@chromium.org changed reviewers: + estark@chromium.org
https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1064: "id": "SecurityState", estark@: I used "SecurityState" here because "SecurityStyle" leads to a conflicting generated code type. When talking about DevTools or the lock icon, I usually talk about the *status* of a page/resource, whose value is a state (depending on several other states). But that's all arbitrary. Any opinions?
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1061: "description": "Security", add "hidden": true https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1066: "enum": ["SECURE", "WARNING", "INSECURE", "HTTP"], Please use lower case for the protocol enum literals. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1080: "commands": [ types, then commands, then events. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1083: "parameters": [], no need to specify https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1086: "hidden": true lets hide entire domain instead. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1093: "hidden": true ditto
https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1061: "description": "Security", On 2015/06/04 at 09:21:55, pfeldman wrote: > add "hidden": true Done. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1066: "enum": ["SECURE", "WARNING", "INSECURE", "HTTP"], On 2015/06/04 at 09:21:55, pfeldman wrote: > Please use lower case for the protocol enum literals. Done. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1080: "commands": [ On 2015/06/04 at 09:21:54, pfeldman wrote: > types, then commands, then events. Done. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1083: "parameters": [], On 2015/06/04 at 09:21:55, pfeldman wrote: > no need to specify Done. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1086: "hidden": true On 2015/06/04 at 09:21:55, pfeldman wrote: > lets hide entire domain instead. Done. https://codereview.chromium.org/1167693010/diff/1/Source/devtools/protocol.js... Source/devtools/protocol.json:1093: "hidden": true On 2015/06/04 at 09:21:55, pfeldman wrote: > ditto Done (also with parameters).
lgtm
https://codereview.chromium.org/1167693010/diff/40001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1167693010/diff/40001/Source/devtools/protoco... Source/devtools/protocol.json:1067: "enum": ["unknown", "http", "insecure", "warning", "secure"], Added "unknown", since the security handler can technically send that value.
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1167693010/#ps40001 (title: "Add "unknown" as a security state, and reorder to match the SecurityStyle enumn (also followed in SecurityStyleToProtocolSecurityState()).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167693010/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1167693010/#ps60001 (title: "Exclude Security from the agents-enable-disable test (like ServiceWorker).")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167693010/60001
The trybots failed because LayoutTests/inspector/agents-enable-disable.html tries to call `enable` and disable` on the Security agent – except there isn't one, because the the only handler is in the browser. This is a similar situation to `ServiceWorker`, so I've excluded it from the test in the same way.
The CQ bit was unchecked by lgarron@chromium.org
The CQ bit was checked by lgarron@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/1167693010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pfeldman: This already has your LGTM; would you mind confirming that the test fix is okay?
lgtm
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167693010/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196883 |
