Issue 29333265: Issue 3478 - Hide genericblock and generichide from the composer (Closed)
|
Created:
Jan. 6, 2016, 3:32 p.m. by Sebastian Noack Modified:
Jan. 7, 2016, 1:41 p.m. CC:
kzar Visibility:
Public. |
DescriptionIssue 3478 - Hide genericblock and generichide from the composer
Patch Set 1 #
Total comments: 4
Patch Set 2 : Hide (instead disable) generichide and genericblock #MessagesTotal messages: 8
LGTM
https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... File chrome/content/ui/composer.js (right): https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... chrome/content/ui/composer.js:227: if (value == "document" || value == "genericblock" || value == "generichide") What about "elemhide"? Similar to these options "elemhide" also only makes sense for exception rules.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... File chrome/content/ui/composer.js (right): https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... chrome/content/ui/composer.js:227: if (value == "document" || value == "genericblock" || value == "generichide") On 2016/01/06 15:44:51, Thomas Greiner wrote: > What about "elemhide"? Similar to these options "elemhide" also only makes sense > for exception rules. Interestingly, "ELEMHIDE" is skipped above. So no checkbox is added. I wonder why we treat elemhide differently from document here?
Message was sent while issue was closed.
https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... File chrome/content/ui/composer.js (right): https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... chrome/content/ui/composer.js:227: if (value == "document" || value == "genericblock" || value == "generichide") On 2016/01/06 15:49:33, Sebastian Noack wrote: > On 2016/01/06 15:44:51, Thomas Greiner wrote: > > What about "elemhide"? Similar to these options "elemhide" also only makes > sense > > for exception rules. > > Interestingly, "ELEMHIDE" is skipped above. So no checkbox is added. I wonder > why we treat elemhide differently from document here? I assume that's because the composer only creates rules for blocking filters. While ELEMHIDE is used in the context of a blocking filter it only applies to element hiding filters (which you'd need Element Hiding Helper to create). So it seems that if we want to be consistent with the existing logic, we'd have to treat GENERICHIDE the same way as ELEMHIDE and only consider GENERICBLOCK at this part of the code.
https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... File chrome/content/ui/composer.js (right): https://codereview.adblockplus.org/29333265/diff/29333266/chrome/content/ui/c... chrome/content/ui/composer.js:227: if (value == "document" || value == "genericblock" || value == "generichide") On 2016/01/06 16:07:02, Thomas Greiner wrote: > On 2016/01/06 15:49:33, Sebastian Noack wrote: > > On 2016/01/06 15:44:51, Thomas Greiner wrote: > > > What about "elemhide"? Similar to these options "elemhide" also only makes > > sense > > > for exception rules. > > > > Interestingly, "ELEMHIDE" is skipped above. So no checkbox is added. I wonder > > why we treat elemhide differently from document here? > > I assume that's because the composer only creates rules for blocking filters. > While ELEMHIDE is used in the context of a blocking filter it only applies to > element hiding filters (which you'd need Element Hiding Helper to create). > > So it seems that if we want to be consistent with the existing logic, we'd have > to treat GENERICHIDE the same way as ELEMHIDE and only consider GENERICBLOCK at > this part of the code. I wonder whether we should expose generichide and genericblock here at all. I agree that it makes no sense to have generichide exposed, but not elemhide which is a superset of it. But having only genericblock exposed seems inconsistent, besides the genricblock/generichide options being experimental anyway. Here is a new patch set.
LGTM
Message was sent while issue was closed.
LGTM 
|
