Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5989914281771008: Use Shadow DOM for element hiding (Closed)

Created:
April 15, 2014, 9:48 a.m. by Sebastian Noack
Modified:
April 16, 2014, 7:53 a.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Adblock Plus breaks web pages that rely on the order of their own <style> tags e.g. by using document.styleSheets as currently reported by #309. However we can use Shadow DOM in order to hide our stylesheet from the web page.

Patch Set 1 #

Total comments: 13

Patch Set 2 : Fixed Shadow DOM usage and simplified code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -29 lines) Patch
M background.js View 1 chunk +1 line, -1 line 0 comments Download
M include.preload.js View 1 1 chunk +25 lines, -28 lines 0 comments Download

Messages

Total messages: 4
Sebastian Noack
http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js File include.preload.js (left): http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js#oldcode26 include.preload.js:26: elemhideElt.parentNode.removeChild(elemhideElt); Storing the inserted <style> tag into a global ...
April 15, 2014, 9:58 a.m. (2014-04-15 09:58:06 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js File include.preload.js (left): http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js#oldcode26 include.preload.js:26: elemhideElt.parentNode.removeChild(elemhideElt); On 2014/04/15 09:58:06, Sebastian Noack wrote: > Storing ...
April 15, 2014, 1:13 p.m. (2014-04-15 13:13:14 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/include.preload.js#newcode28 include.preload.js:28: if (selectors.length == 0) On 2014/04/15 13:13:14, Wladimir Palant ...
April 15, 2014, 7:21 p.m. (2014-04-15 19:21:54 UTC) #3
Wladimir Palant
April 15, 2014, 9:19 p.m. (2014-04-15 21:19:47 UTC) #4
This is LGTM if you verify that it works in Chrome 19. We also need to check how
the memory usage changes with this change, if webpages take up much more memory
now we might have to revert it. Ideally we would measure the impact on the
processing time as well, if the stylesheet applies synchronously that might
work.

http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5989914281771008/diff/5629499534213120/incl...
include.preload.js:50: if (!elt.sheet)
I didn't remember but hg blame points to
https://adblockplus.org/forum/viewtopic.php?t=8834. As with any issues that
cannot be reproduced, it is really hard to tell whether the issue is gone.
However, it seems that this was running on a timeout even before the change in
question - so older Chrome versions weren't initializing the element.sheet
property immediately.

Please verify that your code works in Chrome 19. You can get Chromium 19 (trunk
build, right before the release branch was created) from
https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.htm...
(builds for other operating systems are available as well).

Powered by Google App Engine
This is Rietveld