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

Issue 29670575: Issue 5899 - Use CSS attribute selectors for collapsing media elements (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by Manish Jethani
Modified:
1 month, 4 weeks ago
Reviewers:
kzar, Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Patch Set 1 #

Total comments: 7

Patch Set 2 : Rebase #

Patch Set 3 : Use CSS attribute selectors for collapsing #

Total comments: 15

Patch Set 4 : Address comments to Patch Set 3 #

Patch Set 5 : Only append new selectors #

Total comments: 2

Patch Set 6 : Do not trace collapsing selectors #

Total comments: 3

Patch Set 7 : Check for "collapsing" group name #

Total comments: 2

Patch Set 8 : Rebase #

Patch Set 9 : Compare child name directly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -11 lines) Patch
M include.preload.js View 1 2 3 4 5 6 7 8 5 chunks +58 lines, -8 lines 0 comments Download
M lib/cssInjection.js View 1 2 3 4 5 6 7 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 20
Manish Jethani
5 months ago (2018-01-16 10:28:20 UTC) #1
Manish Jethani
Patch Set 1 So far we have been using the same element hiding functionality for ...
5 months ago (2018-01-16 10:47:27 UTC) #2
kzar
On 2018/01/16 10:47:27, Manish Jethani wrote: > (Also I think we could switch to the ...
4 months, 3 weeks ago (2018-01-24 14:37:23 UTC) #3
Manish Jethani
Changed the implementation now to use CSS attribute selectors. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#newcode164 include.preload.js:164: ...
3 months, 3 weeks ago (2018-02-22 06:55:06 UTC) #4
kzar
I wonder if the code would be easier to grok if you split isMediaElement out ...
3 months, 3 weeks ago (2018-02-26 17:18:59 UTC) #5
Manish Jethani
Patch Set 3 On 2018/02/26 17:18:59, kzar wrote: > I wonder if the code would ...
3 months, 2 weeks ago (2018-02-27 13:11:49 UTC) #6
Manish Jethani
On 2018/02/27 13:11:49, Manish Jethani wrote: > Patch Set 3 Uh ... I mean Patch ...
3 months, 2 weeks ago (2018-02-27 13:12:11 UTC) #7
kzar
https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#newcode164 include.preload.js:164: elemhide.addSelectors(Array.from(collapsingSelectors), null, "collapsing"); On 2018/02/22 06:55:06, Manish Jethani wrote: ...
3 months, 2 weeks ago (2018-02-27 14:18:39 UTC) #8
Manish Jethani
Patch Set 5: Only append new selectors https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#newcode164 include.preload.js:164: elemhide.addSelectors(Array.from(collapsingSelectors), null, ...
3 months, 2 weeks ago (2018-02-27 16:00:23 UTC) #9
kzar
LGTM, you want to take a look Sebastian? https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js#newcode104 lib/cssInjection.js:104: let ...
3 months, 2 weeks ago (2018-02-27 17:25:23 UTC) #10
Manish Jethani
Patch Set 6: Do not trace collapsing selectors https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#newcode527 include.preload.js:527: // ...
3 months, 2 weeks ago (2018-02-28 10:31:16 UTC) #11
kzar
https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#newcode527 include.preload.js:527: // Only trace selectors that are based directly on ...
3 months, 2 weeks ago (2018-03-02 09:37:28 UTC) #12
Manish Jethani
Patch Set 7: Check for "collapsing" group name https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#newcode527 include.preload.js:527: // ...
3 months, 2 weeks ago (2018-03-02 11:09:36 UTC) #13
Sebastian Noack
Let me know if I miss something, but from a first glance at the latest ...
3 months, 1 week ago (2018-03-10 23:43:49 UTC) #14
Manish Jethani
On 2018/03/10 23:43:49, Sebastian Noack wrote: > Let me know if I miss something, but ...
3 months, 1 week ago (2018-03-11 00:24:34 UTC) #15
Sebastian Noack
On 2018/03/11 00:24:34, Manish Jethani wrote: > So I think the point is to avoid ...
3 months, 1 week ago (2018-03-11 01:14:56 UTC) #16
Manish Jethani
On 2018/03/11 01:14:56, Sebastian Noack wrote: > On 2018/03/11 00:24:34, Manish Jethani wrote: > > ...
3 months ago (2018-03-13 14:53:11 UTC) #17
kzar
Otherwise this LGTM. I read through your discussion and I just wanted to add that ...
2 months ago (2018-04-18 14:56:13 UTC) #18
Manish Jethani
Patch Set 8: Rebase Patch Set 9: Compare child name directly https://codereview.adblockplus.org/29670575/diff/29713555/include.preload.js File include.preload.js (right): ...
1 month, 4 weeks ago (2018-04-19 21:57:20 UTC) #19
kzar
1 month, 4 weeks ago (2018-04-20 10:56:25 UTC) #20
Extra LGTM
Sign in to reply to this message.

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