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

improve DOM inspector #3254

Merged
merged 3 commits into from
Nov 21, 2017
Merged

improve DOM inspector #3254

merged 3 commits into from
Nov 21, 2017

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Nov 21, 2017

  • Fix race between userCSS injection and element highlight resulting in none or not all elements highlighted.
  • Fix page being scanned twice resulting in unneeded slowdown.

Hi there,

I've found that the page is scanned twice. First in bootstrap() and later in onReady() both resulted in cosmeticFilterMapper.reset(); causing massive slowdowns. Initial open of the dom inspector can take a long time ~3s or so, so it is quite important to not do the work twice by accident.

Also I've fixed visual annoyance where elements are highlighted after first scroll because of race condition between dom filtering disable and highlight code.

Update:

  • mutationTimer was not cleared resulting in any mutation update after first one to be ignored
  • And procedural filters were shown with random id instead of actual filter

Thanks.
Kacper

- Fix race between userCSS injection and element highlight resulting in none or not all elements highlighted.
- Fix page being scanned twice resulting in unneeded slowdown.
@kasper93 kasper93 mentioned this pull request Nov 21, 2017
17 tasks
@gorhill
Copy link
Owner

gorhill commented Nov 21, 2017

Thanks, I will look into this a bit later today.

Sorry for the delay, I must have turned off notifications somewhere a while ago, I need to find out how to re-enable again now that the repo is more quiet (there was too much of them when I first published the stable webext version).

filterMap.set(node, entry.raw);
}
// Upgrade declarative selector to procedural one
filterMap.set(node, entry.raw);
Copy link
Owner

@gorhill gorhill Nov 21, 2017

Choose a reason for hiding this comment

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

I will need to address this in the future, there used to be a time were I was reporting all filters matching a node. I can't remember why I removed this ability, I probably felt the code was growing complicated and needed to simplify until I got all under control. Anyway, what is the thinking for overriding plain CSS selector with a procedural one when there is collision? (I don't have a set opinion, just curious).

@@ -363,6 +363,7 @@ var domLayout = (function() {

var journalFromMutations = function() {
var nodelist, node, domNode, nid;
mutationTimer = undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Ugh. Good catch.

@@ -800,8 +800,7 @@ var start = function() {
document.removeEventListener(ev.type, onReady);
}
vAPI.messaging.sendTo(loggerConnectionId, domLayout.get());
vAPI.domFilterer.toggle(false);
highlightElements();
vAPI.domFilterer.toggle(false, highlightElements);
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I think I see what was happening: the highlighter was called before the user stylesheets were removed (from extension process), hence causing the highlighter to see only empty boxes. I was puzzle as to why there was this glitch with Firefox but deferred investigation. Nice catch.

@gorhill
Copy link
Owner

gorhill commented Nov 21, 2017

Looks all good to me.

@gorhill gorhill merged commit ec70c75 into gorhill:master Nov 21, 2017
@gorhill
Copy link
Owner

gorhill commented Nov 21, 2017

Thanks.

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.

2 participants