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

Issue 29410607: Issue 5090 - Use user stylesheets for element hiding if possible (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months ago by Manish Jethani
Modified:
5 months ago
Visibility:
Public.

Description

If the chrome.tabs.insertCSS API is available and supports "user" stylesheets (Firefox 53), we use it instead of injecting the CSS directly into the page.

Patch Set 1 #

Total comments: 8

Patch Set 2 : Use chrome.tabs.insertCSS instead #

Total comments: 4

Patch Set 3 : Add comment explaining when we inject styles #

Total comments: 3

Patch Set 4 : Check platform and platform version for user stylesheet support #

Total comments: 6

Patch Set 5 : Remove try/catch #

Total comments: 11

Patch Set 6 : Move get-selectors and hide-elements to elemHideHelper #

Patch Set 7 : Use feature detection #

Total comments: 21

Patch Set 8 : Make hideElements return a promise #

Total comments: 10

Patch Set 9 : Change inject to insertMode #

Total comments: 1

Patch Set 10 : Make hideElements synchronous #

Patch Set 11 : Simplify addSelectors #

Patch Set 12 : Further simplify addSelectors #

Total comments: 6

Patch Set 13 : Update message and file names #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -36 lines) Patch
M background.js View 1 2 3 4 5 6 7 1 chunk +0 lines, -29 lines 0 comments Download
M ext/background.js View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M include.preload.js View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +35 lines, -6 lines 0 comments Download
A lib/cssInjection.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +93 lines, -0 lines 0 comments Download
M metadata.chrome View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 43
Manish Jethani
Patch Set 1 If the browser.tabs.insertCSS API is available and supports "user" stylesheets (Firefox 53), ...
8 months ago (2017-04-12 11:03:49 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode51 background.js:51: if (!tryInsertCSS) Chrome and Firefox should use the same ...
8 months ago (2017-04-12 12:21:19 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode71 background.js:71: port.on("hide-elements", (msg, sender) => On 2017/04/12 12:21:18, Sebastian Noack ...
8 months ago (2017-04-12 12:25:56 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode51 background.js:51: if (!tryInsertCSS) On 2017/04/12 12:21:18, Sebastian Noack wrote: > ...
8 months ago (2017-04-12 12:32:30 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode51 background.js:51: if (!tryInsertCSS) On 2017/04/12 12:32:30, Manish Jethani wrote: > ...
8 months ago (2017-04-12 12:54:43 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode51 background.js:51: if (!tryInsertCSS) On 2017/04/12 12:32:30, Manish Jethani wrote: > ...
8 months ago (2017-04-12 13:37:57 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29410607/diff/29410608/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newcode51 background.js:51: if (!tryInsertCSS) On 2017/04/12 13:37:57, Wladimir Palant wrote: > ...
8 months ago (2017-04-12 14:27:01 UTC) #7
Manish Jethani
Patch Set 2: Use chrome.tabs.insertCSS instead
8 months ago (2017-04-12 14:43:29 UTC) #8
kzar
Your changes so far look really good, glad to have you. Sorry my slowness, but ...
8 months ago (2017-04-13 07:56:25 UTC) #9
Manish Jethani
Patch Set 3: Add comment explaining when we inject styles > - I guess the ...
8 months ago (2017-04-18 09:25:05 UTC) #10
Manish Jethani
On 2017/04/12 12:54:43, Sebastian Noack wrote: > Keep, in mind that with -abp-selector/:has there will ...
8 months ago (2017-04-18 09:34:58 UTC) #11
kzar
On 2017/04/18 09:25:05, Manish Jethani wrote: > Patch Set 3: Add comment explaining when we ...
7 months, 4 weeks ago (2017-04-20 06:17:15 UTC) #12
Manish Jethani
On 2017/04/20 06:17:15, kzar wrote: > On 2017/04/18 09:25:05, Manish Jethani wrote: > > Patch ...
7 months, 4 weeks ago (2017-04-20 12:49:32 UTC) #13
Manish Jethani
Patch Set 4: Check platform and platform version for user stylesheet support https://codereview.adblockplus.org/29410607/diff/29416561/background.js File background.js ...
7 months, 4 weeks ago (2017-04-20 12:50:32 UTC) #14
kzar
https://codereview.adblockplus.org/29410607/diff/29416561/background.js File background.js (right): https://codereview.adblockplus.org/29410607/diff/29416561/background.js#newcode58 background.js:58: if (error && /\bError processing cssOrigin\b/.test(error.message) != -1) On ...
7 months, 3 weeks ago (2017-04-21 11:20:22 UTC) #15
kzar
https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js File lib/css.js (right): https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51 lib/css.js:51: { On 2017/04/21 11:20:22, kzar wrote: > Nit: We'd ...
7 months, 3 weeks ago (2017-04-21 11:21:57 UTC) #16
Manish Jethani
On 2017/04/21 11:20:22, kzar wrote: > Nice, looks good. Could you update the issue description ...
7 months, 3 weeks ago (2017-04-21 13:35:52 UTC) #17
Manish Jethani
Patch Set 5: Remove try/catch https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js File lib/css.js (right): https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode48 lib/css.js:48: try On 2017/04/21 11:20:22, ...
7 months, 3 weeks ago (2017-04-21 13:39:33 UTC) #18
kzar
Otherwise LGTM https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js File lib/css.js (right): https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51 lib/css.js:51: { On 2017/04/21 13:39:32, Manish Jethani wrote: ...
7 months, 3 weeks ago (2017-04-21 14:36:51 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29419615/background.js File background.js (left): https://codereview.adblockplus.org/29410607/diff/29419615/background.js#oldcode48 background.js:48: return {selectors, trace}; How about moving this message handler ...
7 months, 2 weeks ago (2017-04-29 22:11:04 UTC) #20
Manish Jethani
Patch Set 6: Move get-selectors and hide-elements to elemHideHelper Sorry, you'll have to look at ...
7 months, 2 weeks ago (2017-05-01 23:18:06 UTC) #21
kzar
LGTM
7 months, 1 week ago (2017-05-10 08:43:32 UTC) #22
Wladimir Palant
https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js File lib/css.js (right): https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode30 lib/css.js:30: exports.userStylesheetsSupported = info.platform == "gecko" && On 2017/05/01 23:18:05, ...
7 months ago (2017-05-17 11:50:16 UTC) #23
kzar
On 2017/05/17 11:50:16, Wladimir Palant wrote: > Given that the values provided by the info ...
7 months ago (2017-05-17 11:55:02 UTC) #24
Sebastian Noack
On 2017/05/17 11:55:02, kzar wrote: > On 2017/05/17 11:50:16, Wladimir Palant wrote: > > Given ...
7 months ago (2017-05-17 12:07:44 UTC) #25
Manish Jethani
On 2017/05/17 11:50:16, Wladimir Palant wrote: > Given that the values provided by the info ...
7 months ago (2017-05-18 01:55:54 UTC) #26
Manish Jethani
Patch Set 7: Use feature detection https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js File lib/elemHideHelper.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js#newcode55 lib/elemHideHelper.js:55: callback(error); Doing this ...
7 months ago (2017-05-18 02:25:23 UTC) #27
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode600 include.preload.js:600: + addSelectors(selectors, filters, {inject, traceOnly} = {}) Any reason ...
6 months, 3 weeks ago (2017-05-24 08:31:42 UTC) #28
Manish Jethani
Patch Set 8: Make hideElement return a promise Note: I've used hg review now, but ...
6 months, 3 weeks ago (2017-05-24 17:13:35 UTC) #29
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode600 include.preload.js:600: + addSelectors(selectors, filters, {inject, traceOnly} = {}) On 2017/05/24 ...
6 months, 3 weeks ago (2017-05-24 18:14:06 UTC) #30
Manish Jethani
Patch Set 9: Change inject to insertMode Comments inline. https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode600 include.preload.js:600: ...
6 months, 3 weeks ago (2017-05-25 02:52:27 UTC) #31
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode607 include.preload.js:607: + if (this.tracer) On 2017/05/25 02:52:25, Manish Jethani wrote: ...
6 months, 2 weeks ago (2017-05-30 15:50:33 UTC) #32
Manish Jethani
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode607 include.preload.js:607: + if (this.tracer) On 2017/05/30 15:50:30, Sebastian Noack wrote: ...
6 months, 2 weeks ago (2017-05-31 06:21:39 UTC) #33
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode607 include.preload.js:607: + if (this.tracer) On 2017/05/31 06:21:38, Manish Jethani wrote: ...
6 months, 2 weeks ago (2017-05-31 11:13:30 UTC) #34
Manish Jethani
https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#newcode607 include.preload.js:607: + if (this.tracer) On 2017/05/31 11:13:29, Sebastian Noack wrote: ...
6 months, 2 weeks ago (2017-05-31 14:41:15 UTC) #35
Manish Jethani
Patch Set 10: Make hideElements synchronous
6 months, 2 weeks ago (2017-06-01 21:52:48 UTC) #36
Manish Jethani
Patch Set 11: Simplify addSelectors I got rid of the extra option to addSelectors here. ...
6 months, 2 weeks ago (2017-06-02 03:42:19 UTC) #37
Manish Jethani
On 2017/06/02 03:42:19, Manish Jethani wrote: > Patch Set 11: Simplify addSelectors > > I ...
6 months, 2 weeks ago (2017-06-02 03:44:38 UTC) #38
kzar
LGTM, do you want to take another look at this Sebastian? https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js File include.preload.js (right): ...
5 months, 1 week ago (2017-07-07 14:07:45 UTC) #39
Sebastian Noack
https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper.js File lib/elemHideHelper.js (right): https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper.js#newcode57 lib/elemHideHelper.js:57: port.on("get-selectors", (msg, sender) => In new code we use ...
5 months, 1 week ago (2017-07-07 16:16:11 UTC) #40
Manish Jethani
Patch Set 13: Update message and file names https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js#newcode666 include.preload.js:666: + ...
5 months, 1 week ago (2017-07-09 12:09:39 UTC) #41
kzar
LGTM, but please wait for Sebastian to approve this one.
5 months, 1 week ago (2017-07-10 12:36:23 UTC) #42
Sebastian Noack
5 months ago (2017-07-17 09:03:06 UTC) #43
LGTM
Sign in to reply to this message.

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