Issue 1557 - Update to the recent libadblockplus to reduce additional updates in the logic later.

[Eric, 2014-12-30 16:56:08]

On balance, it would be better for the plugin to declare "struct ContentType".
It only needs to contain a single member of type
AdblockPlus::FilterEngine::ContentType.

* This change set is already adding a header ContentType.h, which is a natural
place to define a class. That's what I expected to see there.
* We can declare overloads for input and output operators. Their implementations
can simply forward to those for int32_t and thus can be defined inline.
        std::istream& operator>>( std::istream&, ContentType )
        std::ostream& operator<<( std::ostream&, ContentType )
  Right now there's an explicit cast in each of the places where these operators
are used. Better to centralize the representation of a ContentType as int32_t by
overloading the operators.
* We can forward the CONTENT_TYPE_* definitions in libadblockplus to definitions
within ContentType. This would allow, for example, writing "ContentType::OTHER"
rather than "AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_OTHER".
* We could have a member function "ToWString()" rather than a freestanding
helper function.
* It would shorten all the declarations for ContentType. There's a typedef in
PluginWbPassThrough.cpp already in this change set that serves the same purpose.

There would be more code in ContentType.*, but the usage of this type would be
cleaner.

---

As a minor point, there are a few places where we can convert to_wstring to
ToWstring etc.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/plugin/AdblockPlusDomTraverser.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/AdblockPlusDomTraverser.cpp:20: bool isBlocked =
client->ShouldBlock(to_wstring(url),
ToWstring

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/AdblockPlusDomTraverser.cpp:59: cache->m_isHidden =
client->ShouldBlock(to_wstring(src),
ToWstring

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/PluginFilter.cpp:715: bool result =
client->Matches(to_wstring(srcCString), contentType, domain);
ToWstring

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/PluginFilter.cpp:724:
CPluginDebug::DebugResultBlocking(to_CString(type), srcCString, domain);
ToCString

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/PluginFilter.cpp:728:
CPluginDebug::DebugResultIgnoring(to_CString(type), srcCString, domain);
ToCString

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
Why not just use the implementation in libadblockplus? The analogous function in
the library is declared public-static.

In addition, ContentTypeToString is used only for debugging, so it's low-risk to
use an external definition.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/shared/ContentType.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.h:4: std::string
ContentTypeToString(AdblockPlus::FilterEngine::ContentType contentType);
This function is only used for debugging, and thus it could be declared within
an #ifdef block.

[Oleksandr, 2015-01-11 09:22:46]

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/adbl...
adblockplus.gyp:150: 'libadblockplus/include',
This feels wrong, IMHO. Conceptually, I think in AdblockPlus project we should
use 'shared' project, but not 'libadblockplus' directly. For example we are not
adding libadbockplus as a dependency here, yet adding the includes. See my
suggestion further about moving ContentType enum.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/plugin/PluginWbPassThrough.cpp:78: ,
m_contentType(AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_OTHER)
Don't we have a typedef to shorten this? Though personally I would either have a
typedef in FilterEngine.h and use it everywhere or not use the typedef here at
all, and leave it as is, for consistency with other places in code.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
On 2014/12/30 16:56:08, Eric wrote:
> Why not just use the implementation in libadblockplus? The analogous function
in
> the library is declared public-static.
> 
> In addition, ContentTypeToString is used only for debugging, so it's low-risk
to
> use an external definition.

I guess the idea here was to move the content type related declarations to
shared folder. However it looks like it wasn't fully done. 

I think it would be best if we re-factored libadblockplus and created a shared
folder there as well. We would move all the ContentType declarations there,
including stuff in this file. I think Android code might use that as well. If
not that, then at least we should move ContentType enum from FilterEngine into a
separate file, IMHO.

[sergei, 2015-01-11 23:34:05]

I'm glad to see that you also feel that it's wrong. BTW, we decided in November
to try and see whether it causes any trouble. Since it firstly requires to
change libadblockplus, let's do it as another iteration, please feel free to
create issues for libadblockplus and for IE addon.

14:25:44 <sergz> Ollie: palant: btw, we determine request type in IE addon and
send it the the engine, which calls Match method. I would say that to avoid code
duplication (any typo) it's better to move that enum into a separate header
which can be included by IE addon.
14:27:02 <•palant> sergz: it's part of the FilterEngine class, inside
FilterEngine.h. where else should it be?
14:27:30 <•palant> sergz: and that header *can* be included by ABP/IE...
14:38:54 <Ollie> palant: I would agree that we have to reuse enum definition.
Seeing how it looks like
(https://hg.adblockplus.org/libadblockplus/rev/e4c8240d0d1f) I also think it's a
good idea to move ContentType to a file in shared or in Utils.h, for example.
Just for clarity. Unless it would be a problem for Android.
14:39:01 <Ollie> sergz: ^
14:39:14 <sergz> palant: it can be but it also can be dangerous, that header
pulls other headers of libadblockplus which are not necessary for IE addon. btw,
I would say #includes in the headers of libadblockplus are not correct. The
public headers of the library should not use #include <this-lib-header>, they
should use relative paths #include "this-lib-header". In our case
14:39:14 <sergz> it's not a problem to add -I<path-to-libadblockplus/include>
but in general we should not force the client to do it at the compiler level and
it can be a pain for the clients to isolate headers.
14:40:21 <•palant> Ollie: sergz: ABP/IE uses FilterEngine.h just fine and that's
the first time it is supposed to be an issue...
14:40:49 <•palant> Ollie: moving code from libadblockplus into adblockplusie
would actually cause code duplication.
14:41:05 <sergz> Ollie: palant: no, IE addon does not FilterEngine. ABP engine
does. 
14:41:33 <sergz> Ollie: palant: does not use*
14:41:50 <Ollie> palant: We are not advocating moving the code to adblockplusie.
Only move it to a shared file. That removes code duplication, no?
14:43:29 <•palant> sergz: ok, now I get it.
14:43:30 <sergz> Ollie: palant: in libadblockplus there is no shared folder or
something like this. I would move only this enum into a separate header without
any dependency. That's it.
14:44:03 <•palant> Ollie: no, it creates code duplication because shared/ is
specific to ABP/IE - yet this needs to be available in libadblokcplus and
adblockplusandroid as well.
14:45:22 <•palant> sergz: the dependencies are merely class definitions, no real
issue with those.
14:45:52 <•palant> sergz: and moving that enum into a separate header file would
mean moving it out of the class where it belongs - ugly.
14:46:19 <•palant> sergz: in the end, it's up to fhd really.
14:47:03 <Ollie> palant: sergz Let's update libadblockplus in IE and then see if
there are any problems with including FilterEngine.h? I prefer sergz suggestion,
but it really isn't something to argue a lot about
14:48:02 <sergz> Ollie: palant: OK, I cannot provide a crash case now, but I'm
pretty sure that it's better to keep it separately from these headers. If I
remember something I will let you know.



Now some comments on the details.


> On balance, it would be better for the plugin to declare "struct ContentType".
It only needs to contain a single member of type
AdblockPlus::FilterEngine::ContentType.

I don't think we need something else in the plugin project to avoid introducing
of another ContentType. As well as we don't need `struct ContentType`, `enum
ContentType` in the header and a couple of linked methods from the same
namespace are enough.

* This change set is already adding a header ContentType.h, which is a natural
place to define a class. That's what I expected to see there.

Actually firstly ContentType.h should be in libadblockplus but see above.

* We can declare overloads for input and output operators. Their implementations
can simply forward to those for int32_t and thus can be defined inline.
        std::istream& operator>>( std::istream&, ContentType )
        std::ostream& operator<<( std::ostream&, ContentType )
  Right now there's an explicit cast in each of the places where these operators
are used. Better to centralize the representation of a ContentType as int32_t by
overloading the operators.

It actually looks good. BTW, not using `struct` will allow to use ADL. Despite
they are not used in libadblockplus and thus are dead code there, I would like
to have them in libadblockplus rather then in shared for IE.
BTW, we don't use spaces before ) and after (.

* We can forward the CONTENT_TYPE_* definitions in libadblockplus to definitions
within ContentType. This would allow, for example, writing "ContentType::OTHER"
rather than "AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_OTHER".

- Namespaces are not a big issue for me here.
- Actually we can use `enum class` which is enclosing scope for the
enumerations, thus we can use `ContentType::Other`.
Currently it's possible to use it as
`AdblockPlus::FilterEngine::CONTENT_TYPE_OTHER`, so I guess prefix
`CONTENT_TYPE_` comes from consistency with earlier requirements to avoid
possible overlapping with `OTHER` from any another `enum`.

* We could have a member function "ToWString()" rather than a freestanding
helper function.

I would say it's not necessary to be a member function. Freestanding function
ToString is also fine, JIC, don't forget about ADL.

* It would shorten all the declarations for ContentType. There's a typedef in
PluginWbPassThrough.cpp already in this change set that serves the same purpose.

There would be more code in ContentType.*, but the usage of this type would be
cleaner.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/adbl...
adblockplus.gyp:150: 'libadblockplus/include',
On 2015/01/11 09:22:46, Oleksandr wrote:
> This feels wrong, IMHO. Conceptually, I think in AdblockPlus project we should
> use 'shared' project, but not 'libadblockplus' directly. For example we are
not
> adding libadbockplus as a dependency here, yet adding the includes. See my
> suggestion further about moving ContentType enum.

Yes, see the discussion about declaration in a separate header and wrong
`#include <header>`

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
On 2014/12/30 16:56:08, Eric wrote:
> Why not just use the implementation in libadblockplus? The analogous function
in
> the library is declared public-static.
> 
> In addition, ContentTypeToString is used only for debugging, so it's low-risk
to
> use an external definition.

We don't link IE addon with libadblockplus because it has completely unnecessary
global/static variables as well as it will increase the dll size.

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
On 2015/01/11 09:22:46, Oleksandr wrote:
> On 2014/12/30 16:56:08, Eric wrote:
> > Why not just use the implementation in libadblockplus? The analogous
function
> in
> > the library is declared public-static.
> > 
> > In addition, ContentTypeToString is used only for debugging, so it's
low-risk
> to
> > use an external definition.
> 
> I guess the idea here was to move the content type related declarations to
> shared folder. However it looks like it wasn't fully done. 
> 
> I think it would be best if we re-factored libadblockplus and created a shared
> folder there as well. We would move all the ContentType declarations there,
> including stuff in this file. I think Android code might use that as well. If
> not that, then at least we should move ContentType enum from FilterEngine into
a
> separate file, IMHO.

There was not such idea because it's not libadblockplus project. It's just a
dirty code duplication to have it in IE Addon.
But we definitely need to change it. I'm not sure that it's time for shared
project in libadblockplus, I think it would be completely enough for the first
time to merely move all content type related stuff into separate files which can
be easily included by IE Addon.

[Eric, 2015-01-12 14:31:07]

There's a lot here, but the upshot is clear to me. It would be better to make
the necessary changes in libadblock first. The alternative is to hack up the
code here and eventually get around to fixing it; that course will be slower
overall. Since this change is anticipation of future changes, it would be better
to do it right the first time.

[sergei, 2015-01-13 13:13:14]

Updated and I've created the ticket
https://issues.adblockplus.org/ticket/1791

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
File src/shared/ContentType.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5639274879778816/src/...
src/shared/ContentType.h:4: std::string
ContentTypeToString(AdblockPlus::FilterEngine::ContentType contentType);
On 2014/12/30 16:56:08, Eric wrote:
> This function is only used for debugging, and thus it could be declared within
> an #ifdef block.

I'm not sure that it makes sense because linker removes unused code anyway (JIC,
I've checked the sizes of addon DLL and engine exe in Release, they don't depend
whether there are ContentType.{h,cpp} files or not). Without #ifdef it looks
cleaner, doesn't?

[Eric, 2015-01-13 16:23:49]

On 2015/01/13 13:13:14, sergei wrote:
> 
> I'm not sure that it makes sense because linker removes unused code anyway
(JIC,
> I've checked the sizes of addon DLL and engine exe in Release, they don't
depend
> whether there are ContentType.{h,cpp} files or not). Without #ifdef it looks
> cleaner, doesn't?

Well, we are already putting the rest of the debugging-only support functions
inside #ifdef blocks. If you want to remove it here we should remove it
everywhere else, too.

[Eric, 2015-01-13 17:29:36]

As much as I might seem to be complaining, the latest patch set looks much
cleaner than the previous ones.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/adbl...
adblockplus.gyp:48: 'include_dirs': [
GYP has the syntax 'all_dependent_settings' and 'direct_dependent_settings' that
can propagate 'include_dirs' to dependent project without needing to explicitly
declare them everywhere there's a library dependency.

See http://codereview.adblockplus.org/6216090891845632/ for how this is used in
common.gypi.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/engine/Main.cpp:99:
ReferrerMapping::FrameIndicator::FRAME_INDICATOR_NOT_FRAME;
The indentation is off.

Also, the expression would be easier to read with parentheses around the
condition in the first line.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/engine/Main.cpp:101: referrerMapping.Add(url, documentUrl, isFrame);
ReferrerMapping::Add() is declared with only two arguments. Presumably this
change requires some change to libadblockplus. 

Shouldn't there be a change to .hgsubstate in the present review? The code
should be reviewed as committed.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/plugin/AdblockPlusClient.cpp:293: request << Communication::PROC_MATCHES <<
ToUtf8String(url) << static_cast<int32_t>(contentType) << ToUtf8String(domain);
I still think it would be good to define operator<< for
FilterEngine::ContentType, because this cast looks like it came from outer space
here.

In other words, defining an operator saves the need for a comment here to
explain why there's a cast.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
Again, why not just use the existing implementation?

You yourself just argued in this very review (regarding debug-only functions)
that there's no increase in DLL file size for functions that are present in a
library but never referenced. Why is that true for debug-only functions but not
true for all the stuff in libadblockplus that we don't need?

[sergei, 2015-01-28 13:44:44]

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/adbl...
adblockplus.gyp:48: 'include_dirs': [
On 2015/01/13 17:29:36, Eric wrote:
> GYP has the syntax 'all_dependent_settings' and 'direct_dependent_settings'
that
> can propagate 'include_dirs' to dependent project without needing to
explicitly
> declare them everywhere there's a library dependency.
> 
> See http://codereview.adblockplus.org/6216090891845632/ for how this is used
in
> common.gypi.

Thanks for the tip.

Although I planned to get rid of it in the future because we haven't needed
ContentType in 'shared' project, Now we need it because I've put operators `>>`
and `<<` for ContentType there.

Changed.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/engine/Main.cpp:99:
ReferrerMapping::FrameIndicator::FRAME_INDICATOR_NOT_FRAME;
On 2015/01/13 17:29:36, Eric wrote:
> The indentation is off.
> 
> Also, the expression would be easier to read with parentheses around the
> condition in the first line.

Indentation is fixed.
I don't find it easier to read with parentheses.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/engine/Main.cpp:101: referrerMapping.Add(url, documentUrl, isFrame);
On 2015/01/13 17:29:36, Eric wrote:
> ReferrerMapping::Add() is declared with only two arguments. Presumably this
> change requires some change to libadblockplus. 
> 
> Shouldn't there be a change to .hgsubstate in the present review? The code
> should be reviewed as committed.

Sure, the main aim of the whole change is to update to the recent state of
libadblockplus. Sorry, I forgot to put .hgsubstate. Now, it's fixed.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/plugin/AdblockPlusClient.cpp:293: request << Communication::PROC_MATCHES <<
ToUtf8String(url) << static_cast<int32_t>(contentType) << ToUtf8String(domain);
On 2015/01/13 17:29:36, Eric wrote:
> I still think it would be good to define operator<< for
> FilterEngine::ContentType, because this cast looks like it came from outer
space
> here.
> 
> In other words, defining an operator saves the need for a comment here to
> explain why there's a cast.

done.

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
On 2015/01/13 17:29:36, Eric wrote:
> Again, why not just use the existing implementation?
> 
> You yourself just argued in this very review (regarding debug-only functions)
> that there's no increase in DLL file size for functions that are present in a
> library but never referenced. Why is that true for debug-only functions but
not
> true for all the stuff in libadblockplus that we don't need?

I've tried it and the size is indeed suspiciously the same (in Release). BTW, in
Debug with libadblockplus the size of the DLL is about 13MB and without is
2.5MB. However I'm not confident that the linker is enough smart to remove all
not used stuff from libadblockplus. Also pay attention that libadblockplus
references v8 projects, even if they don't hurt right now, nobody can guarantee
that it won't hurt in the future after updating of v8. So, I would like to avoid
any linking with v8 for the addon DLL.

[Oleksandr, 2015-01-29 15:36:29]

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5661458385862656/src/...
src/shared/ContentType.cpp:3: // Based on impl from libadblockplus
On 2015/01/28 13:44:45, sergei wrote:
> On 2015/01/13 17:29:36, Eric wrote:
> > Again, why not just use the existing implementation?
> > 
> > You yourself just argued in this very review (regarding debug-only
functions)
> > that there's no increase in DLL file size for functions that are present in
a
> > library but never referenced. Why is that true for debug-only functions but
> not
> > true for all the stuff in libadblockplus that we don't need?
> 
> I've tried it and the size is indeed suspiciously the same (in Release). BTW,
in
> Debug with libadblockplus the size of the DLL is about 13MB and without is
> 2.5MB. However I'm not confident that the linker is enough smart to remove all
> not used stuff from libadblockplus. Also pay attention that libadblockplus
> references v8 projects, even if they don't hurt right now, nobody can
guarantee
> that it won't hurt in the future after updating of v8. So, I would like to
avoid
> any linking with v8 for the addon DLL.

While I personally agree that it would be best to have all the ContentType stuff
in a separate file in libadblockplus - that is not the case now and who knows if
it ever will be. Including FilterEngine.h directly now doesn't introduce any
problems that we know of and it results in less code duplication and better
readability, IMO. So I think at this stage it is best to just use the existing
implementation as Eric suggests.

[sergei, 2015-01-29 16:07:15]

changed to link with libadblockplus

[Oleksandr, 2015-01-30 12:39:11]

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
src/engine/Main.cpp:115: ReferrerMapping::FrameIndicator::FRAME_INDICATOR_FRAME:
This doesn't seem to be defined in the referenced libadblockplus version, is it?

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
src/shared/ContentType.cpp:8: InputBuffer& operator>>(InputBuffer& in,
FilterEngine::ContentType& contentType)
This seems like a lot of code for very little purpose. How about we just remove
the ContentType files in general? We were fine without overloading these in your
previous change-sets

[sergei, 2015-01-30 13:20:06]

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
src/engine/Main.cpp:115: ReferrerMapping::FrameIndicator::FRAME_INDICATOR_FRAME:
On 2015/01/30 12:39:11, Oleksandr wrote:
> This doesn't seem to be defined in the referenced libadblockplus version, is
it?

Sorry, I accidentally was working with my local version which is a little bit
ahead of the current state.

Now it's removed.

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
src/shared/ContentType.cpp:8: InputBuffer& operator>>(InputBuffer& in,
FilterEngine::ContentType& contentType)
On 2015/01/30 12:39:11, Oleksandr wrote:
> This seems like a lot of code for very little purpose. How about we just
remove
> the ContentType files in general? We were fine without overloading these in
your
> previous change-sets

I've removed these files.

[Oleksandr, 2015-01-30 14:17:35]

Great! LGTM

[Eric, 2015-02-02 06:58:27]

LGTM, because this has taken long enough already and the comments I have could
all be done in future change sets.

1) I'd recommend removing all the unnecessary class members in the present
change set, if it's not too much trouble. If it is, we should make the changes
immediately thereafter. Per our code quality meeting, it is definitions like
these that are in the wrong place that make the code hard to read.

2) I think it would be cleaner to use the typedef I suggested, but I'm not
wedded to it.

3) I prefer defining the stream overloads, but I would rather not argue the
point in the present review.

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
File src/shared/ContentType.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5635415851663360/src/...
src/shared/ContentType.cpp:8: InputBuffer& operator>>(InputBuffer& in,
FilterEngine::ContentType& contentType)
On 2015/01/30 12:39:11, Oleksandr wrote:
> This seems like a lot of code for very little purpose. How about we just
remove
> the ContentType files in general? We were fine without overloading these in
your
> previous change-sets

Sergei added this at my suggestion in order to gather the representation of
ContentType into a single place. The way it is now we have casts appearing out
of nowhere. Defining these operators is a better design.

I'm not going to argue the point strongly, but using these definitions is better
than not using them.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/AdblockPlusClient.h:70: bool ShouldBlock(const std::wstring& src,
AdblockPlus::FilterEngine::ContentType contentType, const std::wstring& domain,
bool addDebug=false);
All these declarations would be a lot cleaner if we were to hoist the definition
of ContentType into this class.

    typedef AdblockPlus::FilterEngine::ContentType ContentType;

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginFilter.cpp:274: m_contentType = filter.m_contentType;
Here it is in the copy constructor.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginFilter.cpp:291: m_isThirdParty(false),
m_contentType(AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_OTHER),
Here it is in the default constructor.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginFilter.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginFilter.h:110: AdblockPlus::FilterEngine::ContentType
m_contentType;
This member variable isn't used. It only appears in the default constructor and
the copy constructor.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.cpp:96: ,
m_contentType(ContentType::CONTENT_TYPE_OTHER)
Removing member variables means removing initializers.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.cpp:323: m_contentType =
GetContentType(ATL::CString(acceptHeader.c_str()), m_boundDomain, src);
Declare this variable 'auto' on this line.

Well, it also needs to be renamed to remove the "m_" prefix.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.h:36: AdblockPlus::FilterEngine::ContentType
m_contentType;
This variable is only used as a local variable inside
WBPassthruSink::BeginningTransaction(). It really doesn't belong as a member
variable.

It can be eliminated and declared 'auto' where it's used. There's also a
constructor initialization for it that needs to be removed.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.h:37: std::wstring m_boundDomain;
If you want to, this variable is also a local variable needlessly declared as a
member variable, and in the same function as m_contentType.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.h:42: AdblockPlus::FilterEngine::ContentType
GetContentType(const CString& mimeType, const std::wstring& domain, const
std::wstring& src);
These functions are only used (ultimately) by
WBPassthruSink::BeginningTransaction(). (The first two are each called once by
the third). In addition, they don't reference any class members, neither
ordinary nor static class members.

Thus these three declarations can be removed entirely from this class definition
and converted to plain functions where they're currently defined.

They could be also put in an anonymous namespace, but there's no pressing need
for that, especially in the present change set.

[sergei, 2015-02-04 12:51:43]

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginFilter.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginFilter.h:110: AdblockPlus::FilterEngine::ContentType
m_contentType;
On 2015/02/02 06:58:27, Eric wrote:
> This member variable isn't used. It only appears in the default constructor
and
> the copy constructor.

Excellent!
Thanks, removed.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.h:37: std::wstring m_boundDomain;
On 2015/02/02 06:58:27, Eric wrote:
> If you want to, this variable is also a local variable needlessly declared as
a
> member variable, and in the same function as m_contentType.

Right, they were introduced as members when we were using them in Start and in
BeginningTransaction. Now there is no necessity for that.
I've created a ticket #1937.

http://codereview.adblockplus.org/5316782940225536/diff/5180918117433344/src/...
src/plugin/PluginWbPassThrough.h:42: AdblockPlus::FilterEngine::ContentType
GetContentType(const CString& mimeType, const std::wstring& domain, const
std::wstring& src);
On 2015/02/02 06:58:27, Eric wrote:
> These functions are only used (ultimately) by
> WBPassthruSink::BeginningTransaction(). (The first two are each called once by
> the third). In addition, they don't reference any class members, neither
> ordinary nor static class members.
> 
> Thus these three declarations can be removed entirely from this class
definition
> and converted to plain functions where they're currently defined.
> 
> They could be also put in an anonymous namespace, but there's no pressing need
> for that, especially in the present change set.

I completely agree with it, it's also in the ticket #1937.

[Eric, 2015-02-04 17:02:40]

Patch Set 10 LGTM

[Oleksandr, 2015-02-04 21:19:32]

LGTM