Noissue - Close popup after opening options page

Noissue - Close popup after opening options page

[Manish Jethani, 2017-09-08 19:41:57]

I just noticed that Firefox doesn't close the popup automatically when the
options page is opened. It needs to be closed by calling window.close.

Please test this on Edge for me, Ollie.

[Oleksandr, 2017-09-08 20:07:59]

LGTM. This is not a problem on Edge. But other browsers are affected by this as
well, aren't they?

[Sebastian Noack, 2017-09-08 20:18:57]

FWIW, ext.closePopup() has been introduced because window.close() doesn't work
on Safari. With Safari support having been dropped, we should go back to use
window.close() directly, and remove ext.closePopup. But this should be done
separately from this change, so LGTM.

Also note that we are in code freeze right now for the upcoming release for
Chrome/Opera. So please don't push anything to "master" for now.

But just an idea, how about, creating a "next" bookmark to land changes during
code freeze? They won't end up in any development build, but we could easily
just merge that bookmark after the release.

[Manish Jethani, 2017-09-08 20:57:54]

On 2017/09/08 20:07:59, Oleksandr wrote:
> LGTM. This is not a problem on Edge. But other browsers are affected by this
as
> well, aren't they?

Thanks, this affects Firefox mainly, but I was hoping it wouldn't have any
negative effects on Edge as I am unable to test it here.

[Manish Jethani, 2017-09-08 20:58:52]

On 2017/09/08 20:18:57, Sebastian Noack wrote:
> FWIW, ext.closePopup() has been introduced because window.close() doesn't work
> on Safari. With Safari support having been dropped, we should go back to use
> window.close() directly, and remove ext.closePopup. But this should be done
> separately from this change, so LGTM.

I'll do that.

> Also note that we are in code freeze right now for the upcoming release for
> Chrome/Opera. So please don't push anything to "master" for now.

Acknowledged.

> But just an idea, how about, creating a "next" bookmark to land changes during
> code freeze? They won't end up in any development build, but we could easily
> just merge that bookmark after the release.

I like the idea, let me look into it.

[Manish Jethani, 2017-09-08 21:04:46]

On 2017/09/08 20:58:52, Manish Jethani wrote:
> On 2017/09/08 20:18:57, Sebastian Noack wrote:

> > But just an idea, how about, creating a "next" bookmark to land changes
during
> > code freeze? They won't end up in any development build, but we could easily
> > just merge that bookmark after the release.
> 
> I like the idea, let me look into it.

Done.

https://hg.adblockplus.org/adblockpluschrome/rev/next

[Manish Jethani, 2017-09-08 21:38:00]

On 2017/09/08 20:57:54, Manish Jethani wrote:
> On 2017/09/08 20:07:59, Oleksandr wrote:
> > LGTM. This is not a problem on Edge. But other browsers are affected by this
> as
> > well, aren't they?
> 
> Thanks, this affects Firefox mainly, but I was hoping it wouldn't have any
> negative effects on Edge as I am unable to test it here.

Ah, sorry, I just completely forgot that Edge has its own bookmark. I'm not sure
how you share code between master and edge but anyway it was worth getting your
opinion on this.

[Sebastian Noack, 2017-09-08 21:57:46]

On 2017/09/08 21:38:00, Manish Jethani wrote:
> Ah, sorry, I just completely forgot that Edge has its own bookmark. I'm not
sure
> how you share code between master and edge but anyway it was worth getting
your
> opinion on this.

The "master" branch is merged back into the "edge" branch, every now and then.
But this is just a temporary situation. Most of the changes for Microsoft Edge
have already been included in the "master", and at some point we will abandon
the "edge" bookmark. So either way, the code we land in the "master" (and
"next") bookmarks will eventually be used on Microsoft Edge.