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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Manish Jethani
Modified:
3 weeks 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -35 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 4 chunks +34 lines, -5 lines 0 comments Download
A lib/elemHideHelper.js View 1 2 3 4 5 6 7 8 9 1 chunk +93 lines, -0 lines 0 comments Download
M metadata.chrome View 1 2 3 4 5 6 7 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 38
Manish Jethani
Patch Set 1 If the browser.tabs.insertCSS API is available and supports "user" stylesheets (Firefox 53), ...
2 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week ago (2017-04-12 14:27:01 UTC) #7
Manish Jethani
Patch Set 2: Use chrome.tabs.insertCSS instead
2 months, 1 week 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 months, 1 week ago (2017-04-13 07:56:25 UTC) #9
Manish Jethani
Patch Set 3: Add comment explaining when we inject styles > - I guess the ...
2 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 ...
2 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 ...
2 months 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 ...
2 months 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 ...
2 months 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 ...
2 months 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 ...
2 months 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 ...
2 months 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, ...
2 months 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: ...
2 months 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 ...
1 month, 3 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 ...
1 month, 3 weeks ago (2017-05-01 23:18:06 UTC) #21
kzar
LGTM
1 month, 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, ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 ...
4 weeks, 1 day 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 ...
4 weeks, 1 day 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 ...
4 weeks, 1 day 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: ...
4 weeks, 1 day 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: ...
3 weeks, 2 days 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: ...
3 weeks, 1 day 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: ...
3 weeks, 1 day 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: ...
3 weeks, 1 day ago (2017-05-31 14:41:15 UTC) #35
Manish Jethani
Patch Set 10: Make hideElements synchronous
3 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. ...
3 weeks ago (2017-06-02 03:42:19 UTC) #37
Manish Jethani
3 weeks ago (2017-06-02 03:44:38 UTC) #38
On 2017/06/02 03:42:19, Manish Jethani wrote:
> Patch Set 11: Simplify addSelectors
> 
> I got rid of the extra option to addSelectors here.
> 
> Patch Set 12: Further simplify addSelectors
> 
> Since we no longer care about asynchronous errors from chrome.tabs.insertCSS,
we
> can simply ignore whether "hide-elements" succeeded the second time when
called
> via ElementHideEmulation. This greatly simplifies the logic.
      ^^^^

Correction: ElemHideEmulation

Also you can view the unified diff for include.preload.js, it's very
straightforward now.
Sign in to reply to this message.

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