Issue 6684 - Issue Reporter Active Tab Screenshot

Already landed.

[a.giammarchi, 2018-05-22 15:07:20]

Ready for review

[Sebastian Noack, 2018-05-22 15:29:00]

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 new permission for that (in metadata.chrome)?
Also did you verify that adding that permission won't cause the extension being
disabled for existing users (some permissions would)?

https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode122
popup.js:122: screenshot
Nit: It appears that this literal would fit on one line without wrapping.

https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode130
popup.js:130: selected: true,
The "selected" property is deprecated, use "active" instead.

[a.giammarchi, 2018-05-22 15:55:31]

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 wrote:
> I suppose we also have to add a new permission for that (in metadata.chrome)?
> Also did you verify that adding that permission won't cause the extension
being
> disabled for existing users (some permissions would)?

AFAIK there's no need for extra permission, tabs would do (tested in b oth
chrome and FF)

https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode130
popup.js:130: selected: true,
On 2018/05/22 15:28:59, Sebastian Noack wrote:
> The "selected" property is deprecated, use "active" instead.

Should I use both to be sure it works? I think I used selected after trying in
older Chrome/FF, but I'm not fully sure anymore.

Thanks for hints.

[Sebastian Noack, 2018-05-22 16:01:10]

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 15:28:59, Sebastian Noack wrote:
> > I suppose we also have to add a new permission for that (in
metadata.chrome)?
> > Also did you verify that adding that permission won't cause the extension
> being
> > disabled for existing users (some permissions would)?
> 
> AFAIK there's no need for extra permission, tabs would do (tested in b oth
> chrome and FF)

Oh cool, never mind then.

https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode130
popup.js:130: selected: true,
On 2018/05/22 15:55:31, a.giammarchi wrote:
> On 2018/05/22 15:28:59, Sebastian Noack wrote:
> > The "selected" property is deprecated, use "active" instead.
> 
> Should I use both to be sure it works? I think I used selected after trying in
> older Chrome/FF, but I'm not fully sure anymore.
> 
> Thanks for hints.

That is not necessary, "active" was introduced long before the oldest version of
any browser we support.

[a.giammarchi, 2018-05-22 16:14:09]

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 wrote:
> Nit: It appears that this literal would fit on one line without wrapping.

Done.

https://codereview.adblockplus.org/29785555/diff/29785556/popup.js#newcode130
popup.js:130: selected: true,
On 2018/05/22 16:01:10, Sebastian Noack wrote:
> On 2018/05/22 15:55:31, a.giammarchi wrote:
> > On 2018/05/22 15:28:59, Sebastian Noack wrote:
> > > The "selected" property is deprecated, use "active" instead.
> > 
> > Should I use both to be sure it works? I think I used selected after trying
in
> > older Chrome/FF, but I'm not fully sure anymore.
> > 
> > Thanks for hints.
> 
> That is not necessary, "active" was introduced long before the oldest version
of
> any browser we support.

Done.

[Sebastian Noack, 2018-05-23 14:48:59]

The changes look mostly good to me, just a couple more nits.

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
Nit: This comment seems superfluous (it essentially reads like the source code
in the line below).

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode110
popup.js:110: (screenshot) =>
Nit: We omit the optional parenthesis in arrow functions with a single argument.

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode117
popup.js:117: if (event.type === "issue-reporter")
Nit: We prefer == over ===, as per the Mozilla coding style guide which our
coding style guide is based on.

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode125
popup.js:125: // create an active/selected issue reporter tab
Nit: This comment seems superfluous as well.

[a.giammarchi, 2018-05-23 15:06:36]

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
On 2018/05/23 14:48:59, Sebastian Noack wrote:
> Nit: This comment seems superfluous (it essentially reads like the source code
> in the line below).

Done.

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode110
popup.js:110: (screenshot) =>
On 2018/05/23 14:48:59, Sebastian Noack wrote:
> Nit: We omit the optional parenthesis in arrow functions with a single
argument.

Done.

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode117
popup.js:117: if (event.type === "issue-reporter")
On 2018/05/23 14:48:59, Sebastian Noack wrote:
> Nit: We prefer == over ===, as per the Mozilla coding style guide which our
> coding style guide is based on.

Done.

https://codereview.adblockplus.org/29785555/diff/29787558/popup.js#newcode125
popup.js:125: // create an active/selected issue reporter tab
On 2018/05/23 14:48:59, Sebastian Noack wrote:
> Nit: This comment seems superfluous as well.

Done.

[Sebastian Noack, 2018-05-23 15:10:43]

LGTM

[a.giammarchi, 2018-05-23 15:18:53]

On 2018/05/23 15:10:43, Sebastian Noack wrote:
> LGTM

Thanks Sebastian. How do I proceed now, since this change is not needed for the
incoming release? Should I just mail you the diff?

[Sebastian Noack, 2018-05-23 16:07:56]

On 2018/05/23 15:18:53, a.giammarchi wrote:
> On 2018/05/23 15:10:43, Sebastian Noack wrote:
> > LGTM
> 
> Thanks Sebastian. How do I proceed now, since this change is not needed for
the
> incoming release? Should I just mail you the diff?

I see. In that case, I would prefer if we perform the changes here together with
the future dependency update that will import the related UI changes, requiring
these adaptions.

[a.giammarchi, 2018-05-24 10:39:44]

On 2018/05/23 16:07:56, Sebastian Noack wrote:
> I see. In that case, I would prefer if we perform the changes here together
with
> the future dependency update that will import the related UI changes,
requiring
> these adaptions.

works for me, and please have a look also at the meta changes, I forgot the new
issue reporter also needs two new images.

Thanks

[Sebastian Noack, 2018-05-24 11:39:53]

On 2018/05/24 10:39:44, a.giammarchi wrote:
> On 2018/05/23 16:07:56, Sebastian Noack wrote:
> > I see. In that case, I would prefer if we perform the changes here together
> with
> > the future dependency update that will import the related UI changes,
> requiring
> > these adaptions.
> 
> works for me, and please have a look also at the meta changes, I forgot the
new
> issue reporter also needs two new images.
> 
> Thanks

Looks still good to me (once the dependency update is in).

[a.giammarchi, 2018-06-12 08:13:20]

Apologies for the various updates, but this version delegates to the issue
reporter the task to take the screenshot of the currently active tab, without
bothering with the postMessage dance which also stopped working at some point
(but it works again now).

This is again ready for review.

[Thomas Greiner, 2018-06-12 13:57:18]

LGTM