Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4805561958793216: Noissue - Remove dead code in CPluginClass::SetSite (Closed)

Created:
March 6, 2015, 10:57 a.m. by Eric
Modified:
March 31, 2015, 12:32 p.m.
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Noissue - Remove dead code in CPluginClass::SetSite Remove legacy code in CPluginClass::SetSite for when it was "loaded as a toolbar handle". This branch cannot be activated, since CPluginClass is never registered as anything but a BHO. Removed now-unused argument for InitObject. Added a 'try' statement around the body of 'SetSite', since it's an entry point. Removed an unused statement and a macro with empty expansion at the beginning. Removed header "abp.h", since all that was in it was the empty macro and it did not appear elsewhere.

Patch Set 1 #

Total comments: 13

Patch Set 2 : rebased + removed more dead code #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -176 lines) Patch
M adblockplus.gyp View 1 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/PluginClass.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/PluginClass.cpp View 1 5 chunks +129 lines, -155 lines 1 comment Download
R src/plugin/abp.h View 1 1 chunk +0 lines, -19 lines 0 comments Download

Messages

Total messages: 25
Eric
The initial patch set conflicts with http://codereview.adblockplus.org/4937147073167360/ Issue 1672 - Subscribe and listen COM events ...
March 6, 2015, 11:28 a.m. (2015-03-06 11:28:36 UTC) #1
sergei
I have not checked he rest yet but want to comment the following On 2015/03/06 ...
March 6, 2015, 3:30 p.m. (2015-03-06 15:30:35 UTC) #2
Eric
On 2015/03/06 11:28:36, Eric wrote: > We're using an ATL implementation base class for 'IOleCommandTarget', ...
March 6, 2015, 3:57 p.m. (2015-03-06 15:57:38 UTC) #3
Eric
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode131 src/plugin/PluginClass.cpp:131: m_webBrowser2.Release(); Explicit release. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode263 src/plugin/PluginClass.cpp:263: m_webBrowser2 = unknownSite; Assigned ...
March 6, 2015, 3:58 p.m. (2015-03-06 15:58:04 UTC) #4
sergei
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode263 src/plugin/PluginClass.cpp:263: m_webBrowser2 = unknownSite; On 2015/03/06 15:58:04, Eric wrote: > ...
March 6, 2015, 4:07 p.m. (2015-03-06 16:07:19 UTC) #5
Eric
It might be a good idea to eliminate 'm_webBrowser2' entirely, since it's only a copy ...
March 6, 2015, 4:27 p.m. (2015-03-06 16:27:22 UTC) #6
sergei
On 2015/03/06 16:27:22, Eric wrote: > Calling 'Release' through 'CComPtr' apparently also zeros out the ...
March 6, 2015, 4:41 p.m. (2015-03-06 16:41:28 UTC) #7
Eric
On 2015/03/06 16:41:28, sergei wrote: > It's a spirit of ATL, they have some foolstable ...
March 6, 2015, 4:53 p.m. (2015-03-06 16:53:27 UTC) #8
sergei
On 2015/03/06 16:53:27, Eric wrote: > For example, MS never bothered to update ATL to ...
March 9, 2015, 10:12 a.m. (2015-03-09 10:12:47 UTC) #9
sergei
LGTM but checkout the comments. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode244 src/plugin/PluginClass.cpp:244: * but it is ...
March 9, 2015, 10:23 a.m. (2015-03-09 10:23:50 UTC) #10
Eric
On 2015/03/09 10:12:47, sergei wrote: > Could you explain the trouble here? ATL is being ...
March 9, 2015, 12:26 p.m. (2015-03-09 12:26:16 UTC) #11
Eric
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode244 src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for ...
March 9, 2015, 1:31 p.m. (2015-03-09 13:31:29 UTC) #12
Oleksandr
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode244 src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for ...
March 10, 2015, 8:38 a.m. (2015-03-10 08:38:26 UTC) #13
Oleksandr
Sorry, I have published my old comments before refreshing the codereview. Turns out Sergei and ...
March 10, 2015, 8:42 a.m. (2015-03-10 08:42:04 UTC) #14
Eric
On 2015/03/10 08:42:04, Oleksandr wrote: > https://msdn.microsoft.com/en-us/library/bb250489%28v=vs.85%29.aspx#basics I'll stipulate that this was the behavior of ...
March 10, 2015, 2:20 p.m. (2015-03-10 14:20:00 UTC) #15
Eric
I forgot one. (7) It only fails to call 'SetSite(nullptr)' when IE is shut down ...
March 10, 2015, 3:05 p.m. (2015-03-10 15:05:41 UTC) #16
sergei
On 2015/03/09 12:26:16, Eric wrote: > On 2015/03/09 10:12:47, sergei wrote: > > Could you ...
March 11, 2015, 1:27 p.m. (2015-03-11 13:27:03 UTC) #17
Oleksandr
LGTM
March 12, 2015, 4:46 a.m. (2015-03-12 04:46:27 UTC) #18
Eric
New patch set. Need another round of review. Big-looking modifications from rebasing, but the essence ...
March 17, 2015, 10:41 a.m. (2015-03-17 10:41:07 UTC) #19
Oleksandr
So what has been done here is: 1. Removed a parameter from InitObject 2. Remove ...
March 17, 2015, 5:45 p.m. (2015-03-17 17:45:53 UTC) #20
Eric
On 2015/03/17 17:45:53, Oleksandr wrote: > So what has been done here is: > 1. ...
March 17, 2015, 5:51 p.m. (2015-03-17 17:51:28 UTC) #21
sergei
LGTM, but comment is still here.
March 30, 2015, 10:54 a.m. (2015-03-30 10:54:35 UTC) #22
Eric
On 2015/03/30 10:54:35, sergei wrote: > but comment is still here. Which comment are you ...
March 30, 2015, 11:04 a.m. (2015-03-30 11:04:03 UTC) #23
sergei
http://codereview.adblockplus.org/4805561958793216/diff/5653164804014080/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5653164804014080/src/plugin/PluginClass.cpp#newcode221 src/plugin/PluginClass.cpp:221: */ This comment
March 30, 2015, 11:26 a.m. (2015-03-30 11:26:23 UTC) #24
Eric
March 31, 2015, 12:31 p.m. (2015-03-31 12:31:49 UTC) #25
I've pushed with the comment in place. I've been working on these life cycle
issues generally, and the code herein is due to change again soon. When that
happens so will the comment. We'll need much more comprehensive documentation on
life cycle, probably not in code comments but in a doxygen page. The content of
this comment will move there at that point.

Powered by Google App Engine
This is Rietveld