Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1165293005:
Add security handler stub for sending the lock icon status to DevTools. (Closed)
|
Created:
5 years, 6 months ago by lgarron Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, yurys Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd security handler stub for sending the lock icon status to DevTools.
This CL will be followed up with:
1. https://codereview.chromium.org/1167693010 (Blink)
2. https://codereview.chromium.org/1163963002 (Chrome)
BUG=445359, 484401
Committed: https://crrev.com/05cbd867a2832098acb777109f2431ec87813fb0
Cr-Commit-Position: refs/heads/master@{#333573}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove SetClient() call. #
Total comments: 7
Patch Set 3 : Remove spare include. #Patch Set 4 : virtual destructor + SetClient #
Total comments: 2
Messages
Total messages: 30 (8 generated)
lgarron@chromium.org changed reviewers: + estark@chromium.org, pfeldman@chromium.org
lgarron@chromium.org changed reviewers: - estark@chromium.org, pfeldman@chromium.org
estark@chromium.org changed reviewers: + estark@chromium.org
https://codereview.chromium.org/1165293005/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:22: void SetClient(scoped_ptr<Client> client); Does this build? I thought the Client definition would be generated code, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...
lgarron@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1165293005/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:22: void SetClient(scoped_ptr<Client> client); On 2015/06/09 at 05:07:48, estark wrote: > Does this build? I thought the Client definition would be generated code, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont... It does not build. Not sure how I ended up committing that. Fixed int he latest patch.
lgtm https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:9: #include "content/public/common/security_style.h" Unused? https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); virtual
https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:21: Sorry, I should have been more specific in my last comment. I think we *do* need a SetClient() method here (since generated code will attempt to call it: https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). But SetClient() can't take a devtools::security::Client as an argument because that class isn't defined yet. Instead, I guess it should take a scoped_ptr<DevToolsProtocolClient*> as an argument (which is the base class of the devtools::*::Client classes), and we can change that to a devtools::security::Client once the protocol.json changes land.
https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:9: #include "content/public/common/security_style.h" On 2015/06/09 at 18:35:53, pfeldman wrote: > Unused? Indeed. It'll be needed later, but I've removed it anyhow. https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); On 2015/06/09 at 18:35:53, pfeldman wrote: > virtual Are you saying I should make it virtual? Other handlers are inconsistent about whether to mark this as `virtual`, `override`, or neither.
https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); virtual for now, change to override later. https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:21: On 2015/06/09 18:48:25, estark wrote: > Sorry, I should have been more specific in my last comment. I think we *do* need > a SetClient() method here (since generated code will attempt to call it: > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). > But SetClient() can't take a devtools::security::Client as an argument because > that class isn't defined yet. Instead, I guess it should take a > scoped_ptr<DevToolsProtocolClient*> as an argument (which is the base class of > the devtools::*::Client classes), and we can change that to a > devtools::security::Client once the protocol.json changes land. This is a good point - you need a generic SetClient for the first commit.
On 2015/06/09 at 19:04:23, pfeldman wrote: > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > File content/browser/devtools/protocol/security_handler.h (right): > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); > virtual for now, change to override later. > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > content/browser/devtools/protocol/security_handler.h:21: > On 2015/06/09 18:48:25, estark wrote: > > Sorry, I should have been more specific in my last comment. I think we *do* need > > a SetClient() method here (since generated code will attempt to call it: > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). > > But SetClient() can't take a devtools::security::Client as an argument because > > that class isn't defined yet. Instead, I guess it should take a > > scoped_ptr<DevToolsProtocolClient*> as an argument (which is the base class of > > the devtools::*::Client classes), and we can change that to a > > devtools::security::Client once the protocol.json changes land. > > This is a good point - you need a generic SetClient for the first commit. So, I'm getting Chrome to compile and run without SetClient() – if it's actually needed, am I missing something?
Okay, the latest patch has the virtual destructor and SetClient().
On 2015/06/09 19:41:54, lgarron wrote: > Okay, the latest patch has the virtual destructor and SetClient(). Did you try building this with the changes to protocol.json?
On 2015/06/09 at 19:44:50, pfeldman wrote: > On 2015/06/09 19:41:54, lgarron wrote: > > Okay, the latest patch has the virtual destructor and SetClient(). > > Did you try building this with the changes to protocol.json? Yes, I just built and ran it with https://codereview.chromium.org/1167693010 exactly.
I also just specifically built patch 4 against (a recent) ToT Blink, and it also builds and runs fine.
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/1165293005/#ps60001 (title: "virtual destructor + SetClient")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165293005/60001
pfeldman@: I'm doing a CQ dry run just to make sure. You've technically already given LGTM, but any last thoughts before landing?
On 2015/06/09 19:29:16, lgarron wrote: > On 2015/06/09 at 19:04:23, pfeldman wrote: > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > File content/browser/devtools/protocol/security_handler.h (right): > > > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); > > virtual for now, change to override later. > > > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > content/browser/devtools/protocol/security_handler.h:21: > > On 2015/06/09 18:48:25, estark wrote: > > > Sorry, I should have been more specific in my last comment. I think we *do* > need > > > a SetClient() method here (since generated code will attempt to call it: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). > > > But SetClient() can't take a devtools::security::Client as an argument > because > > > that class isn't defined yet. Instead, I guess it should take a > > > scoped_ptr<DevToolsProtocolClient*> as an argument (which is the base class > of > > > the devtools::*::Client classes), and we can change that to a > > > devtools::security::Client once the protocol.json changes land. > > > > This is a good point - you need a generic SetClient for the first commit. > > So, I'm getting Chrome to compile and run without SetClient() – if it's actually > needed, am I missing something? It's just that the protocol.json change would cause it to stop building if you didn't have SetClient, because the protocl.json change generates code that tries to call SetClient.
On 2015/06/09 at 20:05:42, estark wrote: > On 2015/06/09 19:29:16, lgarron wrote: > > On 2015/06/09 at 19:04:23, pfeldman wrote: > > > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > > File content/browser/devtools/protocol/security_handler.h (right): > > > > > > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > > content/browser/devtools/protocol/security_handler.h:20: ~SecurityHandler(); > > > virtual for now, change to override later. > > > > > > > > https://codereview.chromium.org/1165293005/diff/20001/content/browser/devtool... > > > content/browser/devtools/protocol/security_handler.h:21: > > > On 2015/06/09 18:48:25, estark wrote: > > > > Sorry, I should have been more specific in my last comment. I think we *do* > > need > > > > a SetClient() method here (since generated code will attempt to call it: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). > > > > But SetClient() can't take a devtools::security::Client as an argument > > because > > > > that class isn't defined yet. Instead, I guess it should take a > > > > scoped_ptr<DevToolsProtocolClient*> as an argument (which is the base class > > of > > > > the devtools::*::Client classes), and we can change that to a > > > > devtools::security::Client once the protocol.json changes land. > > > > > > This is a good point - you need a generic SetClient for the first commit. > > > > So, I'm getting Chrome to compile and run without SetClient() – if it's actually > > needed, am I missing something? > > It's just that the protocol.json change would cause it to stop building if you didn't have SetClient, because the protocl.json change generates code that tries to call SetClient. Hmm, I understand that, but I think I *did* build with the protocol.json change. That was an hour ago, so maybe I recall incorrectly. I'll try again later, out of curiosity.
https://codereview.chromium.org/1165293005/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:9: #include "content/browser/devtools/protocol/devtools_protocol_client.h" nit: you can forward-declare this and do the #include in the .cc file Not a big deal though because this will go away in the follow-up patch.
https://codereview.chromium.org/1165293005/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1165293005/diff/60001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:9: #include "content/browser/devtools/protocol/devtools_protocol_client.h" On 2015/06/09 at 20:39:20, estark wrote: > nit: you can forward-declare this and do the #include in the .cc file > > Not a big deal though because this will go away in the follow-up patch. Will keep in mind for the future. Since it'll be removed in the follow-up, I'll leave it here for now and avoid another rebase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1165293005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05cbd867a2832098acb777109f2431ec87813fb0 Cr-Commit-Position: refs/heads/master@{#333573} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
