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

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

Can't Edit
Can't Publish+Mail
Start Review
2 years, 4 months ago by saroyanm
2 years, 4 months ago
Thomas Greiner
Sebastian Noack


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


Total messages: 3
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
2 years, 4 months ago (2017-09-29 16:58:56 UTC) #3
File messageResponder.js (right):

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
> only provides subscription property values so renaming it to something like
> "props" would clarify what data we expect to find in it.


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`,
> 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