Issue 5171515343503360: Issue #41 - Bring method of determining IE version up to date (Closed)
DescriptionAdded retrieval functions for the installed IE version, both for the full
version string as well as the numeric major version. Implemented class
Registry_Key. Added unit tests for these.
Removed CAdblockPlusClient::GetIEVersion(), replacing it with the new IE
major version function.
Populate the 'AppInfo::applicationVersion' field. This fixes
https://issues.adblockplus.org/ticket/41
Kept new registry and version functions in their own files. This is in
preparation to use this code as part of the changes needed by
https://issues.adblockplus.org/ticket/404
Patch Set 1 #
Total comments: 13
Patch Set 2 : Second version #
Total comments: 15
Patch Set 3 : #
Total comments: 8
Patch Set 4 : reformatted #Patch Set 5 : simplify Registry_Key #
Total comments: 80
Patch Set 6 : #
Total comments: 1
Patch Set 7 : #
Total comments: 8
Patch Set 8 : Final (?) #
Total comments: 2
Patch Set 9 : Final (?) 2 #
Total comments: 6
MessagesTotal messages: 32
http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:133: { Registry_Key should be used to refactor use of the registry to manipulate the status bar. It would need to support assignment (write to registry) in order to do that. That's a task for later.
http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (left): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:670: if (CPluginClient::GetInstance()->GetIEVersion() > 6) IIRC IE6 was not starting at all if these checks weren't in place. Are you removing IE6 check here just because we no longer support it? I don't see any point in removing the support if we already have it. Maybe we shouldn't work on any new issues, but why remove this? http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/plugin/PluginWbPassThrough.cpp (left): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/plugin/PluginWbPassThrough.cpp:109: if (mimeType.empty() && domain.empty() && CPluginClient::GetInstance()->GetIEVersion() >= 8) The means we would falsely classify requests as XmlHttpRequest on IE6 and IE7. I'm in favor of leaving this and below checks http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:142: HKEY _key; Nit: Either m_key, as have in other parts of code, or just key without underline, since we're not using Hungarian notation http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:169: Registry_Key( Predefined root, std::wstring key_name ); Nit: no spaces after and before the parentheses here and further in this file http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:187: inline HKEY hkey_of_prefined( Predefined root ) I guess should've been hkey_of_predefined? http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:189: static HKEY predefined_hkeys[ 5 ] = Nit: no spaces before and after brackets here and further in this file
Please note that I am no longer reviewing IE changes - fhd should be the reviewer here. http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:290: throw std::runtime_error( "IE version string has unexpected format" ); So, we crash and burn if the we see an unexpected version format? I don't think the callers are handling this exception, meaning that it will actually terminate the application. If catching this exception in the callers is too complicated, maybe we should just return some default value like 0 or -1 here?
On 2014/06/23 06:54:49, Wladimir Palant wrote: > Please note that I am no longer reviewing IE changes - fhd should be the > reviewer here. I know. I put up this review just a couple days before I saw the change announcement on Monday.
Wladimir, I've added Felix and I'll take you off right after this mail goes out. http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (left): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:670: if (CPluginClient::GetInstance()->GetIEVersion() > 6) On 2014/06/22 21:53:17, Oleksandr wrote: > Are you > removing IE6 check here just because we no longer support it? Exactly. You and I spoke about this, and it became clear that we ought to land a number of changes simultaneously to do this right, so I'll leave in the version changes for now. http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:142: HKEY _key; On 2014/06/22 21:53:17, Oleksandr wrote: > Nit: Either m_key, as have in other parts of code, or just key without > underline, since we're not using Hungarian notation Done. http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:187: inline HKEY hkey_of_prefined( Predefined root ) On 2014/06/22 21:53:17, Oleksandr wrote: > I guess should've been hkey_of_predefined? Yes. Derp. Done. http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:290: throw std::runtime_error( "IE version string has unexpected format" ); On 2014/06/23 06:54:49, Wladimir Palant wrote: > I don't think > the callers are handling this exception, meaning that it will actually terminate > the application. IE_version_string() is only called (at present) by the two functions immediately below, which do exactly as you suggest, returning default values of L"" and 0 respectively if they catch an exception.
OK. I've uploaded a whole new version of the same review. (1) Felix is now on the review. (2) I reverted some changes and simply replaced the old function in the check rather than removing the check entirely. This is much easier to read.
http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5629499534213120/src/... src/shared/Utils.cpp:189: static HKEY predefined_hkeys[ 5 ] = It's fixed here, but not everywhere else :) On 2014/06/22 21:53:17, Oleksandr wrote: > Nit: no spaces before and after brackets here and further in this file http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/engine/Main.cpp:377: appInfo.applicationVersion = ToUtf8String( ABP::IE::installed_version_string() ); STYLE NIT: no whitespace after and before the parentheses http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:137: class Registry_Key I'd vote to move class definition to Utils.h and leave only implementation here. Or better yet to have something like Registry.h and Registry.cpp. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:156: : unsigned int I don't understand the concept behind this class. Can't we just pass stuff like HKEY_CLASSES_ROOT to our functions directly? Why do we need to map these constants to our enum? http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:233: throw std::runtime_error( "Value is not string type" ); Just returning an empty string and logging the error would be fine in "exceptional" cases here, IMHO. That's what we do in the end anyway. This would also eliminate all the try-catch blocks. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:245: std::unique_ptr< wchar_t[] > p( new wchar_t[ psize ] ); std::wstring p; p.resize(psize); RegQueryValueExW(...., &p[0],...) should work just as fine http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:276: */ I think it's better to remove most of the comments in this file. What's going on in the code should be self evident, and if it's not - one should just double check against the documentation. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:318: catch ( ... ) Catching specifically std::runtime_error would make more sense here
http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:137: class Registry_Key On 2014/07/15 18:25:11, Oleksandr wrote: > I'd vote to move class definition to Utils.h and leave only implementation here. We actually need a more radical organization than that. For issue 404 "Validate Internet Explorer version in the installer", we should have this code in a library shared both by the plugin/engine and the installer. We don't have a place for that yet. https://issues.adblockplus.org/ticket/404 I'd recommend a second change set where we alter the build system as well as move this code into its own source files. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:156: : unsigned int On 2014/07/15 18:25:11, Oleksandr wrote: > I don't understand the concept behind this class. Can't we just pass stuff like > HKEY_CLASSES_ROOT to our functions directly? Why do we need to map these > constants to our enum? The way the Windows registry API works, you must always pass some predefined registry key to the API. The enum isolates a dependency on having the symbols floating around to an implementation class. It doesn't matter for the narrow purpose in this class, but when we move the code to act as more of a library function, we don't need to incur a header dependency defining and of the predefined keys. Alternately, we could change the constructor signature to accept another 'Registry_Key' and then predefine some static class members. That seems less efficient. The other alternative, using an HKEY in the constructor signature, doesn't accomplish the goal of isolating header dependencies to the implementation. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:233: throw std::runtime_error( "Value is not string type" ); As a rule, eliminating exception handling shouldn't be a goal in itself. In the present case, the try-catch statements in the version functions below could be eliminated _if_ we had exception handlers at all the entry points already. We don't, so they're in the code below as a safety net. As for this particular exception, unless IE is malfunctioning reporting its own version number or something has been playing in the registry, this condition should never be hit. That's exactly what exceptions are for. Logging the exception can happen once in the code, at the highest level, rather than having it sprinkled throughout the code base. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:245: std::unique_ptr< wchar_t[] > p( new wchar_t[ psize ] ); On 2014/07/15 18:25:11, Oleksandr wrote: > should work just as fine Except that string buffers are 'const' and you're not supposed to write into them. Even if it might not cause problems at the present, it's still a violation of contract and may cause problems in the future. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:276: */ On 2014/07/15 18:25:11, Oleksandr wrote: > one should just double > check against the documentation. Well, that's what the problem is, that the documentation for this isn't exactly a quick read, nor is it completely clear. Once you know what's going on, it's clear enough, but that's not the standard of judgement for whether a comment is needed. Also, this comment is in a Doxygen header, since it's part of the specification of the function itself, rather than just an implementation detail. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:318: catch ( ... ) On 2014/07/15 18:25:11, Oleksandr wrote: > Catching specifically std::runtime_error would make more sense here I referred to this 'catch' block above. If we had proper exception handling throughout the code, I'd just eliminate the try-catch block here in its entirety (as well as the one below). The user of this function is the one that has to decide how it's going to behave differently if there's a failure in retrieving the version number.
Rebased to current head. Added Sergei.
http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:156: : unsigned int I think this is an overkill. I'd be ok if we assumed that the caller has HKEY_CLASSES_ROOT and stuff defined. It gets defined by windows.h after all. Let's get a third opinion here. @Sergei? On 2014/07/20 18:18:46, Eric wrote: > On 2014/07/15 18:25:11, Oleksandr wrote: > > I don't understand the concept behind this class. Can't we just pass stuff > like > > HKEY_CLASSES_ROOT to our functions directly? Why do we need to map these > > constants to our enum? > > The way the Windows registry API works, you must always pass some predefined > registry key to the API. The enum isolates a dependency on having the symbols > floating around to an implementation class. It doesn't matter for the narrow > purpose in this class, but when we move the code to act as more of a library > function, we don't need to incur a header dependency defining and of the > predefined keys. > > Alternately, we could change the constructor signature to accept another > 'Registry_Key' and then predefine some static class members. That seems less > efficient. The other alternative, using an HKEY in the constructor signature, > doesn't accomplish the goal of isolating header dependencies to the > implementation. http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String( AdblockPlus::IE::installed_version_string() ); Whitespace remained here http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/IE_version.cpp:21: Registry_Key IE_key( Registry_Key::Predefined::HKLM, L"Software\\Microsoft\\Internet Explorer" ); Spacing before and after parentheses etc in this whole file http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/IE_version.h (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/IE_version.h:5: namespace IE I don't think we actually need any of these namespaces. http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/Registry.cpp:15: Registry_Key::Registry_Key( Predefined root, std::wstring key_name ) Formatting in this file is too spacious as well http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/Registry.h (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/Registry.h:9: class Registry_Key Formatting is incorrect in this file as well http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/test... test/RegistryTest.cpp:34: TEST( Registry_Test, roots_0 ) Formatting in this file is incorrect as well
Reformatted code only in patch set 4. Rework of Registry_Key constructor to follow. http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5676830073815040/src/... src/shared/Utils.cpp:156: : unsigned int On 2014/07/25 22:23:46, Oleksandr wrote: > I think this is an overkill. I'd be ok if we assumed that the caller has > HKEY_CLASSES_ROOT and stuff defined. It gets defined by windows.h after all. I'm inclined to agree with you for practical reasons. The current implementation doesn't even satisfy my own criteria for eliminating the need to include windows.h. The trouble is HKEY, which is declared a pointer to an otherwise-unused class. I tried using void * instead, but I was getting some errors with type conversions, which I'm pretty sure it were defects in VS 2012. Whatever. I'm giving up on perfect encapsulation. I'll do a rework in a change set to follow the present one on formatting. http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/IE_version.h (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/IE_version.h:5: namespace IE On 2014/07/25 22:23:46, Oleksandr wrote: > I don't think we actually need any of these namespaces. 1) I'm in favor of using AdblockPlus for the namespace for library code. The registry and IE version facilities are definitely in that category. 2) The 'IE' namespace is what I plan on using for all IE support. As of this moment it's only these two functions. I also intend to use it for all the IE wrapper classes. And it will definitely be a good idea to have a namespace for those, because there are lots of generic words there that we should preemptively disambiguate. These wrapper classes are not merely cosmetic. I'm fairly sure that we can fix #264 (ABP attributes are added to every element in contentEditable elements) with some COM tricks and a wrapper class around IHTMLElement.
Almost there, it seems! http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... File src/shared/IE_version.h (right): http://codereview.adblockplus.org/5171515343503360/diff/5689792285114368/src/... src/shared/IE_version.h:5: namespace IE It does make sense. We might start doing that. @sergei, would you agree on the naming? On 2014/07/26 15:54:46, Eric wrote: > On 2014/07/25 22:23:46, Oleksandr wrote: > > I don't think we actually need any of these namespaces. > > 1) I'm in favor of using AdblockPlus for the namespace for library code. The > registry and IE version facilities are definitely in that category. > > 2) The 'IE' namespace is what I plan on using for all IE support. As of this > moment it's only these two functions. I also intend to use it for all the IE > wrapper classes. And it will definitely be a good idea to have a namespace for > those, because there are lots of generic words there that we should preemptively > disambiguate. > > These wrapper classes are not merely cosmetic. I'm fairly sure that we can fix > #264 (ABP attributes are added to every element in contentEditable elements) > with some COM tricks and a wrapper class around IHTMLElement. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:70: if (version[ 1 ] == L'.') Nit: The array index should not be enclosed in whitespaces as well. Just version[1] is fine. Below as well. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:64: if (p[ psize - 1 ] == L'\0') Nit: No spaces before and after array index: if (p[psize - 1] == L'\0') http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.h (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:19: */ Extensive comments make the code hard to read, IMHO. This file is a prime example of that, but in other files I would remove almost all of comments as well. Even though I am at awe at your willingness to explain everything in writing :) But I won't insist here, unless others agree. @sergei, @fhd? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:88: ASSERT_EQ(version, INSTALLED_IE_VERSION); What if we just tested here for a range of values instead? Like: ASSERT_LT(version, 12) ASSERT_GE(version, 6) This will be a decent check on it's own, IMHO. It will eliminate that explicit define above and it will also automatically remind us to test the IE version routine when IE 12 goes public. Same for the version string test.
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); Why do we need it here? The engine should not know about IE anything. If it's useful for the log, then pass User-Agent in the first message (pipe) while establishing the connection with the engine. Even without digging deeper I'm pretty sure `appInfo.applicationVersion` denotes the version of the current application, thus engine. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/plugin/PluginWbPassThrough.cpp:99: if (mimeType.IsEmpty() && domain.IsEmpty() && AdblockPlus::IE::installed_major_version() >= 8) Just wonder, what happens if we throw an exception while the execution in some callback of `WBPassthruSink`? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" I don't think it should be in shared, because it's used only by the plugin. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:17: * Warning: IE version 12 and later might not behave the same, so this function should be reviewed with each new release. Tests should run on all machines with all IE versions. If there is something unknown, then the test fails and we can fix it. It's impossible to handle such information in the head and to review such peaces of the code each time. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:24: * We're expecting a version string that matches the regex "^[1-9]\.". It's also mentioned in tests, why do we need the point at the end? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') strictly speaking we have to check the length of `version`. What if it's zero length? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:50: std::wstring AdblockPlus::IE::installed_version_string() What is the aim of this method? I think we can get rid of it. If we need it often that let's change the original method to return an empty string in case of problem. It might be useful to add std::error_code http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:70: if (version[ 1 ] == L'.') I would like to check the size. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:79: if (version[ 0 ] < L'0' || version[ 0 ] > L'9' || version[ 1 ] < L'0' || version[ 1 ] > L'9' || version[ 2 ] != L'.') Is it really better than atoi? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.h (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.h:1: #include <string> Where is the guard? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.h:7: std::wstring installed_version_string(); That's not our convention, InstalledVesionAsString http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); It's not descriptive. Is it possible to find a root of problem if there is a long list or only several entries of "Failure in RegQueryValueEx to query name" in the log? Include at least the `name` in the message, as well as the error code. For now it will be better to facilitate system_error, despite `std::system_category` is not properly implemented by MS and we need to have our own impl. It will be easier to find and fix all cases just looking for `std::system_category`. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:38: throw std::runtime_error("Value is not string type"); The same as above, it's completely not descriptive. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[ psize ]); not sure about it, One one hand it's correct, but I would like to declare `std::wstring retValue(psize, L'\0');` here. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.h (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:1: #include <string> Where is the guard? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:19: */ I find this particular comment and the comment for the ctr to be useful, because they describe the peculiarities of the class behaviour. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:20: class Registry_Key remove underscore http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:23: * Handle to registry key that is open and not predefined. But, for example, this comment and the comment for the dtr are not necessary. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:42: Registry_Key(HKEY parent, std::wstring key_name); - const references - keyName http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:55: std::wstring value_wstring(std::wstring name) const; To be consistent with the rest code, it's better to name it `StringValue` http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test It's not needed, remove it. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:34: TEST(Registry_Test, simple_0) What does `simple_0` mean? Call it like `RegistryKeyTest, ctr_should_successfully_open_CLSID` http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:39: TEST(Registry_Test, constructor_illegal_argument_0) `ctr_should_throw_exception_if_key_is_empty` http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:44: TEST(Registry_Test, value_notfound_0) StringValue_should_throw_if_value_not_found http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:50: //---------------------------------- Move it into another cpp file http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:54: class IE_Version_Test It's not needed http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:65: #define expected_version_string L"11." Why do we need the point at the end? http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:88: ASSERT_EQ(version, INSTALLED_IE_VERSION); Agree about the range, get rid of the hacks `perform_version_test`
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:70: if (version[ 1 ] == L'.') On 2014/07/27 22:13:30, Oleksandr wrote: > Nit: The array index should not be enclosed in whitespaces as well. Just > version[1] is fine. Below as well. I reformatted this code with Artistic Style. (It's standalone, unlike the Eclipse formatter.) It appears that it doesn't handle white space around square brackets. Bah. http://astyle.sourceforge.net/ http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.h (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.h:19: */ These comments are formatted for consumption by Doxygen. It would help to see the Doxygen output to get an idea what it looks like. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:88: ASSERT_EQ(version, INSTALLED_IE_VERSION); On 2014/07/27 22:13:30, Oleksandr wrote: > What if we just tested here for a range of values instead? Like: > ASSERT_LT(version, 12) > ASSERT_GE(version, 6) The problem is that we shouldn't make many assumptions about the installation state of this test suite. For developers, sure, we can assume they have the current version. On the other hand, if/when we ever get to automated testing on older platforms, say XP, we may get stuck with an older version of IE. I thought about a number of ways of dealing with this problem. The previous version commented out this check, performing just the sanity check. I settled on putting the version number in a preprocessor definition, allowing it to be passed on the command line. This will support automation at a future point and also provide a decent default for developers.
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); On 2014/07/28 11:46:27, sergei wrote: > Why do we need it here? The engine should not know about IE anything. This assignment statement is the core of issue #41. Your comment is appropriate to differing with the goal itself, but not to its solution. https://issues.adblockplus.org/ticket/41 http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/plugin/PluginWbPassThrough.cpp:99: if (mimeType.IsEmpty() && domain.IsEmpty() && AdblockPlus::IE::installed_major_version() >= 8) On 2014/07/28 11:46:27, sergei wrote: > Just wonder, what happens if we throw an exception while the execution in some > callback of `WBPassthruSink`? As of today, the exception will propagate up through an entry point, with undefined behavior. It's the main reason we need to get try/catch(...) in place throughout the code. We might already have had failures in the field from something throwing bad_alloc. Instead of altering all the PassthroughAPP code right now, the version functions don't throw at all, but rather convert exceptions into default values. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" On 2014/07/28 11:46:27, sergei wrote: > I don't think it should be in shared, because it's used only by the plugin. See #404. This code is destined to move again soon. https://issues.adblockplus.org/ticket/404 http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:17: * Warning: IE version 12 and later might not behave the same, so this function should be reviewed with each new release. On 2014/07/28 11:46:27, sergei wrote: > Tests should run on all machines with all IE versions. If that's the case, the best you can get is a sanity check. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:24: * We're expecting a version string that matches the regex "^[1-9]\.". On 2014/07/28 11:46:27, sergei wrote: > It's also mentioned in tests, why do we need the point at the end? We're validating the format of the version string. This isn't anywhere near the most stringent format test we could perform. Microsoft version numbers are consistently dotted triples or quads. A full regex match seems overkill here. The vast majority of cases we'll see are either (a) no IE or (b) valid IE. A corrupt registry key is a pretty rare case, so a simple test will suffice. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') On 2014/07/28 11:46:27, sergei wrote: > strictly speaking we have to check the length of `version`. What if it's zero > length? You're right; this is a defect, since if the registry key is corrupt, the most likely way it will be is to have an empty value. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:50: std::wstring AdblockPlus::IE::installed_version_string() On 2014/07/28 11:46:27, sergei wrote: > What is the aim of this method? It's called in engine/Main.cpp to fix #41. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:70: if (version[ 1 ] == L'.') On 2014/07/28 11:46:27, sergei wrote: > I would like to check the size. Yes; same kind of defect as above. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:79: if (version[ 0 ] < L'0' || version[ 0 ] > L'9' || version[ 1 ] < L'0' || version[ 1 ] > L'9' || version[ 2 ] != L'.') On 2014/07/28 11:46:27, sergei wrote: > Is it really better than atoi? The condition is validating the format, which we need to do anyway. The return statement is doing the conversion. We could use atoi for it, but we'd need to truncate the string first. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.h (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.h:1: #include <string> On 2014/07/28 11:46:27, sergei wrote: > Where is the guard? Yep. It needs one. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); On 2014/07/28 11:46:27, sergei wrote: > It's not descriptive. No, it's not, and it doesn't really matter at the moment. The propagation exceptions here are all truncated in the interface functions and their content dropped. So it matters little what these exceptions are at the moment. The main reason I didn't just use std::exception() is to provide a bit of documentation for the debugger. It would be premature at this moment to over-engineer these exceptions. We need three things in place to make the worthwhile: (1) try-catch{...) at all entry points (2) consistent behavior in the catch-all blocks to at least log the exception (including field-installed Release configurations), and (3) our own exception classes that provide consistent information. When we've got these, it will straightforward to update all the exceptions in the code, including these. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[ psize ]); On 2014/07/28 11:46:27, sergei wrote: > not sure about it, One one hand it's correct, but I would like to declare > `std::wstring retValue(psize, L'\0');` here. We need an allocated buffer, because writing into buffers declared const violates class invariants. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test On 2014/07/28 11:46:27, sergei wrote: > It's not needed, remove it. Explicit class declarations are required for command-line option --gtest_filter works well. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:50: //---------------------------------- On 2014/07/28 11:46:27, sergei wrote: > Move it into another cpp file At least for now, that's overkill. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:65: #define expected_version_string L"11." On 2014/07/28 11:46:27, sergei wrote: > Why do we need the point at the end? It's the last character of the longest initial substring guaranteed to be shared by all version strings.
If it's related to some issue from issues.adblockplus.org it should be pointed here.
On 2014/07/29 13:06:48, sergei wrote: > If it's related to some issue from http://issues.adblockplus.org it should be pointed > here. Did you read the review description? There are two such links there.
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); Still disagree here. Why do we need the current solution, if it's not good and not what we really need? It will raise again, let's do it correctly at the first round. Let's move the discussion to issues.adblockplus.org, I've updated the ticket. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/plugin/PluginWbPassThrough.cpp:99: if (mimeType.IsEmpty() && domain.IsEmpty() && AdblockPlus::IE::installed_major_version() >= 8) I cannot say, that I'm happy having such call here - it's not clear here that this function does not throw any exception. At the same time similar functions does throw and the hell of calls (they are even from different threads) in WbPassThrough looks very fragile, so to have at least something potentially harmful here looks very dangerous. - try{}catch(...) looses the information about the problem. - I expect this method to be called several times, and to read the version each time from the windows registry looks very inefficient. We might should create a ticket for that to inject the version in the constructor and use a constant access time value without any possibility for the exception, at least here. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" Clear, it should be here. But why will it be moved? Let's link with the shared library (which sounds absolutely awfully, because it's a static library and 'shared' is the name. Originally proposed 'utils' is much better). http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') How can you make an assumption that the most likely it's the empty string? I would like to say, that it can be any arbitrary string, which depends on the fantasy of the author of the next malware. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:50: std::wstring AdblockPlus::IE::installed_version_string() If it's only the place, then do try-catch where it's called, as well I expect an opportunity there to dump the error description into the log. Currently that information is lost. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:79: if (version[ 0 ] < L'0' || version[ 0 ] > L'9' || version[ 1 ] < L'0' || version[ 1 ] > L'9' || version[ 2 ] != L'.') We don't need to truncate it, anyway it looks good - only the check for the length is required - it might be an overkill, but to have something like `int GetMajorNumber(const std::string& version);` allows to easily test this method. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); It does always matter. - There is no real reason to drop the error message. - It is very useful for postmortem investigation as well as for everyday debugging. It allows to catch problem earlier. - if we use the unified and regular error handling there is no problem or difficulties with it, except the laziness. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[ psize ]); But we have already decided, that `&x[0]` is OK for us. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test This class is even not used, because tests are not feature based, they are TEST(), not TEST_F(). Command line works well with for the test name as well. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:50: //---------------------------------- It's not an overkill - it will be difficult to locate the tests, confusing and and will cause questions when we need it. - it accumulates the technical debt. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:65: #define expected_version_string L"11." Still I have not get your point. "11" or "10" or "8" or "453" works fine. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:88: ASSERT_EQ(version, INSTALLED_IE_VERSION); What does this test tests? `sanity_major` says nothing. If it's intended to test the extraction of the major version from the version string, then it's better to have the function `GetMajorNumber` is I've mentioned above and test it.
I didn't get to all the renaming possible, only the ones visible in the plugin code. http://codereview.adblockplus.org/5171515343503360/diff/5697070107197440/src/... File src/plugin/PluginUserSettings.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5697070107197440/src/... src/plugin/PluginUserSettings.cpp:141: return DISP_E_TYPEMISMATCH; This is a fix for an unrelated defect that slipped into this patch set. Oops.
Comments on previous draft. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); On 2014/07/29 14:45:56, sergei wrote: > Still disagree here. OK, but this is a code review, not an architecture review. As I see it, Wladimir opened ticket #41, and it's to support the JS code in libadblockplus, so it's beyond my scope to worry about the details of why he wants it. Personally, even if it's only for error logging it's a good enough reason for me. Populating a single string member clearly does not introduce a new defect. The bulk of the code here is also required for #404, so knocking of an otherwise-harmless change at the same time is just not a problem. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/plugin/PluginWbPassThrough.cpp:99: if (mimeType.IsEmpty() && domain.IsEmpty() && AdblockPlus::IE::installed_major_version() >= 8) On 2014/07/29 14:45:56, sergei wrote: > - it's not clear here that this function does not throw any exception. How was it clear that the previous code didn't throw an exception? If you want to insist on solving every problem that any code modification raises, development will grind to a halt. Incremental improvement is the name of the game. > - I expect this method to be called several times, and to read the version each > time from the windows registry looks very inefficient. We can memoize the retrieval functions later. Right now, I'm trying to get something like a small change in place. > We might should create a ticket for that to inject the version in the > constructor and use a constant access time value without any possibility for the > exception, at least here. That's one way to solve the problem, and it's not the one I'd use. If you want a ticket here, make the task to improve the efficiency, not to pick a particular solution. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" On 2014/07/29 14:45:56, sergei wrote: > Clear, it should be here. But why will it be moved? Go read #404 again. Fixing that ticket requires a library in the build system we do not currently have. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') On 2014/07/29 14:45:56, sergei wrote: > the next malware. It's beyond the scope of this class to defend against malware. Besides, I've just posted a rewritten version of these functions to rationalize pattern matching and length handling. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); On 2014/07/29 14:45:56, sergei wrote: > - if we use the unified and regular error handling there is no problem or > difficulties with it, except the laziness. Yes, and when we get unified and regular error handling, then it will be time to modify this. You are making an implicit claim that simply using system_error will be sufficient, and I don't agree with that. This code will also end up in the installer custom action code (see #404 yet again), not just in the engine and the plugin, and we do not have any shared error handling yet that can simply be ported over there easily yet. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[ psize ]); On 2014/07/29 14:45:56, sergei wrote: > But we have already decided, that `&x[0]` is OK for us. I am not convinced that we have already decided this. I certainly have not conceded the point. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test On 2014/07/29 14:45:56, sergei wrote: > Command line works well with for the test name as well. Go read the documentation about how --gtest_filter works. I use extensively.
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); > OK, but this is a code review, not an architecture review. Code review includes the architecture review. > As I see it, Wladimir opened ticket #41, and it's to support the JS code in libadblockplus, so it's beyond my scope to worry about the details of why he wants it. Personally, even if it's only for error logging it's a good enough reason for me. When we do something we should think about the aim of it, why do we need it, how to do it the proper way and so on. If we say it's OK for me here and it's OK for me here and so on, then it will very soon become an unsupportable set of special cases. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" In #404 there is nothing about any problems with libraries. It's clear that currently 'shared' library is not used by the installer, but we can link it. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') >It's beyond the scope of this class to defend against malware. All code should be safe. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); I'm pretty sure that std::system_error can be used in the installer as well because it inherits std::runtime_error. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test I've checked the documentation and tried it and it does work. `Registry_Test` is not needed and this class is even not used by gtest.
Updated to remove extraneous test class definitions. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); On 2014/07/30 10:42:04, sergei wrote: > Code review includes the architecture review. Code review, I will restate, is not an architecture review. Wladimir is not on this code review. He's the one who opened the issue. If you want to persist in thinking that this is the right place to talk about this architectural decision, you are welcome to. There's already a venue for talking about such issues. It's on #41 in Trac. Until that ticket changes, I'll consider it valid. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:1: #include "IE_version.h" On 2014/07/30 10:42:04, sergei wrote: > In #404 there is nothing about any problems with libraries. It's clear that > currently 'shared' library is not used by the installer, but we can link it. Yes, that's because the ticket states what the problem is, not what the solution is. Almost all the code in the existing shared library is part of the engine/plugin pair only and does not belong in a generally shared library. I've said this before in review comments I would have thought you had read, but we need a new library for #404 that shares only that which needs sharing. This registry code is the first such. The other thing that needs to go in there is debugger, trace, and log support. They're not in good enough shape in the plugin yet to easily move. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/IE_version.cpp:27: if (version[1] != '.' || version[0] < L'0' || version[0] > L'9') On 2014/07/30 10:42:04, sergei wrote: > >It's beyond the scope of this class to defend against malware. > > All code should be safe. OK. That's what I'll agree with. But "safe" and "defends against malware" are two different categories. The reason that not checking length was a defect before (it's fixed in the latest patch set) is that evaluating 's[1]' on a string of length 1 is undefined behavior and may throw. If it did, that would misreport an error, at best. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/shared/Registry.cpp:34: throw std::runtime_error("Failure in RegQueryValueEx to query name"); On 2014/07/30 10:42:04, sergei wrote: > I'm pretty sure that std::system_error can be used in the installer as well > because it inherits std::runtime_error. It's not the exception that's the main problem, it's the lack of general-purpose code to catch it and, at the least, log it. If we don't have that, it doesn't matter at all what you throw. And, as I pointed out elsewhere in the present set of comments, we don't have such general-purpose code yet, much less in a library available to the installer. Furthermore, If we want to identify the source of the error in the log, we'll also need our own exception class that identifies the source explicitly, such as a fully qualified function name. http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/test... test/RegistryTest.cpp:29: class Registry_Test On 2014/07/30 10:42:04, sergei wrote: > I've checked the documentation and tried it and it does work. `Registry_Test` is > not needed and this class is even not used by gtest. OK. Tested on my end. Where in the googletest documentation does it state that you don't need to declare the test class? I've missed it, but that documentation isn't the best organized, either.
http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/6063544634179584/src/... src/engine/Main.cpp:410: appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::installed_version_string()); > Code review, I will restate, is not an architecture review. I'm moving the discussion to intraforum. https://intraforum.adblockplus.org/t/what-is-the-relation-between-code-review... > Wladimir is not on this code review. He's the one who opened the issue. If you want to persist in thinking that this is the right place to talk about this architectural decision, you are welcome to. > There's already a venue for talking about such issues. It's on #41 in Trac. Until that ticket changes, I'll consider it valid. See my comment above, "Let's move the discussion to issues.adblockplus.org, I've updated the ticket."
Bump. Can we finish this? I've addressed all the technical comments and there's been a fresh patch set incorporating all the technical feedback for almost a week now.
LGTM!
http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/IE_version.cpp:23: std::wstring IE_version_string() In this file names also should be without underscore http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:6: Registry_Key::Registry_Key(HKEY parent, const std::wstring& key_name) I think we use different naming convention, without underscore. http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:48: size_t psize = size >> 1; It's not critical, only my opinion: - I don't like using two variables psize and size below, using only one is better. - Despite here it looks correct, there is so plenty of undefined and implementation defined behaviour with bit logic that if it's possible I would like to avoid it. http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[psize]); no additional spaces
Let's hope this is the final round. http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... File src/shared/IE_version.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/IE_version.cpp:23: std::wstring IE_version_string() On 2014/10/21 11:18:33, sergei wrote: > In this file names also should be without underscore Done. http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... File src/shared/Registry.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:6: Registry_Key::Registry_Key(HKEY parent, const std::wstring& key_name) On 2014/10/21 11:18:33, sergei wrote: > I think we use different naming convention, without underscore. Done. http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:48: size_t psize = size >> 1; On 2014/10/21 11:18:33, sergei wrote: > - I don't like using two variables psize and size below, using only one is > better. Can't do it with just one variable without introducing redundant calls. RegQueryValueExW modifies its argument. If we don't save the length of 'p' across that call, we would have to call 'strlen' again. > - Despite here it looks correct, there is so plenty of undefined and > implementation defined behaviour with bit logic that if it's possible I would > like to avoid it. Rewritten to use integer arithmetic. No dependency on binary representation (not that that's ever goes to change for IE). http://codereview.adblockplus.org/5171515343503360/diff/4551610919288832/src/... src/shared/Registry.cpp:50: std::unique_ptr< wchar_t[] > p(new wchar_t[psize]); On 2014/10/21 11:18:33, sergei wrote: > no additional spaces Done.
http://codereview.adblockplus.org/5171515343503360/diff/5682617542246400/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5682617542246400/src/... src/plugin/PluginWbPassThrough.cpp:13: <Merge Conflict>#include "../shared/IE_version.h" "Merge Conflict" Do we need this header here?
http://codereview.adblockplus.org/5171515343503360/diff/5682617542246400/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5682617542246400/src/... src/plugin/PluginWbPassThrough.cpp:13: <Merge Conflict>#include "../shared/IE_version.h" On 2015/01/05 12:24:23, sergei wrote: > "Merge Conflict" > Do we need this header here? And this is why I dislike rebase. The relevant change that requires the header also got lost.
LGTM, It takes too long, I'm pretty sure we will change some parts of it later. http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/src/... src/plugin/PluginWbPassThrough.cpp:158: if (mimeType.IsEmpty() && domain.empty() && AdblockPlus::IE::InstalledMajorVersion() >= 8) Actually we are supporting only versions starting from 8, so this version check is not necessary here. BTW, how does it affect the performance? I would remove it. http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... test/RegistryTest.cpp:45: // IE_Version The tests below should be in IE_versionTest.cpp http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... test/RegistryTest.cpp:52: #define exact(x) exact_##x What is the aim of it? I would remove it.
http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/src/... File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/src/... src/plugin/PluginWbPassThrough.cpp:158: if (mimeType.IsEmpty() && domain.empty() && AdblockPlus::IE::InstalledMajorVersion() >= 8) On 2015/01/06 13:39:11, sergei wrote: > Actually we are supporting only versions starting from 8, so this version check > is not necessary here. You and Oleksandr and I had a discussion about this several weeks ago. The decision made at the time was to leave all the version checks in, because we were going to continue to allow people to install the plugin on old versions, even if that meant shooting themselves in the foot. Regardless, we need to ensure that the plugin does not install on old version of IE before we simply remove checks that operate at run time. http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... File test/RegistryTest.cpp (right): http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... test/RegistryTest.cpp:45: // IE_Version On 2015/01/06 13:39:11, sergei wrote: > The tests below should be in IE_versionTest.cpp OK. I'll do it in a later change. We need to move the registry and IE version classes into a library shared by both the plugin/engine and the installer. We don't currently have such a library in our build system. http://codereview.adblockplus.org/5171515343503360/diff/5679846214598656/test... test/RegistryTest.cpp:52: #define exact(x) exact_##x On 2015/01/06 13:39:11, sergei wrote: > What is the aim of it? > > I would remove it. Ordinarily unit tests should always run independent of the system that they're installed on. Unfortunately that can't work for testing the installed version of IE, since the relevant data is held in system-global variable in the registry. These definitions allow a working copy to adapt these unit tests to local conditions. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
