Opened 14 months ago

Closed 2 weeks 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: Unknown Verified working: no
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 5 months ago.
Apply on revision 107b12edc115

Download all attachments as: .zip

Change History (26)

comment:1 Changed 12 months ago by jsonesen

  • Cc jsonesen added
  • Owner set to jsonesen

comment:2 Changed 12 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 12 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 12 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 12 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 5 months ago by mjethani

  • Cc tjansen added

comment:7 follow-up: Changed 5 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 5 months ago by mjethani

Apply on revision 107b12edc115

comment:8 Changed 5 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 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.

Last edited 5 months ago by mjethani (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 5 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 5 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 7 weeks ago by jsonesen

  • Cc sporz added

comment:12 Changed 7 weeks ago by jsonesen

  • Cc timm added

comment:13 Changed 7 weeks ago by jsonesen

  • Cc timm removed

comment:14 Changed 6 weeks ago by jsonesen

  • Description modified (diff)

comment:15 Changed 6 weeks ago by jsonesen

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

comment:16 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:17 Changed 6 weeks ago by mjethani

  • Description modified (diff)

comment:18 Changed 6 weeks ago by mjethani

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

comment:19 Changed 2 weeks ago by abpbot

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

comment:20 Changed 2 weeks ago by jsonesen

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

comment:21 Changed 2 weeks ago by mjethani

  • Description modified (diff)

comment:22 Changed 2 weeks ago by mjethani

  • Description modified (diff)

comment:23 Changed 2 weeks ago by mjethani

  • Description modified (diff)

comment:24 Changed 2 weeks ago by jsonesen

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:25 Changed 2 weeks ago by mjethani

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.