Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1163963002:
Implement SecurityHandler to send 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. |
DescriptionImplement SecurityHandler to send the lock icon status to DevTools.
BUG=445359, 484401
Committed: https://crrev.com/109a12898b9a46a620b67cde853e5ef431fa2799
Cr-Commit-Position: refs/heads/master@{#334223}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Make SecurityHandler a WebContentsObserver itself and rebase onto stub CL. #
Total comments: 8
Patch Set 3 : Style fixes. #
Total comments: 3
Patch Set 4 : Add #include that was removed from stub CL. #Patch Set 5 : Restore stub as the diff base. #Patch Set 6 : Change destructor to override. #
Total comments: 5
Patch Set 7 : Fix method order, include, and NOTREACHED(). #Patch Set 8 : Add web_contents.h to security_handler.cc for WebContents::FromRenderFrameHost(). #
Total comments: 10
Patch Set 9 : Address Pavel's comments. #
Total comments: 2
Patch Set 10 : Only attach SecurityHandler to top-level frames. #
Messages
Total messages: 45 (9 generated)
estark@chromium.org changed reviewers: + estark@chromium.org
I'm still thinking about how to test this one. Any ideas? Left a few style comments inline in the meantime. For testing, I'd like if we could do something along the lines of devtools_protocol_browsertest.cc, but it's tricky because we don't yet have the generated code that we'll get from the protocol.json changes. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. copyright should be 2015 https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:8: #include "content/browser/devtools/protocol/security_handler.h" This should be at the top of the #include list. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:43: return Response::FallThrough(); I think this should be Response::OK() like Enable() above. When I was playing around it seemed that Response::FallThrough() would pass through and try to find a renderer handler for the command, which we don't have in this case. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:50: return "UNKNOWN"; It looks like constants for these enum values will get generated (see e.g. https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). So maybe put a TODO here to come back and use the constants once the protocol.json changes have been rolled in? https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:59: default: Same comment here that I put in the other CL; I prefer to leave off the default cause and end with 'NOTREACHED(); return "UNKNOWN";' so that it'll fail to compile if a SecurityStyle value isn't handled in this switch statement. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. copyright needs to be 2015 https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:15: class ColorPicker; not needed https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:34: std::string SecurityStyleToProtocolSecurityState( This should go before the data members. (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... content/browser/devtools/render_frame_devtools_agent_host.h:103: void SecurityStyleChanged(content::SecurityStyle security_style) override; #include SecurityStyle in this file
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Mostly nits with one important comment that would make you implement it all a bit differently. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:59: default: @estark: it is a good idea here, but don't do that when switching over the generated enum literals. Otherwise two-sided patches become impossible. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... content/browser/devtools/render_frame_devtools_agent_host.h:40: namespace security { maintain consistent code style? https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... content/browser/devtools/render_frame_devtools_agent_host.h:103: void SecurityStyleChanged(content::SecurityStyle security_style) override; How is this going to be called? RFDTAH is created lazily and security subsystem would not know when to start notifying it. You should instead reverse this dependency and add a stylechange listener upon SecurityHandler::enable.
https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... content/browser/devtools/render_frame_devtools_agent_host.h:103: void SecurityStyleChanged(content::SecurityStyle security_style) override; On 2015/06/04 09:28:35, pfeldman wrote: > How is this going to be called? RFDTAH is created lazily and security subsystem > would not know when to start notifying it. You should instead reverse this > dependency and add a stylechange listener upon SecurityHandler::enable. This is a WebContentsObserver method that gets called whenever the lock icon on the page changes. (crrev.com/1162663004 is where the WebContentsObserver method got added) I don't see why the lazy initialization of RFDTAH is a problem: it looks like it's initialized when the dev tools window is toggled, at which point we'll start getting these WebContentsObserver events. We've been following e.g. PageHandler::DidAttachInterstitialPage as an example. Why is this different?
That's almost what I was suggesting. But you want to register WCO in the security handler itself upon enable (unregister upon disable). You don't want security aspects to leak from SecurityHandler to RFDAH, you want your handler to be self contained.
On 2015/06/04 15:40:37, pfeldman wrote: > That's almost what I was suggesting. But you want to register WCO in the > security handler itself upon enable (unregister upon disable). You don't want > security aspects to leak from SecurityHandler to RFDAH, you want your handler to > be self contained. Ah, ok, I think I'm starting to understand... so is the following roughly what you are suggesting? * SecurityHandler should itself be a WebContentsObserver and override the SecurityStyleChanged method. RFDTAH does not need to override the WebContentsObserver::SecurityStyleChanged method. * When SecurityHandler receives an Enable command, it calls Observe(web_contents) to start observing (and similarly for Disable). * RFDTAH is still in charge of instantiating the SecurityHandler (just like it instantiates the InputHandler, EmulationHandler, etc.) and owning it, but RDFTAH does not need to know about the actual events that SecurityHandler listens for. Is that correct?
> Is that correct? Exactly! // At some point, handlers will be added externally and derive from a base interface RFDTAH is aware. But it does not need to know about Security part of it.
On 2015/06/04 16:03:33, pfeldman wrote: > > Is that correct? > > Exactly! > > // At some point, handlers will be added externally and derive from a base > interface RFDTAH is aware. But it does not need to know about Security part of > it. Great, makes sense. Thanks!
https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:32: client_->SecurityStateChanged( Ah, just realized this won't build without the protocol.json patch, right? So I guess we need to take this bit out for now (probably with a TODO in its place), then add it back in once the protocol.json changes are rolled in. (And at that point it'll probably be a good time to write a test too, since there's not much to test once we take this bit out.)
I agree with pretty much everything so far. I've fixed the nits locally; I'm working on making the SecurityHandler an observer itself, as per the latest comments. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/06/04 at 01:09:15, estark wrote: > copyright should be 2015 Done. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:43: return Response::FallThrough(); On 2015/06/04 at 01:09:14, estark wrote: > I think this should be Response::OK() like Enable() above. When I was playing around it seemed that Response::FallThrough() would pass through and try to find a renderer handler for the command, which we don't have in this case. Makes sense (I trust you on verifying that's what it does). Done. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:50: return "UNKNOWN"; On 2015/06/04 at 01:09:15, estark wrote: > It looks like constants for these enum values will get generated (see e.g. https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...). So maybe put a TODO here to come back and use the constants once the protocol.json changes have been rolled in? Done. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:59: default: On 2015/06/04 at 09:28:35, pfeldman wrote: > @estark: it is a good idea here, but don't do that when switching over the generated enum literals. Otherwise two-sided patches become impossible. Good point. I'll leave it this way for now. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/06/04 at 01:09:15, estark wrote: > copyright needs to be 2015 Good catch. Done. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:15: class ColorPicker; On 2015/06/04 at 01:09:15, estark wrote: > not needed Yes. :-) (Done.) https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.h:34: std::string SecurityStyleToProtocolSecurityState( On 2015/06/04 at 01:09:15, estark wrote: > This should go before the data members. (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) Done. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/re... content/browser/devtools/render_frame_devtools_agent_host.h:40: namespace security { On 2015/06/04 at 09:28:35, pfeldman wrote: > maintain consistent code style? I thought I had fixed this. :-( Oh well. Done.
I've uploaded a new patch with the suggested change. However, there is a problem: the new patch fails to fire the event sometimes. I have yet to understand when or why, but I'll look at it next week. https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... content/browser/devtools/protocol/security_handler.cc:32: client_->SecurityStateChanged( On 2015/06/05 at 00:59:10, estark wrote: > Ah, just realized this won't build without the protocol.json patch, right? So I guess we need to take this bit out for now (probably with a TODO in its place), then add it back in once the protocol.json changes are rolled in. (And at that point it'll probably be a good time to write a test too, since there's not much to test once we take this bit out.) I think it would make more sense to land protocol.json before this CL, then.
On 2015/06/06 00:57:41, lgarron wrote: > I've uploaded a new patch with the suggested change. However, there is a > problem: the new patch fails to fire the event sometimes. I have yet to > understand when or why, but I'll look at it next week. > > https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... > File content/browser/devtools/protocol/security_handler.cc (right): > > https://codereview.chromium.org/1163963002/diff/1/content/browser/devtools/pr... > content/browser/devtools/protocol/security_handler.cc:32: > client_->SecurityStateChanged( > On 2015/06/05 at 00:59:10, estark wrote: > > Ah, just realized this won't build without the protocol.json patch, right? So > I guess we need to take this bit out for now (probably with a TODO in its > place), then add it back in once the protocol.json changes are rolled in. (And > at that point it'll probably be a good time to write a test too, since there's > not much to test once we take this bit out.) > > I think it would make more sense to land protocol.json before this CL, then. I don't think that'll work? The protocol.json change generates code that expects a SecurityHandler class to exist. (a la https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont...)
https://codereview.chromium.org/1163963002/diff/20001/chrome/browser/ssl/conn... File chrome/browser/ssl/connection_security_helper.h (right): https://codereview.chromium.org/1163963002/diff/20001/chrome/browser/ssl/conn... chrome/browser/ssl/connection_security_helper.h:64: static content::SecurityStyle GetSecurityStyleForWebContents( Is this diff supposed to be here? It looks like this change is from the previous CL.
Ok, I just played around with this a little bit to try to see what's going on. I
think I've led you slightly astray in two ways:
a. I think the construct-SecurityHandler-in-OnClientAttached approach will not
quite work, because dispatcher->Set*Handler() is meant to be called only once.
Instead I guess we should construct the SecurityHandler when all the other
Handlers are constructed in the RDFTAH constructor, and give the SecurityHandler
a RenderFrameHost* data member that gets updated via
RFDTAH::ConnectWebContents(). This is a pattern that seems to be followed by
other handlers such as PageHandler. Here's a small patch to show you what I
mean: crrev.com/1151133005 You can apply it and play around with it; it probably
needs a sanity-check and a little clean-up.
b. I just realized that interleaving this patch with the protocol.json patch is
going to be quite a bit more complicated than I thought. As is, this CL uses
quite a bit of generated code that doesn't exist yet (such as
DevToolsProtocolDispatcher::SetSecurityHandler()). As far as I can see, the way
we'll have to do this is to split up this CL into two different CLs, and commit
the various CLs in this order:
CL 1: Add security_handler.h and security_handler.cc.
SecurityHandler::SecurityStyleChanged should be a stub with a TODO to call
client_->SecurityStateChanged(), and TODOs for any other bits of SecurityHandler
that require generated code.
CL 2: protocol.json patch
CL 3: Fix the TODOs in SecurityHandler and add the code to
RenderFrameDevToolsAgentHost to instantiate the SecurityHandler.
Unfortunately I can't think of any simpler way to do it. :(
With the above patch that I linked, I seem to get SecurityStyleChanged() firing
as expected. If you still have problems with it not working sometimes, maybe try
fetching and rebasing -- RenderFrameDevToolsAgentHost seems to have changed a
bit recently.
Correct, that is the only way until the repos merge: 1) stub handlers 2) protocol.json and potential fron-end code 3) handler bodies
Patchset #2 (id:20001) has been deleted
On 2015/06/08 at 22:30:03, lgarron wrote: > See the latest patch. Corresponding to Pavel's numbering, we now have: 1) https://codereview.chromium.org/1165293005 2) https://codereview.chromium.org/1167693010 3) This CL. I've tested each of the stages, and I'm now ready to land all of them (or respond to additional comments ;-).
https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:9: #include "base/logging.h" not needed https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:58: // TODO: Replace these values with literals once the Security domain has You can do this TODO now, right? Since you're planning to land this after protocol.json https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (left): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:296: page_handler_(nullptr), Why did this get deleted? Mistake? https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.h:14: #include "content/browser/devtools/protocol/security_handler.h" You can forward-declare like the other *Handler classes, and then do the #include in the .cc file.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:9: #include "base/logging.h" On 2015/06/09 at 05:18:54, estark wrote: > not needed Done. (This got lost in a re-rebase. Thanks for catching.) https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:58: // TODO: Replace these values with literals once the Security domain has On 2015/06/09 at 05:18:54, estark wrote: > You can do this TODO now, right? Since you're planning to land this after protocol.json Done. https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (left): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:296: page_handler_(nullptr), On 2015/06/09 at 05:18:55, estark wrote: > Why did this get deleted? Mistake? This was deleted in an intervening ToT commit, which causes a spurious crrev diff. https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/1163963002/diff/40001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.h:14: #include "content/browser/devtools/protocol/security_handler.h" On 2015/06/09 at 05:18:55, estark wrote: > You can forward-declare like the other *Handler classes, and then do the #include in the .cc file. Done.
https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.cc:60: return "unknown"; estark@: Since this value can theoretically be passed in, what do you think of adding "unknown" to protocol.json?
https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:18: class SecurityHandler : private WebContentsObserver { Please rebaseline it on top of the stub.
https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/80001/content/browser/devtool... content/browser/devtools/protocol/security_handler.h:18: class SecurityHandler : private WebContentsObserver { On 2015/06/09 at 18:36:44, pfeldman wrote: > Please rebaseline it on top of the stub. The latest patch is already rebased onto the #include "content/public/common/security_style.h" change. Anything else?
> The latest patch is already rebased onto the #include > "content/public/common/security_style.h" change. Anything else? instead of git cl upload, do git cl upload HEAD^ so that this change had the delta wrt previous one.
On 2015/06/09 at 19:05:19, pfeldman wrote: > > The latest patch is already rebased onto the #include > > "content/public/common/security_style.h" change. Anything else? > > instead of git cl upload, do git cl upload HEAD^ so that this change had the delta wrt previous one. Argh, elementary mistake. :-( Done, although I'm still looking at the SetClient() issue.
Rebased. Also used kSecurityStateUnknown based on the latest protocol.json patch (https://codereview.chromium.org/1167693010).
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
Message was sent while issue was closed.
https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:25: void SecurityHandler::SetRenderFrameHost(RenderFrameHost* host) { swap these two definitions (SetRFH and SecurityStyleChanged) to match the order in the .h https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:68: NOTREACHED(); I think builds on some platforms will fail without a return value here. https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:10: #include "content/public/browser/web_contents.h" not needed, I think
Message was sent while issue was closed.
On 2015/06/09 21:51:45, estark wrote: > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > File content/browser/devtools/protocol/security_handler.cc (right): > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > content/browser/devtools/protocol/security_handler.cc:25: void > SecurityHandler::SetRenderFrameHost(RenderFrameHost* host) { > swap these two definitions (SetRFH and SecurityStyleChanged) to match the order > in the .h > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > content/browser/devtools/protocol/security_handler.cc:68: NOTREACHED(); > I think builds on some platforms will fail without a return value here. > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > File content/browser/devtools/protocol/security_handler.h (right): > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > content/browser/devtools/protocol/security_handler.h:10: #include > "content/public/browser/web_contents.h" > not needed, I think Wait, why is this closed? I'm confused.
On 2015/06/09 at 21:52:16, estark wrote: > On 2015/06/09 21:51:45, estark wrote: > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > > File content/browser/devtools/protocol/security_handler.cc (right): > > > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > > content/browser/devtools/protocol/security_handler.cc:25: void > > SecurityHandler::SetRenderFrameHost(RenderFrameHost* host) { > > swap these two definitions (SetRFH and SecurityStyleChanged) to match the order > > in the .h > > > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > > content/browser/devtools/protocol/security_handler.cc:68: NOTREACHED(); > > I think builds on some platforms will fail without a return value here. > > > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > > File content/browser/devtools/protocol/security_handler.h (right): > > > > https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... > > content/browser/devtools/protocol/security_handler.h:10: #include > > "content/public/browser/web_contents.h" > > not needed, I think > > Wait, why is this closed? I'm confused. I don't know why. I've re-opened.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:25: void SecurityHandler::SetRenderFrameHost(RenderFrameHost* host) { On 2015/06/09 at 21:51:45, estark wrote: > swap these two definitions (SetRFH and SecurityStyleChanged) to match the order in the .h I've swapped the declarations in the .h (which keeps the two Set* methods together). https://codereview.chromium.org/1163963002/diff/140001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:68: NOTREACHED(); On 2015/06/09 at 21:51:45, estark wrote: > I think builds on some platforms will fail without a return value here. Done (although I admit, I kind of want to see some buildbots fail on this).
Patchset #9 (id:220001) has been deleted
pfeldman@: Ping. I don't think there are any more expected changes from me here.
https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:17: SecurityHandler::SecurityHandler() : enabled_(false), host_(nullptr) { http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I... https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:34: if (!enabled_) DCHECK(enabled_); https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:56: std::string SecurityHandler::SecurityStyleToProtocolSecurityState( Utility function could be defined in a local static method above or in an anonymous namespace above. https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:17: class SecurityHandler : private WebContentsObserver { When using inheritance, make it public: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:26: void SecurityStyleChanged(SecurityStyle security_style) override; Blank line, then // WebContentsObserver overrides, then this line. Also you can move it into private section.
https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.cc (right): https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:17: SecurityHandler::SecurityHandler() : enabled_(false), host_(nullptr) { On 2015/06/11 at 06:45:50, pfeldman wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I... This matches the "all on one line" case from the style guide, and `git cl format` is happy with it. Other handlers are inconsistent about when to break lines, and where to place the brace. I'm assuming you are asking me to put this on multiple lines anyhow; I've gone ahead and done so. https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:34: if (!enabled_) On 2015/06/11 at 06:45:50, pfeldman wrote: > DCHECK(enabled_); Done. https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.cc:56: std::string SecurityHandler::SecurityStyleToProtocolSecurityState( On 2015/06/11 at 06:45:50, pfeldman wrote: > Utility function could be defined in a local static method above or in an anonymous namespace above. Done. https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... File content/browser/devtools/protocol/security_handler.h (right): https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:17: class SecurityHandler : private WebContentsObserver { On 2015/06/11 at 06:45:50, pfeldman wrote: > When using inheritance, make it public: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance Done. https://codereview.chromium.org/1163963002/diff/200001/content/browser/devtoo... content/browser/devtools/protocol/security_handler.h:26: void SecurityStyleChanged(SecurityStyle security_style) override; On 2015/06/11 at 06:45:50, pfeldman wrote: > Blank line, then // WebContentsObserver overrides, then this line. Also you can move it into private section. Done.
lgtm % comment https://codereview.chromium.org/1163963002/diff/240001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/1163963002/diff/240001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:299: security_handler_(new devtools::security::SecurityHandler()), Initialize it on line 325/326 for top level frames only for now.
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/1163963002/#ps260001 (title: "Only attach SecurityHandler to top-level frames.")
https://codereview.chromium.org/1163963002/diff/240001/content/browser/devtoo... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/1163963002/diff/240001/content/browser/devtoo... content/browser/devtools/render_frame_devtools_agent_host.cc:299: security_handler_(new devtools::security::SecurityHandler()), On 2015/06/12 at 17:46:02, pfeldman wrote: > Initialize it on line 325/326 for top level frames only for now. Ooh, cool. Done. :-)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163963002/260001
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/109a12898b9a46a620b67cde853e5ef431fa2799 Cr-Commit-Position: refs/heads/master@{#334223} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
