[on hold] Issue 6455 - Use tabs.removeCSS on Chromium

On hold until https://crbug.com/608854 is fixed.

[Manish Jethani, 2018-03-08 18:06:16]

Patch Set 1

I've explained everything in the source comments.

I think we can remove the Gecko check right away, because at least for our
purposes the implementation in Chromium is already fairly stable. I have written
extensive tests so we can be certain of the exact behavior. [1] Besides, we're
going to test before releasing this.

[1]:
https://chromium-review.googlesource.com/c/chromium/src/+/918606/17/chrome/te...

[Manish Jethani, 2018-03-08 18:09:33]

On 2018/03/08 18:06:16, Manish Jethani wrote:

> I have written extensive tests so we can be certain of the exact behavior.

Also this [1]

[1]:
https://chromium.googlesource.com/chromium/src.git/+/9a107b936d3e7fbd9d7771e7...

[kzar, 2018-03-19 18:42:49]

> I have written extensive tests so we can be certain of the exact behavior.
> Also this [1]

If you have a good link to back up what you've said in the comment maybe add
that to the comment?

https://codereview.adblockplus.org/29717611/diff/29717612/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29717611/diff/29717612/lib/cssInjection.js...
lib/cssInjection.js:116: // The behavior of tabs.insertCSS and tabs.removeCSS is
different between
Thanks for taking the time to write such a good comment.

https://codereview.adblockplus.org/29717611/diff/29717612/lib/cssInjection.js...
lib/cssInjection.js:133: // So: (1) we do not want to remove first to avoid
flicker; (2) if the old
I think this stanza can be removed.

https://codereview.adblockplus.org/29717611/diff/29717612/lib/cssInjection.js...
lib/cssInjection.js:139: if (groupName == "standard")
Perhaps the logic would be clearer something like this?

  let removeOld = styleSheet == oldStyleSheet;

  if (removeOld && groupName == "standard")
  {
    removeStyleSheet(tabId, frameId, oldStyleSheet);
    frame.injectedStyleSheets.delete(groupName);
    removeOld = false;
  }

  if (styleSheet && !addStyleSheet(tabId, frameId, styleSheet))
    return false;

  if (removeOld)
    removeStyleSheet(tabId, frameId, oldStyleSheet);

https://codereview.adblockplus.org/29717611/diff/29717612/lib/cssInjection.js...
lib/cssInjection.js:141: // Style sheets in the "standard" group are typically
only added once and
I think instead of saying 'the "standard" group' and 'other style sheets' you
should explain what they are. I can see you're checking for the standard group
in the code, but it's not immediately obvious what a standard or not-standard
filter is.

[Sebastian Noack, 2018-03-19 20:58:17]

For reference, the discussion in https://codereview.adblockplus.org/29714638/,
whether it's worth to remove the old stylesheet only if unchanged, is also
relevant for this change. I didn't reply on this review yet, since we should
resolve that discussion first.

[Manish Jethani, 2018-03-21 16:23:35]

On 2018/03/19 20:58:17, Sebastian Noack wrote:
> For reference, the discussion in https://codereview.adblockplus.org/29714638/,
> whether it's worth to remove the old stylesheet only if unchanged, is also
> relevant for this change. I didn't reply on this review yet, since we should
> resolve that discussion first.

Let's put this patch on hold until we've figured out the optimizations in the
element hiding emulation implementation and how we're going to use style sheets
exactly, esp. how to keep the path-based CSS selectors updated in an efficient
way (#6437, #6504).

Also my comment here:

https://codereview.adblockplus.org/29714638/#msg19

I'll come back once those issues are resolved.