Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9535022:
Exposed GetSecurityState and GetPageType to pyAuto. (Closed)
|
Created:
8 years, 9 months ago by dyu1 Modified:
8 years, 9 months ago CC:
chromium-reviews, dennis_jeffrey, John Grabowski, anantha, Ryan Sleevi Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionExposed GetSecurityState and GetPageType to pyAuto.
The GetSecurityState retrieves different security states for the current tab.
The GetPageType returns the type of page showing (normal, interstitial, error).
Added additional tests that uses the exposed hooks.
- testSSLCertOK
- testSSLCertIsExpiredAndCertNameMismatches
- testSSLCertAuthorityOK
TEST=none
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126777
Patch Set 1 #
Total comments: 43
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 38
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:105: def testSSLCertIsExpired(self): testSSLCertIsExpired and remaining two functions are pretty much doing the same things except for ServerCertificate names and CertStatus. Can you move the common code to a separate function? http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib.i File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:157: %feature("docstring", "Retrieves the different security states for the current tab.") Indentation doesn't look correct. Can you please check?
I took a quick look through the .py file and have a few comments there, but will take a closer look in the next round after you address Anantha's comment. http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:146: security_style, ssl_cert_status, insecure_content_status) two of the arguments here aren't used. maybe we can just inline the right-hand sides of lines 142, 144 above into the current line, then delete lines 142, 144? If you agree to do so, make the same change in the other tests above. Or, address Anantha's comment first, then make the change in only 1 spot! http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:150: pyauto.CERT_STATUS_AUTHORITY_INVALID).value(), indent the above 2 lines by 1 more space each. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib.i File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:21: %include "cpointer.i" I'm not very familiar with swig syntax. Is there a reason why these includes are specified with quotes, but the ones in lines 22-23 below use angle brackets? http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" swap the above 2 lines to put them in alphabetical order http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:153: "button, if false to 'Take me out of there' button.") indent the above 2 lines by 1 more space each to be consistent with the other lines above. Yeah, I know I'm nit-picky :-P http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:157: %feature("docstring", "Retrieves the different security states for the current tab.") Take a look at how other long docstrings are specified above (e.g., lines 151-153). It looks like separate strings in multiples lines will be automatically concatenated, so you should be able to separate the docstring here into 2 separate lines. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:158: GetSecurityState; this can probably be moved to the line above, like how it's specified in other cases above (e.g., line 145). http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:159: bool GetSecurityState(int *security_style, uint32 *ssl_cert_status, move the * over so that it touches the type: int* uint32* http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:160: int * insecure_content_status) { same comment as line 159 above. Also do the same where * is used down below (lines 161, 162, 168, 169) http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:166: %feature("docstring", "Returns the type of page currently showing (normal, interstitial, error.") Make sure lines 161, 162, and 166 don't exceed 80 chars each. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:168: bool GetPageType(int *OUTPUT) { OUTPUT --> output http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:501: %pointer_class(uint32, uint32p); Just curious: what do these %pointer lines do? Are they required to be down here at the bottom of the file? Should we add a blank line above line 500 to separate the typedef above from these pointer_class lines?
http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib.i File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:27: // NOTE: All files included in this file should also be listed under Heed this. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:159: bool GetSecurityState(int *security_style, uint32 *ssl_cert_status, use int*, not int * (ie, * should go with the type) http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:160: int * insecure_content_status) { int* http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:161: return $self->GetSecurityState(reinterpret_cast<content::SecurityStyle *>(security_style), reduce to 80 chars per line http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:161: return $self->GetSecurityState(reinterpret_cast<content::SecurityStyle *>(security_style), Wouldn't it have been easier to implement the method yourself (like GetPort() below) so that you wouldn't have to deal with all this swig magic. Besides, I'm not sure it's a good idea to expose pointer types to python. BUT, if it works, I won't be totally opposed to your implementation. Just pointing out that it could have been simpler and also easier on the python side. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:500: %pointer_class(int, intp); intp -> int_ptr http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:501: %pointer_class(uint32, uint32p); uint32p -> uint32_ptr
refactored pyautolib.i and the tests. Rewrote GetSecurityState() in pyautolib, so it's more pythonic by returning a tuple consisting of a boolean and a dict rather than having to call it with args. Makes writing tests in pyAuto easier now. http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:105: def testSSLCertIsExpired(self): I re-factored pyautolib.i so the tests have changed. It's now easier to write the tests going forward. I rewrote GetSecurityState() in pyautolib, so it's more pythonic by returning a tuple consisting of a boolean and a dict rather than having to call it with args like now. On 2012/02/29 22:30:02, anantha wrote: > testSSLCertIsExpired and remaining two functions are pretty much doing the same > things except for ServerCertificate names and CertStatus. Can you move the > common code to a separate function? http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:146: security_style, ssl_cert_status, insecure_content_status) Re-wrote the tests. This is now N/A. On 2012/02/29 23:37:13, dennis_jeffrey wrote: > two of the arguments here aren't used. maybe we can just inline the right-hand > sides of lines 142, 144 above into the current line, then delete lines 142, 144? > If you agree to do so, make the same change in the other tests above. Or, > address Anantha's comment first, then make the change in only 1 spot! http://codereview.chromium.org/9535022/diff/1/chrome/test/functional/https.py... chrome/test/functional/https.py:150: pyauto.CERT_STATUS_AUTHORITY_INVALID).value(), On 2012/02/29 23:37:13, dennis_jeffrey wrote: > indent the above 2 lines by 1 more space each. Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib.i File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:21: %include "cpointer.i" It works with angle brackets as well but I just used the quoted form which I had found in the swig documentation. On 2012/02/29 23:37:13, dennis_jeffrey wrote: > I'm not very familiar with swig syntax. Is there a reason why these includes > are specified with quotes, but the ones in lines 22-23 below use angle brackets? http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:27: // NOTE: All files included in this file should also be listed under On 2012/02/29 23:41:08, Nirnimesh wrote: > Heed this. Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" If I swap the two include lines from above I get a compile error. target/pyautolib/geni/pyautolib_wrap.cc', 'test/pyautolib/pyautolib.i'] ../net/base/cert_status_flags.h:50: Error: Syntax error in input(1). make: *** [out/Debug/obj.target/pyautolib/geni/pyautolib_wrap.cc] Error 1 http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:153: "button, if false to 'Take me out of there' button.") On 2012/02/29 23:37:13, dennis_jeffrey wrote: > indent the above 2 lines by 1 more space each to be consistent with the other > lines above. > > Yeah, I know I'm nit-picky :-P Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:157: %feature("docstring", "Retrieves the different security states for the current tab.") On 2012/02/29 22:30:02, anantha wrote: > Indentation doesn't look correct. Can you please check? Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:157: %feature("docstring", "Retrieves the different security states for the current tab.") On 2012/02/29 23:37:13, dennis_jeffrey wrote: > Take a look at how other long docstrings are specified above (e.g., lines > 151-153). It looks like separate strings in multiples lines will be > automatically concatenated, so you should be able to separate the docstring here > into 2 separate lines. Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:158: GetSecurityState; In the examples I see, these are added after the docstring %feature. On 2012/02/29 23:37:13, dennis_jeffrey wrote: > this can probably be moved to the line above, like how it's specified in other > cases above (e.g., line 145). http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:159: bool GetSecurityState(int *security_style, uint32 *ssl_cert_status, The Swig doc actually shows the examples this way. But I'll change it. On 2012/02/29 23:37:13, dennis_jeffrey wrote: > move the * over so that it touches the type: > > int* > uint32* http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:159: bool GetSecurityState(int *security_style, uint32 *ssl_cert_status, On 2012/02/29 23:41:08, Nirnimesh wrote: > use int*, not int * > (ie, * should go with the type) Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:160: int * insecure_content_status) { On 2012/02/29 23:37:13, dennis_jeffrey wrote: > same comment as line 159 above. Also do the same where * is used down below > (lines 161, 162, 168, 169) Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:160: int * insecure_content_status) { On 2012/02/29 23:41:08, Nirnimesh wrote: > int* Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:161: return $self->GetSecurityState(reinterpret_cast<content::SecurityStyle *>(security_style), On 2012/02/29 23:41:08, Nirnimesh wrote: > reduce to 80 chars per line Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:161: return $self->GetSecurityState(reinterpret_cast<content::SecurityStyle *>(security_style), On 2012/02/29 23:41:08, Nirnimesh wrote: > reduce to 80 chars per line Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:161: return $self->GetSecurityState(reinterpret_cast<content::SecurityStyle *>(security_style), This is now done. It wasn't easy to implement as there was a lot of reading to be done about swig and c++ interface with python. It's much more pythonic now and easier to write the tests. GetSecurityState() in pyautolib returns a tuple consisting of a boolean and a dict rather than having to call it with args like before. When writing the tests the function can be called without any args and it returns a tuple consisting of a boolean and a dict like this one: (True, {'security_style': 2, 'ssl_cert_status': 5, 'insecure_content_status': 0}) On 2012/02/29 23:41:08, Nirnimesh wrote: > Wouldn't it have been easier to implement the method yourself (like GetPort() > below) so that you wouldn't have to deal with all this swig magic. Besides, I'm > not sure it's a good idea to expose pointer types to python. > > BUT, if it works, I won't be totally opposed to your implementation. Just > pointing out that it could have been simpler and also easier on the python side. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:166: %feature("docstring", "Returns the type of page currently showing (normal, interstitial, error.") On 2012/02/29 23:37:13, dennis_jeffrey wrote: > Make sure lines 161, 162, and 166 don't exceed 80 chars each. Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:168: bool GetPageType(int *OUTPUT) { On 2012/02/29 23:37:13, dennis_jeffrey wrote: > OUTPUT --> output OUTPUT variable is actually a typemap and it cannot be changed. The swig translates it in a special way and makes it a returning value. Look here: http://www.swig.org/Doc1.3/Arguments.html It says you can use another name by adding this directive: %apply int *OUTPUT { int *output }; but this didn't work in my case. If you change it to "output" it will still compile but the GetPageType won't work correctly. It will expect a pointer arg and all it will return will be a boolean (success or not). http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:500: %pointer_class(int, intp); On 2012/02/29 23:41:08, Nirnimesh wrote: > intp -> int_ptr Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:501: %pointer_class(uint32, uint32p); On 2012/02/29 23:41:08, Nirnimesh wrote: > uint32p -> uint32_ptr Done. http://codereview.chromium.org/9535022/diff/1/chrome/test/pyautolib/pyautolib... chrome/test/pyautolib/pyautolib.i:501: %pointer_class(uint32, uint32p); They are required to dereference constants like this: pyauto.uint32_ptr.frompointer(pyauto.CERT_STATUS_AUTHORITY_INVALID).value() They can be put anywhere. I blanked the previous line. On 2012/02/29 23:37:13, dennis_jeffrey wrote: > Just curious: what do these %pointer lines do? Are they required to be down > here at the bottom of the file? Should we add a blank line above line 500 to > separate the typedef above from these pointer_class lines?
Thanks for making it simpler on the python side. I have a few more comments. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:35: %include "content/public/common/page_type.h" Do you still get a compilation error if you add just this and next line to chrome_tests.gypi? http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:159: PyObject* GetSecurityState() { This should be aligned at regular indent level, not under "docstring" http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:178: return tpl; I don't like the idea of returning a tuple. Why not return an empty dict, or None in case of an error? The |success| field is redundant. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:185: bool GetPageType(int* OUTPUT) { This should be aligned at regular indent level, not under "docstring" http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:186: return $self->GetPageType(reinterpret_cast<content::PageType*>(OUTPUT)); 80+ chars http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:186: return $self->GetPageType(reinterpret_cast<content::PageType*>(OUTPUT)); To get rid of pointers in python, why not implement this function too like the previous one? int GetPageType() { ... } Here again I don't think it's necessary to return a tuple.
http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:35: %include "content/public/common/page_type.h" No, it worked fine. The compile errors were related to another line I had changed. On 2012/03/05 19:46:01, Nirnimesh wrote: > Do you still get a compilation error if you add just this and next line to > chrome_tests.gypi? http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:159: PyObject* GetSecurityState() { On 2012/03/05 19:46:01, Nirnimesh wrote: > This should be aligned at regular indent level, not under "docstring" Done. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:178: return tpl; On 2012/03/05 19:46:01, Nirnimesh wrote: > I don't like the idea of returning a tuple. Why not return an empty dict, or > None in case of an error? The |success| field is redundant. Done. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:185: bool GetPageType(int* OUTPUT) { On 2012/03/05 19:46:01, Nirnimesh wrote: > This should be aligned at regular indent level, not under "docstring" Done. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:186: return $self->GetPageType(reinterpret_cast<content::PageType*>(OUTPUT)); On 2012/03/05 19:46:01, Nirnimesh wrote: > 80+ chars Done. http://codereview.chromium.org/9535022/diff/7001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyautolib.i:186: return $self->GetPageType(reinterpret_cast<content::PageType*>(OUTPUT)); On 2012/03/05 19:46:01, Nirnimesh wrote: > To get rid of pointers in python, why not implement this function too like the > previous one? > > int GetPageType() { > ... > } > Here again I don't think it's necessary to return a tuple. Done.
Hey Nirnimesh, if you get some time while on your trip can you have a look at the changes and suggest anything else that may need to be changed? If you have no time I can include frankf into this CL for his suggestions.
Looping in Frank if Nirnimesh can't review.
http://codereview.chromium.org/9535022/diff/12001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9535022/diff/12001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:19: '../net/base/cert_status_flags.h', nit: swap the above 2 lines to maintain alphabetical order http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. 2011 --> 2012 http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:94: """Verify Certificate OK does not display interstitial page.""" This test is doing more than that; it's actually asserting that the page type is "normal" http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:101: msg="Cert OK displayed interstitial page.") Isn't this assertion redundant? You're already asserting in the next line below that the page type is a particular type. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:103: msg="Cert OK displayed error page.") Change the error message to say that the page type is not "normal" as expected, then also print the actual page type identified. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:111: ('Cert has not expired', 'Cert name does not mismatch')): nit: I recommend this indentation: for server, cert_status_flag, msg in zip( (self._https_server_expired, self._https_server_mismatched), (pyauto.CERT_STATUS_DATE_INVALID, pyauto.CERT_STATUS_COMMON_NAME_INVALID), ('Cert has not expired', 'Cert name does not mismatch')): http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:117: cert_status_flag).value(), msg=msg) nit: move the msg=msg down to the next line, and indent it to line up underneath the "result_dict" in line 116 above: self.assertTrue( result_dict['ssl_cert_status'] & pyauto.uint32_ptr.frompointer( cert_status_flag).value(), msg=msg) http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:23: %include <std_string.i> nit: put the above 4 lines in alphabetical order http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" nit: swap the above 2 lines to maintain alphabetical order http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:165: &security_style, &ssl_cert_status, &insecure_content_status)){ add a space before the { http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:166: PyObject* result_dict = PyDict_New(); move this definition above the loop, then remove line 173 below, then change line 175 to "return result_dict;" http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:171: PyDict_SetItem(result_dict, PyString_FromString("insecure_content_status"), make sure this line doesn't exceed 80 chars http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:172: PyInt_FromLong(insecure_content_status)); Indent lines 167 - 172 by 2 more spaces each http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:184: if ($self->GetPageType(&page_type)){ add a space before the { http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:185: PyObject* result_dict = PyDict_New(); move this line to above the loop, then remove line 188 below, then change line 190 to "return result_dict;" http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:186: PyDict_SetItem(result_dict, PyString_FromString("page_type"), indent this line by 2 more spaces http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:187: PyInt_FromLong(page_type)); indent this further to line up underneath the first argument in the previous line
http://codereview.chromium.org/9535022/diff/12001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9535022/diff/12001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:19: '../net/base/cert_status_flags.h', This is not by accident. The ordering needs to be this way or there will be a compile error. On 2012/03/09 02:07:47, dennis_jeffrey wrote: > nit: swap the above 2 lines to maintain alphabetical order http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/03/09 02:07:47, dennis_jeffrey wrote: > 2011 --> 2012 Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:94: """Verify Certificate OK does not display interstitial page.""" On 2012/03/09 02:07:47, dennis_jeffrey wrote: > This test is doing more than that; it's actually asserting that the page type is > "normal" Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:101: msg="Cert OK displayed interstitial page.") Well one is NotEqual and one is Equal. The errors are different and needs to be checked which interstitial type is actually showing up. On 2012/03/09 02:07:47, dennis_jeffrey wrote: > Isn't this assertion redundant? You're already asserting in the next line below > that the page type is a particular type. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:103: msg="Cert OK displayed error page.") If it's not a normal page it will display and error page. Should I just say "Cert OK displayed error page. Should display a normal page." ? On 2012/03/09 02:07:47, dennis_jeffrey wrote: > Change the error message to say that the page type is not "normal" as expected, > then also print the actual page type identified. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:111: ('Cert has not expired', 'Cert name does not mismatch')): On 2012/03/09 02:07:47, dennis_jeffrey wrote: > nit: I recommend this indentation: > > for server, cert_status_flag, msg in zip( > (self._https_server_expired, self._https_server_mismatched), > (pyauto.CERT_STATUS_DATE_INVALID, > pyauto.CERT_STATUS_COMMON_NAME_INVALID), > ('Cert has not expired', 'Cert name does not mismatch')): Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:117: cert_status_flag).value(), msg=msg) On 2012/03/09 02:07:47, dennis_jeffrey wrote: > nit: move the msg=msg down to the next line, and indent it to line up underneath > the "result_dict" in line 116 above: > > self.assertTrue( > result_dict['ssl_cert_status'] & pyauto.uint32_ptr.frompointer( > cert_status_flag).value(), > msg=msg) Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:23: %include <std_string.i> On 2012/03/09 02:07:47, dennis_jeffrey wrote: > nit: put the above 4 lines in alphabetical order Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" The ordering is correct. If I swap it there will be a compile error. On 2012/03/09 02:07:47, dennis_jeffrey wrote: > nit: swap the above 2 lines to maintain alphabetical order http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:165: &security_style, &ssl_cert_status, &insecure_content_status)){ On 2012/03/09 02:07:47, dennis_jeffrey wrote: > add a space before the { Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:166: PyObject* result_dict = PyDict_New(); On 2012/03/09 02:07:47, dennis_jeffrey wrote: > move this definition above the loop, then remove line 173 below, then change > line 175 to "return result_dict;" Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:171: PyDict_SetItem(result_dict, PyString_FromString("insecure_content_status"), On 2012/03/09 02:07:47, dennis_jeffrey wrote: > make sure this line doesn't exceed 80 chars Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:172: PyInt_FromLong(insecure_content_status)); On 2012/03/09 02:07:47, dennis_jeffrey wrote: > Indent lines 167 - 172 by 2 more spaces each Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:184: if ($self->GetPageType(&page_type)){ On 2012/03/09 02:07:47, dennis_jeffrey wrote: > add a space before the { Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:185: PyObject* result_dict = PyDict_New(); On 2012/03/09 02:07:47, dennis_jeffrey wrote: > move this line to above the loop, then remove line 188 below, then change line > 190 to "return result_dict;" Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:186: PyDict_SetItem(result_dict, PyString_FromString("page_type"), On 2012/03/09 02:07:47, dennis_jeffrey wrote: > indent this line by 2 more spaces Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:187: PyInt_FromLong(page_type)); On 2012/03/09 02:07:47, dennis_jeffrey wrote: > indent this further to line up underneath the first argument in the previous > line Done.
few more comments http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:101: msg="Cert OK displayed interstitial page.") On 2012/03/14 19:00:10, dyu1 wrote: > Well one is NotEqual and one is Equal. The errors are different and needs to be > checked which interstitial type is actually showing up. > > On 2012/03/09 02:07:47, dennis_jeffrey wrote: > > Isn't this assertion redundant? You're already asserting in the next line > below > > that the page type is a particular type. > I still don't understand why both assertions are needed. If we only assert that the page type is NORMAL, and the assertion passes, then doesn't that already imply that the page type is neither ERROR nor INTERSTITIAL? http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:103: msg="Cert OK displayed error page.") On 2012/03/14 19:00:10, dyu1 wrote: > If it's not a normal page it will display and error page. Should I just say > "Cert OK displayed error page. Should display a normal page." ? > > On 2012/03/09 02:07:47, dennis_jeffrey wrote: > > Change the error message to say that the page type is not "normal" as > expected, > > then also print the actual page type identified. > You're right that if it's not a normal page, it will be the error page. But that's only because we've asserted above that the page type is not INTERSTITIAL. And it implicitly assumes that there are only 3 page types: NORMAL, ERROR, or INTERSTITAL. If another page type is added later, this implicit assumption will be invalid. I think a cleaner solution is to only have 1 assertion: assert that the page type is normal. If that assertion fails, we should include in the error message which page type it actually was. http://codereview.chromium.org/9535022/diff/21001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9535022/diff/21001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:19: '../net/base/cert_status_flags.h', Since the order here is important, maybe we should add a comment to make sure the dependency here is made explicit? http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:22: %include <std_string.i> nit: swap above 2 lines for alphabetical order (assuming order doesn't matter) http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" If order is important here, maybe we should have a comment to state this explicitly so people know that? http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:173: PyInt_FromLong(insecure_content_status)); recommend this indentation: PyDict_SetItem(result_dict, PyString_FromString("insecure_content_status"), PyInt_FromLong(insecure_content_status)); http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:182: PyObject* GetPageType(){ add a space before the {
http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:101: msg="Cert OK displayed interstitial page.") On 2012/03/14 21:55:26, dennis_jeffrey wrote: > On 2012/03/14 19:00:10, dyu1 wrote: > > Well one is NotEqual and one is Equal. The errors are different and needs to > be > > checked which interstitial type is actually showing up. > > > > On 2012/03/09 02:07:47, dennis_jeffrey wrote: > > > Isn't this assertion redundant? You're already asserting in the next line > > below > > > that the page type is a particular type. > > > > I still don't understand why both assertions are needed. If we only assert that > the page type is NORMAL, and the assertion passes, then doesn't that already > imply that the page type is neither ERROR nor INTERSTITIAL? Done. http://codereview.chromium.org/9535022/diff/12001/chrome/test/functional/http... chrome/test/functional/https.py:103: msg="Cert OK displayed error page.") On 2012/03/14 21:55:26, dennis_jeffrey wrote: > On 2012/03/14 19:00:10, dyu1 wrote: > > If it's not a normal page it will display and error page. Should I just say > > "Cert OK displayed error page. Should display a normal page." ? > > > > On 2012/03/09 02:07:47, dennis_jeffrey wrote: > > > Change the error message to say that the page type is not "normal" as > > expected, > > > then also print the actual page type identified. > > > > You're right that if it's not a normal page, it will be the error page. But > that's only because we've asserted above that the page type is not INTERSTITIAL. > And it implicitly assumes that there are only 3 page types: NORMAL, ERROR, or > INTERSTITAL. If another page type is added later, this implicit assumption will > be invalid. I think a cleaner solution is to only have 1 assertion: assert that > the page type is normal. If that assertion fails, we should include in the > error message which page type it actually was. Done. http://codereview.chromium.org/9535022/diff/21001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9535022/diff/21001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:19: '../net/base/cert_status_flags.h', On 2012/03/14 21:55:27, dennis_jeffrey wrote: > Since the order here is important, maybe we should add a comment to make sure > the dependency here is made explicit? Done. http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... File chrome/test/pyautolib/pyautolib.i (right): http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:22: %include <std_string.i> On 2012/03/14 21:55:27, dennis_jeffrey wrote: > nit: swap above 2 lines for alphabetical order (assuming order doesn't matter) Done. http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:38: %include "net/base/cert_status_flags.h" On 2012/03/14 21:55:27, dennis_jeffrey wrote: > If order is important here, maybe we should have a comment to state this > explicitly so people know that? Done. http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:173: PyInt_FromLong(insecure_content_status)); On 2012/03/14 21:55:27, dennis_jeffrey wrote: > recommend this indentation: > > PyDict_SetItem(result_dict, > PyString_FromString("insecure_content_status"), > PyInt_FromLong(insecure_content_status)); Done. http://codereview.chromium.org/9535022/diff/21001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyautolib.i:182: PyObject* GetPageType(){ On 2012/03/14 21:55:27, dennis_jeffrey wrote: > add a space before the { Done.
LGTM with 1 nit. Thanks! http://codereview.chromium.org/9535022/diff/25002/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/25002/chrome/test/functional/http... chrome/test/functional/https.py:105: msg="Cert OK displayed error page %s." % result_dict['page_type']) nit: 'error page' --> 'page type'
http://codereview.chromium.org/9535022/diff/25002/chrome/test/functional/http... File chrome/test/functional/https.py (right): http://codereview.chromium.org/9535022/diff/25002/chrome/test/functional/http... chrome/test/functional/https.py:105: msg="Cert OK displayed error page %s." % result_dict['page_type']) On 2012/03/14 22:51:21, dennis_jeffrey wrote: > nit: 'error page' --> 'page type' Done.
I don't see any tryjobs after patchset 3. Please run tryjobs or use the commit-queue. As it is, this change broke the commit queue, as the 'win' trybot (which builds all targets as a shared library) was no longer able to link pyautolib. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Creating library ..\build\Debug\lib\_pyautolib.lib and object ..\build\Debug\lib\_pyautolib.exp pyautolib_wrap.obj :error LNK2019: unresolved external symbol "unsigned int __cdecl net::MapNetErrorToCertStatus(int)" (?MapNetErrorToCertStatus@net@@YAIH@Z) referenced in function __wrap_MapNetErrorToCertStatus ..\build\Debug\_pyautolib.pyd : fatalerror LNK1120: 1 unresolved externals |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
