Opened on 09/28/2018 at 11:22:53 AM

Closed on 10/02/2018 at 10:10:15 PM

Last modified on 10/02/2018 at 10:38:55 PM

#6999 closed change (fixed)

Always generate style sheets in the background page

Reported by: mjethani Assignee: mjethani
Priority: Unknown Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian Blocked By: #6957
Blocking: #7000 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29894564/
https://codereview.adblockplus.org/29893559/

Description (last modified by mjethani)

Background

Currently the background page only generates the style sheet if user style sheets are supported. If user style sheets are not supported, the background page asks core to return the selectors for the domain and passes the selectors on to the content script, and the content script generates the style sheet.

After #6957, the background page will get the style sheet from core "for free" (almost), at least on most domains. It could then just pass the style sheet to the content script for injection. The overhead of passing the style sheet will simply replace the overhead of passing the selectors, as long as DevTools is not open. There is no additional cost of doing this for the most common case.

Dependency update

This change involves an adblockpluscore dependency update to hg:d555190eee2b git:a9327e3, which includes the following changes:

  • #6957 - Combine element hiding selector matching with style sheet generation
  • #7001 - Use instance property maxCacheEntries in CombinedMatcher
  • #6504 - Remove element hiding emulation code related to style sheets

What to change

Update the adblockpluscore dependency to hg:d555190eee2b git:a9327e3.

In lib/contentFiltering.js:

  • Replace the call to ElemHide.getSelectorsForDomain (now removed) with a call to ElemHide.generateStyleSheetForDomain. The last argument should be true if the function should return selectors, which is required when DevTools logging is on (basically the value of the trace variable); otherwise it should be false.
  • Pass the code property of the returned object to updateFrameStyles, which should now take a style sheet (string) instead of a list of selectors (array) and should then not have to generate the style sheet itself by calling createStyleSheet.
  • If user style sheets are not supported (the local inline variable is true), use the rulesFromStyleSheet function from adblockpluscore/lib/elemHide.js to extract the rules from the style sheet and return these rules as part of the response.
  • In the content.injectSelectors handler, use the createStyleSheet function to create a style sheet out of the selectors in the message; pass this style sheet to updateFrameStyles. If user style sheets are not supported, return the rules from the style sheet (using rulesFromStyleSheet, as in the preceding point) back to the content script.

In include.preload.js:

  • Remove the first argument to the ElemHideEmulation constructor as it is no longer present after the dependency update.
  • Rename addSelectorsInline to addRulesInline and let it take a list of rules that are directly inserted into the style sheet.
  • In the addSelectors method, send the content.injectSelectors message unconditionally, and if the response is a list of CSS rules, call addRulesInline with these rules.
  • Similarly, call addRulesInline if the response to the content.applyFilters message includes a list of rules.
  • Remove the filters parameter and related code from the addSelectors method of ContentFiltering.
  • Remove the filters parameter to the addSelectors method of ElementHidingTracer.
  • Remove the selectorGroupSize and inline properties of ContentFiltering.

Hints for testers

This change touches every part of the code that involves style sheets: element hiding, element hiding emulation, and element collapsing.

  • For element hiding, make sure that the feature works correctly, on two classes of platforms: platforms that support user style sheets, which is Firefox 53+ and Chrome 66+ (see #5090), and platforms that do not support user style sheets, which is older versions of Firefox and Chrome and all versions of Edge. In the case of platforms that do not support user style sheets, #242 will still be an issue, and this is expected; but on platforms that do support user style sheets, test #242 again to make sure the feature is still being used.
  • For element hiding emulation, make sure that it is still working correctly in general (all of :-abp-properties(), :-abp-has(), and :-abp-contains()) and that there are no errors/exceptions in the console.
  • For element collapsing, make sure the behavior that was tested for in #5899 and #6645 is still working.
  • When the DevTools panel is open, filter hits for element hiding and element hiding emulation filters should be displayed correctly.
  • Test element hiding in combination with element collapsing (#5899 and #6645) to make sure that one style sheet does not overwrite the other. In other words, when an element is collapsed, it should not cause an element hidden by an element hiding filter to reappear.
  • When a new filter is added via the Block element tool, it should cause the target element to be hidden, without causing other elements that were previously hidden to be redisplayed. As described in #6967, however, removing the filter will still keep the target element hidden, and this is the expected behavior.

All tests should be done on platforms both with and without user style sheets support, including Chrome, Firefox, and Edge.

Keep an eye out for any errors/exceptions in both the background page console and the console attached to the tab in which the test page is loaded.

Also see #6957 for more specific tests related to element hiding.

Attachments (0)

Change History (11)

comment:1 Changed on 09/28/2018 at 11:23:56 AM by mjethani

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

comment:2 Changed on 09/28/2018 at 11:44:49 AM by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed on 09/29/2018 at 02:19:51 AM by abpbot

A commit referencing this issue has landed:
Issue 6999 - Remove inlineEmulated flag in content.applyFilters

comment:4 Changed on 10/02/2018 at 08:25:58 PM by abpbot

A commit referencing this issue has landed:
Issue 6999 - Generate style sheets in background page

comment:5 Changed on 10/02/2018 at 09:12:57 PM by mjethani

  • Description modified (diff)

comment:6 Changed on 10/02/2018 at 10:07:37 PM by mjethani

  • Description modified (diff)

comment:7 Changed on 10/02/2018 at 10:10:15 PM by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:8 Changed on 10/02/2018 at 10:10:42 PM by mjethani

  • Summary changed from Always generate style sheets in background page to Always generate style sheets in the background page

comment:9 Changed on 10/02/2018 at 10:11:30 PM by mjethani

  • Blocking 7000 added

comment:10 Changed on 10/02/2018 at 10:14:30 PM by mjethani

  • Blocked By 6957 added

comment:11 Changed on 10/02/2018 at 10:38:55 PM by mjethani

  • Description modified (diff)

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
 
Note: See TracTickets for help on using tickets.