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/ |
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:3 Changed on 09/29/2018 at 02:19:51 AM by abpbot
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: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)
A commit referencing this issue has landed:
Issue 6999 - Remove inlineEmulated flag in content.applyFilters