Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(261)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by Oleksandr
Modified:
4 years, 2 months ago
Reviewers:
Eric, sergei
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 ...
4 years, 4 months ago (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. ...
4 years, 4 months ago (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 ...
4 years, 4 months ago (2015-03-27 13:53:19 UTC) #3
sergei
In general it should work, however I would say that the it seems that the ...
4 years, 4 months ago (2015-04-01 18:49:32 UTC) #4
Oleksandr
I have ran more tests and I agree with Sergei that merely removing the code ...
4 years, 4 months ago (2015-04-13 03:29:09 UTC) #5
sergei
LGTM
4 years, 4 months ago (2015-04-13 07:24:40 UTC) #6
Eric
4 years, 3 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5