Issue 29564767: Issue 242 - Use user style sheets on Chromium (Closed)
|
Created:
Oct. 4, 2017, 10:32 p.m. by Manish Jethani Modified:
Feb. 1, 2018, 11:33 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 242 - Use user style sheets on Chromium
Patch Set 1 #Patch Set 2 : Rebase and update target version #Patch Set 3 : Rewrite #
Total comments: 13
Patch Set 4 : Check for Gecko #Patch Set 5 : Use feature detection for cssOrigin #Patch Set 6 : Update comment #Patch Set 7 : Rebase #
Total comments: 6
Patch Set 8 : Fix regex matching and check only for Gecko #Patch Set 9 : Do not check error message #MessagesTotal messages: 27
Patch Set 1 If we assume that Chromium version 60+ will support user style sheets (we don't know this yet) and there won't be any way to detect such support other than to specifically check the version number, then we have to do this. I'm uploading this prematurely so we can get the discussion started early.
LGTM. But before landing this, can we at least wait until the fix for https://crbug.com/632009 has been landed in the Chrome development builds at least?
On 2017/10/04 22:43:01, Sebastian Noack wrote: > LGTM. Thanks. > But before landing this, can we at least wait until the fix for > https://crbug.com/632009 has been landed in the Chrome development builds at > least? Of course, there's still some work to be done there (e.g. fixing the style inspector in devtools), the main hasn't patch even landed yet. I'll commit and push this only once we're sure what version of Chromium we can target.
Patch Set 2: Rebase and update target version User style sheet support is already in Canary (version 64), I think maybe we should commit this and start testing it.
I wonder whether we should better wait until we can remove the stylesheet as well. Otherwise, we are stuck with the bug we had on Firefox that we keep adding redundant stylesheets.
On 2017/10/20 01:29:58, Sebastian Noack wrote: > I wonder whether we should better wait until we can remove the stylesheet as > well. Otherwise, we are stuck with the bug we had on Firefox that we keep adding > redundant stylesheets. Yes, I agree we should wait until we can do it correctly. But on the other hand it may be a while before Chrome has tabs.removeCSS. A compromise would be to use tabs.insertCSS only for the normal selectors but not the emulated selectors. This way we would get the maximum benefit out of the current situation. The composer only adds selectors, it doesn't remove any selectors, so it should be OK to just add another style sheet with an additional selector.
On 2017/10/20 01:54:51, Manish Jethani wrote: > On 2017/10/20 01:29:58, Sebastian Noack wrote: > > I wonder whether we should better wait until we can remove the stylesheet as > > well. Otherwise, we are stuck with the bug we had on Firefox that we keep > adding > > redundant stylesheets. > > Yes, I agree we should wait until we can do it correctly. But on the other hand > it may be a while before Chrome has tabs.removeCSS. A compromise would be to use > tabs.insertCSS only for the normal selectors but not the emulated selectors. > This way we would get the maximum benefit out of the current situation. > > The composer only adds selectors, it doesn't remove any selectors, so it should > be OK to just add another style sheet with an additional selector. Sounds good to me. However, there is still an edge case in which this would break current behavior: Since the user can freely edit the filters to be added in the composer, the user could potentially add a whitelisting filter which would show no immediate effect anymore, if the old stylesheet remains in effect. Also we are essentially leaking memory, when the user repeatedly uses the composer, and we keep adding stylesheets. Anyway, I guess, these scenarios are rare enough, while the benefit of user stylesheets is huge, that it might be worth it.
On 2017/10/20 01:54:51, Manish Jethani wrote: > [...] > it may be a while before Chrome has tabs.removeCSS. A compromise would be to use > tabs.insertCSS only for the normal selectors but not the emulated selectors. I'm following up on tabs.removeCSS https://crbug.com/608854#c21
I have rewritten this now based on master. I think we should start using tabs.insertCSS soon. The sooner we do it the sooner any errors will be caught. https://codereview.adblockplus.org/29564767/diff/29685558/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/include.preload.js#... include.preload.js:510: this.inlineEmulated = !!response.inlineEmulated; So there's a second setting called inlineEmulated, which is checked after this.inline in addSelectors.
On 2018/01/31 08:24:55, Manish Jethani wrote: > I have rewritten this now based on master. And based on the above short discussion with Sebastian.
https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; IMO we only know removeCSS works on Gecko and so we are disabling it for the other platforms. I think it would be better to check that the platform is Gecko here, rather than that it's not Chromium. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:81: // Chromium's support for tabs.removeCSS is still a work in progress and the IMO this comment should go by the styleSheetRemovalSupported definition.
https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:34: parseInt(info.platformVersion, 10) >= 66); Perhaps, we should consider going back to the try-and-error approach to detect user stylesheets, rather than relying on the user agent string. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:11:32, kzar wrote: > IMO we only know removeCSS works on Gecko and so we are disabling it for the > other platforms. I think it would be better to check that the platform is Gecko > here, rather than that it's not Chromium. Why not just checking for presence of "removeCSS"? It doesn't exist on Chrome, and once they add it, it surely would be great to use it.
https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:23:37, Sebastian Noack wrote: > On 2018/01/31 11:11:32, kzar wrote: > > IMO we only know removeCSS works on Gecko and so we are disabling it for the > > other platforms. I think it would be better to check that the platform is > Gecko > > here, rather than that it's not Chromium. > > Why not just checking for presence of "removeCSS"? It doesn't exist on Chrome, > and once they add it, it surely would be great to use it. Well as we discussed in IRC don't we agree it would be better to first have QA test the feature works OK on Chromium before switching it on for our users?
https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:25:29, kzar wrote: > On 2018/01/31 11:23:37, Sebastian Noack wrote: > > On 2018/01/31 11:11:32, kzar wrote: > > > IMO we only know removeCSS works on Gecko and so we are disabling it for the > > > other platforms. I think it would be better to check that the platform is > > Gecko > > > here, rather than that it's not Chromium. > > > > Why not just checking for presence of "removeCSS"? It doesn't exist on Chrome, > > and once they add it, it surely would be great to use it. > > Well as we discussed in IRC don't we agree it would be better to first have QA > test the feature works OK on Chromium before switching it on for our users? Well, we were talking about cssOrigin, which is now available in Canary. Whether we should generally prefer browser detection over feature detection, is a different question, and seems quite radical to me just because it backfired once. On the other hand, how do we make sure not to forget to turn such features on, and even if we do, there will be some delay until we get a release out. Also FWIW removing the CSS seems to less likely break something than inserting it.
https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:54:44, Sebastian Noack wrote: > On 2018/01/31 11:25:29, kzar wrote: > > On 2018/01/31 11:23:37, Sebastian Noack wrote: > > > On 2018/01/31 11:11:32, kzar wrote: > > > > IMO we only know removeCSS works on Gecko and so we are disabling it for > the > > > > other platforms. I think it would be better to check that the platform is > > > Gecko > > > > here, rather than that it's not Chromium. > > > > > > Why not just checking for presence of "removeCSS"? It doesn't exist on > Chrome, > > > and once they add it, it surely would be great to use it. > > > > Well as we discussed in IRC don't we agree it would be better to first have QA > > test the feature works OK on Chromium before switching it on for our users? > > Well, we were talking about cssOrigin, which is now available in Canary. Whether > we should generally prefer browser detection over feature detection, is a > different question, and seems quite radical to me just because it backfired > once. On the other hand, how do we make sure not to forget to turn such features > on, and even if we do, there will be some delay until we get a release out. Also > FWIW removing the CSS seems to less likely break something than inserting it. Fair enough, if you both agree to do it that way I won't block it.
Patch Set 4: Check for Gecko Comments inline. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:34: parseInt(info.platformVersion, 10) >= 66); On 2018/01/31 11:23:37, Sebastian Noack wrote: > Perhaps, we should consider going back to the try-and-error approach to detect > user stylesheets, rather than relying on the user agent string. What if Edge adds support for cssOrigin but behaves differently than Chrome and Firefox, or throws a different error (we were checking for the error string)? Anyway since Edge is still off a separate branch (right?) it's probably OK to go back to that approach now. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:11:32, kzar wrote: > IMO we only know removeCSS works on Gecko and so we are disabling it for the > other platforms. I think it would be better to check that the platform is Gecko > here, rather than that it's not Chromium. Makes sense. Done. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:23:37, Sebastian Noack wrote: > On 2018/01/31 11:11:32, kzar wrote: > > IMO we only know removeCSS works on Gecko and so we are disabling it for the > > other platforms. I think it would be better to check that the platform is > Gecko > > here, rather than that it's not Chromium. > > Why not just checking for presence of "removeCSS"? It doesn't exist on Chrome, > and once they add it, it surely would be great to use it. With tabs.removeCSS specifically we're thinking of having a different API on Chrome, and we haven't decided how it'll work exactly. See this comment and the next: https://bugs.chromium.org/p/chromium/issues/detail?id=608854#c26 So even if it's there it'll probably work differently. https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:36: info.platform != "chromium"; On 2018/01/31 11:54:44, Sebastian Noack wrote: > On 2018/01/31 11:25:29, kzar wrote: > > On 2018/01/31 11:23:37, Sebastian Noack wrote: > > > On 2018/01/31 11:11:32, kzar wrote: > > > > IMO we only know removeCSS works on Gecko and so we are disabling it for > the > > > > other platforms. I think it would be better to check that the platform is > > > Gecko > > > > here, rather than that it's not Chromium. > > > > > > Why not just checking for presence of "removeCSS"? It doesn't exist on > Chrome, > > > and once they add it, it surely would be great to use it. > > > > Well as we discussed in IRC don't we agree it would be better to first have QA > > test the feature works OK on Chromium before switching it on for our users? > > Well, we were talking about cssOrigin, which is now available in Canary. Whether > we should generally prefer browser detection over feature detection, is a > different question, and seems quite radical to me just because it backfired > once. On the other hand, how do we make sure not to forget to turn such features > on, and even if we do, there will be some delay until we get a release out. Also > FWIW removing the CSS seems to less likely break something than inserting it. See my other comment. In the case of tabs.removeCSS we _know_ it will break ABP once Canary has it (if we go with feature detection). https://codereview.adblockplus.org/29564767/diff/29685558/lib/cssInjection.js... lib/cssInjection.js:81: // Chromium's support for tabs.removeCSS is still a work in progress and the On 2018/01/31 11:11:32, kzar wrote: > IMO this comment should go by the styleSheetRemovalSupported definition. Done.
Patch Set 5: Use feature detection for cssOrigin
Patch Set 6: Update comment
Patch Set 7: Rebase
https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:36: info.platform == "gecko"; tabs.removeCSS() was added in Firefox 49. The oldest version we still support is Firefox 51. So the check for info.platform should be sufficient. https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:84: if (/\bError processing cssOrigin\b/.test(error.message) == -1) I wonder how safe the assumption is that this will match any error indicating unsupported cssOrigin on all versions of Chromium and Firefox. On the other hand, without this check we would just fall back to content script injected author style sheets, as well in the theoretical scenario where insertCSS() might fail for another reason. That doesn't seem too bad to me.
Patch Set 8: Fix regex matching and check only for Gecko https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:36: info.platform == "gecko"; On 2018/01/31 14:30:05, Sebastian Noack wrote: > tabs.removeCSS() was added in Firefox 49. The oldest version we still support is > Firefox 51. So the check for info.platform should be sufficient. Done. https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:84: if (/\bError processing cssOrigin\b/.test(error.message) == -1) On 2018/01/31 14:30:05, Sebastian Noack wrote: > I wonder how safe the assumption is that this will match any error indicating > unsupported cssOrigin on all versions of Chromium and Firefox. On the other > hand, without this check we would just fall back to content script injected > author style sheets, as well in the theoretical scenario where insertCSS() might > fail for another reason. That doesn't seem too bad to me. OK, I think we should just look for /\bcssOrigin\b/ here. We already know it's an error, then if we know the error message contains the word "cssOrigin", and since we've passed the value "user" to that option, we can conclude that user style sheets are not supported. This is more general than the previous check. Done.
https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:84: if (/\bError processing cssOrigin\b/.test(error.message) == -1) On 2018/02/01 08:05:07, Manish Jethani wrote: > On 2018/01/31 14:30:05, Sebastian Noack wrote: > > I wonder how safe the assumption is that this will match any error indicating > > unsupported cssOrigin on all versions of Chromium and Firefox. On the other > > hand, without this check we would just fall back to content script injected > > author style sheets, as well in the theoretical scenario where insertCSS() > might > > fail for another reason. That doesn't seem too bad to me. > > OK, I think we should just look for /\bcssOrigin\b/ here. We already know it's > an error, then if we know the error message contains the word "cssOrigin", and > since we've passed the value "user" to that option, we can conclude that user > style sheets are not supported. This is more general than the previous check. > > Done. I still think, we should just remove that check all together. But it's not too important. Let's see what Dave says.
Patch Set 9: Do not check error message https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29564767/diff/29685587/lib/cssInjection.js... lib/cssInjection.js:84: if (/\bError processing cssOrigin\b/.test(error.message) == -1) On 2018/02/01 10:32:50, Sebastian Noack wrote: > On 2018/02/01 08:05:07, Manish Jethani wrote: > > On 2018/01/31 14:30:05, Sebastian Noack wrote: > > > I wonder how safe the assumption is that this will match any error > indicating > > > unsupported cssOrigin on all versions of Chromium and Firefox. On the other > > > hand, without this check we would just fall back to content script injected > > > author style sheets, as well in the theoretical scenario where insertCSS() > > might > > > fail for another reason. That doesn't seem too bad to me. > > > > OK, I think we should just look for /\bcssOrigin\b/ here. We already know it's > > an error, then if we know the error message contains the word "cssOrigin", and > > since we've passed the value "user" to that option, we can conclude that user > > style sheets are not supported. This is more general than the previous check. > > > > Done. > > I still think, we should just remove that check all together. But it's not too > important. Let's see what Dave says. Yeah I'm cool with just removing the check too. If it fails, it fails, then the reason for failure doesn't matter. We lose the actual error but that's OK.
LGTM
On 2018/02/01 11:03:23, Sebastian Noack wrote: > LGTM Thanks! Dave, would you like to take a look?
On 2018/02/01 11:08:40, Manish Jethani wrote: > On 2018/02/01 11:03:23, Sebastian Noack wrote: > > LGTM > > Thanks! > > Dave, would you like to take a look? No, you go ahead if Sebastian's happy that's good enough for me. 
|
