Issue 5165 - Remove SubscriptionPtr

Issue 5165 - Remove SubscriptionPtr

[sergei, 2017-04-21 15:19:05]

Could you please make constructor accepting JsValue&& as protected, add
FilterEngine as a friend class, so only the latter can instantiate Subscription
(I hope we are not doing it anywhere else) and add proper copy and move
constructors?

[hub, 2017-04-21 15:46:38]

On 2017/04/21 15:19:05, sergei wrote:
> Could you please make constructor accepting JsValue&& as protected, add
> FilterEngine as a friend class, so only the latter can instantiate
Subscription
> (I hope we are not doing it anywhere else) and add proper copy and move
> constructors?

done

[sergei, 2017-04-21 17:51:22]

LGTM, though move constructor is absent

[sergei, 2017-04-24 08:18:30]

Could you please add move-ctrs? They are essential for performance, especially
on android. Copy constructors cause the blocking of the execution of JS, but
move-ctrs won't do it.

[hub, 2017-04-24 13:45:59]

On 2017/04/24 08:18:30, sergei wrote:
> Could you please add move-ctrs? They are essential for performance, especially
> on android. Copy constructors cause the blocking of the execution of JS, but
> move-ctrs won't do it.

Added move ctor. The copy ctor end up being deleted.

[sergei, 2017-04-24 14:12:36]

https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:130: Subscription(Subscription&&) = default;
On windows we are currently bound to MSVS2012 and cannot use " = default". One
has to define the implementation manually.

https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:130: Subscription(Subscription&&) = default;
It would be better to put the ctrs on the top of the public methods.

https://codereview.adblockplus.org/29419623/diff/29420587/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29419623/diff/29420587/test/FilterEngine.c...
test/FilterEngine.cpp:735: aaSubscription.reset(new
Subscription(std::move(subscriptions[0])));
I would not expect that android code is never copying the objects, so moving
here is not necessary, just add a copy-ctr.

[hub, 2017-04-24 15:02:42]

updated patch.

https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:130: Subscription(Subscription&&) = default;
On 2017/04/24 14:12:36, sergei wrote:
> On windows we are currently bound to MSVS2012 and cannot use " = default". One
> has to define the implementation manually.

Done.

https://codereview.adblockplus.org/29419623/diff/29420587/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29419623/diff/29420587/test/FilterEngine.c...
test/FilterEngine.cpp:735: aaSubscription.reset(new
Subscription(std::move(subscriptions[0])));
On 2017/04/24 14:12:36, sergei wrote:
> I would not expect that android code is never copying the objects, so moving
> here is not necessary, just add a copy-ctr.

Done.

[sergei, 2017-04-24 18:19:36]

https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:108: Subscription& operator=(const
Subscription&);
The same comments as in https://codereview.adblockplus.org/29419629/#msg8

[hub, 2017-04-24 19:16:51]

updated patch.

https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:108: Subscription& operator=(const
Subscription&);
On 2017/04/24 18:19:36, sergei wrote:
> The same comments as in https://codereview.adblockplus.org/29419629/#msg8

Done.

[sergei, 2017-04-25 07:38:26]

LGTM