Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(400)

Issue 29559647: Issue 5808 - Easyprivacy URL is being shown instead of the title (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 4 months ago by saroyanm
Modified:
2 years, 4 months ago
Reviewers:
Thomas Greiner
CC:
Sebastian Noack
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 7

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -17 lines) Patch
M messageResponder.js View 1 4 chunks +21 lines, -17 lines 0 comments Download

Messages

Total messages: 3
saroyanm
This is ready, can anybody of you guys have a look please ? https://codereview.adblockplus.org/29559647/diff/29559648/messageResponder.js File ...
2 years, 4 months ago (2017-09-29 14:35:17 UTC) #1
Thomas Greiner
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#newcode146 messageResponder.js:146: function addSubscription(subscription, message) Detail: ...
2 years, 4 months ago (2017-09-29 16:39:19 UTC) #2
saroyanm
2 years, 4 months ago (2017-09-29 16:58:56 UTC) #3
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5