Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(87)

Issue 5455501458407424: Issue 1325 - Use Shadow DOM if possible when highlighting page elements (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 5 months ago by Sebastian Noack
Modified:
5 years, 4 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -35 lines) Patch
M include.postload.js View 1 2 3 6 chunks +116 lines, -35 lines 0 comments Download

Messages

Total messages: 10
Sebastian Noack
5 years, 5 months ago (2014-09-06 14:59:13 UTC) #1
Wladimir Palant
Way too many unrelated changes for my taste but somewhat justified here given how bad ...
5 years, 5 months ago (2014-09-10 21:16:33 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5629499534213120/include.postload.js#newcode36 include.postload.js:36: return null; On 2014/09/10 21:16:33, Wladimir Palant wrote: > ...
5 years, 4 months ago (2014-09-22 11:55:42 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); Please don't use variables outside ...
5 years, 4 months ago (2014-09-22 18:31:08 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/22 18:31:09, Wladimir Palant ...
5 years, 4 months ago (2014-09-23 11:30:07 UTC) #5
Wladimir Palant
LGTM http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 11:30:07, Sebastian ...
5 years, 4 months ago (2014-09-23 13:29:03 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 13:29:03, Wladimir Palant ...
5 years, 4 months ago (2014-09-23 15:11:39 UTC) #7
Wladimir Palant
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 15:11:39, Sebastian Noack ...
5 years, 4 months ago (2014-09-23 15:41:45 UTC) #8
Sebastian Noack
http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5455501458407424/diff/5639274879778816/include.postload.js#newcode37 include.postload.js:37: var root = clone.createShadowRoot(); On 2014/09/23 15:41:45, Wladimir Palant ...
5 years, 4 months ago (2014-09-23 16:37:33 UTC) #9
Wladimir Palant
5 years, 4 months ago (2014-09-23 23:07:39 UTC) #10
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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5