Issue 5663214012465152: Issue 2467 - Ingore inner quotes when parsing CSS selectors (Closed)
|
Created:
May 7, 2015, 3:25 p.m. by Sebastian Noack Modified:
May 15, 2015, 11:35 a.m. Reviewers:
Thomas Greiner CC:
Wladimir Palant, kzar Visibility:
Public. |
DescriptionIssue 2467 - Ingore inner quotes when parsing CSS selectors
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 8
LGTM Note that, at the moment, not a single filter in EasyList or EasyListGermany contains multiple selectors. http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) This loop is only necessary if there's a `,` in the string and since that's only the case for less than 0.5% of element hiding filters in EasyListGermany it seems to be well worth checking for it in advance. Not really related to this issue though so we can tackle this in a separate ticket.
http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) On 2015/05/07 17:42:58, Thomas Greiner wrote: > This loop is only necessary if there's a `,` in the string and since that's only > the case for less than 0.5% of element hiding filters in EasyListGermany it > seems to be well worth checking for it in advance. > > Not really related to this issue though so we can tackle this in a separate > ticket. Not a bad idea, of course assuming that it makes a notable difference performance-wise. After all this state machine doesn't perform too bad.
http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) On 2015/05/07 18:59:40, Sebastian Noack wrote: > On 2015/05/07 17:42:58, Thomas Greiner wrote: > > This loop is only necessary if there's a `,` in the string and since that's > only > > the case for less than 0.5% of element hiding filters in EasyListGermany it > > seems to be well worth checking for it in advance. > > > > Not really related to this issue though so we can tackle this in a separate > > ticket. > > Not a bad idea, of course assuming that it makes a notable difference > performance-wise. After all this state machine doesn't perform too bad. So I tried following: function convertSelectorsForShadowDOM(selectors) { t = Date.now(); ... for (var i = 0; i < selectors.length; i++) { var selector = selectors[i]; if (selector.indexOf(",") == -1) { result.push(prefix + selector); continue; } ... } console.log(Date.now() - t); result; } However, I couldn't measure any difference. Both, with and without the shortcut the function runs for 4-8ms on my notebook while loading nytimes.com. But feel free to do a little more benchmarking.
http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) On 2015/05/07 19:17:22, Sebastian Noack wrote: > On 2015/05/07 18:59:40, Sebastian Noack wrote: > > On 2015/05/07 17:42:58, Thomas Greiner wrote: > > > This loop is only necessary if there's a `,` in the string and since that's > > only > > > the case for less than 0.5% of element hiding filters in EasyListGermany it > > > seems to be well worth checking for it in advance. > > > > > > Not really related to this issue though so we can tackle this in a separate > > > ticket. > > > > Not a bad idea, of course assuming that it makes a notable difference > > performance-wise. After all this state machine doesn't perform too bad. > > So I tried following: > > function convertSelectorsForShadowDOM(selectors) > { > t = Date.now(); > ... > for (var i = 0; i < selectors.length; i++) > { > var selector = selectors[i]; > if (selector.indexOf(",") == -1) > { > result.push(prefix + selector); > continue; > } > ... > } > console.log(Date.now() - t); > result; > } > > However, I couldn't measure any difference. Both, with and without the shortcut > the function runs for 4-8ms on my notebook while loading http://nytimes.com. But feel > free to do a little more benchmarking. I'd recommend using `window.performance.now()` for that instead of `Date.now()` since it's much more accurate. Also don't forget to consider the accumulative number since it might run in multiple frames and thereby slow down the page load by more than just a few milliseconds. I saw an up to 50% speed improvement on my computer on recode.net (5 frames) compared to the approach without. Generally, there is a speed improvement for about 90% of page loads but the absolute numbers for time saved depend on how many frames the page has and seem to vary between 2ms to 6ms per frame (e.g. 10-30ms on recode.net) from what I've observed.
http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) I'm not sure whether I can confirm your benchmark. But if you think its an improvement I wouldn't mind having a shortcut here, as its a trivial change. Feel free to submit a patch.
http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... File include.preload.js (right): http://codereview.adblockplus.org/5663214012465152/diff/5629499534213120/incl... include.preload.js:156: for (var j = 0; j < selector.length; j++) On 2015/05/07 17:42:58, Thomas Greiner wrote: > This loop is only necessary if there's a `,` in the string and since that's only > the case for less than 0.5% of element hiding filters in EasyListGermany it > seems to be well worth checking for it in advance. > > Not really related to this issue though so we can tackle this in a separate > ticket. There you go: http://codereview.adblockplus.org/4759827771293696/ 
|
||||||||||||||||||||||||||||||||||||||||||||||||||
