Opened 10 months ago

Last modified 3 weeks ago

#6428 new change

Have getSelectorsForDomain provide exception filters if requested

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

Description (last modified by 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 getSelectorsForDomain in lib/elemHide.js to provide exception filters which applied if the optional withExceptions argument was passed.

Note: The getSelectorsForDomain code is a hotspot, so we must be extremely careful when modifying it to avoid extra overhead when this new argument wasn't passed (see #4057). At the same time I'd like to avoid duplicating the entire function. One way to (roughly) profile the code is to run this snippet:

(function() {
  var start = performance.now();
  for (var i = 0; i < 1000; i += 1)
    ElemHide.getSelectorsForDomain("www.extremetech.com");
  return performance.now() - start;
}());

(We should test performance on the various browsers we support too, since in practice a change might speed things up on one browser and slow them down on another.)

Attachments (1)

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

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 months ago by jsonesen

  • Cc jsonesen added
  • Owner set to jsonesen

comment:2 Changed 7 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 7 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 7 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 7 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 3 weeks ago by mjethani

  • Cc tjansen added

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

Apply on revision 107b12edc115

comment:8 Changed 3 weeks 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 3 weeks ago by mjethani (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 3 weeks 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 3 weeks 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.

Note: See TracTickets for help on using tickets.