788224 - Add "type" option to PageMod constructor

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

there can be at least two types, "images" and non-"images".  Default is all.  

https://addons.mozilla.org/en-US/firefox/addon/transparent-standalone-imag
http://pastebin.mozilla.org/1803341

The option would reduce the number of sandboxes opened, and the amount of generic user code that is required.

Comment 1

[Anthony Ricaud (:rik)]

This would also be useful for videos, see bug 844084.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Maybe `image` and `dom` would be more descriptive (as there are audio files, videos, other things that are 'non-images' that are more media than they are documents). Also, for more control, maybe can take mime types or a 'group type' (like `image` maps to `image/jpeg`|`image|png`, etc..)

PageMod({
  includes: '*.mozilla.org',
  type: ['image', 'audio/wav']
})
// only image-ish types and audio that is specifically WAV

Irakli, thoughts on this API change?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik jsbin has expired so I can't really see example there.

I way to specify type of content to apply pageMod to makes sense. Although to be honest something about applying content scripts to non DOM documents does not quite feels right.

Since `tab.contentType` property exists I'd go with the same name for page-mod too. I wonder if 'image/*', 'audio/*' or regexp would be more natural, than just `image` and `audio`. Although either of this option would work fine with me.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

It seems to me the same bug as bug 842141. If it's that so, we can mark bug 842141 as duplicate.

I agreed on the fact that it seems odd applying content scripts to non DOM documents. It should more more explicit that we're talking about the image preview and so on; also because some content type could not have a "preview" associated, or the preview could not returns the content type we expect, like PDF.js (in this case we need some manual dirty trick, if they want to keep as is, see bug 775683).

Comment 5

[Sunny [:darkowlzz]]

This bug had me confused a bit. I felt that it's about `contentType` property which `tab` has and we need the same for page-mod. But I think it's more about simplifying `include` of page-mod.

This came out of irc discussion with ZER0 and zombie_. We could have a new way to specify page-mod `include` with 2 properties like,
PageMod({
  include:{
    url: "",
    contentType: ""
  }
});

This could make defining pages to which we apply page-mod a bit easy.
Just specifying `contentType` as "image" and providing no `url` would mean that the page-mod is applicable only to image pages.
Defining both `url` and `contentType` could mean that the page-mod should be applied only to image pages of a particular url.
And defining just the url should behave the way it behaves now.

We could have `contentType` for other media content as well, like video, audio, pdf, shumway, etc.

Sounds good?

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli what do you think of comment 5 ?

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> Irakli what do you think of comment 5 ?

I think that makes sense, although I'd still think we should stick to a pattern
syntax we have already for URLs. `url: ""` does not match anything `url: "*"`
does match everything. I think that should stay that way & `contentType` should
have a same syntax too. We also do support regexps for `url`s, it's reasonable
to expect that `contentType`'s will support regexps as well.

Overal +1 on `include` destructuring  but we should keep it's values use
our MatchPattern as we do now.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

darkowizz, are you still willing to fix this bug?
I'm working on some component that likely will need this functionality, so let me know! We'll probably needs to make it reusable.

Comment 10

[Sunny [:darkowlzz]]

Sure, let's get started.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Here examples about I think the API should looks like:

https://gist.github.com/ZER0/10018525

Let me know if it seems reasonable to you, or if you have something else in mind!

Comment 12

[Sunny [:darkowlzz]]

Attachment: Link to pull request 1486

Comment 13

[Sunny [:darkowlzz]]

Attachment: Link to pull request 1487

Another patch with implementation of `includeType` for contentType of pages.

Comment 14

[Sunny [:darkowlzz]]

Proposed 2 different way to implement contentType for page-mod as discussed over IRC:
1. By extending the existing `include` to have `url` & `contentType` properties in it. attachment #8420637 [details] .
2. By creating another page-mod attribute `includeType` for page contentType. attachment #8420766 [details] .

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Sunny [:darkowlzz] from comment #14)
> Proposed 2 different way to implement contentType for page-mod as discussed
> over IRC:
> 1. By extending the existing `include` to have `url` & `contentType`
> properties in it. attachment #8420637 [details] .
> 2. By creating another page-mod attribute `includeType` for page
> contentType. attachment #8420766 [details] .

I think there is a misunderstood about what I tried to explain in IRC, unfortunately it was sunday and I was in rush; let me know when you will available and I'll try to explain it better.

In short, I wasn't suggest to add a new attribute. We should have only `include` as attribute. I was talking about the type of values that `include` attribute can have, in order to have a better check.

Basically at this moment we have a sort of "include type value". This type accept String and RegExp, currently. We can also have an array of "include type value".

Of course, we could extend this "include type value" in order to accept Object as well, and check if those objects contains `url` and `contentType`, and perform a check on that. However, that it means that we could also pass an array of this object, in order to set multiple values, where I think it's probably less verbose and less misleading having something like that:

  PageMod({
    include: {
      url: ['*', /foo/i],
      contentType: ['image/gif', 'image/png']
    },
  contentScript: 'console.log("hello")'
  });
 
So, in short, the rules should be, in my opinion, as follow:
1. `include` can accept a String, a RegExp, an Array or a Object as value
2. If the value is a String, RegExp or an Array, just keep the current behavior – in case of Array must be an array of String / RegExp.
3. If the value is an Object, it should have a `url` property, a `contentType` property, or both. Otherwise raise an exception
4. If it has an `url` property, performs on that property the same check for the `include` property when is not an Object (check if the value of `url` is a String, a RegExp, or an Array of them).
5. If it has a `contentType` property, we accept only String or an Array of String. Then we check if the string specified are in the proper format (see http://www.ietf.org/rfc/rfc2045.txt); and allow wildcards after the type (e.g. `image/*` will accept any subtype).

I think that is the general idea. Let me know what do you think!

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8420766 [details]
Link to pull request 1487

f- because the misunderstanding I mentioned above.

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8420637 [details]
Link to pull request 1486

I did the review and I tried to point out what I think could be improved!
f- mostly because the code is outdated now, due the bug 810654.

Comment 18

[Akhil]

Hi, I want to work on this bug.

Comment 19

[Matteo Ferretti [:zer0] [:matteo]]

Hi Akhil, thanks for your interest! I'm not involved in SDK anymore; I'll forward your request to Luca Greco.

Comment 20

[Andy McKay]

Thanks for wanting to take this on Akhil, but by Firefox 57, SDK add-ons will no longer work in Firefox. Given the expected end of life of SDK and difficulty in finding mentors and people to work on this, I wouldn't recommend this as a good first bug. If you'd like to work on it, please feel free.

If you'd like to work on a more active project, could we recommend the new add-on API, WebExtensions? https://www.joshmatthews.net/bugsahoy/?webextensions=1

Comment 21

[Akhil]

Thanks @Andy Mckay  I will go for another bug.

Comment 22

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562