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

Issue 29716600: Issue 6292 - Make issue reporter compatible with test server (Closed)

Created:
March 7, 2018, 3:43 p.m. by saroyanm
Modified:
March 13, 2018, 11:20 a.m.
Reviewers:
a.giammarchi
CC:
Thomas Greiner
Visibility:
Public.

Description

Issue 6292 - Make issue reporter compatible with test server

Patch Set 1 : #

Total comments: 18

Patch Set 2 : Rebased #

Patch Set 3 : Addressed Andrea's comments #

Total comments: 2

Patch Set 4 : Fixed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -3 lines) Patch
M background.js View 1 5 chunks +8 lines, -0 lines 0 comments Download
M ext/content.js View 1 2 3 2 chunks +11 lines, -1 line 0 comments Download
M issue-reporter.html View 1 chunk +2 lines, -0 lines 0 comments Download
M issue-reporter.js View 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 11
saroyanm
This is ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html File issue-reporter.html (right): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html#newcode104 issue-reporter.html:104: <button id="showDataClose" class="i18n_cancel ...
March 7, 2018, 4:17 p.m. (2018-03-07 16:17:00 UTC) #1
a.giammarchi
Overall OK but I've suggested a couple of improvements. Please let me know what you ...
March 7, 2018, 5:11 p.m. (2018-03-07 17:11:04 UTC) #2
a.giammarchi
updated a comment https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On ...
March 7, 2018, 5:17 p.m. (2018-03-07 17:17:50 UTC) #3
saroyanm
https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/07 17:17:49, a.giammarchi ...
March 8, 2018, 3:14 p.m. (2018-03-08 15:14:28 UTC) #4
a.giammarchi
answered to a couple of points. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = ...
March 8, 2018, 5:27 p.m. (2018-03-08 17:27:35 UTC) #5
saroyanm
https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/08 17:27:34, a.giammarchi ...
March 8, 2018, 6:16 p.m. (2018-03-08 18:16:47 UTC) #6
saroyanm
Thanks Andrea, ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = ...
March 12, 2018, 3:45 p.m. (2018-03-12 15:45:21 UTC) #7
saroyanm
Thanks Andrea, ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newcode104 ext/content.js:104: let result = ...
March 12, 2018, 3:45 p.m. (2018-03-12 15:45:22 UTC) #8
a.giammarchi
LGTM but there's a nitpick you can ignore if you want. https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js File ext/content.js (right): ...
March 12, 2018, 3:53 p.m. (2018-03-12 15:53:09 UTC) #9
saroyanm
Thanks, done. https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js#newcode106 ext/content.js:106: const error = new Error("Tab cannot be ...
March 12, 2018, 4:09 p.m. (2018-03-12 16:09:52 UTC) #10
a.giammarchi
March 12, 2018, 5:19 p.m. (2018-03-12 17:19:42 UTC) #11
On 2018/03/12 16:09:52, saroyanm wrote:
> Done. I'm using one ternary operator to make this simpler.

even better. LGTM

Powered by Google App Engine
This is Rietveld