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

Issue 5081266177179648: Issue 1104 - Cannot uncheck Disable on website option in tool bar (Closed)

Feb. 23, 2015, 12:40 p.m. by sergei
March 4, 2015, 1:23 p.m.
Eric, Oleksandr


The trouble is that we are manually crafting the filter text from the current URL. As result for the URL http://www.fotor.com/some-path we get '@@||www.fotor.com^$document' and we cannot remove filter '@@||fotor.com^$document' because it's different. The solution to merely always remove 'www.' does not fit either because it's possible to create '@@||www.fotor.com^$document' filter by adding 'www.fotor.com' via settings. There is also another way (BTW, which is used by our extension for Google Chrome) to find the exception filter and remove it, which I propose to have now. I guess it's closer to the user's intention, he does not care whether the web site is whitelisted by domain or by using another exception filter. # Actually I would like to have such functionality in the core to avoid any different behaviour on different platforms but it's not easy/fast process to make changes in the core and pull them here. # See https://issues.adblockplus.org/ticket/2031 BTW, Chrome removes "www." prefix when the user disables ABP on the particular web site, so I've added it as well. As well as I propose to do it even when the domain is being added through the settings page. So, if user added www.some-domain.com and visit some-domain.com it's whitelisted. Even more, here it's possible to enter http://www.some-domain.com/some-path (we had already received some complains from the users that they are doing it and it does not work) and get some-domain.com to be whitelisted. Additional technical details: - PROC_ADD_FILTER and PROC_REMOVE_FILTER are renamed to PROC_ADD_WHITELSITED_DOMAIN and PROC_REMOVE_WHITELSITED_DOMAIN, respectively because they don't expect a filter text any more but expect the actual domain. - PROC_TOGGLE_WHITELISTING is introduced to reduce the number of trips to the engine as well as to move the logic closer to the libadblockplus. # I hope eventually such functionality will be in the core and it seems quite straightforward to get rid of it in favour of the using API of libadblockplus. - [Utils] ReplaceString is a template function, so we can use it either with std::wstring or std::string. - [Utils] move ASCIIStringToLower to Utils, because we need it also in the engine. # Again, core library normalizes the URL and, as far as I know, supports unicode, so it's yet one reason to share such logic among all platforms rather than implement it in each extension. - [Utils] add BeginsWith. I've seen we use it in some other parts, however I've not touched them because they are not relevant to the current change.

Patch Set 1 #

Total comments: 16
Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -68 lines) Patch
M src/engine/Main.cpp View 2 chunks +50 lines, -9 lines 8 comments Download
M src/plugin/AdblockPlusClient.h View 1 chunk +3 lines, -2 lines 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 chunk +11 lines, -4 lines 0 comments Download
M src/plugin/PluginClass.cpp View 2 chunks +2 lines, -11 lines 2 comments Download
M src/plugin/PluginSettings.h View 1 chunk +2 lines, -2 lines 0 comments Download
M src/plugin/PluginSettings.cpp View 2 chunks +6 lines, -14 lines 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 chunk +0 lines, -13 lines 0 comments Download
M src/shared/Communication.h View 1 chunk +3 lines, -2 lines 1 comment Download
M src/shared/Utils.h View 1 chunk +29 lines, -1 line 5 comments Download
M src/shared/Utils.cpp View 1 chunk +1 line, -10 lines 0 comments Download


Total messages: 11
Feb. 23, 2015, 3:31 p.m. (2015-02-23 15:31:32 UTC) #1
This first round of comments is only on the utility functions. I'm fine with moving ...
Feb. 26, 2015, 4:14 p.m. (2015-02-26 16:14:10 UTC) #2
-- This change set is fixing #1104 by deriving the domain name in the engine, ...
Feb. 26, 2015, 6:16 p.m. (2015-02-26 18:16:00 UTC) #3
I don't think this is a place to refactor the context menu. I do agree ...
Feb. 27, 2015, 10:02 a.m. (2015-02-27 10:02:12 UTC) #4
On 2015/02/27 10:02:12, Oleksandr wrote: > I don't think this is a place to refactor ...
Feb. 28, 2015, 5:28 p.m. (2015-02-28 17:28:59 UTC) #5
I agree that it's better to have Utils refactoring in another commit, so I will ...
March 2, 2015, 1:01 p.m. (2015-03-02 13:01:03 UTC) #6
I agree with most of the stuff, however I'm afraid #2070 and #2038 might drag ...
March 2, 2015, 2:54 p.m. (2015-03-02 14:54:01 UTC) #7
On 2015/03/02 13:01:03, sergei wrote: > That's correct, but let's firstly wait for the decision ...
March 2, 2015, 7:24 p.m. (2015-03-02 19:24:24 UTC) #8
http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/engine/Main.cpp#newcode248 src/engine/Main.cpp:248: { On 2015/03/02 13:01:04, sergei wrote: > I cannot ...
March 2, 2015, 7:24 p.m. (2015-03-02 19:24:31 UTC) #9
I've implemented it in the suggested way, however, I find it as a temporary hack. ...
March 4, 2015, 11:43 a.m. (2015-03-04 11:43:09 UTC) #10
March 4, 2015, 1:23 p.m. (2015-03-04 13:23:20 UTC) #11
On 2015/03/04 11:43:09, sergei wrote:
> What is the wait are you talking about?

When I said this:
> It's taken months for the UI issue that was a Q4 priority to near
> completion, it's not even done. 
I was referring to #1524. See the keyword '2014q4' on the issue.

Powered by Google App Engine
This is Rietveld