Issue 29545645: Issue 5695 - Use tabs.insertCSS if extensionTypes.CSSOrigin exists (Closed)
|
Created:
Sept. 15, 2017, 12:58 p.m. by Manish Jethani Modified:
Sept. 20, 2017, 9:23 a.m. Reviewers:
Sebastian Noack CC:
Wladimir Palant, Oleksandr, kzar Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5695 - Use tabs.insertCSS if extensionTypes.CSSOrigin exists
Patch Set 1 #
Total comments: 12
Patch Set 2 : Skip false properties #
Total comments: 9
Patch Set 3 : Simplify further #
Total comments: 12
Patch Set 4 : Always pass selectors if trace or inject is true #
Total comments: 5
Patch Set 5 : Flatten out logic and remove blank line #MessagesTotal messages: 14
Patch Set 1 https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && Note: "style sheet" is actually two words, I've changed the name of this variable accordingly. https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:69: hideElements(sender.page.id, sender.frame.id, selectors); If userStyleSheetsSupported is true then a call to hideElements is always expected to succeed (rather never for lack of cssOrigin support). https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:82: if (userStyleSheetsSupported) Since there's no try...catch in hideElements now, we need to do a check here explicitly before calling the function.
https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && In which case, does chrome.extensionTypes does not exist? According to MDN, it even exists on Miccrosoft Edge: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extensionTypes Ollie, can you confirm? https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:34: chrome.tabs.insertCSS(tabId, Nit: The additional indentation (and one additional line) seems unnecessary, if you wrap it like this (which also seems consistent with most existing code): chrome.tabs.insertCSS(tabId, { ... }); https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:66: if (selectors.length == 0 || userStyleSheetsSupported) I had a go before. But you were quicker, getting your patch under review. However, there are some more simplifications, here, in my patch: https://gist.github.com/snoack/253a5302b933933e19c5cfb95725cc78 What do you think? https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:82: if (userStyleSheetsSupported) On 2017/09/15 13:02:42, Manish Jethani wrote: > Since there's no try...catch in hideElements now, we need to do a check here > explicitly before calling the function. It seems no message of this type is send by the content script if cssOrigin is not supported, hence inject is true.
Patch Set 2: Skip false properties https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && On 2017/09/15 18:46:37, Sebastian Noack wrote: > In which case, does chrome.extensionTypes does not exist? According to MDN, it > even exists on Miccrosoft Edge: > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extensionTypes It doesn't exist on Chrome. https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:34: chrome.tabs.insertCSS(tabId, On 2017/09/15 18:46:37, Sebastian Noack wrote: > Nit: The additional indentation (and one additional line) seems unnecessary, if > you wrap it like this (which also seems consistent with most existing code): > > chrome.tabs.insertCSS(tabId, { > ... > }); Done. https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:66: if (selectors.length == 0 || userStyleSheetsSupported) On 2017/09/15 18:46:37, Sebastian Noack wrote: > I had a go before. But you were quicker, getting your patch under review. > However, there are some more simplifications, here, in my patch: > https://gist.github.com/snoack/253a5302b933933e19c5cfb95725cc78 > > What do you think? I've incorporated the simplifications and made some more. We don't need to include a property if the value is false. https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:82: if (userStyleSheetsSupported) On 2017/09/15 18:46:37, Sebastian Noack wrote: > On 2017/09/15 13:02:42, Manish Jethani wrote: > > Since there's no try...catch in hideElements now, we need to do a check here > > explicitly before calling the function. > > It seems no message of this type is send by the content script if cssOrigin is > not supported, hence inject is true. Done. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:63: if (!userStyleSheetsSupported) I've kept the userStyleSheetsSupported flag since there's no need to check the existence of extensionTypes.CSSOrigin every time. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:68: if (trace) This return statement could be merged into the next one but then we'd be returning two useless properties. I like the consistency of never returning a property that's false or null.
https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545646/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && On 2017/09/15 21:26:08, Manish Jethani wrote: > On 2017/09/15 18:46:37, Sebastian Noack wrote: > > In which case, does chrome.extensionTypes does not exist? According to MDN, it > > even exists on Miccrosoft Edge: > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extensionTypes > > It doesn't exist on Chrome. Indeed. The MDN compatibility information are misleading here. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && Use const here? https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:61: return {}; Actually, we have to pass "trace", even if there are no selectors here. There might be emulation filters which are retrieved by a different message. But they are traced as well if with return {trace:true} here. Also "elemhide.injectSelectors" messages might still be send unless we return {inject:true}. Perhaps, it is simplest to just get rid of this special case here, and just don't call hideElements() (as it would fail) if selectors are empty, and handle that scenario for the inject=true scenario in the content script. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:68: if (trace) On 2017/09/15 21:26:09, Manish Jethani wrote: > This return statement could be merged into the next one but then we'd be > returning two useless properties. I like the consistency of never returning a > property that's false or null. I see the point for not including selectors=null (as selectors is irrelevant unless either trace or inject is true), but only reporting a boolean state if it is true seems inconsistent to me.
Patch Set 3: Simplify further https://codereview.adblockplus.org/29545645/diff/29545715/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29545645/diff/29545715/include.preload.js#... include.preload.js:444: if (!selectors || selectors.length == 0) This is no longer needed since we're checking this before calling this method. It was never needed for the other case where this method is called for the emulation filters. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:29: let userStyleSheetsSupported = "extensionTypes" in chrome && On 2017/09/15 22:52:44, Sebastian Noack wrote: > Use const here? Done. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:61: return {}; On 2017/09/15 22:52:44, Sebastian Noack wrote: > Actually, we have to pass "trace", even if there are no selectors here. There > might be emulation filters which are retrieved by a different message. But they > are traced as well if with return {trace:true} here. > > Also "elemhide.injectSelectors" messages might still be send unless we return > {inject:true}. > > Perhaps, it is simplest to just get rid of this special case here, and just > don't call hideElements() (as it would fail) if selectors are empty, and handle > that scenario for the inject=true scenario in the content script. Done. See my other comments. https://codereview.adblockplus.org/29545645/diff/29545715/lib/cssInjection.js... lib/cssInjection.js:68: if (trace) On 2017/09/15 22:52:44, Sebastian Noack wrote: > On 2017/09/15 21:26:09, Manish Jethani wrote: > > This return statement could be merged into the next one but then we'd be > > returning two useless properties. I like the consistency of never returning a > > property that's false or null. > > I see the point for not including selectors=null (as selectors is irrelevant > unless either trace or inject is true), but only reporting a boolean state if it > is true seems inconsistent to me. Done. https://codereview.adblockplus.org/29545645/diff/29547555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/include.preload.js#... include.preload.js:504: if (response.selectors) Check for the existence of response.selectors since it will be missing if there are no selectors. https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:47: let inject = !userStyleSheetsSupported; The trace and inject values are pretty much constant. We decide early if we want to inject or not. https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:60: if (!inject && selectors.length > 0) This has been moved inside the if block. If we're not going to inject and we do have selectors, do the tabs.insertCSS thing here. https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:64: let response = {trace, inject}; trace and inject pretty much have to be passed as they are as the values cannot be changed for any reason. https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) Include a selectors property only if either trace or inject are true and if there are any selectors at all. We could skip the last two checks and then just include the selectors property even if it's null or an empty array, but I don't see why we should. It would certainly simplify the code though. For example, only one return statement: let selectors = []; ... ... ... return {selectors: trace || inject ? selectors : [], trace, inject}; i.e. always return an array, even if empty.
https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/17 13:51:38, Manish Jethani wrote: > Include a selectors property only if either trace or inject are true and if > there are any selectors at all. > > We could skip the last two checks and then just include the selectors property > even if it's null or an empty array, but I don't see why we should. It would > certainly simplify the code though. > > For example, only one return statement: > > let selectors = []; > ... > ... > ... > return {selectors: trace || inject ? selectors : [], trace, inject}; > > i.e. always return an array, even if empty. Or: let selectors = null; ... ... ... return {selectors: (trace || inject) && selectors || [], trace, inject};
https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/17 15:09:19, Manish Jethani wrote: > On 2017/09/17 13:51:38, Manish Jethani wrote: > > Include a selectors property only if either trace or inject are true and if > > there are any selectors at all. > > > > We could skip the last two checks and then just include the selectors property > > even if it's null or an empty array, but I don't see why we should. It would > > certainly simplify the code though. > > > > For example, only one return statement: > > > > let selectors = []; > > ... > > ... > > ... > > return {selectors: trace || inject ? selectors : [], trace, inject}; > > > > i.e. always return an array, even if empty. > > Or: > > let selectors = null; > ... > ... > ... > return {selectors: (trace || inject) && selectors || [], trace, inject}; How about this? let selectors = []; ... if (trace || inject) response.selectors = selectors; Or like before: let selectors; if (...) { ... selectors = ... } else selectors = []; Then you also wouldn't need the check for presence of response.selectors in the content script, and you wouldn't have to nest the logic above for calling hideElements().
Patch Set 4: Always pass selectors if trace or inject is true Now selectors is always passed, even if it's an empty array, if either trace or inject is true. https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/18 22:46:37, Sebastian Noack wrote: > On 2017/09/17 15:09:19, Manish Jethani wrote: > > On 2017/09/17 13:51:38, Manish Jethani wrote: > > > Include a selectors property only if either trace or inject are true and if > > > there are any selectors at all. > > > > > > We could skip the last two checks and then just include the selectors > property > > > even if it's null or an empty array, but I don't see why we should. It would > > > certainly simplify the code though. > > > > > > For example, only one return statement: > > > > > > let selectors = []; > > > ... > > > ... > > > ... > > > return {selectors: trace || inject ? selectors : [], trace, inject}; > > > > > > i.e. always return an array, even if empty. > > > > Or: > > > > let selectors = null; > > ... > > ... > > ... > > return {selectors: (trace || inject) && selectors || [], trace, inject}; > > How about this? > > let selectors = []; > ... > if (trace || inject) > response.selectors = selectors; OK, I like this. Done. > Or like before: > > let selectors; > if (...) > { > ... > selectors = ... > } > else > selectors = []; > > Then you also wouldn't need the check for presence of response.selectors in the > content script, and you wouldn't have to nest the logic above for calling > hideElements(). We still have to check if the selectors array is empty before calling hideElements, otherwise it'll bomb. Then it makes more sense to keep that check inside the if block, right?
https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/19 05:47:26, Manish Jethani wrote: > On 2017/09/18 22:46:37, Sebastian Noack wrote: > > Then you also wouldn't need the check for presence of response.selectors in > the > > content script, and you wouldn't have to nest the logic above for calling > > hideElements(). > > We still have to check if the selectors array is empty before calling > hideElements, otherwise it'll bomb. Then it makes more sense to keep that check > inside the if block, right? Yes, you have to check for selectors.length regardless whether you nest in the block above or not, but the reason you moved it inside the block, I suppose, was to not also have to check for selectors being null, which it never is now. So it doesn't need to be nested in that block anymore. https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js File lib/cssInjection.js (left): https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js... lib/cssInjection.js:84: Nit: I would remove this blank line, so that the code preparing the response object is grouped together.
https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/19 23:09:17, Sebastian Noack wrote: > On 2017/09/19 05:47:26, Manish Jethani wrote: > > On 2017/09/18 22:46:37, Sebastian Noack wrote: > > > Then you also wouldn't need the check for presence of response.selectors in > > the > > > content script, and you wouldn't have to nest the logic above for calling > > > hideElements(). > > > > We still have to check if the selectors array is empty before calling > > hideElements, otherwise it'll bomb. Then it makes more sense to keep that > check > > inside the if block, right? > > Yes, you have to check for selectors.length regardless whether you nest in the > block above or not, but the reason you moved it inside the block, I suppose, was > to not also have to check for selectors being null, which it never is now. So it > doesn't need to be nested in that block anymore. With the latest patch there's no check for selectors.length outside of the if block. https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js File lib/cssInjection.js (left): https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js... lib/cssInjection.js:84: On 2017/09/19 23:09:17, Sebastian Noack wrote: > Nit: I would remove this blank line, so that the code preparing the response > object is grouped together. Acknowledged. But this is outdated now with the latest patch.
https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/20 01:36:29, Manish Jethani wrote: > On 2017/09/19 23:09:17, Sebastian Noack wrote: > > On 2017/09/19 05:47:26, Manish Jethani wrote: > > > On 2017/09/18 22:46:37, Sebastian Noack wrote: > > > > Then you also wouldn't need the check for presence of response.selectors > in > > > the > > > > content script, and you wouldn't have to nest the logic above for calling > > > > hideElements(). > > > > > > We still have to check if the selectors array is empty before calling > > > hideElements, otherwise it'll bomb. Then it makes more sense to keep that > > check > > > inside the if block, right? > > > > Yes, you have to check for selectors.length regardless whether you nest in the > > block above or not, but the reason you moved it inside the block, I suppose, > was > > to not also have to check for selectors being null, which it never is now. So > it > > doesn't need to be nested in that block anymore. > > With the latest patch there's no check for selectors.length outside of the if > block. Sure, but why don't you just flatten the logic by moving if (!inject && selectors.length > 0) hideElements(sender.page.id, sender.frame.id, selectors); outside of the if (!checkWhitelisted(...) { ... } block, since `selectors = []` is now defined at the top of the function. https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js File lib/cssInjection.js (left): https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js... lib/cssInjection.js:84: On 2017/09/20 01:36:30, Manish Jethani wrote: > On 2017/09/19 23:09:17, Sebastian Noack wrote: > > Nit: I would remove this blank line, so that the code preparing the response > > object is grouped together. > > Acknowledged. > > But this is outdated now with the latest patch. This comment is on the latest patch in this review.
Patch Set 5: Flatten out logic and remove blank line https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29547555/lib/cssInjection.js... lib/cssInjection.js:66: if ((trace || inject) && selectors && selectors.length > 0) On 2017/09/20 01:51:52, Sebastian Noack wrote: > On 2017/09/20 01:36:29, Manish Jethani wrote: > > On 2017/09/19 23:09:17, Sebastian Noack wrote: > > > On 2017/09/19 05:47:26, Manish Jethani wrote: > > > > On 2017/09/18 22:46:37, Sebastian Noack wrote: > > > > > Then you also wouldn't need the check for presence of response.selectors > > in > > > > the > > > > > content script, and you wouldn't have to nest the logic above for > calling > > > > > hideElements(). > > > > > > > > We still have to check if the selectors array is empty before calling > > > > hideElements, otherwise it'll bomb. Then it makes more sense to keep that > > > check > > > > inside the if block, right? > > > > > > Yes, you have to check for selectors.length regardless whether you nest in > the > > > block above or not, but the reason you moved it inside the block, I suppose, > > was > > > to not also have to check for selectors being null, which it never is now. > So > > it > > > doesn't need to be nested in that block anymore. > > > > With the latest patch there's no check for selectors.length outside of the if > > block. > > Sure, but why don't you just flatten the logic by moving > > if (!inject && selectors.length > 0) > hideElements(sender.page.id, sender.frame.id, selectors); > > outside of the > > if (!checkWhitelisted(...) > { > ... > } > > block, since `selectors = []` is now defined at the top of the function. Done. selectors starting out as null or an empty array is no longer relevant here though, but I suppose it makes the code more readable. https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js File lib/cssInjection.js (left): https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js... lib/cssInjection.js:84: On 2017/09/20 01:51:52, Sebastian Noack wrote: > On 2017/09/20 01:36:30, Manish Jethani wrote: > > On 2017/09/19 23:09:17, Sebastian Noack wrote: > > > Nit: I would remove this blank line, so that the code preparing the response > > > object is grouped together. > > > > Acknowledged. > > > > But this is outdated now with the latest patch. > > This comment is on the latest patch in this review. Rietveld may have messed up, because it looks like you're referring to a bunch of code I just deleted. Anyway, see my other comment. https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29545645/diff/29549555/lib/cssInjection.js... lib/cssInjection.js:65: I'm assuming you meant delete this blank line here. Done.
LGTM! 
|
