Issue 5455501458407424: Issue 1325 - Use Shadow DOM if possible when highlighting page elements (Closed)
|
Created:
Sept. 6, 2014, 2:57 p.m. by Sebastian Noack Modified:
Sept. 23, 2014, 11:07 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 1325 - Use Shadow DOM if possible when highlighting page elements
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #Patch Set 3 : Fixed typo in comment #
Total comments: 11
Patch Set 4 : Addressed comments #MessagesTotal messages: 10
Way too many unrelated changes for my taste but somewhat justified here given how bad the original code is. http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... include.postload.js:36: return null; Where does this list come from? This seems to be hardcoding some outdated state of the Chrome implementation. For reference: https://code.google.com/p/chromium/issues/detail?id=234020. The way I read it, applet, input and embed have been fixed already - img and select are still problematic however. See also: https://bugs.webkit.org/show_bug.cgi?id=102864 Given that these issues are being detected by W3C tests - can't we detect them as well instead of hardcoding this? http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... include.postload.js:91: style.sheet.insertRule(":host { box-shadow: " + boxShadow + " !important; background-color: " + backgroundColor + " !important; }", 0); Use style.textContent = ... here for simplicity? Note that you can do it before adding the style to the document then. Also, please break that string into multiple lines to make it easier to read, e.g.: ":host {\ box-shadow: " + boxShadow + " !important;\ background-color: " + backgroundColor + " !important;\ }"
http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... include.postload.js:36: return null; On 2014/09/10 21:16:33, Wladimir Palant wrote: > Where does this list come from? This seems to be hardcoding some outdated state > of the Chrome implementation. For reference: > https://code.google.com/p/chromium/issues/detail?id=234020. The way I read it, > applet, input and embed have been fixed already - img and select are still > problematic however. See also: https://bugs.webkit.org/show_bug.cgi?id=102864 > > Given that these issues are being detected by W3C tests - can't we detect them > as well instead of hardcoding this? I finally found a way to test insertion points. http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/incl... include.postload.js:91: style.sheet.insertRule(":host { box-shadow: " + boxShadow + " !important; background-color: " + backgroundColor + " !important; }", 0); On 2014/09/10 21:16:33, Wladimir Palant wrote: > Use style.textContent = ... here for simplicity? Note that you can do it before > adding the style to the document then. > > Also, please break that string into multiple lines to make it easier to read, > e.g.: > > ":host {\ > box-shadow: " + boxShadow + " !important;\ > background-color: " + backgroundColor + " !important;\ > }" Done.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); Please don't use variables outside the block where they are defined, even if JavaScript allows it. http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:46: var child = document.createTextNode(''); Double quotation marks please (same two lines below). http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:109: // style while highlighted. Please put this comment before the line you are commenting on (and probably combine it with the comment below) - it's too long to be placed inline.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/22 18:31:09, Wladimir Palant wrote: > Please don't use variables outside the block where they are defined, even if > JavaScript allows it. Done, though I don't like that policy. :/ http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:46: var child = document.createTextNode(''); On 2014/09/22 18:31:09, Wladimir Palant wrote: > Double quotation marks please (same two lines below). Done. http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:109: // style while highlighted. On 2014/09/22 18:31:09, Wladimir Palant wrote: > Please put this comment before the line you are commenting on (and probably > combine it with the comment below) - it's too long to be placed inline. Done.
LGTM http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 11:30:07, Sebastian Noack wrote: > Done, though I don't like that policy. :/ I think I explained my reasoning before - doing this makes the code less obvious and more error-prone, developers don't usually assume that a variable declared inside a block can have side-effects outside it. Besides, it will make switching to block-scoped variables (something that will hopefully be possible for all our code at some point) non-trivial.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 13:29:03, Wladimir Palant wrote: > On 2014/09/23 11:30:07, Sebastian Noack wrote: > > Done, though I don't like that policy. :/ > > I think I explained my reasoning before Yes, we did. And I think it is pointless to repeat that debate. That is why I already addressed that comment. > doing this makes the code less obvious > and more error-prone, developers don't usually assume that a variable declared > inside a block can have side-effects outside it. If you come from C or Java this might be unexpected, like a lot of other things in JavaScript. However, that is how variables work in (almost) all script languages, including JavaScript, Python, Perl, PHP and Bash. So I think it is more likely that this is exactly what somebody writing JavaScript assumes. > Besides, it will make switching > to block-scoped variables (something that will hopefully be possible for all our > code at some point) non-trivial. Just because there will be the "let" statement doesn't mean that we must adopt it for every single variable declaration in our code. I find function-scoped variables very useful in some cases like this. It feels hack-ish and stupid, to add an additional line, to declare a variable in a different scope, just to deny the concept of function-scoped variables.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 15:11:39, Sebastian Noack wrote: > However, that is how variables work in (almost) all script > languages, including JavaScript, Python, Perl, PHP and Bash. You are wrong about Perl, and in Python and PHP local variables simply aren't being declared at all (never mind Bash, does it even have local variables?). Function-scoped variables are one of the least obvious JavaScript features, and quite a few JavaScript developers don't know about this at all. > Just because there will be the "let" statement doesn't mean that we must adopt > it for every single variable declaration in our code. Yes, we should - for the reasons outlined above, and to avoid common gotchas like duplicate variable declarations.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 15:41:45, Wladimir Palant wrote: > On 2014/09/23 15:11:39, Sebastian Noack wrote: > > However, that is how variables work in (almost) all script > > languages, including JavaScript, Python, Perl, PHP and Bash. > > You are wrong about Perl I don't think so. Run following code if you don't believe: sub foo { if (1) { my $foo = 42; } print $foo; } foo(); >, and in Python and PHP local variables simply aren't > being declared at all You could also say that they're automatically or implicit declared on first initialization. But the point is that variables in Python and PHP are function-scoped, too. > (never mind Bash, does it even have local variables?). Yes, bash has local variables, and they are function-scoped: foo () { if [ true ] then local x=42 fi echo $x } foo > Function-scoped variables are one of the least obvious JavaScript features, and > quite a few JavaScript developers don't know about this at all. People are often confused with closures and function contexts in JavaScript, but that is a different topic. But I never heard of somebody who calls himself a JavaScript coder and has experience in at least one more dynamic language, who assumed functions to be block-scoped. So I can only guess that this happend to you at some point, and therefore you consider it a common pitfall. ;) > > Just because there will be the "let" statement doesn't mean that we must adopt > > it for every single variable declaration in our code. > > Yes, we should - for the reasons outlined above As outlined above, I don't agree to those reasons. > , and to avoid common gotchas > like duplicate variable declarations. There is "use strict" and tools like JSLint and JSHint for that purpose.
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/incl... include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 16:37:33, Sebastian Noack wrote: > I don't think so. Run following code if you don't believe: > > sub foo > { > if (1) > { > my $foo = 42; > } > print $foo; > } > foo(); Please add "use strict;" at the top. > Yes, bash has local variables, Still doesn't make it a programming language :) 
|
