Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
441043005:
Cleanup of SSLHostStateDelegate and related code (Closed)
|
Created:
6 years, 4 months ago by jww Modified:
6 years, 3 months ago CC:
jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCleanup of SSLHostStateDelegate and related code.
This is a general cleanup of some minor issues, mostly style and
comments, for SSLHostStateDelegate and related classes and methods. This
was done originally as a C++ readability review request, originally
based on https://codereview.chromium.org/369703002/.
Committed: https://crrev.com/a18a71f89a845d9acc83ca9ddbac35b9a463633f
Cr-Commit-Position: refs/heads/master@{#293443}
Patch Set 1 #Patch Set 2 : Rebase on ToT #
Total comments: 30
Patch Set 3 : Readability changes from willchan #
Total comments: 4
Patch Set 4 : const and pass-by-ref changes #
Total comments: 5
Patch Set 5 : Rebase on ToT #
Total comments: 2
Messages
Total messages: 23 (3 generated)
Reassigned from haranp -> willchan for readability.
Overall, the changelist is quite clean, so thanks to the previous reviewers for maintaining the style guidelines. I had a few nits here and there and ranted a little bit on code coupling (no action needed, just want you to consider it a bit). I didn't finish reviewing the test because eae@ is going to get dumped on right now. I'll bbl. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:75: // non-negative value in seconds or a value of -1 indicating that decisions unnecessary extra horizontal whitespace after the -1 https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:129: base::StringPiece(reinterpret_cast<const char*>(fingerprint.data), Is this reinterpret_cast<> necessary? Shouldn't the char[32] implicitly cast into a const char*? https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:145: // to there not be any values in the dictionary). If create_entries is set to grammar is off here...perhaps s/be/being/? https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:147: // expired, a new dictionary will be created Missing '.' at the end. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:148: base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( You should directly include "base/values.h" for this rather than rely on transitive includes. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:149: base::DictionaryValue* dict, Not for readability, and don't worry, the length of this comment is not correlated to its importance. This is just for you to think about and evaluate on your own, and readability review is a good time for someone to present these ideas to you. And since readability reviewers have the perhaps unjustified perception of being reasonably experienced/knowledgeable here, a short rant will hopefully be less presumptuous/overbearing than some rando. My personal feeling here is that |dict| is confusingly used for both input and output. It's not immediately obvious if the output will be the return value or in |dict|. And the ownership of |cert_error_dict| is a little weird. The implicit function contract seems to be "I stored it nested somewhere within |dict|, and here's a raw pointer to it". But this contract isn't explicit, which means that there's tight coupling between the caller and the callee, since the caller needs to understand what the callee is doing here. Generally, separate functions should try to remain decoupled, since one of the benefits of functions (besides code reuse), is having a smaller unit of comprehension. One example of how you could get this is by returning a scoped_ptr<base::DictionaryValue> instead, and having the caller set the DictionaryValue in the right place. And if you really wanted the code reuse, you could wrap those few lines of conditional checking in another helper function. An analogy for this is tech support for my dad. Let's say I'm installing Skype for him. He gives me his computer, I install Skype, and put a shortcut to the Skype executable on the desktop. Ignoring the fact that desktop shortcuts probably tell where they point to, whereas Foo* just gives some address in memory, now my dad doesn't know where Skype is. He'd have to know where I generally put new applications. Instead, a cleaner interface would be for me to install it to the desktop, and tell him it's his job to move it into the appropriate location. This requires a bit more work on his part, but makes everything more explicit. In programming terms, I paid the cost of a little loss of code reuse (which I could write another helper function to solve, and in this analogy, this helper function would be moving the application to the default installation directory and leaving a shortcut on the desktop) in order to eliminate a side effect and have a more explicit input/output arrangement. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:27: explicit ChromeSSLHostStateDelegate(Profile* profile); Not for readability: in general you should prefer not to pass around Profile* objects. Generally speaking, you should prefer to pass finer grained dependencies, rather than huge objects like Profile which have all these other dependencies. That will enable easier unit testing, componentization (if this ever needs to moved to content/ or components/), etc. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:58: virtual bool HasUserDecision(const std::string& host); This member function reads like it should be const. Is there any reason it isn't? https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:61: void ShutdownOnUIThread() {} Nit: What's up with this? Empty function? Does anyone call this? https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:77: CreateDictionaryEntries, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...: "Enumerators should be named either like constants or like macros: either kEnumName or ENUM_NAME." Chromium's style guide says (http://www.chromium.org/developers/coding-style#Naming): "Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. Continue to use this style for consistency." Therefore, you should use ENUM_NAME. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:96: net::X509Certificate* cert, Is there a reason |cert| isn't a const reference? Do you modify it at all? I'd make the same comment in the member functions above, except those appear to be virtuals inherited from an interface out of your control here. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:132: std::set<BrokenHostEntry> ran_insecure_content_hosts_; You should #include <set>. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...: """ Always #include the file that actually provides the declarations/definitions you need; do not rely on the symbol being brought in transitively via headers not directly included. One exception is that myfile.cc may rely on #includes and forward declarations from its corresponding header file myfile.h. """ https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:14: #include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h" Goes first: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_.... The rule applies to both foo.cc and foo_test.cc: "In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test the stuff in dir2/foo2.h, order your includes as follows" https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:35: const char* kForgetAtSessionEnd = "-1"; These 3 aren't constants. They're non-const pointers to a const char[]. You should do the same as above, const char kFoo[] = ...; to turn them into real constants. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:52: // InProcessBrowserTest because the actual functionality is provided by Not for readability, but would you be able to write a smaller test if you didn't use Profile in ChromeSSLHostStateDelegate?
https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:75: // non-negative value in seconds or a value of -1 indicating that decisions On 2014/08/20 22:17:21, willchan wrote: > unnecessary extra horizontal whitespace after the -1 Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:129: base::StringPiece(reinterpret_cast<const char*>(fingerprint.data), On 2014/08/20 22:17:21, willchan wrote: > Is this reinterpret_cast<> necessary? Shouldn't the char[32] implicitly cast > into a const char*? I was surprised, too, but Clang complains about "no matching constructor" for StringPiece if I don't put in the reinterpret_cast<>. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:145: // to there not be any values in the dictionary). If create_entries is set to On 2014/08/20 22:17:21, willchan wrote: > grammar is off here...perhaps s/be/being/? Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:147: // expired, a new dictionary will be created On 2014/08/20 22:17:21, willchan wrote: > Missing '.' at the end. Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:148: base::DictionaryValue* ChromeSSLHostStateDelegate::GetValidCertDecisionsDict( On 2014/08/20 22:17:21, willchan wrote: > You should directly include "base/values.h" for this rather than rely on > transitive includes. Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:149: base::DictionaryValue* dict, On 2014/08/20 22:17:21, willchan wrote: > Not for readability, and don't worry, the length of this comment is not > correlated to its importance. This is just for you to think about and evaluate > on your own, and readability review is a good time for someone to present these > ideas to you. And since readability reviewers have the perhaps unjustified > perception of being reasonably experienced/knowledgeable here, a short rant will > hopefully be less presumptuous/overbearing than some rando. > > My personal feeling here is that |dict| is confusingly used for both input and > output. It's not immediately obvious if the output will be the return value or > in |dict|. And the ownership of |cert_error_dict| is a little weird. The > implicit function contract seems to be "I stored it nested somewhere within > |dict|, and here's a raw pointer to it". But this contract isn't explicit, which > means that there's tight coupling between the caller and the callee, since the > caller needs to understand what the callee is doing here. > > Generally, separate functions should try to remain decoupled, since one of the > benefits of functions (besides code reuse), is having a smaller unit of > comprehension. One example of how you could get this is by returning a > scoped_ptr<base::DictionaryValue> instead, and having the caller set the > DictionaryValue in the right place. And if you really wanted the code reuse, you > could wrap those few lines of conditional checking in another helper function. > > An analogy for this is tech support for my dad. Let's say I'm installing Skype > for him. He gives me his computer, I install Skype, and put a shortcut to the > Skype executable on the desktop. Ignoring the fact that desktop shortcuts > probably tell where they point to, whereas Foo* just gives some address in > memory, now my dad doesn't know where Skype is. He'd have to know where I > generally put new applications. Instead, a cleaner interface would be for me to > install it to the desktop, and tell him it's his job to move it into the > appropriate location. This requires a bit more work on his part, but makes > everything more explicit. > > In programming terms, I paid the cost of a little loss of code reuse (which I > could write another helper function to solve, and in this analogy, this helper > function would be moving the application to the default installation directory > and leaving a shortcut on the desktop) in order to eliminate a side effect and > have a more explicit input/output arrangement. It sounds like we might want to change this but in a different (non-readability) CL. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:27: explicit ChromeSSLHostStateDelegate(Profile* profile); On 2014/08/20 22:17:21, willchan wrote: > Not for readability: in general you should prefer not to pass around Profile* > objects. Generally speaking, you should prefer to pass finer grained > dependencies, rather than huge objects like Profile which have all these other > dependencies. That will enable easier unit testing, componentization (if this > ever needs to moved to content/ or components/), etc. In this case, we need some pretty beefy stuff from Profile. Namely, we call GetHostContentSettingsMap() several times, and we also call GetRequestContext() on the Profile. Is your suggestion that we should keep around raw pointers to those instead? I haven't seen anything like that, so I sort of assumed it was healthier to grab them off the Profile whenever they were needed (profile.h doesn't mention any guarantees about those pointers not going stale). https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:58: virtual bool HasUserDecision(const std::string& host); On 2014/08/20 22:17:21, willchan wrote: > This member function reads like it should be const. Is there any reason it > isn't? Nope, done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:61: void ShutdownOnUIThread() {} On 2014/08/20 22:17:21, willchan wrote: > Nit: What's up with this? Empty function? Does anyone call this? That was leftover from an initial implementation and is called in chrome_ssl_host_state_delegate_factory.cc. Definitely not needed now, though. Removed. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:77: CreateDictionaryEntries, On 2014/08/20 22:17:22, willchan wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...: > "Enumerators should be named either like constants or like macros: either > kEnumName or ENUM_NAME." > > Chromium's style guide says > (http://www.chromium.org/developers/coding-style#Naming): "Though the Google C++ > Style Guide now says to use kConstantNaming for enums, Chromium was written > using MACRO_STYLE naming. Continue to use this style for consistency." > > Therefore, you should use ENUM_NAME. Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:96: net::X509Certificate* cert, On 2014/08/20 22:17:21, willchan wrote: > Is there a reason |cert| isn't a const reference? Do you modify it at all? > > I'd make the same comment in the member functions above, except those appear to > be virtuals inherited from an interface out of your control here. Nope, those can all be const. And, in fact, the interface is within my control :-) It was part of the original CL, but it seemed like overload to include it as part of the readability submission, but I'll add it in now. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:132: std::set<BrokenHostEntry> ran_insecure_content_hosts_; On 2014/08/20 22:17:21, willchan wrote: > You should #include <set>. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...: > > """ > Always #include the file that actually provides the declarations/definitions you > need; do not rely on the symbol being brought in transitively via headers not > directly included. One exception is that myfile.cc may rely on #includes and > forward declarations from its corresponding header file myfile.h. > """ Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:14: #include "chrome/browser/ssl/chrome_ssl_host_state_delegate.h" On 2014/08/20 22:17:22, willchan wrote: > Goes first: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_.... > > The rule applies to both foo.cc and foo_test.cc: > > "In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test > the stuff in dir2/foo2.h, order your includes as follows" Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:35: const char* kForgetAtSessionEnd = "-1"; On 2014/08/20 22:17:22, willchan wrote: > These 3 aren't constants. They're non-const pointers to a const char[]. You > should do the same as above, const char kFoo[] = ...; to turn them into real > constants. Done. https://codereview.chromium.org/441043005/diff/20001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:52: // InProcessBrowserTest because the actual functionality is provided by On 2014/08/20 22:17:22, willchan wrote: > Not for readability, but would you be able to write a smaller test if you didn't > use Profile in ChromeSSLHostStateDelegate? Probably not because the implementation relies on an actual content settings implementation, which is built per Profile.
OK, last readability comments. Thanks to you and your reviewers for making sure the changelist was so clean. https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:32: const net::X509Certificate* cert, Can/should this be a const reference rather than a pointer to a const object? Can it ever be NULL? In the implementation of this function, there's no NULL check, so I assume it can't be NULL, or else there's a bug. https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:44: const int pid) OVERRIDE; Nit: I'd remove this const. The reason is it's copy by value anyway, so making it const doesn't really protect anything. I just noticed that net::CertStatus is the same. It's a POD type, so it's copy by value too, so no need to make it const.
https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:32: const net::X509Certificate* cert, On 2014/09/04 00:02:38, willchan OOO until 03-22-15 wrote: > Can/should this be a const reference rather than a pointer to a const object? > Can it ever be NULL? In the implementation of this function, there's no NULL > check, so I assume it can't be NULL, or else there's a bug. I believe that historically this was a pointer because the origin of the cert is from SSLInfo, which gives the cert as a pointer. But I think you are correct that it would be a bug for it to be NULL, so I'll make this a const ref. https://codereview.chromium.org/441043005/diff/40001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate.h:44: const int pid) OVERRIDE; On 2014/09/04 00:02:38, willchan OOO until 03-22-15 wrote: > Nit: I'd remove this const. The reason is it's copy by value anyway, so making > it const doesn't really protect anything. > > I just noticed that net::CertStatus is the same. It's a POD type, so it's copy > by value too, so no need to make it const. Done.
lgtm https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: *google_cert.get(), Doesn't bug me, but it might bug some people. *google_cert works, you don't need the .get() anymore. But not a readability thing. Do whatever the OWNERS want here.
After you commit, I'll close the readability bug which will grant you readability.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: *google_cert.get(), On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > Doesn't bug me, but it might bug some people. *google_cert works, you don't need > the .get() anymore. But not a readability thing. Do whatever the OWNERS want > here. Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is correct, as *google_cert relies on the deprecated implicit pointer conversion.
https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: *google_cert.get(), On 2014/09/04 20:28:16, Ryan Sleevi wrote: > On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > > Doesn't bug me, but it might bug some people. *google_cert works, you don't > need > > the .get() anymore. But not a readability thing. Do whatever the OWNERS want > > here. > > Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is correct, as > *google_cert relies on the deprecated implicit pointer conversion. Do what sleevi@ says, although I'm a bit confused on what he's saying. scoped_refptr defines an operator*. I forget the status of whether or not that temporarily needs to be removed during the transition, but I don't see how *google_cert relies on the deprecated implicit pointer conversion.
sleevi@, can you OWNER review of chrome/browser/ssl/*? jam@, can you OWNER review content/*? Thanks! https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: *google_cert.get(), On 2014/09/04 20:33:12, willchan OOO until 03-22-15 wrote: > On 2014/09/04 20:28:16, Ryan Sleevi wrote: > > On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > > > Doesn't bug me, but it might bug some people. *google_cert works, you don't > > need > > > the .get() anymore. But not a readability thing. Do whatever the OWNERS want > > > here. > > > > Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is correct, > as > > *google_cert relies on the deprecated implicit pointer conversion. > > Do what sleevi@ says, although I'm a bit confused on what he's saying. > scoped_refptr defines an operator*. I forget the status of whether or not that > temporarily needs to be removed during the transition, but I don't see how > *google_cert relies on the deprecated implicit pointer conversion. Well, I've already got willchan's lgtm, so I don't need you anymore :-) sleevi@ ftw!
jww@chromium.org changed reviewers: + jam@chromium.org
https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: *google_cert.get(), On 2014/09/04 20:56:16, jww wrote: > On 2014/09/04 20:33:12, willchan OOO until 03-22-15 wrote: > > On 2014/09/04 20:28:16, Ryan Sleevi wrote: > > > On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > > > > Doesn't bug me, but it might bug some people. *google_cert works, you > don't > > > need > > > > the .get() anymore. But not a readability thing. Do whatever the OWNERS > want > > > > here. > > > > > > Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is correct, > > as > > > *google_cert relies on the deprecated implicit pointer conversion. > > > > Do what sleevi@ says, although I'm a bit confused on what he's saying. > > scoped_refptr defines an operator*. I forget the status of whether or not that > > temporarily needs to be removed during the transition, but I don't see how > > *google_cert relies on the deprecated implicit pointer conversion. > > Well, I've already got willchan's lgtm, so I don't need you anymore :-) sleevi@ > ftw! haha :) Btw, I think I see where sleevi@ is getting confused. He must think that this CL already landed: https://codereview.chromium.org/510323002/. That CL disables operator* temporarily.
On 2014/09/04 21:25:23, willchan OOO until 03-22-15 wrote: > https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... > File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): > > https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... > chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: > *google_cert.get(), > On 2014/09/04 20:56:16, jww wrote: > > On 2014/09/04 20:33:12, willchan OOO until 03-22-15 wrote: > > > On 2014/09/04 20:28:16, Ryan Sleevi wrote: > > > > On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > > > > > Doesn't bug me, but it might bug some people. *google_cert works, you > > don't > > > > need > > > > > the .get() anymore. But not a readability thing. Do whatever the OWNERS > > want > > > > > here. > > > > > > > > Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is > correct, > > > as > > > > *google_cert relies on the deprecated implicit pointer conversion. > > > > > > Do what sleevi@ says, although I'm a bit confused on what he's saying. > > > scoped_refptr defines an operator*. I forget the status of whether or not > that > > > temporarily needs to be removed during the transition, but I don't see how > > > *google_cert relies on the deprecated implicit pointer conversion. > > > > Well, I've already got willchan's lgtm, so I don't need you anymore :-) > sleevi@ > > ftw! > > haha :) > > Btw, I think I see where sleevi@ is getting confused. He must think that this CL > already landed: https://codereview.chromium.org/510323002/. That CL disables > operator* temporarily. Well, operator* stays, operatorT* goes, just that the rewrite tool dcheng's been using is a bit zealous because of all the moving pieces. The nice part is just do what you're doing, and then we'll fix it all in post once we get people out of the transition state :)
On 2014/09/04 21:42:10, Ryan Sleevi wrote: > On 2014/09/04 21:25:23, willchan OOO until 03-22-15 wrote: > > > https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... > > File chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc (right): > > > > > https://codereview.chromium.org/441043005/diff/60001/chrome/browser/ssl/chrom... > > chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc:70: > > *google_cert.get(), > > On 2014/09/04 20:56:16, jww wrote: > > > On 2014/09/04 20:33:12, willchan OOO until 03-22-15 wrote: > > > > On 2014/09/04 20:28:16, Ryan Sleevi wrote: > > > > > On 2014/09/04 20:26:49, willchan OOO until 03-22-15 wrote: > > > > > > Doesn't bug me, but it might bug some people. *google_cert works, you > > > don't > > > > > need > > > > > > the .get() anymore. But not a readability thing. Do whatever the > OWNERS > > > want > > > > > > here. > > > > > > > > > > Because it's a scoped_refptr<>, this syntax (*google_cert.get()) is > > correct, > > > > as > > > > > *google_cert relies on the deprecated implicit pointer conversion. > > > > > > > > Do what sleevi@ says, although I'm a bit confused on what he's saying. > > > > scoped_refptr defines an operator*. I forget the status of whether or not > > that > > > > temporarily needs to be removed during the transition, but I don't see how > > > > *google_cert relies on the deprecated implicit pointer conversion. > > > > > > Well, I've already got willchan's lgtm, so I don't need you anymore :-) > > sleevi@ > > > ftw! > > > > haha :) > > > > Btw, I think I see where sleevi@ is getting confused. He must think that this > CL > > already landed: https://codereview.chromium.org/510323002/. That CL disables > > operator* temporarily. > > Well, operator* stays, operatorT* goes, just that the rewrite tool dcheng's been > using is a bit zealous because of all the moving pieces. Oops. I'm still too drowsy and missed the distinction there. > > The nice part is just do what you're doing, and then we'll fix it all in post > once we get people out of the transition state :) Great.
OK, paying attention to the review now, rather than the odd will comment. This LGTM, but with a caveat I note below. I don't have strong feelings, which is why I'm LG'ing this, but I want you to carefully consider the API choice you're making here. More below. https://codereview.chromium.org/441043005/diff/80001/content/public/browser/s... File content/public/browser/ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/80001/content/public/browser/s... content/public/browser/ssl_host_state_delegate.h:30: const net::X509Certificate& cert, So, the trade-off here is that net::X509Certificate is designed to be ref-counted. If anyone wanted to be able to hold on to |cert| (e.g. as part of implementing QueryPolicy), they can no longer do so with this API. Instead, they'd have to create a new net::X509Certificate via the X509Certificate::Create methods (which, because of an in memory cache, would return a pointer to the same object after reparsing the data q_q). This is fine because none of the (Chrome) implementations actually store on to |cert|. But I don't know if that's long-term where you want the API to go, or if that's the contract you make. The benefit is you avoid the need for NULL-checking. The downside is you force more work on the implementers of this interface from being able to implement certain patterns. I'm fine with either, because I don't want to get lost in a see of "what if", but I wanted to make sure you understood the API tradeoff being done here.
lgtm
https://codereview.chromium.org/441043005/diff/80001/content/public/browser/s... File content/public/browser/ssl_host_state_delegate.h (right): https://codereview.chromium.org/441043005/diff/80001/content/public/browser/s... content/public/browser/ssl_host_state_delegate.h:30: const net::X509Certificate& cert, On 2014/09/04 21:46:24, Ryan Sleevi wrote: > So, the trade-off here is that net::X509Certificate is designed to be > ref-counted. > > If anyone wanted to be able to hold on to |cert| (e.g. as part of implementing > QueryPolicy), they can no longer do so with this API. Instead, they'd have to > create a new net::X509Certificate via the X509Certificate::Create methods > (which, because of an in memory cache, would return a pointer to the same object > after reparsing the data q_q). > > This is fine because none of the (Chrome) implementations actually store on to > |cert|. But I don't know if that's long-term where you want the API to go, or if > that's the contract you make. > > The benefit is you avoid the need for NULL-checking. The downside is you force > more work on the implementers of this interface from being able to implement > certain patterns. > > I'm fine with either, because I don't want to get lost in a see of "what if", > but I wanted to make sure you understood the API tradeoff being done here. I am happy with the API choice here, so I'm going to go ahead and commit. I prefer the non-NULL guarantee over the potential API choices in the future, and worst case scenario, we can modify the API if that comes up.
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/441043005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as a0582c6e4a663945c88e675b206102b42eeaa6b4
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a18a71f89a845d9acc83ca9ddbac35b9a463633f Cr-Commit-Position: refs/heads/master@{#293443} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
