Opened 22 months ago

Closed 8 months ago

Last modified 5 months ago

#6428 closed change (fixed)

Have generateStyleSheetForDomain provide exception filters if requested

Reported by: kzar Assignee: jsonesen
Priority: P2 Milestone:
Module: Core Keywords:
Cc: sergz, sebastian, jsonesen, mjethani, tjansen, sporz Blocked By:
Blocking: #6427 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/24

Description (last modified by mjethani)

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.

Attachments (1)

withExceptions.patch (3.9 KB) - added by mjethani 13 months ago.
Apply on revision 107b12edc115

Download all attachments as: .zip

Change History (27)

comment:1 Changed 20 months ago by jsonesen

  • Cc jsonesen added
  • Owner set to jsonesen

comment:2 Changed 20 months ago by kzar

  • Cc mjethani added

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

comment:3 follow-up: Changed 19 months ago by 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.

comment:4 Changed 19 months ago by 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.

comment:5 in reply to: ↑ 3 Changed 19 months ago by kzar

  • Blocked By 6652 added
  • Description modified (diff)

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.

comment:6 Changed 13 months ago by mjethani

  • Cc tjansen added

comment:7 follow-up: Changed 13 months ago by mjethani

  • Blocked By 6652 removed

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

Changed 13 months ago by mjethani

Apply on revision 107b12edc115

comment:8 Changed 13 months ago by 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 list of selectors (array). 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.

Version 0, edited 13 months ago by mjethani (next)

comment:9 in reply to: ↑ 7 Changed 13 months ago by 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

comment:10 Changed 13 months ago by 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.

comment:11 Changed 10 months ago by jsonesen

  • Cc sporz added

comment:12 Changed 9 months ago by jsonesen

  • Cc timm added

comment:13 Changed 9 months ago by jsonesen

  • Cc timm removed

comment:14 Changed 9 months ago by jsonesen

  • Description modified (diff)

comment:15 Changed 9 months ago by jsonesen

  • Description modified (diff)
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:16 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:17 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:18 Changed 9 months ago by mjethani

  • Summary changed from Have getSelectorsForDomain provide exception filters if requested to Have generateStyleSheetForDomain provide exception filters if requested

comment:19 Changed 8 months ago by abpbot

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

comment:20 Changed 8 months ago by jsonesen

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:21 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:22 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:23 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:24 Changed 8 months ago by jsonesen

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:25 Changed 8 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:26 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.