Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162663004:
Introduce browser-side plumbing 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, creis+watch_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce browser-side plumbing for sending the lock icon status to
DevTools.
Based on the plumbing doc at [1]. estark@ co-authored much of this
commit through pair programming.
This CL adds a SecurityStyleChanged() method that is fired when the lock
icon changes. This method will be implemented in
RenderFrameDevToolsAgentHost in a follow-up CL, which will pass it on to
DevTools.
[1] https://docs.google.com/document/d/1kMU5dPi2PwQ7skbgwlfQsenYdTwOi2ya2WYla2uy0OU/edit
BUG=445359, 484395
Committed: https://crrev.com/662dd52cdfea198f6a145f9da5a7c8b0c8133643
Cr-Commit-Position: refs/heads/master@{#333381}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add a browser test for SecurityStyleChanged. #Patch Set 3 : Style changes. #Patch Set 4 : Rebased onto latest master. #
Total comments: 20
Patch Set 5 : Address reviewer comments. #Patch Set 6 : const #
Total comments: 8
Patch Set 7 : unconst #Patch Set 8 : Nits. #
Total comments: 4
Patch Set 9 : #Messages
Total messages: 28 (6 generated)
estark@chromium.org changed reviewers: + estark@chromium.org
Great! Just left a few style comments inline. A couple other suggestions: * wrap CL description to 80 chars * I'd add a very high-level summary sentence at the beginning of the CL description, something like: "Add a WebContentsObserver method to notify observers of the high-level security status of the web contents, which will later be observed by devtools." * I think we should probably be able to figure out a way to test this change. For example, we could add a browser test (maybe in //chrome/browser/ui/browser_browsertest.cc) that sets up a test WebContentsObserver and observes the proper security style fired when visiting pages with various different SSL properties. We can pair on this tomorrow afternoon if you'd like. https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ssl/connecti... File chrome/browser/ssl/connection_security_helper.cc (right): https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ssl/connecti... chrome/browser/ssl/connection_security_helper.cc:140: return content::SECURITY_STYLE_AUTHENTICATED; You can collapse this like so: case EV_SECURE: case SECURE: return content::SECURITY_STYLE_AUTHENTICATED; https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ssl/connecti... chrome/browser/ssl/connection_security_helper.cc:143: case SECURITY_WARNING: same here (can collapse into one case) https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ssl/connecti... chrome/browser/ssl/connection_security_helper.cc:149: default: You could leave off the default case here, and add 'NOTREACHED(); return SECURITY_STYLE_UNKNOWN;' after the switch statement. That way if someone adds a new SecurityLevel value and forgets to update this switch statement, this code will fail to compile. https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1162663004/diff/1/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:309: content::SecurityStyle GetSecurityStyle( This declaration should go in the section starting at line 447, with the other WebContentsDelegate methods. (All overrides from the same class should go in the same section.)
Patchset #3 (id:40001) has been deleted
lgarron@chromium.org changed reviewers: + creis@chromium.org, pkasting@chromium.org, rsleevi@chromium.org
Hi Charlie, Ryan, and Peter,
This is the first of a set of CLs to plumb Chrome's lock icon state to DevTools,
based on past discussions of the design doc. Would you mind reviewing the
following parts of this CL?
rsleevi@: //chrome/browser/ssl (2 files)
pkasting@: //chrome/browser/ui (3 files)
creis@: //content (5 files)
Although it should be easy to figure out from reading the CL, here is the full
flow:
- WebContentsImpl::DidChangeVisibleSSLState (an existing method) fires.
- In order to get the current SecurityStyle, the WebContentsImpl calls
GetSecurityStyle() on its WebContentsDelegate, which reaches the
implementation in //chrome/browser/ui/browser.cc.
- browser.cc calls GetSecurityStyleForWebContents(), which
translates GetSecurityLevelForWebContents() into a SecurityStyle.
- The WebContentsImpl calls SecurityStyleChanged on all its observers.
- SecurityStyleChanged() is currently a virtual WebContentsObserver
stub. In a separate CL, RenderFrameDevToolsAgentHost will
implement SecurityStyleChanged and pass it on to DevTools.
A few design decisions:
- WebContentsImpl calls GetSecurityStyle() on a delegate so that the
calculation can be done at the //chrome/browser layer.
- We translate the SecurityLevel to a SecurityStyle so that the rest of
the code (in //content) can send it to DevTools.
- In order to allow the SecurityLevel to be translated into a
SecurityStyle that represents a lock icon without adding a new enum,
we add the value SECURITY_STYLE_WARNING to the SecurityStyle enum.
Thanks,
»Lucas
I'm OK with this. You don't need my LGTM, but LGTM from the perspective of //content & //chrome and their interactions. I didn't review any of the notification design & communication https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... File chrome/browser/ssl/connection_security_helper.cc (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... chrome/browser/ssl/connection_security_helper.cc:148: NOTREACHED(); nit: tend to prefer a newline between 147 & 148 for visual separation https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... File chrome/browser/ssl/connection_security_helper.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... chrome/browser/ssl/connection_security_helper.h:58: // cases, you should use GetSecurityLevelForWebContents() directly instead. nit: "pronouns in comments considered harmful" (see discussion at https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... ) This comment can be reworded to be more explicit and direct. Also, from a Style Guide, this runs somewhat afoul of the "Don't say how" rule from http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... // Returns the content::SecurityStyle for the given |web_contents|. // Note: This is a lossy operation; that is, not all of the policies // that can be expressed by a SecurityLevel (a //chrome concept) can // be expressed by a content::SecurityStyle. // In general, code in //chrome should prefer to use // GetSecurityLevelForWebContents() to determine security policy, and // only use this function when policy needs to be supplied back to // layers in //content. The reword has several goals: 1) For phrases like "in most cases" and "in general", it helps to enumerate some of the exceptions, or if there are only a few, all of them. I tried to do that with the "and only use" 2) The "you" is ambiguous (see above pronouns), and so I tried to make it clear the subject ("code in //chrome") being discussed 3) Don't describe how ("from GetSecurityLevelForWebContents()"), describe the what. The remaining part "translated to a SecurityStyle so that lower layers can use it" is sort of a borderline comment. It's somewhat good (in the "how to use" sense from the style guide), but it borderlines on describing "how it is used" (which is bad, because code changes, and comments aren't always kept in sync; further, they can sometimes be layering violation if lower code describes how higher code uses it) I'm not saying you have to take my wording - I no English good - but wanted to describe the broader issues w/ the comment and what I was trying to accomplish with the reword. https://codereview.chromium.org/1162663004/diff/80001/content/public/common/s... File content/public/common/security_style.h (right): https://codereview.chromium.org/1162663004/diff/80001/content/public/common/s... content/public/common/security_style.h:33: // with the retrieval or the object interacted with less secure objects. comment nit: I'm not sure what "higher-level state" is supposed to mean here? If you drop "is a higher-level state that" from the comment, I think it's fine and would resolve my unease at a possible layering issue in the comment. (The reason I don't think this concept is itself a layering issue would because Mike's current mixedcontent spec details "deprecated TLS-protection" to cover this phase)
LGTM https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:465: content::SecurityStyle GetSecurityStyle( Nit: GetSecurityStyleForWebContents()? Otherwise it sounds like it fetches the window's current style or something. Can this be const? (I'd love for it to be static, but unfortunately you can't have static virtual methods.) https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:330: static bool GetFilePathWithHostAndPortReplacement( Nit: This doesn't need "static" as it's already in an anonymous namespace. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:341: // A WebContentsObserver useful for testing the |SecurityStyleChanged| Nit: || go around data members, not function names. (Use () on function names.) https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:354: content::SecurityStyle latest_security_style() { Nit: const https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2815: // with the current style on HTTPS, broken HTTPS, and valid HTTPS pages. Nit: First HTTPS should maybe be HTTP? https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2817: // Create an HTTPS server for testing a valid security style. Nit: Seems like this block does more than just this, so either add more comments, remove this comment, or split the block into separate blocks.
https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... File chrome/browser/ssl/connection_security_helper.cc (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... chrome/browser/ssl/connection_security_helper.cc:148: NOTREACHED(); On 2015/06/03 at 23:43:52, Ryan Sleevi wrote: > nit: tend to prefer a newline between 147 & 148 for visual separation Done. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... File chrome/browser/ssl/connection_security_helper.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ssl/conn... chrome/browser/ssl/connection_security_helper.h:58: // cases, you should use GetSecurityLevelForWebContents() directly instead. On 2015/06/03 at 23:43:52, Ryan Sleevi wrote: > nit: "pronouns in comments considered harmful" (see discussion at https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... ) > > This comment can be reworded to be more explicit and direct. > > Also, from a Style Guide, this runs somewhat afoul of the "Don't say how" rule from http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > > // Returns the content::SecurityStyle for the given |web_contents|. > // Note: This is a lossy operation; that is, not all of the policies > // that can be expressed by a SecurityLevel (a //chrome concept) can > // be expressed by a content::SecurityStyle. > // In general, code in //chrome should prefer to use > // GetSecurityLevelForWebContents() to determine security policy, and > // only use this function when policy needs to be supplied back to > // layers in //content. > > > The reword has several goals: > 1) For phrases like "in most cases" and "in general", it helps to enumerate some of the exceptions, or if there are only a few, all of them. I tried to do that with the "and only use" > > 2) The "you" is ambiguous (see above pronouns), and so I tried to make it clear the subject ("code in //chrome") being discussed > > 3) Don't describe how ("from GetSecurityLevelForWebContents()"), describe the what. > > The remaining part "translated to a SecurityStyle so that lower layers can use it" is sort of a borderline comment. It's somewhat good (in the "how to use" sense from the style guide), but it borderlines on describing "how it is used" (which is bad, because code changes, and comments aren't always kept in sync; further, they can sometimes be layering violation if lower code describes how higher code uses it) > > I'm not saying you have to take my wording - I no English good - but wanted to describe the broader issues w/ the comment and what I was trying to accomplish with the reword. I admit that I don't have practice writing this kind of comment; thanks for the details (and reference). I think your version is pretty good, so I've taken it pretty much verbatim in the latest patch. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:465: content::SecurityStyle GetSecurityStyle( On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: GetSecurityStyleForWebContents()? Otherwise it sounds like it fetches the window's current style or something. > > Can this be const? (I'd love for it to be static, but unfortunately you can't have static virtual methods.) "error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]" Apparently not. :-/ https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:330: static bool GetFilePathWithHostAndPortReplacement( On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: This doesn't need "static" as it's already in an anonymous namespace. Done. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:341: // A WebContentsObserver useful for testing the |SecurityStyleChanged| On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: || go around data members, not function names. (Use () on function names.) Done. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:354: content::SecurityStyle latest_security_style() { On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: const Done. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2815: // with the current style on HTTPS, broken HTTPS, and valid HTTPS pages. On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: First HTTPS should maybe be HTTP? Done. https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:2817: // Create an HTTPS server for testing a valid security style. On 2015/06/03 at 23:59:34, Peter Kasting wrote: > Nit: Seems like this block does more than just this, so either add more comments, remove this comment, or split the block into separate blocks. I've removed the comment. https://codereview.chromium.org/1162663004/diff/80001/content/public/common/s... File content/public/common/security_style.h (right): https://codereview.chromium.org/1162663004/diff/80001/content/public/common/s... content/public/common/security_style.h:33: // with the retrieval or the object interacted with less secure objects. On 2015/06/03 at 23:43:52, Ryan Sleevi wrote: > comment nit: I'm not sure what "higher-level state" is supposed to mean here? > > If you drop "is a higher-level state that" from the comment, I think it's fine and would resolve my unease at a possible layering issue in the comment. > > (The reason I don't think this concept is itself a layering issue would because Mike's current mixedcontent spec details "deprecated TLS-protection" to cover this phase) Done. (Thanks for the "deprecated TLS-protection" terminology.)
https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:465: content::SecurityStyle GetSecurityStyle( On 2015/06/04 00:26:34, lgarron wrote: > On 2015/06/03 at 23:59:34, Peter Kasting wrote: > > Nit: GetSecurityStyleForWebContents()? Otherwise it sounds like it fetches > the window's current style or something. > > > > Can this be const? (I'd love for it to be static, but unfortunately you can't > have static virtual methods.) > > "error: 'const' type qualifier on return type has no effect > [-Werror,-Wignored-qualifiers]" > Apparently not. :-/ You const-qualified the wrong thing :). I was suggesting const-qualifying the function, not the return type.
content/ LGTM with nits. https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle GetSecurityStyle(const WebContents* web_contents) const; I know pkasting asked for this const, but it's against the content public API guidelines: http://www.chromium.org/developers/content-module/content-api "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed." https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:160: // This method is invoked when the security style of the WebContents changes. nit: s/security style/SecurityStyle/ (The term isn't immediately obvious, but using the class name will direct people where to look.) https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:161: virtual void SecurityStyleChanged(content::SecurityStyle security_style) {} I'd suggest moving this between DidNavigateAnyFrame and DocumentAvailableInMainFrame, since Start+Commit+Fail are closely related.
https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle GetSecurityStyle(const WebContents* web_contents) const; On 2015/06/04 18:49:16, Charlie Reis wrote: > I know pkasting asked for this const, but it's against the content public API > guidelines: > > http://www.chromium.org/developers/content-module/content-api > > "don't add the const identifier to interfaces. For interfaces implemented by the > embedder, we can't make assumptions about what the embedder needs to implement > it. For interfaces implemented by content, the implementation details doesn't > have to be exposed." I think we should be optimizing for the needs of our own embedder implementations and then removing problematic consts if something makes us aware of problems. I think this directive is a mistake. More importantly, what you describe is inconsistent with the rest of this file. There are already several other functions (I count at least seven) in this file alone marked "const". That seems like strong evidence that in this file at least, we're OK with this and it's not causing embedders problems.
creis@chromium.org changed reviewers: + jam@chromium.org
[+jam for content API discussion] https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle GetSecurityStyle(const WebContents* web_contents) const; On 2015/06/04 20:30:20, Peter Kasting wrote: > On 2015/06/04 18:49:16, Charlie Reis wrote: > > I know pkasting asked for this const, but it's against the content public API > > guidelines: > > > > http://www.chromium.org/developers/content-module/content-api > > > > "don't add the const identifier to interfaces. For interfaces implemented by > the > > embedder, we can't make assumptions about what the embedder needs to implement > > it. For interfaces implemented by content, the implementation details doesn't > > have to be exposed." > > I think we should be optimizing for the needs of our own embedder > implementations and then removing problematic consts if something makes us aware > of problems. I think this directive is a mistake. I've added John if you want to discuss changing the policy, but that's what we've been enforcing when we see it. > More importantly, what you describe is inconsistent with the rest of this file. > There are already several other functions (I count at least seven) in this file > alone marked "const". That seems like strong evidence that in this file at > least, we're OK with this and it's not causing embedders problems. John has also been clear in the past that the existence of policy violations shouldn't justify more exceptions (but are rather reasons to fix the old ones).
On 2015/06/04 20:41:50, Charlie Reis wrote: > [+jam for content API discussion] > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > File content/public/browser/web_contents_delegate.h (right): > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle > GetSecurityStyle(const WebContents* web_contents) const; > On 2015/06/04 20:30:20, Peter Kasting wrote: > > On 2015/06/04 18:49:16, Charlie Reis wrote: > > > I know pkasting asked for this const, but it's against the content public > API > > > guidelines: > > > > > > http://www.chromium.org/developers/content-module/content-api > > > > > > "don't add the const identifier to interfaces. For interfaces implemented by > > the > > > embedder, we can't make assumptions about what the embedder needs to > implement > > > it. For interfaces implemented by content, the implementation details > doesn't > > > have to be exposed." > > > > I think we should be optimizing for the needs of our own embedder > > implementations and then removing problematic consts if something makes us > aware > > of problems. I think this directive is a mistake. > > I've added John if you want to discuss changing the policy, but that's what > we've been enforcing when we see it. > > > > More importantly, what you describe is inconsistent with the rest of this > file. > > There are already several other functions (I count at least seven) in this > file > > alone marked "const". That seems like strong evidence that in this file at > > least, we're OK with this and it's not causing embedders problems. > > John has also been clear in the past that the existence of policy violations > shouldn't justify more exceptions (but are rather reasons to fix the old ones). nothing new in these comments :) Having content assume that the implementation of any of its embedder methods must be const is a layering violation.
On 2015/06/04 21:38:25, jam wrote: > On 2015/06/04 20:41:50, Charlie Reis wrote: > > [+jam for content API discussion] > > > > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > > File content/public/browser/web_contents_delegate.h (right): > > > > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > > content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle > > GetSecurityStyle(const WebContents* web_contents) const; > > On 2015/06/04 20:30:20, Peter Kasting wrote: > > > On 2015/06/04 18:49:16, Charlie Reis wrote: > > > > I know pkasting asked for this const, but it's against the content public > > API > > > > guidelines: > > > > > > > > http://www.chromium.org/developers/content-module/content-api > > > > > > > > "don't add the const identifier to interfaces. For interfaces implemented > by > > > the > > > > embedder, we can't make assumptions about what the embedder needs to > > implement > > > > it. For interfaces implemented by content, the implementation details > > doesn't > > > > have to be exposed." > > > > > > I think we should be optimizing for the needs of our own embedder > > > implementations and then removing problematic consts if something makes us > > aware > > > of problems. I think this directive is a mistake. > > > > I've added John if you want to discuss changing the policy, but that's what > > we've been enforcing when we see it. > > > > > > > More importantly, what you describe is inconsistent with the rest of this > > file. > > > There are already several other functions (I count at least seven) in this > > file > > > alone marked "const". That seems like strong evidence that in this file at > > > least, we're OK with this and it's not causing embedders problems. > > > > John has also been clear in the past that the existence of policy violations > > shouldn't justify more exceptions (but are rather reasons to fix the old > ones). > > nothing new in these comments :) > > Having content assume that the implementation of any of its embedder methods > must be const is a layering violation. That didn't address my concerns, you just reiterated the other document. Note that _not_ adding const is just as much of an assumption as adding const: it explicitly says the method is not const and blocks its use by anyone holding a pointer-to-const-object. Const here should be applied from the perspective of what's logically correct for the API state. If a method is const, that means the embedder must not change any state visible (directly or transitively) through this API. That's a reasonable restriction to have even without knowing the embedder implementation details as it governs how different API calls can interact with each other. Embedders that need to implement this logical const restriction in a way that's not physically const can -- and should -- use mutable. In other words, you can comply with the Effective C++ recommendations here without actually imposing a layering violation. I think the guidelines here that assume this _is_ a violation are confusing physical and logical constness. (That said, I also still don't see why we shouldn't err on the side of allowing const until an embedder has a problem with that. We're building the content API for ourselves, so let's meet our own embedders' needs.)
https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1162663004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:465: content::SecurityStyle GetSecurityStyle( On 2015/06/04 at 00:29:15, Peter Kasting wrote: > On 2015/06/04 00:26:34, lgarron wrote: > > On 2015/06/03 at 23:59:34, Peter Kasting wrote: > > > Nit: GetSecurityStyleForWebContents()? Otherwise it sounds like it fetches > > the window's current style or something. > > > > > > Can this be const? (I'd love for it to be static, but unfortunately you can't > > have static virtual methods.) > > > > "error: 'const' type qualifier on return type has no effect > > [-Werror,-Wignored-qualifiers]" > > Apparently not. :-/ > > You const-qualified the wrong thing :). I was suggesting const-qualifying the function, not the return type. *shakes fist at C++* (Done. :-) https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle GetSecurityStyle(const WebContents* web_contents) const; On 2015/06/04 at 20:41:50, Charlie Reis wrote: > On 2015/06/04 20:30:20, Peter Kasting wrote: > > On 2015/06/04 18:49:16, Charlie Reis wrote: > > > I know pkasting asked for this const, but it's against the content public API > > > guidelines: > > > > > > http://www.chromium.org/developers/content-module/content-api > > > > > > "don't add the const identifier to interfaces. For interfaces implemented by > > the > > > embedder, we can't make assumptions about what the embedder needs to implement > > > it. For interfaces implemented by content, the implementation details doesn't > > > have to be exposed." > > > > I think we should be optimizing for the needs of our own embedder > > implementations and then removing problematic consts if something makes us aware > > of problems. I think this directive is a mistake. > > I've added John if you want to discuss changing the policy, but that's what we've been enforcing when we see it. > > > > More importantly, what you describe is inconsistent with the rest of this file. > > There are already several other functions (I count at least seven) in this file > > alone marked "const". That seems like strong evidence that in this file at > > least, we're OK with this and it's not causing embedders problems. > > John has also been clear in the past that the existence of policy violations shouldn't justify more exceptions (but are rather reasons to fix the old ones). I'm happy to go with whatever the convention is. Since creis@ is the reviewer for this part of the code, I've removed const in the latest patch for now. https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:160: // This method is invoked when the security style of the WebContents changes. On 2015/06/04 at 18:49:17, Charlie Reis wrote: > nit: s/security style/SecurityStyle/ > > (The term isn't immediately obvious, but using the class name will direct people where to look.) Done. (Also changed web_contents_delegate.h:490 to refer to a specific web_contents.) However, this kind of implies that a WebContents "has" a SecurityStyle – but it's actually indirectly derived from the security level. I'm comfortable with that, but I'd be curious what rsleevi@ has to say. https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:161: virtual void SecurityStyleChanged(content::SecurityStyle security_style) {} On 2015/06/04 at 18:49:17, Charlie Reis wrote: > I'd suggest moving this between DidNavigateAnyFrame and DocumentAvailableInMainFrame, since Start+Commit+Fail are closely related. Done.
LGTM. I'd like for John to respond to my comments on the content API restrictions, but that's orthogonal to this patch. https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:354: const content::SecurityStyle latest_security_style() { Again, you made the wrong thing const here. The function, not the return value, should be const. https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:2814: // Test that the WebContentsObserver::SecurityStyleChanged event fires Nit: Test -> Tests (Style guide: function comments should be declarative, not imperative)
On 2015/06/04 21:51:00, Peter Kasting wrote: > On 2015/06/04 21:38:25, jam wrote: > > On 2015/06/04 20:41:50, Charlie Reis wrote: > > > [+jam for content API discussion] > > > > > > > > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > > > File content/public/browser/web_contents_delegate.h (right): > > > > > > > > > https://codereview.chromium.org/1162663004/diff/120001/content/public/browser... > > > content/public/browser/web_contents_delegate.h:491: virtual SecurityStyle > > > GetSecurityStyle(const WebContents* web_contents) const; > > > On 2015/06/04 20:30:20, Peter Kasting wrote: > > > > On 2015/06/04 18:49:16, Charlie Reis wrote: > > > > > I know pkasting asked for this const, but it's against the content > public > > > API > > > > > guidelines: > > > > > > > > > > http://www.chromium.org/developers/content-module/content-api > > > > > > > > > > "don't add the const identifier to interfaces. For interfaces > implemented > > by > > > > the > > > > > embedder, we can't make assumptions about what the embedder needs to > > > implement > > > > > it. For interfaces implemented by content, the implementation details > > > doesn't > > > > > have to be exposed." > > > > > > > > I think we should be optimizing for the needs of our own embedder > > > > implementations and then removing problematic consts if something makes us > > > aware > > > > of problems. I think this directive is a mistake. > > > > > > I've added John if you want to discuss changing the policy, but that's what > > > we've been enforcing when we see it. > > > > > > > > > > More importantly, what you describe is inconsistent with the rest of this > > > file. > > > > There are already several other functions (I count at least seven) in this > > > file > > > > alone marked "const". That seems like strong evidence that in this file > at > > > > least, we're OK with this and it's not causing embedders problems. > > > > > > John has also been clear in the past that the existence of policy violations > > > shouldn't justify more exceptions (but are rather reasons to fix the old > > ones). > > > > nothing new in these comments :) > > > > Having content assume that the implementation of any of its embedder methods > > must be const is a layering violation. > > That didn't address my concerns, you just reiterated the other document. Sorry I glanced through the comments and didn't read all the ones. > > Note that _not_ adding const is just as much of an assumption as adding const: > it explicitly says the method is not const and blocks its use by anyone holding > a pointer-to-const-object. per the content api docs, the embedder callbacks are for content to call, and not the embedder. so the scenario you give, if i'm understanding correctly, is not a use case for the api. if chrome wants to call other code in chrome, it should call it directly and not through content api embedder callbacks. this is mostly to not clutter the embedder interfaces, and also it helps avoid layering violations. > > Const here should be applied from the perspective of what's logically correct > for the API state. If a method is const, that means the embedder must not > change any state visible (directly or transitively) through this API. a method might not change state visible (I assume you mean UI releated), but might still change member variables. > That's a > reasonable restriction to have even without knowing the embedder implementation > details as it governs how different API calls can interact with each other. > Embedders that need to implement this logical const restriction in a way that's > not physically const can -- and should -- use mutable. > > In other words, you can comply with the Effective C++ recommendations here > without actually imposing a layering violation. I think the guidelines here > that assume this _is_ a violation are confusing physical and logical constness. > > (That said, I also still don't see why we shouldn't err on the side of allowing > const until an embedder has a problem with that. We're building the content API > for ourselves, so let's meet our own embedders' needs.) i agree we develop this for ourselves. but even in our repo, there are sometimes many implementors of a method. early in the api creation, we found it cumbersome to have to update all the implementations of an interface in the repo every time a method that was const now needed to be non-const for one of them.
On 2015/06/05 04:15:15, jam wrote: > On 2015/06/04 21:51:00, Peter Kasting wrote: > > Note that _not_ adding const is just as much of an assumption as adding const: > > it explicitly says the method is not const and blocks its use by anyone > holding > > a pointer-to-const-object. > > per the content api docs, the embedder callbacks are for content to call, and > not the embedder. so the scenario you give, if i'm understanding correctly, is > not a use case for the api. if chrome wants to call other code in chrome, it > should call it directly and not through content api embedder callbacks. this is > mostly to not clutter the embedder interfaces, and also it helps avoid layering > violations. That may be true, but it doesn't falsify anything I said. Saying "a non-const method cannot be called by callers who hold a pointer-to-const object" is still just as applicable in the content layer itself. In other words, this policy still has negative consequences, even if in non-content layers they could perhaps be mitigated by writing a second set of APIs and using that instead of the content APIs. > > Const here should be applied from the perspective of what's logically correct > > for the API state. If a method is const, that means the embedder must not > > change any state visible (directly or transitively) through this API. > > a method might not change state visible (I assume you mean UI releated), but > might still change member variables. That's precisely the difference between physical and logical constness. The guideline here is based on enforcing physical constness. Ideally you want to enforce logical constness; see the Effective C++ item on const for a complete description. In short, changes to internal implementation-related variables that don't affect externally visible state should not cause a method to be marked non-const, and said variables should be marked mutable. > i agree we develop this for ourselves. but even in our repo, there are sometimes > many implementors of a method. early in the api creation, we found it cumbersome > to have to update all the implementations of an interface in the repo every time > a method that was const now needed to be non-const for one of them. I'm sure that's true. Then again, it's also been very inconvenient for some of us trying to clean up const-correctness across the codebase (not in the content layer specifically) to go through and add const where it was needed to base classes and all of their implementations. It kinda cuts both ways. :( The issue I've seen with people having to add and remove "const" is usually when the initial design of an API didn't really take into account whether a method should truly be logically const from the perspective of that API, but was simply based on whether the implementation could be done in a physically const fashion for the known implementations of the time. (For example, people writing APIs that are "const" but return non-const pointers, which is a big no-no.) This doesn't mean the right answer is to globally ban const; it means it should be considered carefully by reviewers and applied if and only if that API call should really be const. I don't think you need to go and start blanket adding const to all kinds of content APIs, but I do think the current directive is overbroad. I would suggest removing it entirely and instead taking a skeptical review eye towards const methods, as well as ensuring content reviewers are familiar with the physical/logical distinction.
Lucas, I think you can go ahead and land this one after fixing Peter's final couple nits. (Unless anyone objects, of course -- I'm assuming per Peter's comment that we don't need to block on the const content discussion.) Maybe the const content discussion should move to chromium-dev? I think other people (present and future) would find it interesting, and it'll be much easier to find there.
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, creis@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1162663004/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162663004/180001
https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:354: const content::SecurityStyle latest_security_style() { On 2015/06/05 at 00:37:48, Peter Kasting wrote: > Again, you made the wrong thing const here. The function, not the return value, should be const. Done. https://codereview.chromium.org/1162663004/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:2814: // Test that the WebContentsObserver::SecurityStyleChanged event fires On 2015/06/05 at 00:37:48, Peter Kasting wrote: > Nit: Test -> Tests (Style guide: function comments should be declarative, not imperative) Done.
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/662dd52cdfea198f6a145f9da5a7c8b0c8133643 Cr-Commit-Position: refs/heads/master@{#333381}
Message was sent while issue was closed.
wait, peter isn't a content/public owner. charlie and i had commented that the const needs to be removed. this landed with it. please remove it in a followup now.
Message was sent while issue was closed.
On 2015/06/10 at 22:22:04, jam wrote: > wait, peter isn't a content/public owner. charlie and i had commented that the const needs to be removed. this landed with it. please remove it in a followup now. jam@: I'm assuming you mean GetSecurityStyle() should not take a const WebContents (in addition to not being a const function), similar to other WebContentsDelegate functions. I've created a CL: https://codereview.chromium.org/1173213002 |
