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

Issue 29575739: Issue 5864 - Remove previous style sheet before adding one (Closed)

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Remove style sheet if selectors list is empty #

Total comments: 1

Patch Set 3 : Rename inject to inline #

Patch Set 4 : Maintain separate style sheet for emulated selectors #

Total comments: 6

Patch Set 5 : Always remove old rules #

Total comments: 5

Patch Set 6 : Remove some code duplication #

Patch Set 7 : Create style sheet in include.preload.js #

Total comments: 9

Patch Set 8 : Maintain all style sheets in lib/cssInjection.js #

Total comments: 2

Patch Set 9 : Rebase #

Patch Set 10 : Inline replaceStyleSheet #

Total comments: 16

Patch Set 11 : Rebase and fix errors #

Patch Set 12 : Use "standard" group name #

Patch Set 13 : Fix rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -31 lines) Patch
M include.preload.js View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +29 lines, -21 lines 0 comments Download
M lib/cssInjection.js View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +52 lines, -10 lines 0 comments Download
M polyfill.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 26
Manish Jethani
2 years, 1 month ago (2017-10-13 23:59:27 UTC) #1
Manish Jethani
Patch Set 1 Currently we just keep adding new style sheets every time the DOM ...
2 years, 1 month ago (2017-10-14 00:08:03 UTC) #2
Manish Jethani
On 2017/10/14 00:08:03, Manish Jethani wrote: > Take this HTML for example: > [...] I ...
2 years, 1 month ago (2017-10-14 00:11:36 UTC) #3
Manish Jethani
It's a little worse actually. If you go into the console and enter "document.head.removeChild(document.head.children[0])" the ...
2 years, 1 month ago (2017-10-14 00:36:06 UTC) #4
Manish Jethani
Patch Set 2: Remove style sheet if selectors list is empty https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js File include.preload.js (right): ...
2 years, 1 month ago (2017-10-14 13:38:19 UTC) #5
Manish Jethani
Patch Set 3: Rename inject to inline Clarifying the terminology a bit. "inject" means inject ...
2 years, 1 month ago (2017-10-14 13:44:57 UTC) #6
Manish Jethani
Patch Set 4: Maintain separate style sheet for emulated selectors This takes care of inline ...
2 years, 1 month ago (2017-10-14 14:23:23 UTC) #7
Manish Jethani
Patch Set 5: Always remove old rules https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#newcode520 include.preload.js:520: if (this.style ...
2 years, 1 month ago (2017-10-14 21:35:34 UTC) #8
Manish Jethani
Patch Set 6: Remove some code duplication
2 years, 1 month ago (2017-10-14 21:55:14 UTC) #9
Manish Jethani
Patch Set 7: Create style sheet in include.preload.js It makes sense to create the style ...
2 years, 1 month ago (2017-10-14 23:38:56 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#newcode520 include.preload.js:520: if (this.style && this.style.parentNode) On 2017/10/14 21:35:33, Manish Jethani ...
2 years, 1 month ago (2017-10-15 21:29:13 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#oldcode490 include.preload.js:490: this.style = null; What if the "Block element" dialog ...
2 years, 1 month ago (2017-10-17 22:00:38 UTC) #12
Manish Jethani
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#oldcode490 include.preload.js:490: this.style = null; On 2017/10/17 22:00:38, Sebastian Noack wrote: ...
2 years, 1 month ago (2017-10-17 22:14:28 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#oldcode490 include.preload.js:490: this.style = null; On 2017/10/17 22:14:27, Manish Jethani wrote: ...
2 years, 1 month ago (2017-10-18 00:53:07 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#oldcode490 include.preload.js:490: this.style = null; On 2017/10/18 00:53:07, Sebastian Noack wrote: ...
2 years, 1 month ago (2017-10-18 01:48:37 UTC) #15
Manish Jethani
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#newcode386 include.preload.js:386: return null; On 2017/10/18 01:48:37, Manish Jethani wrote: > ...
2 years, 1 month ago (2017-10-18 01:49:56 UTC) #16
Manish Jethani
Patch Set 8: Maintain all style sheets in lib/cssInjection.js We were not removing the old ...
2 years, 1 month ago (2017-10-18 13:16:46 UTC) #17
Manish Jethani
Patch Set 9: Rebase
2 years, 1 month ago (2017-10-18 13:31:18 UTC) #18
Manish Jethani
Patch Set 10: Inline replaceStyleSheet
2 years, 1 month ago (2017-10-18 13:41:32 UTC) #19
kzar
Once again really sorry for being so slow to review! https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#oldcode499 ...
1 year, 9 months ago (2018-01-24 14:30:54 UTC) #20
Manish Jethani
Patch Set 11: Rebase and fix errors https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#oldcode499 include.preload.js:499: else if ...
1 year, 9 months ago (2018-01-24 16:37:45 UTC) #21
kzar
Otherwise this LGTM https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#oldcode499 include.preload.js:499: else if (this.tracer) On 2018/01/24 16:37:44, ...
1 year, 9 months ago (2018-01-25 11:52:37 UTC) #22
Manish Jethani
Patch Set 12: Use "standard" group name https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#newcode505 include.preload.js:505: this.addSelectorsInline(response.selectors); On ...
1 year, 9 months ago (2018-01-25 14:29:34 UTC) #23
kzar
LGTM, Sebastian you want to take another look?
1 year, 9 months ago (2018-01-25 14:34:45 UTC) #24
Manish Jethani
Patch Set 13: Fix rebase Sorry the previous rebase wasn't right, now fixed.
1 year, 9 months ago (2018-01-25 14:38:27 UTC) #25
Sebastian Noack
1 year, 9 months ago (2018-01-25 15:44:36 UTC) #26
On 2018/01/25 14:34:45, kzar wrote:
> LGTM, Sebastian you want to take another look?

I wouldn't get around to have a deep look soon. Feel free to remove me as a
reviewer from this one, and push it (into the "next" bookmark).
Sign in to reply to this message.

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