501259 - Implement JEP 17: Page Mods

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect)

Description

[David Dahl :ddahl]

syntactic sugar that makes it really easy to run code that modifies a page. It makes Greasemonkey-esque functionality easy to implement.

Comment 1

[David Dahl :ddahl]

Attachment: v 0.1 WIP - Non-working

Just saving my work.

Comment 2

[Ed Lee :Mardak]

Curious, how are you going to listen for all page loads on all browsers on all tabs? Do page mods run on about:jetpack reload? Or when you first add a page mod on an already-loaded page?

Comment 3

[David Dahl :ddahl]

Not sure yet. I have only been implementing the regex facility so far. I think adw wrote a jetpack that keeps track of all tabs in all browsers. I am hoping to re-use some code.

Comment 4

[David Dahl :ddahl]

Attachment: v 0.2 MatchPatterns are working with tests - WIP

Next I need to implement the event handling for each pagemod

Comment 5

[David Dahl :ddahl]

Attachment: v 0.3 WIP - cannot get it to "lazy load"

Trying to get the module to lazy load. I think the way I am trying to reference the jetpack singleton is a bit wonky yet.

Comment 6

[David Dahl :ddahl]

Attachment: v 0.4 WIP tests work - but cannot access jetpack.tabs it is "undefined" - "sort of"

jetpack as logged to console is "undefined" - but is introspectable from Firebug. Not sure how to get a handle to jetpack properly

Comment 7

[David Dahl :ddahl]

here is a screenshot of what i am seeing in firebug: http://img194.imageshack.us/img194/1329/screenshot020s.png

Comment 8

[David Dahl :ddahl]

Attachment: 0.4 (again) forgot to take out an experiment

Comment 9

[David Dahl :ddahl]

Attachment: 0.5 Working Patch, tests pass

Working patch for JEP 17 - jetpack.pageMods

Comment 10

[Ed Lee :Mardak]

Comment on attachment 386616 [details] [diff] [review]
0.5 Working Patch, tests pass

