Issue 5808 - Easyprivacy URL is being shown instead of the title

Issue 5808 - Easyprivacy URL is being shown instead of the title

[saroyanm, 2017-09-29 14:35:17]

This is ready, can anybody of you guys have a look please ?

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:149: if ("title" in message)
Lack of this check were causing the unexpected behavior, but still can't say for
sure why the URL were assigned to the title, we are not assigning the URL to the
title as in the current/old option page.

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:155: if (subscription instanceof DownloadableSubscription &&
Previously we were not checking if it's a DownloadableSubscription in "toggle
message", I think it wasn't intentional, but will value your comment here.

[Thomas Greiner, 2017-09-29 16:39:19]

LGTM with an optional comment

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:146: function addSubscription(subscription, message)
Detail: The name "message" here is quite generic. For this function the argument
only provides subscription property values so renaming it to something like
"props" would clarify what data we expect to find in it.

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:149: if ("title" in message)
On 2017/09/29 14:35:17, saroyanm wrote:
> Lack of this check were causing the unexpected behavior, but still can't say
for
> sure why the URL were assigned to the title, we are not assigning the URL to
the
> title as in the current/old option page.
> 

The URL is used as a fallback whenever there's no title (see
https://hg.adblockplus.org/adblockplusui/file/tip/new-options.js#l89) so the
title seems to have mistakenly been overwritten with either `undefined`, `null`
or `""` which caused the URL to show up.

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:155: if (subscription instanceof DownloadableSubscription &&
On 2017/09/29 14:35:17, saroyanm wrote:
> Previously we were not checking if it's a DownloadableSubscription in "toggle
> message", I think it wasn't intentional, but will value your comment here.

Acknowledged.

[saroyanm, 2017-09-29 16:58:56]

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:146: function addSubscription(subscription, message)
On 2017/09/29 16:39:18, Thomas Greiner wrote:
> Detail: The name "message" here is quite generic. For this function the
argument
> only provides subscription property values so renaming it to something like
> "props" would clarify what data we expect to find in it.

Done.

https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js...
messageResponder.js:149: if ("title" in message)
On 2017/09/29 16:39:18, Thomas Greiner wrote:
> On 2017/09/29 14:35:17, saroyanm wrote:
> > Lack of this check were causing the unexpected behavior, but still can't say
> for
> > sure why the URL were assigned to the title, we are not assigning the URL to
> the
> > title as in the current/old option page.
> > 
> 
> The URL is used as a fallback whenever there's no title (see
> https://hg.adblockplus.org/adblockplusui/file/tip/new-options.js#l89) so the
> title seems to have mistakenly been overwritten with either `undefined`,
`null`
> or `""` which caused the URL to show up.

I see, thanks for the info.