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

Issue 5168444685156352: Issue 1201 - Enabling/disabling ABP everywhere applies only in the next opened tab (Closed)

Created:
March 27, 2015, 7:34 a.m. by Oleksandr
Modified:
June 1, 2015, 9:23 a.m.
Reviewers:
sergei, Eric
CC:
Felix Dahlke
Visibility:
Public.

Description

I have added a comment in a line that actually fixes the issue. The other stuff is just derivative from that.

Patch Set 1 #

Total comments: 11

Patch Set 2 : Only fix the issue, no refactoring #

Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -5 lines) Patch
M src/plugin/PluginClass.cpp View 1 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 7
Oleksandr
http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/plugin/PluginClass.cpp#newcode657 src/plugin/PluginClass.cpp:657: CPluginClientFactory::ReleaseMimeFilterClientInstance(); This fixes the issue. Just calling Unregister() here ...
March 27, 2015, 8:02 a.m. (2015-03-27 08:02:29 UTC) #1
Eric
I don't think the initial patch set works, but it's headed in the right direction. ...
March 27, 2015, 1:35 p.m. (2015-03-27 13:35:49 UTC) #2
Eric
One more point. The code for ID_MENU_DISABLE in 'DisplayPluginMenu' illustrates the problem. It's explicitly creating ...
March 27, 2015, 1:53 p.m. (2015-03-27 13:53:19 UTC) #3
sergei
In general it should work, however I would say that the it seems that the ...
April 1, 2015, 6:49 p.m. (2015-04-01 18:49:32 UTC) #4
Oleksandr
I have ran more tests and I agree with Sergei that merely removing the code ...
April 13, 2015, 3:29 a.m. (2015-04-13 03:29:09 UTC) #5
sergei
LGTM
April 13, 2015, 7:24 a.m. (2015-04-13 07:24:40 UTC) #6
Eric
May 14, 2015, 3:26 p.m. (2015-05-14 15:26:13 UTC) #7
LGTM

I would suggest, however, changing the description of #1201 to "Disabling ABP
everywhere ..." and dropping the "Enabling/" part. Enabling is not actually
mentioned in the symptom descriptions, nor is it affected by the present change
set.

Powered by Google App Engine
This is Rietveld