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

Issue 29604555: Issue 5965 - Handle edge case when searching for options page tab (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by kzar
Modified:
1 year, 11 months ago
Reviewers:
Wladimir Palant
CC:
Sebastian Noack
Visibility:
Public.

Description

Issue 5965 - Handle edge case when searching for options page tab

Patch Set 1 #

Patch Set 2 : Correct mistake in callback logic #

Total comments: 10

Patch Set 3 : Addressed Wladimir's initial feedback #

Total comments: 2

Patch Set 4 : Renamed urlMatch variable #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -1 line) Patch
M lib/options.js View 1 2 3 1 chunk +47 lines, -1 line 0 comments Download

Messages

Total messages: 7
kzar
Patch Set 1
2 years ago (2017-11-11 16:37:49 UTC) #1
kzar
Patch Set 2 : Correct mistake in callback logic
2 years ago (2017-11-11 16:42:11 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newcode44 lib/options.js:44: callback(optionsTab); Maybe reduce the indentation level here: if (optionsTab) ...
2 years ago (2017-11-11 18:34:15 UTC) #3
kzar
Patch Set 3 : Addressed Wladimir's initial feedback https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29604555/diff/29604558/lib/options.js#newcode44 lib/options.js:44: callback(optionsTab); ...
2 years ago (2017-11-13 12:32:58 UTC) #4
Wladimir Palant
LGTM, one nit below. https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js File lib/options.js (right): https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js#newcode67 lib/options.js:67: let urlMatches = tab.url == ...
1 year, 11 months ago (2017-11-23 11:37:13 UTC) #5
kzar
Patch Set 4 : Renamed urlMatch variable Will retest this now. https://codereview.adblockplus.org/29604555/diff/29606555/lib/options.js File lib/options.js (right): ...
1 year, 11 months ago (2017-11-23 11:51:08 UTC) #6
Wladimir Palant
1 year, 11 months ago (2017-11-23 13:51:35 UTC) #7
Message was sent while issue was closed.
LGTM
Sign in to reply to this message.

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