Issue 29587555: Issue 5771 - ABP prevents loading of Edge about:flags page (Closed)
|
Created:
Oct. 24, 2017, 7:39 a.m. by Oleksandr Modified:
Nov. 8, 2017, 12:55 p.m. Visibility:
Public. |
DescriptionIssue 5771 - ABP prevents loading of Edge about:flags page
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 10
https://codereview.adblockplus.org/29587555/diff/29587556/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29587555/diff/29587556/ext/background.js#n... ext/background.js:612: }, {urls: ["*://*/*"]}, ["blocking"]); I have tested that this still works fine for both HTTP and Websocket requests on Edge, Firefox and Chrome. I have not tested mobile versions.
NOT LGTM. I think we should rather check for a protocol of about:, or the URL of about:flags.
On 2017/10/24 14:54:30, kzar wrote: > NOT LGTM. I think we should rather check for a protocol of about:, or the URL of > about:flags. We don't get a chance to do such check in Edge. If we merely register a listener for `<all_urls>` it breaks `about:` pages (as well as some others). The callback is never called for those pages. See: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14255469/
On 2017/10/24 14:57:52, Oleksandr wrote: > On 2017/10/24 14:54:30, kzar wrote: > > NOT LGTM. I think we should rather check for a protocol of about:, or the URL > of > > about:flags. > > We don't get a chance to do such check in Edge. If we merely register a listener > for `<all_urls>` it breaks `about:` pages (as well as some others). The callback > is never called for those pages. See: > https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14255469/ Oh dang, that really sucks! Well in that case perhaps a workaround like you suggested would be OK, but it would belong on the edge branch. (Also looking at that issue I noticed the Edge guy looking into the problem mentioned he couldn't reproduce it...)
> Oh dang, that really sucks! Well in that case perhaps a workaround like you > suggested would be OK, but it would belong on the edge branch. Is there any reason why this wouldn't be good for master branch? > > (Also looking at that issue I noticed the Edge guy looking into the problem > mentioned he couldn't reproduce it...) Yeah, it seems to be their strategy of weeding out unimportant bugs. Ask as many questions as possible, so that those bugs about which people don't care will just get automatically closed. It is marked as `confirmed` and I have uploaded a simple extension to reproduce the bug too.
On 2017/10/24 15:08:36, Oleksandr wrote: > > Oh dang, that really sucks! Well in that case perhaps a workaround like you > > suggested would be OK, but it would belong on the edge branch. > > Is there any reason why this wouldn't be good for master branch? Can we just do a platform check here using the info module?
On 2017/10/24 15:14:30, Manish Jethani wrote: > On 2017/10/24 15:08:36, Oleksandr wrote: > > > Oh dang, that really sucks! Well in that case perhaps a workaround like you > > > suggested would be OK, but it would belong on the edge branch. > > > > Is there any reason why this wouldn't be good for master branch? Well these request listeners get quite complicated with edge cases and different behaviour between platforms and even platform versions. IMO messing with them like this for all platforms just to workaround a ridiculous Edge bug is a bad idea. But it's Sebastian's call, so if you disagree with me let's see what he thinks. > Can we just do a platform check here using the info module? Well I don't think ext/background.js is part of the lib/adblockplus.js bundle, so I don't think we can require the info module here.
On 2017/10/24 15:26:09, kzar wrote: > On 2017/10/24 15:14:30, Manish Jethani wrote: > > On 2017/10/24 15:08:36, Oleksandr wrote: > > > > Oh dang, that really sucks! Well in that case perhaps a workaround like > you > > > > suggested would be OK, but it would belong on the edge branch. > > > > > > Is there any reason why this wouldn't be good for master branch? > > Well these request listeners get quite complicated with edge cases and different > behaviour between platforms and even platform versions. IMO messing with them > like this for all platforms just to workaround a ridiculous Edge bug is a bad > idea. But it's Sebastian's call, so if you disagree with me let's see what he > thinks. > > > Can we just do a platform check here using the info module? > > Well I don't think ext/background.js is part of the lib/adblockplus.js bundle, > so I don't think we can require the info module here. Edge doesn't support runtime.getPlatformInfo: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getPlat... Maybe that's a good check?
Using "<all_urls>" here was a hack to start with. What we actually want here is ["http://*/*", "https://*/*", "ws://*/*", "wss://*/*"]. This, however, breaks on older Chrome versions (and doesn't work on Edge either). So IMO, if "*://*/*" has the desired result on all platforms, this isn't any worse than the current workaround. So as far as I am concerned, and if I don't miss anything, LGTM.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
