Bug 837494 - re-implement `contentStyle*` using the new nsIDOMWindowUtils methods #839

Merged
merged 1 commit into from Apr 10, 2013

Projects

None yet

2 participants

@ZER0
ZER0 commented Mar 6, 2013
  • added mod module, with attach, detach and getWindowTarget
  • added loadSheet and removeSheet to window/utils
  • added values method to util/object
  • replace usage of stylesheet services with attach/detach a Style object
@Gozala Gozala was assigned Mar 6, 2013
@ZER0
ZER0 commented Mar 6, 2013

@gozala, can you take a look of this? As I mention to you there a couple of things missing, like how to proper documents the attach, detach and getWindowTarget methods.

@Gozala Gozala commented on an outdated diff Mar 19, 2013
lib/sdk/content/mod.js
+ "stability": "experimental"
+};
+
+const { Ci } = require("chrome");
+const method = require("method/core");
+
+exports.attach = method("attach");
+exports.detach = method("detach");
+
+let getTargetWindow = method("getTargetWindow");
+
+getTargetWindow.define(function (target) {
+ if (target instanceof Ci.nsIDOMWindow)
+ return target;
+
+ throw TypeError("Type does not implements method: getTargetWindow");
@Gozala
Gozala Mar 19, 2013 Mozilla member

I think we decided to return null in this case instead.

@Gozala Gozala commented on an outdated diff Mar 19, 2013
lib/sdk/content/style.js
+const LOCAL_URI_SCHEMES = ['resource', 'data'];
+const SHEET_TYPE = ["agent", "user", "author"];
+
+const styles = ns();
+
+function isLocalURL(item) {
+ try {
+ if (LOCAL_URI_SCHEMES.indexOf(URL(item).scheme) > -1)
+ return true;
+ }
+ catch(e) {}
+
+ return false;
+}
+
+function isString(item) {
@Gozala Gozala and 1 other commented on an outdated diff Mar 19, 2013
lib/sdk/content/style.js
+function isLocalURL(item) {
+ try {
+ if (LOCAL_URI_SCHEMES.indexOf(URL(item).scheme) > -1)
+ return true;
+ }
+ catch(e) {}
+
+ return false;
+}
+
+function isString(item) {
+ return typeof(item) === "string";
+}
+
+function isValidType(type) {
+ return SHEET_TYPE.indexOf(type) > -1;
@Gozala
Gozala Mar 19, 2013 Mozilla member

I personally prefer using hashes and doing SHEET_TYPE[type] || false

@ZER0
ZER0 Mar 19, 2013

In this case we have just a list of values, an hashmap will looks like:

    const SHEET_TYPE = {"agent": true, "user": true, "author": true};

instead of just simply:

   const SHEET_TYPE = ["agent", "user", "author"];

I don't see a value here to have an hash map, when we have formally a list. I would prefer keep the array, also it's what we use in other places for the same purpose (see attachTo in PageMod)

@Gozala Gozala commented on an outdated diff Mar 19, 2013
lib/sdk/content/style.js
+ if (source && !source.every(isString))
+ throw new Error('Style.source must be a string or an array of strings.');
+
+ if (uri && !uri.every(isLocalURL))
+ throw new Error('Style.uri must be a local URL or an array of lcoal URLs');
+
+ if (type && !isValidType(type))
+ throw new Error('Style.type must be "agent", "user" or "author"');
+
+ Object.defineProperties(this, {
+ "source": { value: source },
+ "uri": { value: uri },
+ "type": { value: type }
+ });
+
+ let style = styles(this);
@Gozala
Gozala Mar 19, 2013 Mozilla member

It's unfortunate that we can't really get window from an inner Id but I think we should still define proper abstractions
and hopefully patch platform later to make it better.

What I mean we need:

  1. getWindowsFor(style)
  2. getStylesFor(window)

To implement these you can use combination of weak maps:

  • styles that maps window to array of applied styles
  • windows that maps style to a hash of windows

Hopefully in a future we could avoid storing window hashes in second weak map by
implementing getInnerWindowWithId and using array of id's.

I would also make destroyListener global that would cleanup windows map
for the associated selections.

@Gozala
Gozala Mar 19, 2013 Mozilla member
function getWindowsFor(style) {
   return styledWindows.get(style).map(getInnerWindowWithId).filter(isntNull)
}
@Gozala Gozala and 1 other commented on an outdated diff Mar 19, 2013
lib/sdk/content/style.js
+attach.define(Style, function (style, target) {
+ let window = getTargetWindow(target);
+
+ if (style.uri)
+ for (let uri of style.uri)
+ loadSheet(window, uri, style.type);
+
+ if (style.source) {
+ let uri = "data:text/css;charset=utf-8,";
+
+ uri += encodeURIComponent(style.source.join(""));
+
+ loadSheet(window, uri, style.type);
+ }
+
+ styles(style).windows[getInnerId(window)] = window;
@Gozala
Gozala Mar 19, 2013 Mozilla member

Let's submit a platform bug to get equivalent of getOuterWindowWithId but for innerWindows. In order to avoid nasty references like this one.

@Gozala Gozala commented on an outdated diff Mar 19, 2013
lib/sdk/page-mod.js
- if (contentStyle)
- styleRules += [].concat(contentStyle).join("");
-
- if (styleRules) {
- this._onRuleUpdate = this._onRuleUpdate.bind(this);
-
- this._styleRules = styleRules;
-
- this._registerStyleSheet();
- rules.on('add', this._onRuleUpdate);
- rules.on('remove', this._onRuleUpdate);
- }
+ this._style = Style({
+ uri: contentStyleFile,
+ source: contentStyle
+ });
@Gozala
Gozala Mar 19, 2013 Mozilla member

I would rather use weak maps for style association instead of embracing traits :) Would just make de-traitification easier. Feel free to leave as is if you think otherwise.

@Gozala Gozala and 1 other commented on an outdated diff Mar 19, 2013
lib/sdk/window/utils.js
+ if (!(type && type in SHEET_TYPE))
+ type = "author";
+
+ type = SHEET_TYPE[type];
+
+ if (!(url instanceof Ci.nsIURI))
+ url = io.newURI(url, null, null);
+
+ let winUtils = getDOMWindowUtils(window);
+
+ try {
+ winUtils.loadSheet(url, winUtils[type]);
+ }
+ catch (e) {};
+};
+exports.loadSheet = loadSheet;
@Gozala
Gozala Mar 19, 2013 Mozilla member

I would prefer to move stylesheet util functions into separate module as window/utils keeps growing and this does not really relates to window that much.

@Gozala
Gozala Mar 19, 2013 Mozilla member

Maybe stylesheet/utils or something ?

@ZER0
ZER0 Mar 20, 2013

I thought that everything is under DOMWindowUtils would basically be in our window/utils. But I'm fine with moving on another module.

@Gozala
Member
Gozala commented Mar 19, 2013

To summarize review notes and in person discussions:

  1. We should not maintain windows hash, array or any other data structure holding refs to a windows.
    Instead we should just have a weak map that maps Style instances to sets of window IDs. This way
    we avoid any serious memory leaks.
  2. Style should not observe windows to do a cleanups. Only time list of associated windows matters is
    on detach. There for we can just iterate over associated windows (one way would be mapping stored
    window id's back to windows if such still exist) and detach style from those.
@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
@@ -0,0 +1,76 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `mod` module provides functions to modified the content of a page.
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would phrase it as:

"functions to modify a page content"
or
"functions for page content modifications"

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
@@ -0,0 +1,76 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `mod` module provides functions to modified the content of a page.
+
+<api name="attach">
+@function
+ This function applies content modifications to a target representing, or
+ containing, a content page.
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would phrase it more like this: Function applies given modification to a given target representing a content to be modified.

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
@@ -0,0 +1,76 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `mod` module provides functions to modified the content of a page.
+
+<api name="attach">
+@function
+ This function applies content modifications to a target representing, or
+ containing, a content page.
+
+ For example, the following code applies a style to a content window, adding
@Gozala
Gozala Mar 21, 2013 Mozilla member

please quote arguments with backticks so that generated html will wrap them in code elements -> style.

@ZER0
ZER0 Mar 21, 2013

I don't see any arguments here, what are you referring to?

@Gozala
Gozala Mar 21, 2013 Mozilla member

I mean modification and target

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+
+ For example, the following code applies a style to a content window, adding
+ a border to all divs in page:
+
+ var attach = require("sdk/content/mod").attach;
+ var Style = require("sdk/stylesheet/style").Style;
+
+ var style = Style({
+ source: "div { border: 4px solid gray }"
+ });
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+
+@param modification {object}
+ The modification we want to apply to the target
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would mention maybe in parentheses that value is considered modification if it implements attach.

@ZER0
ZER0 Mar 21, 2013

that's the tricky part about document functions that are actually "methods" created by the method library. How modification can "implements" a function like attach if we don't explain what the methods are? I mean, regular JS developers could be a bit puzzled here, and if we said something like that we should be more clear at least about what we meant with "implements attach".

@Gozala
Gozala Mar 21, 2013 Mozilla member

Yeah that's where we should point to method documentation which I'm going to write :D

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+
+ var attach = require("sdk/content/mod").attach;
+ var Style = require("sdk/stylesheet/style").Style;
+
+ var style = Style({
+ source: "div { border: 4px solid gray }"
+ });
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+
+@param modification {object}
+ The modification we want to apply to the target
+
+@param target
+ The object that representing, or containing, a content page. Depends by
@Gozala
Gozala Mar 21, 2013 Mozilla member

Target is a value that representing content to be modified. It is valid only when
getTargetWindow(target) returns nsIDOMWindow of content it represents.

@ZER0
ZER0 Mar 21, 2013

Not sure I'm following you here. What are you suggesting to write here?

@Gozala
Gozala Mar 21, 2013 Mozilla member

Not sure I'm following you here. What are you suggesting to write here?

Exactly what I wrote in comment

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+ var Style = require("sdk/stylesheet/style").Style;
+
+ var style = Style({
+ source: "div { border: 4px solid gray }"
+ });
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+
+@param modification {object}
+ The modification we want to apply to the target
+
+@param target
+ The object that representing, or containing, a content page. Depends by
+ the `modification` given, it could be optional. In that case, the modification
+ will be attached to any target that makes sense for this kind of modification.
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would rather not make modification optional for attach.

@ZER0
ZER0 Mar 21, 2013

You meant target?

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+
+@param modification {object}
+ The modification we want to apply to the target
+
+@param target
+ The object that representing, or containing, a content page. Depends by
+ the `modification` given, it could be optional. In that case, the modification
+ will be attached to any target that makes sense for this kind of modification.
+</api>
+
+<api name="detach">
+@function
+ This function removes content modifications previously added with `attach`
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would rather say: Function removes attached modification. If target is specified modification
is removed from that target only, otherwise modification is removed from all the targets it's being
attached to.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+
+ For example, the following code applies and removes a style to a content
+ window, adding a border to all divs in page:
+
+ var { attach, detach } = require("sdk/content/mod");
+ var Style = require("sdk/stylesheet/style").Style;
+
+ var style = Style({
+ source: "div { border: 4px solid gray }"
+ });
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+ // ...
+ detach(style, window);
+
@Gozala
Gozala Mar 21, 2013 Mozilla member

Wanna add example for detach(style) ?

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+ var style = Style({
+ source: "div { border: 4px solid gray }"
+ });
+
+ // assuming window points to the content page we want to modify
+ attach(style, window);
+ // ...
+ detach(style, window);
+
+@param modification {object}
+ The modification we want to remove from the target
+
+@param target
+ The object that representing, or containing, a content page. Depends by
+ the `modification` given, it could be optional. In that case, the modification
+ will be removed from any target that makes sense for this kind of modification.
@Gozala
Gozala Mar 21, 2013 Mozilla member

Target is a value that representing content to be modified. It is valid only when
getTargetWindow(target) returns nsIDOMWindow of content it represents.
If target is not provided modification is removed from all targets it's being attached to.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+ detach(style, window);
+
+@param modification {object}
+ The modification we want to remove from the target
+
+@param target
+ The object that representing, or containing, a content page. Depends by
+ the `modification` given, it could be optional. In that case, the modification
+ will be removed from any target that makes sense for this kind of modification.
+</api>
+
+<api name="getTargetWindow">
+@function
+ This function returns a window represented, or contained, by the `target`
+ given. If no proper window can be found for the `target`, it will returns
+ `null`.
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would move this to the top since attach and detach can refer to it. I would also rephrase it to:

Function takes target, value representing content (page) and returns nsIDOMWindow
for that content. If target does not represents valid content null is returned. For example
target can be a content window itself in which case it's will be returned back.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/content/mod.md
+ The object that representing, or containing, a content page. Depends by
+ the `modification` given, it could be optional. In that case, the modification
+ will be removed from any target that makes sense for this kind of modification.
+</api>
+
+<api name="getTargetWindow">
+@function
+ This function returns a window represented, or contained, by the `target`
+ given. If no proper window can be found for the `target`, it will returns
+ `null`.
+
+@param target {object}
+ The object for which we want to obtain the window represented or contained.
+ If a `nsIDOMWindow` is given, it works as an identify function, returns
+ `target` itself.
+@returns {window}
@Gozala
Gozala Mar 21, 2013 Mozilla member

{nsIDOMWindow|null}

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/stylesheet/style.md
@@ -0,0 +1,50 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `style` module provides a Style object that represent one or a set of
@Gozala
Gozala Mar 21, 2013 Mozilla member

Module provides Style function that can be used to construct content style modification via stylesheet files or CSS rules.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/stylesheet/style.md
@@ -0,0 +1,50 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `style` module provides a Style object that represent one or a set of
+stylesheet files or CSS rules.
+
+<api name="Style">
+@class
+<api name="Style">
+@constructor
+ The Style constructor creates an object that represents one or a set of
@Gozala
Gozala Mar 21, 2013 Mozilla member

... that represents style modifications via stylesheet file(s) or/and CSS rules. Stylesheet file
URL(s) are verified to be local to an add-on, while CSS rules are virified to be a string or
array of strings.

@Gozala Gozala commented on the diff Mar 21, 2013
doc/module-source/sdk/stylesheet/style.md
+ option; `null` if no `source` option was given to the constructor.
+ This property is read-only.
+</api>
+<api name="uri">
+@property {string}
+ An array of strings that contains the stylesheet local URI(s) specified in the
+ constructor's option; `null` if no `uri` option was given to the
+ constructor.
+ This property is read-only.
+</api>
+<api name="type">
+ @property {string}
+ The type of the sheet. If no type is provided in constructor's option,
+ it returns the default value, `"author"`. This property is read-only.
+</api>
+</api>
@Gozala
Gozala Mar 21, 2013 Mozilla member

I would also mention in docs that style can be applied to a content by calling attach(style, window) or removed using
detach(style, window) and would refer to content/mod docs for more details.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
doc/module-source/sdk/stylesheet/utils.md
@@ -0,0 +1,42 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+ - License, v. 2.0. If a copy of the MPL was not distributed with this
+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+The `stylesheet/utils` module provides helper functions for working with
+stylesheets.
@Gozala
Gozala Mar 21, 2013 Mozilla member

It's better to always say Module provides ... specifying id is fragile and can get out of sync.

@Gozala Gozala commented on the diff Mar 21, 2013
doc/module-source/sdk/stylesheet/utils.md
+ If not provided, the default value is `"author"`.
+</api>
+
+<api name="removeSheet">
+ @function
+ Remove the document style sheet at `sheetURI` from the list of additional
+ style sheets of the document. The removal takes effect immediately.
+ @param window {nsIDOMWindow}
+ @param uri {string, nsIURI}
+ @param [type="author"] {string}
+ The type of the sheet. It accepts the following values: `"agent"`, `"user"`
+ and `"author"`.
+ If not provided, the default value is `"author"`.
+</api>
+
+<api name="isTypeValid">
@Gozala
Gozala Mar 21, 2013 Mozilla member

I think isValidSheetType makes more sense this name is too generic specially because type is generic by it's own.

@ZER0
ZER0 Mar 21, 2013

I checked with native language speaker (aka, @jsantell) about if noun should be before or after the the "valid" part, and he said that isTypeValid sounds more correct than isValidType (that was the original name).

About the too generic name: we have tons of generic name, but I always though that the module worked as context (see for instance add or once).
If you think isTypeValid is too generic, then what about the other generic name we have in the code base? Should we change them too?
If we should be more specific here, why we shouldn't more specific also in the other modules? What is the discriminant?

@Gozala Gozala commented on the diff Mar 21, 2013
doc/module-source/sdk/stylesheet/utils.md
+
+<api name="loadSheet">
+ @function
+ Synchronously loads a style sheet from `uri` and adds it to the list of
+ additional style sheets of the document.
+ The sheets added takes effect immediately, and only on the document of the
+ `window` given.
+ @param window {nsIDOMWindow}
+ @param uri {string, nsIURI}
+ @param [type="author"] {string}
+ The type of the sheet. It accepts the following values: `"agent"`, `"user"`
+ and `"author"`.
+ If not provided, the default value is `"author"`.
+</api>
+
+<api name="removeSheet">
@Gozala
Gozala Mar 21, 2013 Mozilla member

I don't remember if it was me who suggested it in first place but I think it's better to have load/unload or add/remove Sheet

@ZER0
ZER0 Mar 21, 2013

This was made to be consistent with the platform's name. I think they're actually good and they express exactly what they did. Why you think is no good with them?

@Gozala Gozala commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+
+ if (style.source) {
+ let uri = "data:text/css;charset=utf-8,";
+
+ uri += encodeURIComponent(style.source.join(""));
+
+ removeSheet(window, uri, style.type);
+ }
+
+ remove(style, window.document);
+}
+
+attach.define(Style, function (style, target) {
+ let window = getTargetWindow(target);
+
+ if (style.uri)
@Gozala
Gozala Mar 21, 2013 Mozilla member

I think I'd prefer {} for statements that take more than one line.

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+
+function isLocalURL(item) {
+ try {
+ return LOCAL_URI_SCHEMES.indexOf(URL(item).scheme) > -1;
+ }
+ catch(e) {}
+
+ return false;
+}
+
+const Style = Class({
+ extends: Disposable,
+ setup: function(options) {
+ let { source, uri, type } = options;
+
+ source = source == null ? null : [].concat(source);
@Gozala
Gozala Mar 21, 2013 Mozilla member

Freeze if it's array so new lines can't be pushed to it.

@ZER0
ZER0 Mar 21, 2013

How they could be pushed?

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+function isLocalURL(item) {
+ try {
+ return LOCAL_URI_SCHEMES.indexOf(URL(item).scheme) > -1;
+ }
+ catch(e) {}
+
+ return false;
+}
+
+const Style = Class({
+ extends: Disposable,
+ setup: function(options) {
+ let { source, uri, type } = options;
+
+ source = source == null ? null : [].concat(source);
+ uri = uri == null ? null : [].concat(uri);
@Gozala
Gozala Mar 21, 2013 Mozilla member

Freeze if it's arry so new values can't be pushed on it.

@ZER0
ZER0 Mar 21, 2013

same as above.

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+
+ if (type && !isTypeValid(type))
+ throw new Error('Style.type must be "agent", "user" or "author"');
+
+ Object.defineProperties(this, {
+ "source": { value: source },
+ "uri": { value: uri },
+ "type": { value: type }
+ });
+ },
+ dispose: function dispose() {
+ detach(this);
+ }
+});
+
+exports.Style = Style;
@Gozala
Gozala Mar 21, 2013 Mozilla member

I think we complicate Style too much here I think should be more simple like:

function Style({ source, uri, type }) {
  source = source == null ? null : Object.freeze([].concat(source));
  uri = uri == null ? null : Object.freeze([].concat(uri));
  type = type == null ? "author" : type;

  if (source && !source.every(isString))
    throw new TypeError('Style.source must be a string or an array of strings.');
  if (uri && !uri.every(isLocalURL))
    throw new TypeError('Style.uri must be a local URL or an array of local URLs');
  if (type && !isTypeValid(type))
    throw new TypeError('Style.type must be "agent", "user" or "author"');

  return Object.freeze(Object.create(Style.prototype, {
    source: { value: source, enumerable: true },
    uri: { value: uri,  enumerable: true },
    type: { value: type, enumerable: true } 
  });
}

My point is it should be a simple struct no special powers (well aside of attach / detach) knowing how to deal with it :)
No real need to dispose it since page-mod detach-es style anyway on own desposal.

@ZER0
ZER0 Mar 21, 2013

I actually liked the fact that style takes care of their disposal, no matter who has this style attached (e.g. page-mod, tabs, etc). But I'm not picky about that.

@Gozala Gozala commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+ uri = uri == null ? null : [].concat(uri);
+ type = type == null ? "author" : type;
+
+ if (source && !source.every(isString))
+ throw new Error('Style.source must be a string or an array of strings.');
+
+ if (uri && !uri.every(isLocalURL))
+ throw new Error('Style.uri must be a local URL or an array of local URLs');
+
+ if (type && !isTypeValid(type))
+ throw new Error('Style.type must be "agent", "user" or "author"');
+
+ Object.defineProperties(this, {
+ "source": { value: source },
+ "uri": { value: uri },
+ "type": { value: type }
@Gozala
Gozala Mar 21, 2013 Mozilla member

Why not enumerable ?

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+ extends: Disposable,
+ setup: function(options) {
+ let { source, uri, type } = options;
+
+ source = source == null ? null : [].concat(source);
+ uri = uri == null ? null : [].concat(uri);
+ type = type == null ? "author" : type;
+
+ if (source && !source.every(isString))
+ throw new Error('Style.source must be a string or an array of strings.');
+
+ if (uri && !uri.every(isLocalURL))
+ throw new Error('Style.uri must be a local URL or an array of local URLs');
+
+ if (type && !isTypeValid(type))
+ throw new Error('Style.type must be "agent", "user" or "author"');
@Gozala
Gozala Mar 21, 2013 Mozilla member

I think these should be TypeErrors

@ZER0
ZER0 Mar 21, 2013

In all the rest of our code base we throw Error instead of TypeError for this kind of scenario. I just kept it consistent. Should we change the other errors too?

@Gozala Gozala and 1 other commented on an outdated diff Mar 21, 2013
lib/sdk/stylesheet/style.js
+ source = source == null ? null : [].concat(source);
+ uri = uri == null ? null : [].concat(uri);
+ type = type == null ? "author" : type;
+
+ if (source && !source.every(isString))
+ throw new Error('Style.source must be a string or an array of strings.');
+
+ if (uri && !uri.every(isLocalURL))
+ throw new Error('Style.uri must be a local URL or an array of local URLs');
+
+ if (type && !isTypeValid(type))
+ throw new Error('Style.type must be "agent", "user" or "author"');
+
+ Object.defineProperties(this, {
+ "source": { value: source },
+ "uri": { value: uri },
@Gozala
Gozala Mar 21, 2013 Mozilla member

Wondering if we should call this property uris instead..

@ZER0
ZER0 Mar 21, 2013

I had the same feeling but I concluded that we shouldn't. We have singular for all other properties in our code base that share the same behavior, as far as I know.

@Gozala
Member
Gozala commented Mar 21, 2013

@ZER0 Please don't get upset I know it's my fault in many ways as I mislead you, but after having time to think about this in more details I think Style does to much and part of what it does is quite generic not really style specific. Here is what I mean:

  • As I mentioned above Style should be no more than a struct no refs to anything but style defs it contains no need to dispose etc...
  • I think we should replace attach detach methods with attachTo and detachFrom polymorphic
    methods that do only one thing attach / detach modification to the given target. More specifically I mean
    that these new functions should not maintain any internal refs like weak sets. Instead that should be done
    by other component (see next section)
  • We should make normal attach / detach functions that use attachTo and detachFrom polymorphic
    function under the hood and maintain weak set of modification and target associations. Here is an example:
const { add, remove, iterator } = require("../lang/weak-set");
const method = require("method/core");

let attachTo = method("attachTo")
exports.attachTo = attachTo;

let detachFrom = method("detachFrom");
exports.detachFrom = detachFrom;

function attach(modification, target) {
  let window = getTargetWindow(target);
  attachTo(style, window);
  add(style, window.document);
}
exports.attach = attach;

function detach(modification, target) {
  if (target) {
    let window = getTargetWindow(target);
    detachFrom(style, window);
    remove(style, window.document);
  } else {
    let documents = iterator(style);
    for (let document of documents) {
      detachFrom(style, document.defaultView);
      remove(style, document);
   }
}
exports.detach = detach;

Now above being said, non of the public APIs actually require changes to incorporate my suggestions, there for
I would land this in first iteration, and have another one to address these.

@Gozala
Member
Gozala commented Mar 21, 2013

All the documentation comments are (in my view) improvement suggestions that may or may not make them better, I usually ask will in to provide a feedback but don't block a push on this.

@ZER0
ZER0 commented Mar 21, 2013

About the last comment, we'll discuss it better tomorrow. I'm not quite convinced by the name, but for the rest I see the point.

@ZER0 ZER0 Bug 837494 - re-implement `contentStyle*` using the new nsIDOMWindowU…
…tils methods

- added `mod` module, with `attach`, `detach`, `attachTo`, `detachFrom` and `getWindowTarget`
- added `loadSheet` and `removeSheet` to `stylesheet/utils`
- added `Style` object in `stylesheet/style`
- replace usage of stylesheet services with `attach`/`detach` a `Style` object
- added `weak-set` module
28fb7fa
@ZER0 ZER0 merged commit 34df9d3 into mozilla:master Apr 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment