Issue 5164 - Remove NotificationPtr

Issue 5164 - Remove NotificationPtr

[sergei, 2017-04-21 15:30:33]

https://codereview.adblockplus.org/29419629/diff/29419630/include/AdblockPlus...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/29419629/diff/29419630/include/AdblockPlus...
include/AdblockPlus/Notification.h:60: explicit Notification(JsValue&& jsValue);
I don't think it's a good idea to make this constructor public.

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:51: Notification PeekNotification(const std::string& url =
std::string())
I think it would be better to use std::unique_ptr here.

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:56: std::placeholders::_1, std::ref(retValue)));
JIC, now we may use lambda functions

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:128: EXPECT_EQ("Title", notification->GetTexts().title);
This is not compilable. In order to run this test one has to firstly modify
notifications.js in adblockpluscore, I can fix this place later.

[hub, 2017-04-21 16:29:32]

updated patch

https://codereview.adblockplus.org/29419629/diff/29419630/include/AdblockPlus...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/29419629/diff/29419630/include/AdblockPlus...
include/AdblockPlus/Notification.h:60: explicit Notification(JsValue&& jsValue);
On 2017/04/21 15:30:33, sergei wrote:
> I don't think it's a good idea to make this constructor public.

It was for the tests. I'll find a way.

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:51: Notification PeekNotification(const std::string& url =
std::string())
On 2017/04/21 15:30:33, sergei wrote:
> I think it would be better to use std::unique_ptr here.

and it would solve the other problem with the constructor.

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:56: std::placeholders::_1, std::ref(retValue)));
On 2017/04/21 15:30:33, sergei wrote:
> JIC, now we may use lambda functions
ok will change that while I'm at it.

https://codereview.adblockplus.org/29419629/diff/29419630/test/Notification.c...
test/Notification.cpp:128: EXPECT_EQ("Title", notification->GetTexts().title);
On 2017/04/21 15:30:33, sergei wrote:
> This is not compilable. In order to run this test one has to firstly modify
> notifications.js in adblockpluscore, I can fix this place later.

I realize this code doesn't get built. Will fix the build at least.

[sergei, 2017-04-21 17:57:38]

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:63: static void NotificationAvailableCallback(const
Notification& src, std::unique_ptr<Notification>& dst)
Could you please also remove this method and move the body of it into the
lambda?

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:65: EXPECT_FALSE(src.IsUndefined());
Not sure whether we need that check.

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:158: EXPECT_TRUE(notification);
here it should stay ASSERT because if the pointer is nullptr then we should not
continue the execution of the test.

[hub, 2017-04-21 18:37:28]

updated patch

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:63: static void NotificationAvailableCallback(const
Notification& src, std::unique_ptr<Notification>& dst)
On 2017/04/21 17:57:38, sergei wrote:
> Could you please also remove this method and move the body of it into the
> lambda?

Done

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:65: EXPECT_FALSE(src.IsUndefined());
On 2017/04/21 17:57:38, sergei wrote:
> Not sure whether we need that check.

Removed.

https://codereview.adblockplus.org/29419629/diff/29419642/test/Notification.c...
test/Notification.cpp:158: EXPECT_TRUE(notification);
On 2017/04/21 17:57:38, sergei wrote:
> here it should stay ASSERT because if the pointer is nullptr then we should
not
> continue the execution of the test.

Changing this and elsewhere it applies.

[sergei, 2017-04-24 08:20:28]

could you please also add move-ctr?

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.cpp
File test/Notification.cpp (left):

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.c...
test/Notification.cpp:191: ASSERT_TRUE(notification);
this assert should not be removed.

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.c...
test/Notification.cpp:172: EXPECT_TRUE(notification);
Here should be assert.

[hub, 2017-04-24 13:55:00]

updated patch

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.cpp
File test/Notification.cpp (left):

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.c...
test/Notification.cpp:191: ASSERT_TRUE(notification);
On 2017/04/24 08:20:28, sergei wrote:
> this assert should not be removed.

not sure why this happened. putting it back.

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29419647/test/Notification.c...
test/Notification.cpp:172: EXPECT_TRUE(notification);
On 2017/04/24 08:20:28, sergei wrote:
> Here should be assert.

Acknowledged.

[sergei, 2017-04-24 18:17:53]

merely a couple of tiny things

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
include/AdblockPlus/Notification.h:62: Notification(const Notification&);
do you mind adding src arg in the headers for these methods?

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
include/AdblockPlus/Notification.h:65: Notification& operator=(const
Notification&);
What about operator=(Notification&& src)?

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

updated patch

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
include/AdblockPlus/Notification.h:62: Notification(const Notification&);
On 2017/04/24 18:17:53, sergei wrote:
> do you mind adding src arg in the headers for these methods?

Done.

https://codereview.adblockplus.org/29419629/diff/29420602/include/AdblockPlus...
include/AdblockPlus/Notification.h:65: Notification& operator=(const
Notification&);
On 2017/04/24 18:17:53, sergei wrote:
> What about operator=(Notification&& src)?

Done.

[sergei, 2017-04-24 19:28:15]

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

https://codereview.adblockplus.org/29419629/diff/29420632/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:113: Subscription& operator=(Subscription&&
src);
This change seems unrelated to this codereview, the codereview is about
Notification, for Subscription there is another codereview.

https://codereview.adblockplus.org/29419629/diff/29420632/src/Notification.cpp
File src/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29420632/src/Notification.cp...
src/Notification.cpp:83: *this = std::move(src);
It will cause an infinite recursion. It should be
static_cast<JsValue&>(*this) = std::move(src);

[hub, 2017-04-24 20:17:05]

updated patch

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

https://codereview.adblockplus.org/29419629/diff/29420632/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:113: Subscription& operator=(Subscription&&
src);
On 2017/04/24 19:28:15, sergei wrote:
> This change seems unrelated to this codereview, the codereview is about
> Notification, for Subscription there is another codereview.

I thought for a second I had mixed up code reviews and patches. This only appear
in the interdiff because this patch is based on top of the other (the one for
the subscription). Rebasing has happened.

https://codereview.adblockplus.org/29419629/diff/29420632/src/Notification.cpp
File src/Notification.cpp (right):

https://codereview.adblockplus.org/29419629/diff/29420632/src/Notification.cp...
src/Notification.cpp:83: *this = std::move(src);
On 2017/04/24 19:28:15, sergei wrote:
> It will cause an infinite recursion. It should be
> static_cast<JsValue&>(*this) = std::move(src);

oops. same with the assignment (copy).

[sergei, 2017-04-25 07:37:16]

LGTM

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

https://codereview.adblockplus.org/29419629/diff/29420632/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:113: Subscription& operator=(Subscription&&
src);
On 2017/04/24 20:17:04, hub wrote:
> On 2017/04/24 19:28:15, sergei wrote:
> > This change seems unrelated to this codereview, the codereview is about
> > Notification, for Subscription there is another codereview.
> 
> I thought for a second I had mixed up code reviews and patches. This only
appear
> in the interdiff because this patch is based on top of the other (the one for
> the subscription). Rebasing has happened.

Acknowledged.