Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9378014:
Add website settings backend v 0.1 (Closed)
|
Created:
8 years, 10 months ago by markusheintz_ Modified:
8 years, 9 months ago CC:
chromium-reviews, wtc Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd website settings backend v 0.1
This is the second CL in a row of CLs to land the first version of Website settings. This CL adds a bare minimum website settings backend that only provides site identity and connection information. History and cookies infos will be added next.
CLs:
First CL: https://chromiumcodereview.appspot.com/9383005/
Second CL: UI Backend (this CL)
Third CL: GTK UI ((WIP)https://chromiumcodereview.appspot.com/9379016/)
BUG=113688
TEST=WebsiteSettingsModelTest*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122772
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125161
Patch Set 1 #Patch Set 2 : Move UI and cookies code out of this CL. #Patch Set 3 : Remove unrelated changes #Patch Set 4 : Move cookie related code to another CL. #
Total comments: 20
Patch Set 5 : Addressed review comments. #Patch Set 6 : Add WebsiteSettingsModel files #
Total comments: 8
Patch Set 7 : Fix nits and add tests. #
Total comments: 42
Patch Set 8 : #Patch Set 9 : Fix one nit. #Patch Set 10 : Change names of the time variables for certs dates in the unittest. #
Total comments: 2
Patch Set 11 : Address comments and fix bad includes. #Patch Set 12 : Fix memory leak in unittest #Patch Set 13 : Add MockCertStore #Patch Set 14 : Fix HTTPSBadCertificateTest #Patch Set 15 : Move HTTPSBadCertificateTest to separate CL #Patch Set 16 : Remove unrelated changes #
Messages
Total messages: 19 (0 generated)
@Finnur: Could you please review this CL for me? Thanks. Currently this CL is mostly a copy of the PageInfoModel. But I need this for several reasons: - I develop Website Settings behind a flag and need a separate playground - This is supposed to get much more functionality.
This class isn't used anywhere, but I assume that will follow in another changelist (it is not that you forgot a file in this CL)? I think it is important to write at least a unit test to test the Init function of the Web Settings class. This is something that the old class should have had, but for some reason didn't. I also think wtc should be involved in any advancement of the old model class -- he was my go-to guy whenever I needed to change it, so I've cc'ed him in case he has any concerns. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... File chrome/browser/website_settings.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:33: bool show_history) |wrapper| and |show_history| are unused. Do you plan to use those later? https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:160: } No braces around single line if statements. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:182: site_connection_status_ = SITE_CONNECTION_STATUS_ENCRYPTED; It feels like we should assume STATUS_NA until we find that it is encrypted. Oh, wait, I guess this is the way it was before (ICON_STATE_OK)... https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... File chrome/browser/website_settings.h (right): https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:8: #include "base/memory/scoped_ptr.h" You need this here? https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:20: class WebsiteSettings { Isn't this a WebsiteSettingsModel class? Or did you intentionally leave out the Model from the name? https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:20: class WebsiteSettings { This class could do with some documentation (including members and functions)... https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:29: }; nit: end all comments with a period (here and below). https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:56: string16 site_identity_details(); All of these functions can have |const| at the end. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:65: string16 organisation_name_; Spelling: According to what I've been reading, English spelling accepts both organization and organisation but US spelling dictates the former (organization). https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:69: SiteConnectionStatus site_connection_status_; nit: My personal preference would be to keep this next to SiteIdentityStatus? Same with the accessors above.
I addressed your comments and will add a unit_tests now. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... File chrome/browser/website_settings.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:33: bool show_history) On 2012/02/14 11:00:36, Finnur wrote: > |wrapper| and |show_history| are unused. Do you plan to use those later? I removed both. I will need the both later. But I since it's no effort to add them in a later CL when I really need them I removed them here. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:160: } On 2012/02/14 11:00:36, Finnur wrote: > No braces around single line if statements. Done. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.cc:182: site_connection_status_ = SITE_CONNECTION_STATUS_ENCRYPTED; On 2012/02/14 11:00:36, Finnur wrote: > It feels like we should assume STATUS_NA until we find that it is encrypted. Oh, > wait, I guess this is the way it was before (ICON_STATE_OK)... It was like this before (that was also my excuse for not having it changed yet)but I feel the same so I changed it now. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... File chrome/browser/website_settings.h (right): https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:8: #include "base/memory/scoped_ptr.h" On 2012/02/14 11:00:36, Finnur wrote: > You need this here? No that should have been moved to a different CL. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:20: class WebsiteSettings { On 2012/02/14 11:00:36, Finnur wrote: > Isn't this a WebsiteSettingsModel class? Or did you intentionally leave out the > Model from the name? I omitted the "Model" bc I think this will be more than just a data model in the end. (But I guess that translates to: too lazy to rename). I added a Model suffix I guess I can remove this once this really becomes a more heavy weight obj. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:20: class WebsiteSettings { On 2012/02/14 11:00:36, Finnur wrote: > This class could do with some documentation (including members and functions)... Done. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:29: }; On 2012/02/14 11:00:36, Finnur wrote: > nit: end all comments with a period (here and below). Done. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:56: string16 site_identity_details(); On 2012/02/14 11:00:36, Finnur wrote: > All of these functions can have |const| at the end. Done. https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:65: string16 organisation_name_; On 2012/02/14 11:00:36, Finnur wrote: > Spelling: According to what I've been reading, English spelling accepts both > organization and organisation but US spelling dictates the former > (organization). Changed to US version https://chromiumcodereview.appspot.com/9378014/diff/5001/chrome/browser/websi... chrome/browser/website_settings.h:69: SiteConnectionStatus site_connection_status_; On 2012/02/14 11:00:36, Finnur wrote: > nit: My personal preference would be to keep this next to SiteIdentityStatus? > Same with the accessors above. I sorted the fields and accessors like you suggested.
LGTM
with nits... https://chromiumcodereview.appspot.com/9378014/diff/7003/chrome/browser/websi... File chrome/browser/website_settings_model.h (right): https://chromiumcodereview.appspot.com/9378014/diff/7003/chrome/browser/websi... chrome/browser/website_settings_model.h:42: // The website provided a valid certificate. nit: valid *DnsSec* certificate? https://chromiumcodereview.appspot.com/9378014/diff/7003/chrome/browser/websi... chrome/browser/website_settings_model.h:44: // The website provided a valid certificate by no revocation check could be s/by/but/ ? https://chromiumcodereview.appspot.com/9378014/diff/7003/chrome/browser/websi... chrome/browser/website_settings_model.h:57: // status object to determin the status of the site's connection. s/determin/determine/ https://chromiumcodereview.appspot.com/9378014/diff/7003/chrome/browser/websi... chrome/browser/website_settings_model.h:93: // encryption strength and ssl protocoll version. s/protocoll/protocol/
Thanks for the review I fixed the nits. And started a unittest. I will complete the unittests and ping you for a final look on the test before I commit. http://codereview.chromium.org/9378014/diff/7003/chrome/browser/website_setti... File chrome/browser/website_settings_model.h (right): http://codereview.chromium.org/9378014/diff/7003/chrome/browser/website_setti... chrome/browser/website_settings_model.h:42: // The website provided a valid certificate. On 2012/02/14 13:50:10, Finnur wrote: > nit: valid *DnsSec* certificate? Done. http://codereview.chromium.org/9378014/diff/7003/chrome/browser/website_setti... chrome/browser/website_settings_model.h:44: // The website provided a valid certificate by no revocation check could be On 2012/02/14 13:50:10, Finnur wrote: > s/by/but/ ? Done. http://codereview.chromium.org/9378014/diff/7003/chrome/browser/website_setti... chrome/browser/website_settings_model.h:57: // status object to determin the status of the site's connection. On 2012/02/14 13:50:10, Finnur wrote: > s/determin/determine/ Done. http://codereview.chromium.org/9378014/diff/7003/chrome/browser/website_setti... chrome/browser/website_settings_model.h:93: // encryption strength and ssl protocoll version. On 2012/02/14 13:50:10, Finnur wrote: > s/protocoll/protocol/ Done.
Drive-by review comments on Patch Set 7: You can commit this CL after fixing the nits (and one bug) below. I am too tired to review WebsiteSettingsModel::Init, which is the meat of the CL. If you would like me to check it, I'll be happy to do that, but don't wait for me. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... File chrome/browser/website_settings_model.cc (right): http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.cc:34: // After initialization there must be a status about the a site's connection Typo: the a Nit: this sentence sounds a little awkward. I guess you meant After initialization the status about the site's connection and it's identity must be available. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.cc:61: string16 WebsiteSettingsModel::organization_name() const { Nit: why don't you define these five getters in the header file? http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... File chrome/browser/website_settings_model.h (right): http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:9: Nit: this blank line is not necessary. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:21: // WebsiteSettingsUI which displays this information. Nit: I don't know why you named this class WebsiteSettingsModel. "Settings" implies configuration or preferences, but this class contains information. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:26: SITE_CONNECTION_STATUS_NA = 0, // No status available. Nit: "N/A" stands for "not applicable" as opposed to "not available". I suggest you use _NONE or _UNKNOWN. This comment also applies to SITE_IDENTITY_STATUS_NA on line 37 below. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:31: SITE_CONNECTION_STATUS_INTERNAL_PAGE, // Internal site. I don't know what "Internal" means here. Internal to a company, or internal to Chrome? http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:42: // The website provided a valid DnsSec certificate. Nit: DnsSec => DNSSEC http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:46: SITE_IDENTITY_STATUS_CERT_NOT_VERIFIED, Change "NOT_VERIFIED" to "REVOKE_STATUS_UNKNOWN" or "REVOCATION_UNKNOWN". http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model.h:96: // For websites that provided a EV certificate |orgainization_name_| contains Nit: a EV => an EV http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... File chrome/browser/website_settings_model_unittest.cc (right): http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:5: #include "chrome/browser/website_settings_model.h" Nit: I suggest adding a blank line to separate this header (which declares the class under test) from the other headers. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:8: #include "testing/gtest/include/gtest/gtest.h" Nit: list in alphabetical order. http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:14: // SSL Cipher Suits like specified in RFC5246 Appendix A.5. Typo: Suits => Suites http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:19: connection_status = connection_status & (3 << 20); Nit: use the &= operator: connection_status &= (3 << 20); BUG: this looks wrong. I think we need to do connection_status &= ~(7 << 20); because 3 has only two bits and the ~ is necessary. You should replace 7 with net::SSL_CONNECTION_VERSION_MASK and 20 with net::SSL_CONNECTION_VERSION_SHIFT http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:26: connection_status = connection_status & (~0 << 16); Nit: I suggest you say connection_status &= ~net::SSL_CONNECTION_CIPHERSUITE_MASK; http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:27: int bitmask = 0 | cipher_suite; Remove "0 | ". Actually you can just say return cipher_suit | connection_status; http://codereview.chromium.org/9378014/diff/11004/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:59: status = SetSSLCipherSuite(status, TLS_RSA_WITH_NULL_MD5); Nit: TLS_RSA_WITH_NULL_MD5 is a cipher suite that does no encryption (0 security bits). So this is a bad example.
Still LGTM, now with nits on the unit test. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... File chrome/browser/website_settings_model_unittest.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:18: // Clear ssl version bits (20, 21, 22); nit: period, not semicolon. :) Also: s/ssl/SSL/ https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:39: nit: Extra line, don't need it. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:66: model->site_connection_status()); What about site_connection_details, etc? https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:70: // TODO(markusheintz): Implement Look forward to seeing those. Thanks for adding this! https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:78: // TODO(markusheintz): Implement nit: indentation (see also above)
Thanks a lot to both of your for reviewing. I finished the unit_test. I don't what the SSLStatus security bit's mean so I set them to values that work for the test cases. @finnur: I did not add a check for the YYY_details() yet as I want them to change from a string to a struct. @wtc: Any review of you is valuable. But most of the Init method is taken from the PageInfoModel. I did not change much yet. If you are fine with the connection states and identity states I added than thats good to me. Once I start to break the init method in pieces I will return to you for more reviews. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... File chrome/browser/website_settings_model.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.cc:34: // After initialization there must be a status about the a site's connection On 2012/02/15 02:14:29, wtc wrote: > > Typo: the a > > Nit: this sentence sounds a little awkward. I guess you meant > After initialization the status about the site's connection > and it's identity must be available. Done. Thanks a lot for correcting this. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.cc:61: string16 WebsiteSettingsModel::organization_name() const { On 2012/02/15 02:14:29, wtc wrote: > > Nit: why don't you define these five getters in the header > file? True. I changed this. The getters were not defined in the header for "historical" reasons. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... File chrome/browser/website_settings_model.h (right): https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:9: On 2012/02/15 02:14:29, wtc wrote: > > Nit: this blank line is not necessary. Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:21: // WebsiteSettingsUI which displays this information. On 2012/02/15 02:14:29, wtc wrote: > > Nit: I don't know why you named this class WebsiteSettingsModel. > "Settings" implies configuration or preferences, but > this class contains information. This will become more than just a data model. Originally I named it "WebsiteSettings" and I plan to return to this name. I will provide you more details via email. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:26: SITE_CONNECTION_STATUS_NA = 0, // No status available. On 2012/02/15 02:14:29, wtc wrote: > > Nit: "N/A" stands for "not applicable" as opposed to > "not available". I suggest you use _NONE or > _UNKNOWN. > > This comment also applies to SITE_IDENTITY_STATUS_NA on > line 37 below. Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:31: SITE_CONNECTION_STATUS_INTERNAL_PAGE, // Internal site. On 2012/02/15 02:14:29, wtc wrote: > > I don't know what "Internal" means here. Internal to a > company, or internal to Chrome? Internal to chrome. Internal pages like chrome://settings, chrome://ABC https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:42: // The website provided a valid DnsSec certificate. On 2012/02/15 02:14:29, wtc wrote: > > Nit: DnsSec => DNSSEC Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:46: SITE_IDENTITY_STATUS_CERT_NOT_VERIFIED, On 2012/02/15 02:14:29, wtc wrote: > > Change "NOT_VERIFIED" to "REVOKE_STATUS_UNKNOWN" > or "REVOCATION_UNKNOWN". Done. I chose REVOCATION_UNKNOWN. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model.h:96: // For websites that provided a EV certificate |orgainization_name_| contains On 2012/02/15 02:14:29, wtc wrote: > > Nit: a EV => an EV Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... File chrome/browser/website_settings_model_unittest.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:5: #include "chrome/browser/website_settings_model.h" On 2012/02/15 02:14:29, wtc wrote: > > Nit: I suggest adding a blank line to separate this header > (which declares the class under test) from the other headers. Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:8: #include "testing/gtest/include/gtest/gtest.h" On 2012/02/15 02:14:29, wtc wrote: > > Nit: list in alphabetical order. Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:14: // SSL Cipher Suits like specified in RFC5246 Appendix A.5. On 2012/02/15 02:14:29, wtc wrote: > > Typo: Suits => Suites Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:18: // Clear ssl version bits (20, 21, 22); On 2012/02/15 08:59:49, Finnur wrote: > nit: period, not semicolon. :) Also: s/ssl/SSL/ Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:19: connection_status = connection_status & (3 << 20); On 2012/02/15 02:14:29, wtc wrote: > > Nit: use the &= operator: > connection_status &= (3 << 20); > > BUG: this looks wrong. I think we need to do > connection_status &= ~(7 << 20); > because 3 has only two bits and the ~ is necessary. > > You should replace 7 with > net::SSL_CONNECTION_VERSION_MASK > and 20 with > net::SSL_CONNECTION_VERSION_SHIFT Done. I am ashamed to death https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:26: connection_status = connection_status & (~0 << 16); On 2012/02/15 02:14:29, wtc wrote: > > Nit: I suggest you say > connection_status &= ~net::SSL_CONNECTION_CIPHERSUITE_MASK; Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:27: int bitmask = 0 | cipher_suite; On 2012/02/15 02:14:29, wtc wrote: > > Remove "0 | ". Actually you can just say > return cipher_suit | connection_status; Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:39: On 2012/02/15 08:59:49, Finnur wrote: > nit: Extra line, don't need it. Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:59: status = SetSSLCipherSuite(status, TLS_RSA_WITH_NULL_MD5); On 2012/02/15 02:14:29, wtc wrote: > > Nit: TLS_RSA_WITH_NULL_MD5 is a cipher suite that does no > encryption (0 security bits). So this is a bad example. Done. I'm using a different one now. Maybe you want to double check. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:66: model->site_connection_status()); On 2012/02/15 08:59:49, Finnur wrote: > What about site_connection_details, etc? Actually I'd like to change the details from a string to a struct and create the final string to display in the UI. So I have no test for the details yet. If it is fine with you I'll add a test in the second iteration. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:70: // TODO(markusheintz): Implement On 2012/02/15 08:59:49, Finnur wrote: > Look forward to seeing those. Thanks for adding this! Done. https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:78: // TODO(markusheintz): Implement On 2012/02/15 08:59:49, Finnur wrote: > nit: indentation (see also above) Done.
Yup. Sounds good. On 2012/02/15 19:04:18, markusheintz_ wrote: > Thanks a lot to both of your for reviewing. > > I finished the unit_test. > > I don't what the SSLStatus security bit's mean so I set them to values that work > for the test cases. > > @finnur: I did not add a check for the YYY_details() yet as I want them to > change from a string to a struct. > > @wtc: Any review of you is valuable. But most of the Init method is taken from > the PageInfoModel. I did not change much yet. If you are fine with the > connection states and identity states I added than thats good to me. Once I > start to break the init method in pieces I will return to you for more reviews. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > File chrome/browser/website_settings_model.cc (right): > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.cc:34: // After initialization there must > be a status about the a site's connection > On 2012/02/15 02:14:29, wtc wrote: > > > > Typo: the a > > > > Nit: this sentence sounds a little awkward. I guess you meant > > After initialization the status about the site's connection > > and it's identity must be available. > > Done. Thanks a lot for correcting this. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.cc:61: string16 > WebsiteSettingsModel::organization_name() const { > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: why don't you define these five getters in the header > > file? > > True. I changed this. > > The getters were not defined in the header for "historical" reasons. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > File chrome/browser/website_settings_model.h (right): > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:9: > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: this blank line is not necessary. > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:21: // WebsiteSettingsUI which displays > this information. > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: I don't know why you named this class WebsiteSettingsModel. > > "Settings" implies configuration or preferences, but > > this class contains information. > > This will become more than just a data model. Originally I named it > "WebsiteSettings" and I plan to return to this name. > I will provide you more details via email. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:26: SITE_CONNECTION_STATUS_NA = 0, > // No status available. > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: "N/A" stands for "not applicable" as opposed to > > "not available". I suggest you use _NONE or > > _UNKNOWN. > > > > This comment also applies to SITE_IDENTITY_STATUS_NA on > > line 37 below. > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:31: > SITE_CONNECTION_STATUS_INTERNAL_PAGE, // Internal site. > On 2012/02/15 02:14:29, wtc wrote: > > > > I don't know what "Internal" means here. Internal to a > > company, or internal to Chrome? > > Internal to chrome. Internal pages like chrome://settings, chrome://ABC > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:42: // The website provided a valid > DnsSec certificate. > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: DnsSec => DNSSEC > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:46: > SITE_IDENTITY_STATUS_CERT_NOT_VERIFIED, > On 2012/02/15 02:14:29, wtc wrote: > > > > Change "NOT_VERIFIED" to "REVOKE_STATUS_UNKNOWN" > > or "REVOCATION_UNKNOWN". > > Done. I chose REVOCATION_UNKNOWN. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model.h:96: // For websites that provided a EV > certificate |orgainization_name_| contains > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: a EV => an EV > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > File chrome/browser/website_settings_model_unittest.cc (right): > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:5: #include > "chrome/browser/website_settings_model.h" > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: I suggest adding a blank line to separate this header > > (which declares the class under test) from the other headers. > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:8: #include > "testing/gtest/include/gtest/gtest.h" > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: list in alphabetical order. > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:14: // SSL Cipher Suits like > specified in RFC5246 Appendix A.5. > On 2012/02/15 02:14:29, wtc wrote: > > > > Typo: Suits => Suites > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:18: // Clear ssl version bits > (20, 21, 22); > On 2012/02/15 08:59:49, Finnur wrote: > > nit: period, not semicolon. :) Also: s/ssl/SSL/ > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:19: connection_status = > connection_status & (3 << 20); > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: use the &= operator: > > connection_status &= (3 << 20); > > > > BUG: this looks wrong. I think we need to do > > connection_status &= ~(7 << 20); > > because 3 has only two bits and the ~ is necessary. > > > > You should replace 7 with > > net::SSL_CONNECTION_VERSION_MASK > > and 20 with > > net::SSL_CONNECTION_VERSION_SHIFT > > Done. I am ashamed to death > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:26: connection_status = > connection_status & (~0 << 16); > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: I suggest you say > > connection_status &= ~net::SSL_CONNECTION_CIPHERSUITE_MASK; > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:27: int bitmask = 0 | > cipher_suite; > On 2012/02/15 02:14:29, wtc wrote: > > > > Remove "0 | ". Actually you can just say > > return cipher_suit | connection_status; > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:39: > On 2012/02/15 08:59:49, Finnur wrote: > > nit: Extra line, don't need it. > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:59: status = > SetSSLCipherSuite(status, TLS_RSA_WITH_NULL_MD5); > On 2012/02/15 02:14:29, wtc wrote: > > > > Nit: TLS_RSA_WITH_NULL_MD5 is a cipher suite that does no > > encryption (0 security bits). So this is a bad example. > > Done. I'm using a different one now. Maybe you want to double check. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:66: > model->site_connection_status()); > On 2012/02/15 08:59:49, Finnur wrote: > > What about site_connection_details, etc? > > Actually I'd like to change the details from a string to a struct and create the > final string to display in the UI. > > So I have no test for the details yet. > > If it is fine with you I'll add a test in the second iteration. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:70: // TODO(markusheintz): > Implement > On 2012/02/15 08:59:49, Finnur wrote: > > Look forward to seeing those. Thanks for adding this! > > Done. > > https://chromiumcodereview.appspot.com/9378014/diff/11004/chrome/browser/webs... > chrome/browser/website_settings_model_unittest.cc:78: // TODO(markusheintz): > Implement > On 2012/02/15 08:59:49, Finnur wrote: > > nit: indentation (see also above) > > Done.
markusheintz: I am busy today. You can commit this CL without waiting for me.
Patch Set 10 LGTM. I only reviewed the diffs between Patch Sets 7 and 10. I didn't review the new unit tests you added. I didn't check WebsiteSettingsModel::Init(). https://chromiumcodereview.appspot.com/9378014/diff/18001/chrome/browser/webs... File chrome/browser/website_settings_model_unittest.cc (right): https://chromiumcodereview.appspot.com/9378014/diff/18001/chrome/browser/webs... chrome/browser/website_settings_model_unittest.cc:27: ~(net::SSL_CONNECTION_VERSION_MASK << net::SSL_CONNECTION_VERSION_MASK); BUG: the second one (to the right of <<) should be _SHIFT.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/9378014/30003
Thanks a lot for the review. http://codereview.chromium.org/9378014/diff/18001/chrome/browser/website_sett... File chrome/browser/website_settings_model_unittest.cc (right): http://codereview.chromium.org/9378014/diff/18001/chrome/browser/website_sett... chrome/browser/website_settings_model_unittest.cc:27: ~(net::SSL_CONNECTION_VERSION_MASK << net::SSL_CONNECTION_VERSION_MASK); On 2012/02/17 00:48:56, wtc wrote: > > BUG: the second one (to the right of <<) should be _SHIFT. Done.
Change committed as 122772
@Finnur The unit_test caused some memory leaks last time I tried to commit. In order to have a nice and slim unit_tests I needed to touch some other code. http://codereview.chromium.org/9545011 allows me to use a mock for the CertStore instead of the singleton. However I have one more test that does not work well with the mock since it indirectly still uses the singleton CertStore (see |SSLErrorInfo|). This obviously does not go well together with a MockCertStore. In order to make them all play nicely together I need to touch other code. Would it be ok if I do that in a separate CL (http://codereview.chromium.org/9597024/)?
Yes, I'm fine with that in a separate changelist. On 2012/03/05 17:37:21, markusheintz_ wrote: > @Finnur > > The unit_test caused some memory leaks last time I tried to commit. In order to > have a nice and slim unit_tests I needed to touch some other code. > > http://codereview.chromium.org/9545011 allows me to use a mock for the CertStore > instead of the singleton. > > However I have one more test that does not work well with the mock since it > indirectly still uses the singleton CertStore (see |SSLErrorInfo|). This > obviously does not go well together with a MockCertStore. In order to make them > all play nicely together I need to touch other code. > > Would it be ok if I do that in a separate CL > (http://codereview.chromium.org/9597024/%29?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/9378014/51008
Change committed as 125161 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
