Initial libadblockplus integration

Initial libadblockplus integration

[Felix Dahlke, 2013-04-03 05:35:27]

I don't really see the subscription management functions being called anywhere,
did I miss them?

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj
File AdBlocker/AdBlocker.vcxproj (right):

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj#...
AdBlocker/AdBlocker.vcxproj:810: <ProjectReference
Include="..\..\V8\MSVS\libadblockplus.vcxproj">
Is this a relative file system path? If so, shouldn't it be
..\libadblockplus\MSVS\...?

http://codereview.adblockplus.org/9998007/diff/1/AdPlugin.sln
File AdPlugin.sln (right):

http://codereview.adblockplus.org/9998007/diff/1/AdPlugin.sln#newcode21
AdPlugin.sln:21: Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "abpshell",
"..\V8\MSVS\abpshell.vcxproj", "{B8C5ABC4-2076-B368-19EB-531BAE26CEF1}"
How come all these projects where linked already?

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h
File Shared/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:10: using namespace AdblockPlus;
I'd really prefer to avoid using namespace in headers, can get messy.

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:18: std::auto_ptr<std::istream> Read(const
std::string& path) const
Should be declared inline or, preferably, have its implementation moved to the
.cpp file.

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:29: void operator()(const std::string& message)
Should be declared inline or, preferably, have its implementation moved to the
.cpp file.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp
File Shared/PluginFilter.cpp (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:863: //just ignore all errors we might get when adding
filters
We should probably at least log exceptions.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:1208: std::string url = (char*)src.GetBuffer();
I'm really leaving my comfort zone here, but some quick googling makes it look
like "(LPCSTR) src" is the canonical way to convert a CString to an std::string.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginSettings.cpp
File Shared/PluginSettings.cpp (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginSettings.cpp#ne...
Shared/PluginSettings.cpp:408: bool CPluginSettings::MakeRequestForUpdate()
Shouldn't this call UpdateSubscription()?

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginSettings.cpp#ne...
Shared/PluginSettings.cpp:488: //       
CPluginFilter::DownloadFilterFile(downloadFilterName, filename);
Might as well remove this line, likewise below.

[Wladimir Palant, 2013-04-03 12:42:19]

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj
File AdBlocker/AdBlocker.vcxproj (right):

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj#...
AdBlocker/AdBlocker.vcxproj:810: <ProjectReference
Include="..\..\V8\MSVS\libadblockplus.vcxproj">
Actually, this seems to assume a particular directory structure - adding
libadblockplus as a subrepository of adblockplusie repo should be better.

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h
File Shared/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:21: file->open(("lib/" + path).c_str());
Does that assume a particular current work directory? Does that assumption
always work?

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:31: //    std::cerr << "Error: " << message <<
std::endl;
I'm not a big fan of code that has been outcommented. It's not obvious why this
code isn't used and under what circumstances it should be used. If anything, I
would prefer #if defined(DEBUG) here or something like that. But this should
probably just use CPluginDebug instead.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp
File Shared/PluginFilter.cpp (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:823: if (!filter.Trim().IsEmpty() && filter.GetAt(0) !=
'!' && filter.GetAt(0) != '[')
I think you can assume that the filter engine will only give you actual hiding
filters, not comments or empty strings. There won't be any whitelists or
blocking rules either.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:1204: src.OemToCharA();
Are we really getting all URLs as ASCII? What about IDNs (http://россия.рф/), do
we get them as Punycode or are they completely broken?

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:1206: std::string contentTypeString = "";
This needs a TODO comment: "Need to guess the content type".

[Felix Dahlke, 2013-04-03 12:51:04]

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj
File AdBlocker/AdBlocker.vcxproj (right):

http://codereview.adblockplus.org/9998007/diff/1/AdBlocker/AdBlocker.vcxproj#...
AdBlocker/AdBlocker.vcxproj:810: <ProjectReference
Include="..\..\V8\MSVS\libadblockplus.vcxproj">
On 2013/04/03 12:42:19, Wladimir Palant wrote:
> Actually, this seems to assume a particular directory structure - adding
> libadblockplus as a subrepository of adblockplusie repo should be better.

I'm to blame, I told Oleksandr it's fine to use a relative path for now and move
to a subrepository later.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp
File Shared/PluginFilter.cpp (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:1204: src.OemToCharA();
On 2013/04/03 12:42:19, Wladimir Palant wrote:
> Are we really getting all URLs as ASCII? What about IDNs
(http://%D1%80%D0%BE%D1%81%D1%81%D0%B8%D1%8F.%D1%80%D1%84/), do
> we get them as Punycode or are they completely broken?

The problem is that ABP for IE uses UTF-16 internally (like most Windows
programs), but libadblockplus uses UTF-8. We talked about converting to/from
UTF-8 whenever we interface with libadblockplus, but I thought it's fine to just
support ASCII URLs for now and revisit this a bit later. Then again, it's
probably not a huge deal to get it right now.

[Oleksandr, 2013-04-03 13:12:17]

http://codereview.adblockplus.org/9998007/diff/1/AdPlugin.sln
File AdPlugin.sln (right):

http://codereview.adblockplus.org/9998007/diff/1/AdPlugin.sln#newcode21
AdPlugin.sln:21: Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "abpshell",
"..\V8\MSVS\abpshell.vcxproj", "{B8C5ABC4-2076-B368-19EB-531BAE26CEF1}"
It would be great to have them in the solution, for various reasons. My bad, I
should have removed them first, since we don't have a subrepo yet, but I plan
for them to remain here. 


On 2013/04/03 05:35:27, Felix H. Dahlke wrote:
> How come all these projects where linked already?

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h
File Shared/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/AdblockPlusClient.h#n...
Shared/AdblockPlusClient.h:34: 
To be honest, these 2 classes LibFileReader and CerrErrorCallback are straight
from abpshell project. I just used them as placeholders, to have basic filtering
and hiding done through libadblockplus in the meantime. All comments are valid
of course.

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp
File Shared/PluginFilter.cpp (right):

http://codereview.adblockplus.org/9998007/diff/1/Shared/PluginFilter.cpp#newc...
Shared/PluginFilter.cpp:1204: src.OemToCharA();
I've used CT2CA in the end (not included in this patchset). Wladimir it's
multibyte string, not really ASCII. So it supports UTF-8.

On 2013/04/03 12:51:04, Felix H. Dahlke wrote:
> On 2013/04/03 12:42:19, Wladimir Palant wrote:
> > Are we really getting all URLs as ASCII? What about IDNs
>
(http://%25D1%2580%25D0%25BE%25D1%2581%25D1%2581%25D0%25B8%25D1%258F.%25D1%2580%25D1%2584/),
do
> > we get them as Punycode or are they completely broken?
> 
> The problem is that ABP for IE uses UTF-16 internally (like most Windows
> programs), but libadblockplus uses UTF-8. We talked about converting to/from
> UTF-8 whenever we interface with libadblockplus, but I thought it's fine to
just
> support ASCII URLs for now and revisit this a bit later. Then again, it's
> probably not a huge deal to get it right now.