Issue 29317074: Issue 2693 - Update adblockplus dependency - Code Review

Rietveld Code Review Tool

Help | Bug tracker | Discussion group | Source code

Issues

Issue 29317074: Issue 2693 - Update adblockplus dependency (Closed)

Created:
June 18, 2015, 3:31 p.m. by sergei

Modified:
July 3, 2015, 8:10 a.m.

Reviewers:
Felix Dahlke

CC:
Oleksandr, Eric

Visibility:
Public.

Patch Set 1 #

Patch Set 2 : fix js event name #

Total comments: 12

Patch Set 3 : fix according to comments and update revision #

Total comments: 6

Patch Set 4 : address comments #

Total comments: 2

Patch Set 5 : address NITs #

Patch Set 6 : rebase #

Created: July 2, 2015, 1:37 p.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+158 lines, -49 lines)			Patch
	M	dependencies	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
	M	ensure_dependencies.py	View		9 chunks	+53 lines, -11 lines	0 comments	Download
	M	include/AdblockPlus/FilterEngine.h	View	1 2 3 4 5	3 chunks	+22 lines, -4 lines	0 comments	Download
	M	lib/api.js	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
	A +	lib/notificationShowRegistration.js	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
	M	lib/prefs.js	View	1 2 3 4 5	1 chunk	+2 lines, -1 line	0 comments	Download
	M	libadblockplus.gyp	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
	M	src/FilterEngine.cpp	View	1 2 3 4 5	2 chunks	+28 lines, -3 lines	0 comments	Download
	M	test/Notification.cpp	View	1 2 3 4	7 chunks	+46 lines, -24 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

Felix Dahlke

Please list the issues we're importing here in the issue, as in: https://issues.adblockplus.org/ticket/2437 As there, ...

June 25, 2015, 6:35 p.m. (2015-06-25 18:35:49 UTC) #2

sergei

On 2015/06/25 18:35:49, Felix Dahlke wrote: > Please list the issues we're importing here in ...

June 26, 2015, 2:21 p.m. (2015-06-26 14:21:17 UTC) #3

On 2015/06/25 18:35:49, Felix Dahlke wrote:
> Please list the issues we're importing here in the issue, as in:
> https://issues.adblockplus.org/ticket/2437
> 
> As there, if a change has no effect on libadblockplus, listing the issue is
> sufficient. If a change can have an effect however, it should be briefly
summed
> up.
I have updated the issue, what do you think about current appearance?

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

https://codereview.adblockplus.org/29317074/diff/29317165/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:257: void
SetNotificationAvailableCallback(const NotificationAvailableCallback& value);
On 2015/06/25 18:35:48, Felix Dahlke wrote:
> I'd prefer to call this SetNotificationShowCallback - so we have the same
> terminology in Core and Libadblockplus.

