Issue 29596637: Issue 5985 - Work around Firefox bug causing Block element dialog to be empty on Linux (Closed)
|
Created:
Nov. 3, 2017, 1:10 p.m. by Wladimir Palant Modified:
Nov. 3, 2017, 2:43 p.m. Reviewers:
kzar CC:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome Visibility:
Public. |
DescriptionIssue 5985 - Work around Firefox bug causing Block element dialog to be empty on Linux
Patch Set 1 #
Total comments: 8
MessagesTotal messages: 7
https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (left): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:220: ext.windows.create({ I think we can remove ext.windows from ext/background.js now? https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:242: navigator.oscpu.startsWith("Linux")) (I'll take your word for it that this is the best way to check for Linux.)
https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (left): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:220: ext.windows.create({ On 2017/11/03 13:24:06, kzar wrote: > I think we can remove ext.windows from ext/background.js now? Sure - after the release. https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:242: navigator.oscpu.startsWith("Linux")) On 2017/11/03 13:24:06, kzar wrote: > (I'll take your word for it that this is the best way to check for Linux.) It's not a great way, but it's worse than any other approach I know of. It should do for this hack, and we'll hopefully remove it soon (as soon as Firefox 57 gets somewhat out of fashion).
https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (left): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:220: ext.windows.create({ On 2017/11/03 13:49:05, Wladimir Palant wrote: > On 2017/11/03 13:24:06, kzar wrote: > > I think we can remove ext.windows from ext/background.js now? > > Sure - after the release. Well if we're only doing the bare minimum for the release, I wonder why you changed this code around here to avoid using ext.windows. But whatever, I've filed #5987 for that change anyway. https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:242: navigator.oscpu.startsWith("Linux")) On 2017/11/03 13:49:05, Wladimir Palant wrote: > On 2017/11/03 13:24:06, kzar wrote: > > (I'll take your word for it that this is the best way to check for Linux.) > > It's not a great way, but it's worse than any other approach I know of. It > should do for this hack, and we'll hopefully remove it soon (as soon as Firefox > 57 gets somewhat out of fashion). Acknowledged.
LGTM
https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (left): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:220: ext.windows.create({ On 2017/11/03 13:57:06, kzar wrote: > Well if we're only doing the bare minimum for the release, I wonder why you > changed this code around here to avoid using ext.windows. But whatever, I've > filed #5987 for that change anyway. Because the ext API only gives us a tab ID but not a window ID.
https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.js File lib/filterComposer.js (left): https://codereview.adblockplus.org/29596637/diff/29596638/lib/filterComposer.... lib/filterComposer.js:220: ext.windows.create({ On 2017/11/03 13:59:37, Wladimir Palant wrote: > On 2017/11/03 13:57:06, kzar wrote: > > Well if we're only doing the bare minimum for the release, I wonder why you > > changed this code around here to avoid using ext.windows. But whatever, I've > > filed #5987 for that change anyway. > > Because the ext API only gives us a tab ID but not a window ID. Ah, OK. 
|
||||||||||||||||||||||||||||||||||||||||||||||
