Have generateStyleSheetForDomain provide exception filters if requested

[kzar]

Background

With #6427 we'd like to start listing element hiding whitelisting filters which applied to the page in our developer tools panel. To do that we need the list of those whitelisting filters.

What to change

Modify generateStyleSheetForDomain in lib/elemHide.js to provide exception filters which applied if the optional includeExceptions argument is set to true.

The new signature should be:

function generateStyleSheetForDomain(domain, specificOnly = false,
                                     includeSelectors = false, 
                                     includeExceptions = false)

The return value should always contain three properties: code, selectors, and exceptions. By default exceptions should be null, but if includeExceptions is true then it should be the list of exceptions (empty array if no exceptions apply).

Performance should not be negatively affected for the default case in which both includeSelectors and includeExceptions are false.

Integration Notes

Using the trace in the call to elemHide.generateStyleSheetsForDomain as an additoinal argument should suffice for getting the data to the UI. (will update after testing)

Hints for testers

The change here should not cause any change in behavior in the extension, unless the extension makes further changes (for which please see the relevant ticket for the Platform or UI modules (e.g. #6427)).

In order to ensure this, a simple test should suffice:

1. Add two filters ##.foo and example.com##.bar and see that they hide the elements <div class="foo">Foo</div> and <div class="bar">Bar</div> on example.com.
2. On any other domain, Foo should be hidden but Bar should still be visible.
3. Add an exception foo.example.com#@#.foo and see that on foo.example.com only Foo is visible and Bar is hidden instead. Verify 1 and 2 again.
4. Update the filter example.com##.bar to make it example.com,~bar.example.com##.bar and see that on bar.example.com Bar is visible but Foo is hidden. Verify 1, 2, and 3 again.

[kzar]

Note: Manish and I have recently made quite a few changes to this code and are ​still working on one.

[mjethani]

Do we have to include all exceptions or only one that matched (and prevented the filter's selector from getting added)? If it's only one then it should be straightforward, just take the value returned by ElemHide.getException and add it to the return value.

The return value should then be {selectors, exceptions}.

I don't think this will affect performance much, and in any case this should only be done if the DevTools panel is open.

PS: By the way I would prefer if provideExceptionFilters were called withExceptions instead. It's shorter and conveys just as much. Also I think we only want to return the filter text, not the actual Filter object.

[mjethani]

So right now we have this code in ElemHide.getSelectorsForDomain:

    if (!isIncluded)
    {
      excluded.add(filter);
    }
    else if ((excluded.size == 0 || !excluded.has(filter)) &&
             !this.getException(filter, domain))
    {
      selectors.push(filter.selector);
    }

We can just change it to:

    if (!isIncluded)
    {
      excluded.add(filter);
    }
    else if (excluded.size == 0 || !excluded.has(filter))
    {
      let exception = this.getException(filter, domain);

      if (!exception)
        selectors.push(filter.selector);
      else if (withExceptions)
        exceptions.push(exception.text);
    }

    ...

    if (withExceptions)
      return {selectors, exceptions};

    return selectors;

And that should be enough.

Note that I'm trying to avoid calling ElemHide.getException for most domains in ​one of my patches so things might get interesting.

[kzar]

Since we're already working on some changes to this code I've marked this as being blocked by #6652.

Replying to mjethani:

Do we have to include all exceptions or only one that matched (and prevented the filter's selector from getting added)?

Yea, just the one that matched is fine.

PS: By the way I would prefer if provideExceptionFilters were called withExceptions instead.

Fine by me.

[mjethani]

I'll probably close #6652 since we have come a long way, but anyway it should not be blocking this issue now.

[mjethani]

Apply on revision 107b12edc115

[mjethani]

Hi Timm, I have attached a patch that should apply on ​changeset 107b12edc115 in adblockpluscore. If you pass includeSelectors=trueto generateStyleSheetForDomain you'll get a array of selectors. If you additionally pass withExceptions=true, the array will contain an exceptions property, which will be another array containing the exceptions.

I hope this helps.

[jsonesen]

Replying to mjethani:

I'll probably close #6652 since we have come a long way, but anyway it should not be blocking this issue now.

Sounds good, thanks for the update

[mjethani]

Jon, if you look further into this, a few things:

1. We have already optimized the style sheet generation for the default case (no DevTools panel open), I don't think we should touch that code path at all now
2. If DevTools is open, we follow a slightly different path, because we need to return the list of selectors as well, which needs to be dynamically generated
3. If DevTools needs the exceptions as well, we should have a completely different code path: In the generateStyleSheetForDomain function, we should check if the proposed withExceptions flag has been passed, and if this is the case then we should just do all of that in a separate block, and also use a different version of getConditionalSelectors (let's call it getConditionalSelectorsWithExceptions) that returns the exceptions as well similar to the patch I posted

These are some of my thoughts on this right now.

For context, DevTools is not open for most users most of the time, and it is also not there on mobile. We should not hurt the performance here for the special case of DevTools being open in the desktop extension.

[abpbot]

A commit referencing this issue has landed:
​Issue 6428 - Expose elemHideExceptions for a domain on request

[Ross]

Done. Working as described.

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2