Issue 4802 - Add back compatibility for Microsoft Edge after #4722 removed it

This adds back the fetch polyfill and makes sure we don't call sendMessage with undefined callback. Also updates the buildtools dependency to the latest version.

[kzar, 2017-01-16 07:27:30]

Is this intended for the master branch or the edge branch? I don't mind most of
these changes going in the edge branch but shouldn't the adblockpluschrome
dependency also be updated in that case?

If these changes are intended for the master branch then I think I'd rather wait
to see Microsoft fix Edge. By the way have you checked to make sure there are
issues filed in their issue tracker?

https://codereview.adblockplus.org/29371945/diff/29371946/dependencies
File dependencies (left):

https://codereview.adblockplus.org/29371945/diff/29371946/dependencies#oldcode3
dependencies:3: buildtools = buildtools hg:4cea55c4bdc0 git:33dfff3
This seems like an unrelated change and it isn't mentioned in the issue. Why
have you updated the buildtools dependency here?

[Oleksandr, 2017-01-16 08:21:13]

On 2017/01/16 07:27:30, kzar wrote:
> Is this intended for the master branch or the edge branch? I don't mind most
of
> these changes going in the edge branch but shouldn't the adblockpluschrome
> dependency also be updated in that case?
> 
> If these changes are intended for the master branch then I think I'd rather
wait
> to see Microsoft fix Edge. By the way have you checked to make sure there are
> issues filed in their issue tracker?

This is intended for the edge bookmark. Not following what you mean by
adblockpluschrome dependency though. There isn't one, is there? Do you mean
there should be a codereview for the merge of branches?

[Oleksandr, 2017-01-16 08:21:37]

https://codereview.adblockplus.org/29371945/diff/29371946/dependencies
File dependencies (left):

https://codereview.adblockplus.org/29371945/diff/29371946/dependencies#oldcode3
dependencies:3: buildtools = buildtools hg:4cea55c4bdc0 git:33dfff3
On 2017/01/16 07:27:30, kzar wrote:
> This seems like an unrelated change and it isn't mentioned in the issue. Why
> have you updated the buildtools dependency here?

Buildtools dependency needed an update to support Edge builds. I did that in a
separate commit already, but looks like I have referenced non-existing revision
84c599613ecb somehow. This mainly fixes that issue.

[kzar, 2017-01-16 08:26:12]

On 2017/01/16 08:21:13, Oleksandr wrote:
> This is intended for the edge bookmark. Not following what you mean by
> adblockpluschrome dependency though. There isn't one, is there? Do you 
> mean there should be a codereview for the merge of branches?

Whoops sorry, I was thinking of the adblock when I said that. Ignore me!

> Buildtools dependency needed an update to support Edge builds. I did 
> that in a separate commit already, but looks like I have referenced 
> non-existing revision 84c599613ecb somehow. This mainly fixes that 
> issue.

Well you should re-open the issue for the last dependency update that went wrong
in that case. Or if not possible file a new issue. Either way the buildtools
dependency update shouldn't get mixed up with these changes IMO.

[Oleksandr, 2017-01-17 05:47:11]

Remove the buildtools dependcy update in patchset 2

[Oleksandr, 2017-01-17 06:00:38]

On 2017/01/16 08:26:12, kzar wrote:

> Well you should re-open the issue for the last dependency update that went
wrong
> in that case. Or if not possible file a new issue. Either way the buildtools
> dependency update shouldn't get mixed up with these changes IMO.

Opened https://issues.adblockplus.org/ticket/4806 for that

[kzar, 2017-01-17 06:54:31]

Otherwise LGTM

https://codereview.adblockplus.org/29371945/diff/29372118/options.js
File options.js (right):

https://codereview.adblockplus.org/29371945/diff/29372118/options.js#newcode59
options.js:59: // Edge silently fails when sendMessage is called with a callback
Did you file an issue for this with Microsoft? (Or is there already an issue?) I
think you should + add a link here. Also could you mention the version of Edge
that the problem happens for here, that will help us when we're considering
removing the workaround in the future.

[Oleksandr, 2017-01-18 03:08:44]

https://codereview.adblockplus.org/29371945/diff/29372118/options.js
File options.js (right):

https://codereview.adblockplus.org/29371945/diff/29372118/options.js#newcode59
options.js:59: // Edge silently fails when sendMessage is called with a callback
On 2017/01/17 06:54:31, kzar wrote:
> Did you file an issue for this with Microsoft? (Or is there already an issue?)
I
> think you should + add a link here. Also could you mention the version of Edge
> that the problem happens for here, that will help us when we're considering
> removing the workaround in the future.

The issue is marked as Fixed, however it is not clear in what release. In
"evergreen browser" reality I guess we should just set up a policy to remove the
workaround no sooner then a predefined period of time (6 months?)

[kzar, 2017-01-18 03:48:41]

https://codereview.adblockplus.org/29371945/diff/29372118/options.js
File options.js (right):

https://codereview.adblockplus.org/29371945/diff/29372118/options.js#newcode59
options.js:59: // Edge silently fails when sendMessage is called with a callback
On 2017/01/18 03:08:44, Oleksandr wrote:
> On 2017/01/17 06:54:31, kzar wrote:
> > Did you file an issue for this with Microsoft? (Or is there already an
issue?)
> I
> > think you should + add a link here. Also could you mention the version of
Edge
> > that the problem happens for here, that will help us when we're considering
> > removing the workaround in the future.
> 
> The issue is marked as Fixed, however it is not clear in what release. In
> "evergreen browser" reality I guess we should just set up a policy to remove
the
> workaround no sooner then a predefined period of time (6 months?)

OK well add a link to that issue here and also update the comment to mention
which version of Edge sendMessage is silently failing for. I guess we can figure
out the exact version which fixes the issue later after it's released.

[Oleksandr, 2017-01-18 03:58:44]

Done. Pushing this.

[kzar, 2017-01-18 04:49:51]

https://codereview.adblockplus.org/29371945/diff/29372257/options.js
File options.js (right):

https://codereview.adblockplus.org/29371945/diff/29372257/options.js#newcode60
options.js:60: // parameter of null, so we work around that here.
The callback parameter will be `undefined` not `null`, I wonder why you changed
it in the comment?

[Oleksandr, 2017-01-19 20:51:01]

https://codereview.adblockplus.org/29371945/diff/29372257/options.js
File options.js (right):

https://codereview.adblockplus.org/29371945/diff/29372257/options.js#newcode60
options.js:60: // parameter of null, so we work around that here.
On 2017/01/18 04:49:50, kzar wrote:
> The callback parameter will be `undefined` not `null`, I wonder why you
changed
> it in the comment?

The issue on Microsoft's issue tracker just mentions a null callback. I thought
I'd align the comment. But I can see how that could be confusing. Fixed that in
a followup commit.

[kzar, 2017-01-20 09:58:30]

On 2017/01/19 20:51:01, Oleksandr wrote:
> Fixed that in a followup commit.

Cool, thanks.