I think the callback should be called `ShowNotificationCallback` rather than
`NotificationShowCallback`, in that case it looks logical to name the method
`SetShowNotificationCallback` (it's in the updated version). Agree?

https://codereview.adblockplus.org/29317074/diff/29317165/lib/api.js
File lib/api.js (right):

https://codereview.adblockplus.org/29317074/diff/29317165/lib/api.js#newcode31
lib/api.js:31: Notification.addShowListener(function (notification)
On 2015/06/25 18:35:48, Felix Dahlke wrote:
> How about moving this, in the spirit of filterUpdateRegistration.js to a
> dedicated file like notificationShowRegistration.js?

Done.

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.c...
test/Notification.cpp:53: bool isCallbackCalled;
On 2015/06/25 18:35:49, Felix Dahlke wrote:
> Can't we derive this by checking if the notification pointer is pointing
> somewhere? Then we could return a NotificationPtr instead of
> PeekNotificationResult.

Yes we can. I just wanted to be sure that it's really not called rather than we
cannot construct `AdblockPlus::Notification` object.

Fixed to use `NotificationPtr`.

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.c...
test/Notification.cpp:56: PeekNotificationResult PeekNotification(const
std::string& url = std::string())
On 2015/06/25 18:35:49, Felix Dahlke wrote:
> Nit: "peek" sounds a bit weird to me here, why not just "show"?
Because I don't mark the notification as shown, so we can say that it stays in
some internal queue, there is even a test for that case
`TEST_F(NotificationTest, MarkAsShown)`.
 
> We've got a very similar function in the JS tests BTW:
> 
>   function showNotifications(url)
>   {
>     let shownNotifications = [];
>     function showListener(notification)
>     {
>       shownNotifications.push(notification);
>       Notification.markAsShown(notification.id);
>     }
>     Notification.addShowListener(showListener);
>     Notification.showNext(url);
>     Notification.removeShowListener(showListener);
>     return shownNotifications;
>   }
> 
> We're working with an array of notifications instead of a single one there, to
> ensure the show handler is not being called multiple times. But we don't have
to
> do that here as well IMO.
I also thought about multiple times calls but decided to check merely that it's
called with the correct notification object.

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.c...
test/Notification.cpp:59:
filterEngine->SetNotificationAvailableCallback([&retValue](const
NotificationPtr& notification)
On 2015/06/25 18:35:49, Felix Dahlke wrote:
> Still C++03 here :)

Done.

Felix Dahlke

On 2015/06/26 14:21:17, sergei wrote: > On 2015/06/25 18:35:49, Felix Dahlke wrote: > > Please ...

June 30, 2015, 7:45 p.m. (2015-06-30 19:45:34 UTC) #4

On 2015/06/26 14:21:17, sergei wrote:
> On 2015/06/25 18:35:49, Felix Dahlke wrote:
> > Please list the issues we're importing here in the issue, as in:
> > https://issues.adblockplus.org/ticket/2437
> > 
> > As there, if a change has no effect on libadblockplus, listing the issue is
> > sufficient. If a change can have an effect however, it should be briefly
> summed
> > up.
> I have updated the issue, what do you think about current appearance?

Looks good now!

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

https://codereview.adblockplus.org/29317074/diff/29317165/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:257: void
SetNotificationAvailableCallback(const NotificationAvailableCallback& value);
On 2015/06/26 14:21:17, sergei wrote:
> On 2015/06/25 18:35:48, Felix Dahlke wrote:
> > I'd prefer to call this SetNotificationShowCallback - so we have the same
> > terminology in Core and Libadblockplus.
> 
> I think the callback should be called `ShowNotificationCallback` rather than
> `NotificationShowCallback`, in that case it looks logical to name the method
> `SetShowNotificationCallback` (it's in the updated version). Agree?

Yeah, fair enough.

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29317074/diff/29317165/test/Notification.c...
test/Notification.cpp:56: PeekNotificationResult PeekNotification(const
std::string& url = std::string())
On 2015/06/26 14:21:17, sergei wrote:
> On 2015/06/25 18:35:49, Felix Dahlke wrote:
> > Nit: "peek" sounds a bit weird to me here, why not just "show"?
> Because I don't mark the notification as shown, so we can say that it stays in
> some internal queue, there is even a test for that case
> `TEST_F(NotificationTest, MarkAsShown)`.

Yeah right, makes sense then.

> > We've got a very similar function in the JS tests BTW:
> > 
> >   function showNotifications(url)
> >   {
> >     let shownNotifications = [];
> >     function showListener(notification)
> >     {
> >       shownNotifications.push(notification);
> >       Notification.markAsShown(notification.id);
> >     }
> >     Notification.addShowListener(showListener);
> >     Notification.showNext(url);
> >     Notification.removeShowListener(showListener);
> >     return shownNotifications;
> >   }
> > 
> > We're working with an array of notifications instead of a single one there,
to
> > ensure the show handler is not being called multiple times. But we don't
have
> to
> > do that here as well IMO.
> I also thought about multiple times calls but decided to check merely that
it's
> called with the correct notification object.

Yeah, surely sufficient, we don't need to repeat the core tests here.

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

https://codereview.adblockplus.org/29317074/diff/29321126/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:261: * available.
Guess this line can be removed?

https://codereview.adblockplus.org/29317074/diff/29321126/test/Notification.cpp
File test/Notification.cpp (right):

https://codereview.adblockplus.org/29317074/diff/29321126/test/Notification.c...
test/Notification.cpp:130:
filterEngine->SetNotificationAvailableCallback([&isNotificationCallbackCalled](const
NotificationPtr& notification)
Still C++03 here :)

https://codereview.adblockplus.org/29317074/diff/29321126/test/Notification.c...
test/Notification.cpp:208: }
Nit: Was a newline at EOF added here, or removed? If it was added, fine, but it
shouldn't be removed.

sergei

https://codereview.adblockplus.org/29317074/diff/29321126/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29317074/diff/29321126/include/AdblockPlus/FilterEngine.h#newcode261 include/AdblockPlus/FilterEngine.h:261: * available. On 2015/06/30 19:45:32, Felix Dahlke wrote: > ...

July 1, 2015, 11:46 a.m. (2015-07-01 11:46:09 UTC) #5

Felix Dahlke

LGTM with one optional and one mandatory nit. https://codereview.adblockplus.org/29317074/diff/29321249/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29317074/diff/29321249/test/Notification.cpp#newcode122 test/Notification.cpp:122: void ...

July 2, 2015, 1:23 p.m. (2015-07-02 13:23:13 UTC) #6

LGTM!

Expand All Messages | Collapse All Messages

Powered by Google App Engine

This is Rietveld