>+++ b/extension/content/js/jetpack-environment.js
>+   "js/page-modification.js":[
>+     "PageMods",
>+     "MatchPattern"
Don't really need MatchPattern here because we don't access it outside of page-modifications. (Actually.. probably don't need to even export it?)

>+   "jetpack.pageMods": function(context) {
>+     var pageMods = new PageMods(context.sandbox.jetpack);
>+     context.addUnloader(pageMods);
>+     return pageMods;
So.. for both SlideBar and Selection, my class is a "super global singleton". It might not be the best way to do things though because I can manually handle different contexts and such. But because it's a global, there's an object available to create per-context exported object.
http://hg.mozilla.org/labs/jetpack/rev/0bbe317c4b9d#l1.1

Right now, this code returns a PageMods instance, which actually exposes an add() and a register(). The JEP only mentions the add() API.

>+++ b/extension/content/js/page-modification.js
>+function PageMods(jetpack) {
>+  if (typeOf(jetpack) === 'object'){
>+    this.jetpack  = jetpack;
One trick I used for Selection is that I just pass in context, and use what I want out of the context.. so locally grab context.sandbox.jetpack and perhaps context.addUnloader.

But I don't think we need the unloader anyway. Do we do anything special on context unload? (This happens when the user reloads about:jetpack or uninstalls.)

A related question.. what should happen when the user reloads about:jetpack? Right now we'll run .add() and nothing happens until the user reloads the page. So that might not actually be too bad, but somebody testing will need to reload about:jetpack then reload the target page.

>+  add: function Jetpack_pageMods_add(callBack, matchPatterns){
According to the JEP, the second arg is actually a dictionary with a .match property which is an array. I remember Aza mentioning that for dictionary args, we could simplifly it with the "common case" arg (this case being a string) and just handle it...)

>+    var _patterns = [];
>+    if (typeOf(matchPatterns) === 'string') {
>+      _patterns.push(matchPatterns);
>+    }
>+    else if (typeOf(matchPatterns) === 'array') {
>+      for (var i=0; i < matchPatterns.length; i++) {
>+        _patterns.push(matchPatterns[i]);
>+      }
>+    }
But if we're going with this, this code could probably be simplified with patterns = [matchPatterns] for string case and patterns = matchPatterns.slice() for the array case.. to make a copy of the original array.

But that's interesting API choice though.. If we just take the original array object passed in through the second arg, theoretically the PageMods feature writer could change the array dynamically...

>+    // store all MatchPattern Objects in a new object with its corrresponding
>+    // callback function
>+    for (var i=0; i < _patterns.length; i++) {
>+      try {
>+        let m = new MatchPattern(_patterns[i]);
>+        this.patterns.push({ pattern: m, callback: callBack });
Curious, why the decision for a new object with its own pattern and callback? Just as pagemods.add() accepts one callback and multiple patterns, we should be able to just store them once as part of |this.addedFeatures| (probably want a better name). We need to keep track of each added feature because .add() could be called multiple times with different callbacks/patterns.

>+  register: function Jetpack_pageMods_register(scope){
>+    // register the callback on jetpack.tabs -
>+    // if the tab matches the matchPattern
>+    this.jetpack.tabs.onReady(function() {
I suppose we would want to unbind this on context unload. We might leak callbacks here otherwise... Have you ever run into issues of reloading a context/feed without reloading all of jetpack? This theoretically would keep the listener on tabs.onReady and have a live function but the scope object should be.. out of scope.

>+  testMatchPattern: function(runner){
I didn't look too closely at the MatchPattern implementation.. but curious, are there any chromium tests for matching? Any discrepancy could lead to bugs and/or security holes.

Comment 11

[Ed Lee :Mardak]

(In reply to comment #10)
> >+    this.jetpack.tabs.onReady(function() {
> I suppose we would want to unbind this on context unload. We might leak
Oh! This is okay in this case. Because if we ever unload a context, the jetpack.tabs will be unloaded as well.

Comment 12

[David Dahl :ddahl]

Attachment: 0.6 Updated patch to follow the JEP's options.matches

Also, cribbed the failure patterns from the chromium dev site and added them to the test. Also added more introspection to the matchPattern creation. 

The module exports "MatchPattern" as well as "PageMods" to allow for thorough testing. 

Moved pageMods to "future" namespace.

Comment 13

[David Dahl :ddahl]

Also, added a "blacklist" demo for GTD style browser control. look in extension/content/demos/page-mod-blacklist-sites.js - the installer is page-mod-demo-install.html

Comment 14

[David Dahl :ddahl]

Forgot to link to the chromium-dev patterns unittests: 
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/url_pattern_unittest.cc?revision=15378&view=markup

Comment 15

[David Dahl :ddahl]

Not sure why yet, but sometimes the jetpack.tabs.onReady does not fire. seems like it is the case when the url is loading all kinds of external images, css, js and other media.

Comment 16

[Ed Lee :Mardak]

Comment on attachment 387261 [details] [diff] [review]
0.6 Updated patch to follow the JEP's options.matches

>+++ b/extension/content/demos/page-mod-demo-install.html
Aza would prefer us to move demos directly to the website instead of packaged with the extension, so we'll probably do a mass move before launch. But you didn't add a link to page-mod-demo-install from the index.html ;)

>+++ b/extension/content/js/jetpack-environment.js
>+   "jetpack.pageMods": function(context) {
>+     Components.utils.import("resource://jetpack/modules/page-modification.js");
>+     var pageMods = new PageMods(context.sandbox.jetpack);
>+     return pageMods;
Could just return on that one same line, but it still has the issue of exposing .register()

>+++ b/extension/content/js/page-modification.js
You forgot to remove this file when switching to modules/page-modifications.js

>+++ b/extension/modules/page-modification.js
>+ * The Original Code is Jetpack code.
You can better describe it with Page Modifications.

>+var EXPORTED_SYMBOLS = ['PageMods', 'MatchPattern'];
Personally I would expose just one object that has others attached to it, but you would need to rework the above jetpack.pageMods registration to make sure jetpack features don't have access to the other objects. I've done this by exposing an "exporting function" for Selection.

>+    // introspect the options, handle string, array, and object
>+    if(typeOf(options) === 'string') {
>+      _patterns.push(options);
>+    }
>+    else if(typeOf(options) === 'object'){
>+      if(options.matches === undefined){
>+        throw new Error("jetpack.pageMods error: options.matches does not exist!");
>+      }
>+      else {
>+        if (typeOf(options.matches) === 'string') {
>+          _patterns.push(options.matches);
>+        }
>+        else if (typeOf(options.matches) === 'array') {
>+          for (var i=0; i < options.matches.length; i++) {
>+            _patterns.push(options.matches[i]);
>+          }
>+        }
>+      }
>+    }
>+    else if(typeOf(options) === 'array'){
>+      for (var i=0; i < options.length; i++) {
>+        _patterns.push(options[i]);
>+      }
>+    }
Should be able to collapse this down by deciding if you need to use options or options.matches, then share the logic to handle string vs array. And for the array, just slicing should copy things over without needing to iterate + push.

_patterns = [string] or _patterns = array.slice();

>+function MatchPattern(pattern, matchCase){
>+  // create and return a match pattern obj
>+  // validate match pattern!
>+  var validateHost = /^(\*|\*\.[^/*]+|[^/*]+)$/;
>+  var validatePath = /^\/.*$/;
Would be useful to say what they match or describe them. like matches literal "*" or "*.text" or "text".

>+  if(matchCase){
>+    caseFlag = "i";
>+  }
Is this every used?

>+  try {
>+    var uri = ioService.newURI(pattern, null, null );
>+  }
There's probably a utils somewhere to do this, but this could be cleaned up later.

Comment 17

[David Dahl :ddahl]

Attachment: 0.7 Updated as per Mardak's review

I left the MatchPattern exported symbol in the module, for now. Perhaps we can figure out the best way to get rid of this in the next release. I tried a few things, but the testing was too complex.

Comment 18

[David Dahl :ddahl]

I left the page-mods example jetpack in the demos directory, i can remove it if we are removing all of them to put them all online.

Comment 19

[Ed Lee :Mardak]

Comment on attachment 387501 [details] [diff] [review]
0.7 Updated as per Mardak's review

r=Mardak

Looks good, we'll just move the demo stuff to the jetpack/website directory after now that the new website is in the repo.

We can clean up the code later.

(FYI, you can use console.log to dump stuff. But maybe only if it's lazy loaded and not a module.. untested.)

Comment 20

[David Dahl :ddahl]

Attachment: moved install page to website/demos from content/demos

Just moved the demos over to the website and used the same css and markup as existing demo install pages.

Comment 21

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/jetpack/rev/95ed3b67e06b

I fixed up the test to unload the context and clean up the tabs it opens. The main problem was that the context wasn't being unloaded after it was done.

Comment 22

[Ed Lee :Mardak]

Also I compactified the test..

3.26 +  testAddModificationString: let (url = "http://0.0.0/String")
3.27 +    testAdd(url, url),
3.28 +
3.29 +  testAddModificationArray: let (url = "http://0.0.0/Array")
3.30 +    testAdd([url], url),
3.31 +
3.32 +  testAddModificationObjectString: let (url = "http://0.0.0/ObjectString")
3.33 +    testAdd({ matches: url }, url),
3.34 +
3.35 +  testAddModificationObjectArray: let (url = "http://0.0.0/ObjectArray")
3.36 +    testAdd({ matches: [url] }, url),

0.0.0 should always fail to load.. and I wanted to make sure the urls were actually different and they weren't triggering off each other's mozilla.com loads. And mozilla.com had tons of jquery spew :(

Comment 23

[melkor]

If you call the add method twice with different parameters all callback functions get executed twice (this is caused by 'add' registering an extra listener for the onReady event).

Also since the source text of the pattern isn't recorded in the objects that fill the patterns array, it is very difficult to find patterns. Which makes it harder to remove them.

The result of these two deficiencies make pageMods impractical for solving the complexities of picky scripts. Fortunately these are pretty trivial to solve.