Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

bug 634764 – Added `contentStyle` and `contentStyleFile` to PageMod's options #366

Merged
merged 5 commits into from Mar 22, 2012

Conversation

Projects
None yet
4 participants
Contributor

ZER0 commented Mar 8, 2012

No description provided.

@Gozala Gozala and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/docs/page-mod.md
+`contentStyle` and `contentStyleFile`:
+
+ var data = require("self").data;
+ var pageMod = require("page-mod");
+
+ pageMod.PageMod({
+ include: "*.org",
+
+ contentStyleFile: data.url("my-page-mod.css"),
+ contentStyle: [
+ "div { padding: 10px; border: 1px solid silver}",
+ "img { display: none}"
+ ]
+ })
+
+It's important to note that `PageMod` will add these styles as [user style sheet](https://developer.mozilla.org/en/CSS/Getting_Started/Cascading_and_inheritance).
@Gozala

Gozala Mar 8, 2012

Member

could you please linebreak after as so it's easier to read in editor.

@ZER0

ZER0 Mar 8, 2012

Contributor

I thought I did. Probably TextMate on plain text just use softwrap instead my usual settings. I'll check.

@Gozala Gozala commented on an outdated diff Mar 8, 2012

packages/addon-kit/docs/page-mod.md
@@ -353,7 +383,7 @@ description of match patterns. Rules can be added to the list by calling its
@method
Stops the page mod from making any more modifications. Once destroyed the page
mod can no longer be used. Note that modifications already made to open pages
-will not be undone.
+will not be undone, except for any stylesheet added by `contentStyle` or `contentStyleFile`, that are unregistered immediately.
@Gozala

Gozala Mar 8, 2012

Member

could you please line break after or ? We're stick to 80 char limit per line (unless it's url).

@Gozala Gozala commented on the diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
+ * Returns the content of the uri given
+ */
+function read(uri) {
+ let channel = io.newChannel(uri, null, null);
+
+ let stream = Cc["@mozilla.org/scriptableinputstream;1"].
+ createInstance(Ci.nsIScriptableInputStream);
+
+ stream.init(channel.open());
+
+ let data = stream.read(stream.available());
+
+ stream.close();
+
+ return data;
+}
@Gozala

Gozala Mar 8, 2012

Member

I think I've seen this function in multiple places already. In any case it would be nicer if you could move it to the separate helper module,
unless it already exists.

@ZER0

ZER0 Mar 8, 2012

Contributor

We have similar functions, but not exactly the same. For instance: https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/utils/data.js#L48

@Gozala

Gozala Mar 8, 2012

Member

No I think we do / were doing something similar either in loader or bootstrap. Also I remember there were some edge cases fixed later on.

@ochameau

ochameau Mar 8, 2012

Member

bootstrap [gonna be hard to factorize] and l10n ;)

@Gozala

Gozala Mar 8, 2012

Member

bootstrap [gonna be hard to factorize] and l10n ;)

Maybe, I don't necessary want to refactor bootstrap.js but my point was that this function is generic enough that it is already used in multiple places, so we'd be better of having it somewhere around other utility functions. Also, because while it feels primitive to implement there were some issues we had to fix later.

@ZER0

ZER0 Mar 8, 2012

Contributor

l10n.js uses XMLHttpRequest, like self module for data.load; bootstrap uses similar approach I did. Shall we tried to unified the methods?

@Gozala

Gozala Mar 8, 2012

Member

