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

Issue 29785555: Issue 6684 - Issue Reporter Active Tab Screenshot

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by a.giammarchi
Modified:
5 days, 19 hours ago
CC:
Thomas Greiner, saroyanm, kzar
Visibility:
Public.

Description

Ready for review.

Patch Set 1 #

Total comments: 9

Patch Set 2 : using active + inlined object literal #

Total comments: 8

Patch Set 3 : solved all nitpicks #

Patch Set 4 : added issue reporter new icons #

Patch Set 5 : avoid screenshot failure #

Patch Set 6 : cleaner procedure through IR tab #

Patch Set 7 : better comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -0 lines) Patch
M metadata.chrome View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M popup.js View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 14
a.giammarchi
Ready for review
3 weeks, 5 days ago (2018-05-22 15:07:20 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29785555/diff/29785556/popup.js File popup.js (right): https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode107 popup.js:107: browser.tabs.captureVisibleTab( I suppose we also have to add a ...
3 weeks, 5 days ago (2018-05-22 15:29:00 UTC) #2
a.giammarchi
answered + question https://codereview.adblockplus.org/29785555/diff/29785556/popup.js File popup.js (right): https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode107 popup.js:107: browser.tabs.captureVisibleTab( On 2018/05/22 15:28:59, Sebastian Noack ...
3 weeks, 5 days ago (2018-05-22 15:55:31 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29785555/diff/29785556/popup.js File popup.js (right): https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode107 popup.js:107: browser.tabs.captureVisibleTab( On 2018/05/22 15:55:31, a.giammarchi wrote: > On 2018/05/22 ...
3 weeks, 5 days ago (2018-05-22 16:01:10 UTC) #4
a.giammarchi
applied required changes https://codereview.adblockplus.org/29785555/diff/29785556/popup.js File popup.js (right): https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode122 popup.js:122: screenshot On 2018/05/22 15:28:59, Sebastian Noack ...
3 weeks, 5 days ago (2018-05-22 16:14:09 UTC) #5
Sebastian Noack
The changes look mostly good to me, just a couple more nits. https://codereview.adblockplus.org/29785555/diff/29787558/popup.js File popup.js ...
3 weeks, 4 days ago (2018-05-23 14:48:59 UTC) #6
a.giammarchi
solved all nitpicks https://codereview.adblockplus.org/29785555/diff/29787558/popup.js File popup.js (right): https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode106 popup.js:106: // capture the current visible tab ...
3 weeks, 4 days ago (2018-05-23 15:06:36 UTC) #7
Sebastian Noack
LGTM
3 weeks, 4 days ago (2018-05-23 15:10:43 UTC) #8
a.giammarchi
On 2018/05/23 15:10:43, Sebastian Noack wrote: > LGTM Thanks Sebastian. How do I proceed now, ...
3 weeks, 4 days ago (2018-05-23 15:18:53 UTC) #9
Sebastian Noack
On 2018/05/23 15:18:53, a.giammarchi wrote: > On 2018/05/23 15:10:43, Sebastian Noack wrote: > > LGTM ...
3 weeks, 4 days ago (2018-05-23 16:07:56 UTC) #10
a.giammarchi
On 2018/05/23 16:07:56, Sebastian Noack wrote: > I see. In that case, I would prefer ...
3 weeks, 3 days ago (2018-05-24 10:39:44 UTC) #11
Sebastian Noack
On 2018/05/24 10:39:44, a.giammarchi wrote: > On 2018/05/23 16:07:56, Sebastian Noack wrote: > > I ...
3 weeks, 3 days ago (2018-05-24 11:39:53 UTC) #12
a.giammarchi
Apologies for the various updates, but this version delegates to the issue reporter the task ...
6 days, 1 hour ago (2018-06-12 08:13:20 UTC) #13
Thomas Greiner
5 days, 19 hours ago (2018-06-12 13:57:18 UTC) #14
LGTM
Sign in to reply to this message.

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