Issue 6215938672164864: [IE] Replace ATL::CRect (Closed)
DescriptionReplace ATL::CRect with a local rectangle class. All the code was using were functions for height and width.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Comments addressed; Rebased #
Total comments: 2
Patch Set 3 : hopefully final #
Total comments: 1
MessagesTotal messages: 14
LGTM
http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:59: namespace { Nit: New line before { http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:61: namespace Local { Nit: New line before {
http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:60: // Without an extra namespace within the anonymous one, the identifier "Rectangle" is ambiguous Hm, guess it conflicts with this one? http://msdn.microsoft.com/en-us/library/windows/desktop/dd162898%28v=vs.85%29... How about calling our class "Rect" or something instead? http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:69: // C++11 noexcept I guess you're marking this so we can make it noexcept later? Trouble is, it seems like VS 2013 won't support noexcept, I don't think we should have markers like this everywhere until we upgrade to a VS version that supports it. Let's rather see what can be made noexcept once we can use it. http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:70: inline int Height() const inline is redundant here, since this will only be defined in one compilation unit. And inline member functions are implicitly inline anyway. http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:85: // CPluginClass We don't do class headers like this usually, I personally think it's just noise. Oleksandr, what do you think?
http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:85: // CPluginClass Visually I think it makes sense, and sometimes it's easier to get the code structure. But let's remove these for consistency with the rest of the code. On 2014/07/10 04:49:03, Felix H. Dahlke wrote: > We don't do class headers like this usually, I personally think it's just noise. > Oleksandr, what do you think?
http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:60: // Without an extra namespace within the anonymous one, the identifier "Rectangle" is ambiguous On 2014/07/10 04:49:03, Felix H. Dahlke wrote: > http://msdn.microsoft.com/en-us/library/windows/desktop/dd162898%2528v=vs.85%... FYI. This URL is exhibited a defect in the code review software. The left and right parentheses are first getting quoted to "percent-28" and "percent-29", then the percent symbols are getting quoted to "percent-25". Manually editing the URL brings it back to working. http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:69: // C++11 noexcept On 2014/07/10 04:49:03, Felix H. Dahlke wrote: > I guess you're marking this so we can make it noexcept later? Yes. > Trouble is, it seems like VS 2013 won't support noexcept Unfortunately, no, it doesn't. I was just reading up on this yesterday. http://msdn.microsoft.com/en-us/library/hh567368.aspx > I don't think we should have markers > like this everywhere until we upgrade to a VS version that supports it. Let's > rather see what can be made noexcept once we can use it. The trouble with that policy is that it means reanalyzing the code from scratch, as a whole, in a year or two. If we leave comments in place, it's a simple search-and-replace later, much faster and more effective. In addition, the comment acts as documentation until then.
Removed patches for ATL_Deprecate.h, which isn't in the public tree yet. http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:60: // Without an extra namespace within the anonymous one, the identifier "Rectangle" is ambiguous On 2014/07/10 04:49:03, Felix H. Dahlke wrote: > How about calling our class "Rect" or something instead? Well, the base class is "RECT", and it doesn't seem wise to make it differ only in case. Instead, I just changed the namespace to AdblockPlus. In case we ever need a library-quality version of this class, this is likely to be its first name.
Added Sergei
LGTM, beside `Local` and the comment below. > The trouble with that policy is that it means reanalyzing the code from scratch, > as a whole, in a year or two. If we leave comments in place, it's a simple > search-and-replace later, much faster and more effective. > > In addition, the comment acts as documentation until then. Just want to share my opinion regarding the comment with `noexcept`, it also confuses me. First of all because this comment basically does not describe any reasons or tricky parts of the implementation, so I cannot say that it's useful. Secondly, I expect the compiler to derive it in this case, otherwise if the programmer has to declare it in every case I'm afraid it will be very painful. I would like to wait for the "best practices" of such new features. If we really want it, I would like to have the empty #define ABP_NOEXCEPT and use it as `noexcept`. When it becomes supported we can update the #define as well as having the #define will significantly simplify search-and-replace if it's needed rather than comment which is typo prone and can have a free form. http://codereview.adblockplus.org/6215938672164864/diff/5717271485874176/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5717271485874176/src/... src/plugin/PluginClass.cpp:1757: Local::Rectangle rcText = rcClient; Should Local be renamed to AdblockPlus?
Removed noexcept comments. Removed extraneous anonymous namespace. > If we really want it, I would like to have the empty > #define ABP_NOEXCEPT and use it as `noexcept`. Using a definition seems like a decent way of dealing with my concern. http://codereview.adblockplus.org/6215938672164864/diff/5717271485874176/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5717271485874176/src/... src/plugin/PluginClass.cpp:1757: Local::Rectangle rcText = rcClient; On 2014/07/23 13:34:49, sergei wrote: > Should Local be renamed to AdblockPlus? That's what I get for compiling in Release configuration. Done.
LGTM
http://codereview.adblockplus.org/6215938672164864/diff/5634387206995968/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6215938672164864/diff/5634387206995968/src/... src/plugin/PluginClass.cpp:56: namespace AdblockPlus This would normally be an anonymous namespace, so I'm not sure if we should have it clash with the namespace used by libadblockplus. I still think the most pragmatic way would be to just give the class a name that won't clash with globals, i.e. "Rect".
On 2014/07/24 07:24:17, Felix H. Dahlke wrote: > I still think the most > pragmatic way would be to just give the class a name that won't clash with > globals, i.e. "Rect". Is this a blocker for a commit, or not? The original class, ATL::CRect, is a rather complete library-quality class. If we ever need such a thing, this class would belong in such a library, our own or someone else's. I see no issue using an existing library namespace, particularly one that's unlikely to need classes for screen rendering. And I really dislike 'Rect', since the base class is 'RECT', a Windows system class, which differs only in capitalization.
On 2014/07/24 11:35:45, Eric wrote: > On 2014/07/24 07:24:17, Felix H. Dahlke wrote: > > I still think the most > > pragmatic way would be to just give the class a name that won't clash with > > globals, i.e. "Rect". > > Is this a blocker for a commit, or not? > > The original class, ATL::CRect, is a rather complete library-quality class. If > we ever need such a thing, this class would belong in such a library, our own or > someone else's. I see no issue using an existing library namespace, particularly > one that's unlikely to need classes for screen rendering. > > And I really dislike 'Rect', since the base class is 'RECT', a Windows system > class, which differs only in capitalization. True, neither way is really clean. LGTM. 
|