BTW: bootstrap used XMLHttpRequest initially but there was some issue either with encodings or maybe it was something else, but we had to move away from XHR (Don't exactly know if that was justified I just saw it post factum after change was landed). Yeah unification is a good idea, but maybe not in bootstrap as it's not as easy or maybe bootstrap could pass it's read to a loader somehow which than would expose it in some way, but again not sure how easy it would be and if it would justify the complexity. l10n, self, and here seems like a good places to do this unification.

Also @ochameau mentioned it's not easy for l10n not sure why.

@ZER0

ZER0 Mar 8, 2012

Contributor

I remember, it was a patch from @Mossop because a problem with OS X, but it was only during the start up or something like that.
Yes, I didn't think about unified in bootstrap but just in the other modules.
What about add this to function to url module?

Wonder if it's better use XMLHttpRequest approach, or the "channel" one – with the proper encoding and everything. Due the past experience with OSX, could be worthy using the channel, maybe. @Gozala, what do you think?

@Mossop

Mossop Mar 8, 2012

Member

The problem with sync XHR was that it spins an event loop and loads the data asynchronously under the hood. This allows other events to fire and JS to run before the call to xhr.send() returns. Sometimes that is ok but it wasn't in early startup. It's also something that is probably pretty unexpected so I'd lean towards avoiding using sync XHRs in general. It's also possible that sync-XHR is going away for us in the future anyway: https://bugzilla.mozilla.org/show_bug.cgi?id=721336

@Gozala

Gozala Mar 8, 2012

Member

In that case channel based approach is probably better.

I would not put read in url module though, ideally our url module would match the one from node so I would prefer not to increase difference even further. Also I think function / module name should be explicit about the fact that it reads only content from the local fs and not just an arbitrary url's. Also I think function should verify that given url is local one regardless of the fact that we do validate pageMode options since it may be misused in other contexts.

@ZER0

ZER0 Mar 8, 2012

Contributor

Sure, if it's something to export the name should be different, maybe something like readLocalURI? But I didn't understand in which module we should add this function.
Also, our url module seems already totally different from the node one. In addition I also put there the DataURL object that I created for the clipboard API images implementation: if you think we shouldn't modify that module, please let me know if and where I have to move this object.

@ochameau

ochameau Mar 8, 2012

Member

I didn't said it is going to be hard to factorize l10n usage, but bootstrap one ;)
So if we have a chance to factorize it and avoid using sync-XHR, please go-ahead!
(it might be relevant to decouple this from this pull request?)

@ZER0

ZER0 Mar 8, 2012

Contributor

Firstly, we have to understand where put this function: I proposed url module because seems suitable, but if there are better place where put it, I'm open to suggestion! Also, I need to save somewhere the DataURL as well for clipboard APIs. I could simply store in the clipboard module and don't expose it, if there is no better options.

@Gozala

Gozala Mar 9, 2012

Member

What about require('url/resource').read ? require('url/bundled').read or maybe require('url/io').read ?
Data uri stuff could go to url/data or maybe we could have just uri for that.

P.S.: Also I'd say url.toFileName belongs wherever read will be.

@Gozala

Gozala Mar 9, 2012

Member

It might be relevant to decouple this from this pull request?

Also, I'm totally fine to have this unification as a followup.

@Gozala

Gozala Mar 9, 2012

Member

I could simply store in the clipboard module and don't expose it, if there is no better options.

If we don't have any other use case and can't fine a good place for it it's absolutely fine to keep it in clipboard module until we discover another use case or a good place to put it to.

@ZER0

ZER0 Mar 9, 2012

Contributor

I'm in favor of require('url/io'). It sounds to me that we should move at least also url.fromFilename and not only url.toFilename. Moving these functions could clean a bit url module itself. I think we should add a "deprecated" warning in url module for a while, but then we could easily remove them.

Is require('url/io').read good enough to understand that is a local URI? I named the function as I proposed before for the moment (readLocalURI), copied the approach used by @Mossop in bootstrap to have mostly the some code even if duplicated.

@Gozala

Gozala Mar 9, 2012

Member

I'm ok with just read as long as it throws on non local url. I'm also fine with readLocal to signify that. I don't think adding URI at the end necessary as function comes from url/io itself it should be pretty clear.

@ZER0

ZER0 Mar 9, 2012

Contributor

Even if I already did the changes in this request I'm thinking I will split the code to have a separate pull request for that, also to makes this one more clear.
What should be consider local? For my purpose, I just need resource and data scheme, but it's not enough if we want reuse this function across modules. For instance, l10n unit test uses jar. But jar could be used also for remote uri as far as I know, so probably we should have: resource, data, file and if it's jar checks if the path starts with one of the local scheme.
How it sounds to you? Should we add chrome as well?

@Gozala

Gozala Mar 9, 2012

Member

I don't have preference on chrome we can add things as necessary. What's use case for data ? It feels bit strange to readURI that is data uri.

@ZER0

ZER0 Mar 9, 2012

Contributor

I honestly consider data as "local" –it's definitely not remote. It's useful especially in unit test (see all our tests where we pass data: instead of real files). If we don't include that scheme, our unit test will be broken.

@Gozala Gozala and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
if ('contentScript' in options)
this.contentScript = options.contentScript;
if ('contentScriptFile' in options)
this.contentScriptFile = options.contentScriptFile;
if ('contentScriptWhen' in options)
this.contentScriptWhen = options.contentScriptWhen;
+ if ('contentStyle' in options)
+ contentStyle = [].concat(options.contentStyle);
+ if ('contentStyleFile' in options)
+ contentStyleFile = [].concat(options.contentStyleFile);
@Gozala

Gozala Mar 8, 2012

Member

We do validate input option types and we should do it here as well.

@ZER0

ZER0 Mar 8, 2012

Contributor

We do not for contentScript and contentScriptFile, I just did the same.

@Gozala

Gozala Mar 8, 2012

Member

Nope it's not true, take a look at content/loader.Loader it defines setters for this properties:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/loader.js#L31-44

@ZER0

ZER0 Mar 8, 2012

Contributor

My bad, I thought it was inherit from Worker – that do not have this check. I will add similar validation then!

@Gozala Gozala and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
@@ -86,6 +117,26 @@ const PageMod = Loader.compose(EventEmitter, {
else
rules.add(include);
+ let styleRules = "";
+
+ if (contentStyleFile)
+ for (let i = 0, l = contentStyleFile.length; i < l; i++)
+ styleRules += read(contentStyleFile[i])
@Gozala

Gozala Mar 8, 2012

Member

What if url points to the remote file 'http://...' blocking reads on such URL's is unreasonable. We should limit to local urls.

@Gozala

Gozala Mar 8, 2012

Member

NIT: Missing semicolon

@Gozala

Gozala Mar 8, 2012

Member

Also encouraged style is using .forEach / .map and friends:

styleRules = contentStyleFile.map(read).join('');

In this case it also reads easier.

@ZER0

ZER0 Mar 8, 2012

Contributor

Agreed!

@Gozala

Gozala Mar 8, 2012

Member

Also if you did validation as with contentScriptFile https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/loader.js#L31-44
that would have ensured that read takes only local uris.

@ZER0

ZER0 Mar 8, 2012

Contributor

I did it now. However, it seems to me that the validation in https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/loader.js#L31-44 is bit broken. It check for local URLs only if we pass an array, and in that case doesn't allow data: URL.
It's not a big deal for the first scenario – a different error is just raised later – but I think should be fixed.

@Gozala

Gozala Mar 8, 2012

Member

I think you can most likely reuse same validator so maybe you could fix that while you're there.

@Gozala Gozala and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
@@ -86,6 +117,26 @@ const PageMod = Loader.compose(EventEmitter, {
else
rules.add(include);
+ let styleRules = "";
+
+ if (contentStyleFile)
+ for (let i = 0, l = contentStyleFile.length; i < l; i++)
+ styleRules += read(contentStyleFile[i])
+
+ if (contentStyle)
+ for (let i = 0, l = contentStyle.length; i < l; i++)
+ styleRules += contentStyle[i]
@Gozala

Gozala Mar 8, 2012

Member

why not styleRules += contentStyle.join('')

@ZER0

ZER0 Mar 8, 2012

Contributor

copy & paste laziness from previous code. :)

@Gozala Gozala commented on the diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
@@ -155,6 +213,64 @@ const PageMod = Loader.compose(EventEmitter, {
_onUncaughtError: function _onUncaughtError(e) {
if (this._listeners('error').length == 1)
console.exception(e);
+ },
+ _onRuleUpdate: function _onRuleUpdate(){
+ this._registerStyleSheet();
+ },
+
+ _registerStyleSheet : function _registerStyleSheet() {
+ let rules = this.include;
+ let styleRules = this._styleRules;
+
+ let documentRules = [];
+
+ this._unregisterStyleSheet();
+
+ for each (let rule in rules) {
@Gozala

Gozala Mar 8, 2012

Member

I think we decided to use following forms instead:

rules.forEach(function(rule) { /*...*/ });
Object.keys(rules).forEach(function(name) { let rule = rules[name]; /*...*/ }) // objects
@ZER0

ZER0 Mar 8, 2012

Contributor

I thought there were still open discussion about it.
For instance, do you really find more readable and less character-typed the two alternative you suggested?

@Gozala

Gozala Mar 8, 2012

Member

I don't think it's more readable but it does not relies on non-standard features that will at some point even be deprecated in favor of different standard coming in ES.next.

@Gozala Gozala commented on the diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
+ let pattern = RULES[rule];
+
+ if (!pattern)
+ continue;
+
+ if (pattern.regexp)
+ documentRules.push("regexp(\"" + pattern.regexp.source + "\")")
+ else if (pattern.exactURL)
+ documentRules.push("url(" + pattern.exactURL + ")")
+ else if (pattern.domain)
+ documentRules.push("domain(" + pattern.domain + ")")
+ else if (pattern.urlPrefix)
+ documentRules.push("url-prefix(" + pattern.urlPrefix + ")")
+ else if (pattern.anyWebPage) {
+ documentRules.length = 0;
+ break;
@Gozala

Gozala Mar 8, 2012

Member

Ok you can discard my previous comment as I don't think there is a way to break in forms listed above.

@Gozala Gozala and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
+
+ styleSheetService.loadAndRegisterSheet(
+ this._registeredStyleURI,
+ styleSheetService.USER_SHEET
+ );
+ },
+
+ _unregisterStyleSheet : function () {
+ let uri = this._registeredStyleURI;
+
+ if (!uri ||
+ !styleSheetService.sheetRegistered(uri, styleSheetService.USER_SHEET))
+ return;
+
+ styleSheetService.unregisterSheet(uri, styleSheetService.USER_SHEET);
+
@Gozala

Gozala Mar 8, 2012

Member

this would be easier to read if condition was other way round:

if (uri && styleSheetService.sheetRegistered(uri, styleSheetService.USER_SHEET))
  styleSheetService.unregisterSheet(uri, styleSheetService.USER_SHEET);

Also you could null-ify this._registeredStyleURI regardless of the conditional, in fact it's more logical to have
it set to null after this function is executed.

@ZER0

ZER0 Mar 8, 2012

Contributor

I did with not because it was hard to read with the lines break, use the constant on top will make it more readable in the "positive" way, I agree.

@Gozala Gozala commented on an outdated diff Mar 8, 2012

packages/addon-kit/lib/page-mod.js
+ else
+ uri += encodeURIComponent(styleRules);
+
+ this._registeredStyleURI = io.newURI(uri, null, null);
+
+ styleSheetService.loadAndRegisterSheet(
+ this._registeredStyleURI,
+ styleSheetService.USER_SHEET
+ );
+ },
+
+ _unregisterStyleSheet : function () {
+ let uri = this._registeredStyleURI;
+
+ if (!uri ||
+ !styleSheetService.sheetRegistered(uri, styleSheetService.USER_SHEET))
@Gozala

Gozala Mar 8, 2012

Member

I'd suggest to define USER_SHEET constant at the top of the file, which will make it easier to read, also
will avoid line breaks.

@ZER0 ZER0 and 1 other commented on an outdated diff Mar 8, 2012

packages/addon-kit/tests/test-page-mod.js
+ "100px",
+ "PageMod contentStyle worked"
+ );
+
+ pageMod.destroy();
+ test.assertEqual(
+ style.width,
+ "200px",
+ "PageMod contentStyle is removed after destroy"
+ );
+
+ done();
+
+ }
+ );
+};
@ZER0

ZER0 Mar 8, 2012

Contributor

I already added a newline here.

@Gozala

Gozala Mar 14, 2012

Member

Are you sure ? It does not seems to be here.

@ZER0

ZER0 Mar 22, 2012

Contributor

Probably my editor removed it, I double checked now and it should be there. :)

Member

ochameau commented Mar 8, 2012

I need to tell you why I didn't start working on this patch. We had a discussion with myk on bugzilla about this.
The platform improvement implemented by David isn't ideal regarding how works page-mod.
Instead of using this regexp css feature, it would have been easier to have an API to apply/disable a userstylesheet on a given document.
So that we do not have to:

  • care about the compatiblity between CSS regexp and our match-pattern API,
  • stringify CSS document in order to inline them in a rule.

In parralel, it would allow to have a much simplier code and way less code specific to CSS feature.
We would only have to listen to start event in any case and apply the CSS immediatly.

Having said that I'm really happy to see this feature implemented, so that we can see this comment as a possible improvement for future!

Member

Gozala commented Mar 8, 2012

Having said that I'm really happy to see this feature implemented, so that we can see this comment as a possible
improvement for future!

I'm +1 on doing that in the next iteration.

Contributor

ZER0 commented Mar 8, 2012

@ochameau the discussion with myk on bugzilla, is the same on the bug 634764? Because from that it seems that everything was necessary is already implemented (see https://bugzilla.mozilla.org/show_bug.cgi?id=634764).

So, reading the bug, this implementation is doing exactly what is said, unless I missed some other discussion. I agreed that stringify CSS document is not exactly optimal, I'd like to avoid it too, but it's something we need to do in any case if we want to implement also contentStyle and not only contentStyleFile.

Unless some big issue, I'd really like provides this feature to the developers 'cause they requested it a lot, and we can always improve it later when the platform will give to us better tools to obtain the same result.
The high level APIs won't change, and we're using user style sheet (that was one of the key point of the bug).

Member

ochameau commented Mar 9, 2012

@ZER0 I wasn't able to find the bug where we discussed this. Me, myk and David had an exchance on another bug that is not linked to this one :( So yes, you implement exactly what is suggested in this bug.
Nevermind this comment, it was just there for context and history background.

@Gozala Gozala commented on an outdated diff Mar 14, 2012

packages/addon-kit/docs/page-mod.md
@@ -98,6 +98,28 @@ have problems getting your add-on approved on AMO.</p>
secure, debug and review.</p>
</div>
+### Styling web pages ###
+
+Sometimes adding a script to web pages is not enough, you also want to styling
+them. `PageMod` provides an easy way to do that through options' properties
+`contentStyle` and `contentStyleFile`:
@Gozala

Gozala Mar 14, 2012

Member

I think it reads better if you move "properties" to the end of the sentence.

@Gozala Gozala commented on the diff Mar 14, 2012

packages/addon-kit/docs/page-mod.md
@@ -98,6 +98,28 @@ have problems getting your add-on approved on AMO.</p>
secure, debug and review.</p>
</div>
+### Styling web pages ###
+
+Sometimes adding a script to web pages is not enough, you also want to styling
+them. `PageMod` provides an easy way to do that through options' properties
+`contentStyle` and `contentStyleFile`:
+
+ var data = require("self").data;
+ var pageMod = require("page-mod");
+
+ pageMod.PageMod({
+ include: "*.org",
@Gozala

Gozala Mar 14, 2012

Member

Nit: I think we'd be better off showing more restricted pattern in the example as many people just copy & paste. Maybe "*.mozilla.org"

@ZER0

ZER0 Mar 22, 2012

Contributor

I just kept the same include rule of the examples above, to be consistent.
Take a look to the whole page-mod docs, if you still thinking that this example should be a special case compare to the others I will modify it.

ZER0 added a commit that referenced this pull request Mar 22, 2012

Merge pull request #366 from ZER0/bug634764
bug 634764 – Added `contentStyle` and `contentStyleFile` to PageMod's options

@ZER0 ZER0 merged commit cb55370 into mozilla:master Mar 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment