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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 1 day ago by Manish Jethani
Modified:
6 days, 11 hours 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -7 lines) Patch
M background.js View 1 2 3 2 chunks +27 lines, -1 line 0 comments Download
M ext/background.js View 1 chunk +1 line, -0 lines 0 comments Download
M include.preload.js View 1 2 6 chunks +45 lines, -6 lines 0 comments Download
A lib/css.js View 1 2 3 4 1 chunk +60 lines, -0 lines 0 comments Download
M metadata.chrome View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19
Manish Jethani
Patch Set 1 If the browser.tabs.insertCSS API is available and supports "user" stylesheets (Firefox 53), ...
2 weeks, 1 day 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 ...
2 weeks, 1 day 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 ...
2 weeks, 1 day 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: > ...
2 weeks, 1 day 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: > ...
2 weeks, 1 day 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: > ...
2 weeks, 1 day 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: > ...
2 weeks, 1 day ago (2017-04-12 14:27:01 UTC) #7
Manish Jethani
Patch Set 2: Use chrome.tabs.insertCSS instead
2 weeks, 1 day 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 ...
2 weeks ago (2017-04-13 07:56:25 UTC) #9
Manish Jethani
Patch Set 3: Add comment explaining when we inject styles > - I guess the ...
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week 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 ...
1 week 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 ...
1 week 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 ...
6 days, 15 hours 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 ...
6 days, 15 hours 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 ...
6 days, 12 hours 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, ...
6 days, 12 hours ago (2017-04-21 13:39:33 UTC) #18
kzar
6 days, 11 hours ago (2017-04-21 14:36:51 UTC) #19
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:
> Let me know if I should change it.

I'll defer to Sebastian there.
Sign in to reply to this message.

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