Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid layout re-flow for cosmetic filtering #4585

Merged
merged 4 commits into from
Feb 14, 2020
Merged

Conversation

petemill
Copy link
Member

@petemill petemill commented Feb 11, 2020

Resolves brave/brave-browser#8197

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill petemill self-assigned this Feb 11, 2020
@petemill petemill force-pushed the insta-cosmetic-filtering branch from 1a65948 to d7b7094 Compare February 11, 2020 18:58
@petemill petemill force-pushed the insta-cosmetic-filtering branch from d7b7094 to 9c24069 Compare February 11, 2020 22:36
Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

.map(selector =>
allSelectorsToRules.get(selector)
)
.filter(i => i !== undefined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this checking? I dont see why this could happen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if allSelectorsToRules is passed a selector it does not contain then it will give us an undefined entry, which is going to mess with knowing the count to shift other indexes, as well as we'll have to check for it anyway before attempting to remove the undefined index rule.
We may know this cannot happen, but this function does not, and if we pass undefined to CSSStyleSheet.deleteRule() then it's not going to delete the rule we intended and all our indexes will be off.

}
continue
}
if (oldIdx !== i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, whats going on here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A sanity check that any previous re-indexes have kept this item in-sync. Though since we expect the actual index of an item in the Map to match the lookup value of the Map's item, we probably may as well use a combination of Set and array. Either way we would be doing something like converting Set to Array to find an index, or using indexOf for the lookup. So, this works for now and seems just as performant. Having said that, would love to optimize if you can think of a way.

@petemill petemill force-pushed the insta-cosmetic-filtering branch from 9c24069 to 5ec0bd5 Compare February 12, 2020 04:02
@petemill petemill added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Feb 12, 2020
@petemill
Copy link
Member Author

Just re-building macOS. Sure it's fine, but being double-safe

pes10k and others added 4 commits February 13, 2020 22:29
…they are found to be 1st-party

Fixes layout re-flow

Co-authored-by: Pete Miller <[email protected]>
Since we do not have a chrome.tabs.removeCSS API, we need another method for restoring the page's original css `display` rule value. Adding and removing rule via a constructed CSSStyleSheet injected via document.adoptedStyleSheets serves this purpose.
@petemill petemill force-pushed the insta-cosmetic-filtering branch from 5ec0bd5 to 36c8577 Compare February 14, 2020 06:29
@petemill petemill removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Feb 14, 2020
@petemill petemill merged commit 31f37c1 into master Feb 14, 2020
@petemill petemill deleted the insta-cosmetic-filtering branch February 14, 2020 19:26
@bsclifton bsclifton added this to the 1.6.x - Nightly milestone Feb 14, 2020
@bsclifton
Copy link
Member

Updated to add missing milestone (both to this PR and the issue 👍)

@bbondy bbondy modified the milestones: 1.6.x - Beta, 1.7.x - Dev Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cosmetic filtering causes a layout re-flow during page-load
5 participants