Use proper content scripts

[rocketeerbkw]

In an effort to help get firefox a working plugin again, I took a stab at resolving some of the "injecting 3rd party libraries" problems.

I removed the content scope runner hack, but content scripts run in a sandbox so I also had to add some dependencies as part of the extension:

• jQuery 1.4.4 - Using an old version since drupal.org uses it and I don't want to break anything
• jQuery.once.js
• Drupal.js - Needs the Drupal global object and the attachBehaviors function. This is a temporary stop gap until it can be refactored in #155

I was able to build extensions for Firefox, Chrome and Safari. I tested them very minimally but it appears to be working correctly.

I think the check on https://dreditor.org/ to see if the extension is installed already or not is broken now though. I verified that the <div id="dreditor-is-installed"></div> element gets added but the button still says it's not installed.

This would fix #215 and #156.

[markhalliwell]

This is a good start, for sure. Thanks!!

My only hesitation is the comment @sun made here https://github.com/unicorn-fail/dreditor/issues/155#issuecomment-44039906

The reason we're currently using Drupal.behaviors is because it's actually tying into's d.o's native Drupal.behaviors system (because the script was actually injected into d.o's DOM and was allowed to interactive with d.o's JS.

I'm not sure if we actually need that or not (my memory on the existing plugins is absent ATM since I haven't worked on the code in a while). If not... we might as well create a proper Dreditor namespace with event handling (#155) with this PR too.

I know that extensions are sandboxed, but this issue about "injecting 3rd party libraries" doesn't mean we cannot still inject something into the DOM (which is allowed). The reason why the current content script injection is a hack is because it's dumping the entire extension script into the DOM (which is generally frowned upon, especially by a.m.o).

If we do need Drupal.behaviors, then we can still inject a small script that only binds to Drupal.behaviors and then triggers a custom event to the sandboxed extension script. This allows us to control exactly what's happening and "convert" it to whatever Dreditor namespaced event API that's created.

See: http://stackoverflow.com/questions/21731635

[rocketeerbkw]

I agree with all that. Copying Drupal.behaviors was the smallest/quickest change to get it working again.

I'm even less familiar with the plugins than you. I could "fix" the Drupal.behaviors so that it's functionally the same (reloading on ajax, etc) and ties in with d.o. That wouldn't require intimate knowledge of everything, puts stuff back in a working state, and is one step closer to a firefox release. But it's not the ideal code.

Or skip all that and go for #155. I don't know what a new plugin API looks like, or how long it'd take to refactor everything.

If the first option is acceptable, that's something I could do.

[markhalliwell]

I'd rather not copy any portion of drupal.js into Dreditor (even if it's for a temporary workaround). It just leads to additional technical debt and ultimately doesn't really solve the problem mentioned above about tying into d.o's behavior events.

[markhalliwell]

In all reality this PR kind of has to work in tandem with #155 or, essentially, replace it trying to achieve the same goal. I don't think doing one before the other in separate PRs will work. We have to keep the code fully functional in case a release needs to be made. Any PR regarding this stuff will, unfortunately, likely be a massive rewrite.

[rocketeerbkw]

I can confirm that the plugins rely on at least one part of Drupal.behaviors, and that's rerunning them when some content has been changed via AJAX. The easiest way to test is to change the project on an issue and it will reload the entire new comment section. Many of the plugins (patch.name.suggestion, form.sticky, issue.summary.template) need to be rerun to work correctly.

The docs for all three browsers said to use the postMessage() API to communicate between the content script and the page. I created a small library that injects a script and sends a message back to dreditor when Drupal.attachBehaviors() gets called. Then I was able to rerun the plugins.

Again, I did the smallest change to get re-running plugins, which was keeping the copied Drupal behaviors. But this is a lot closer to being functionally equivalent to the old content scope runner method.

[rocketeerbkw]

I followed the observer pattern to create a small plugin manager. I also converted one plugin as an example.

All plugins register themselves, so the execution order can be set alphabetically the way it is now, or a new file just for registrations can be used to set the order.

The notify() functionality makes it easy to add new plugin calls as they're needed, but if a rigid API is preferable it can be changed after all the plugins get converted (since right now, I'm not sure the init/bind is enough).

If this is the right track, I can convert all the plugins and remove the Drupal.* namespace (I don't think there's a need to switch to Dreditor.* since content scrips are sandboxed).

[markhalliwell]

I will have to take a look at this next weekend (swamped with client work right now).

[rocketeerbkw]

Cool, thanks for being responsive

[rocketeerbkw]

Performance regression on long issues (this one has 119 comments https://www.drupal.org/node/2301317), this is just a reminder for me.

[rocketeerbkw]

I'm still interested in continuing, if I'm on the right track.

[markhalliwell]

Yes, the comments above need to be addressed.

Also, is it possible for you to [re-]base this off of the newly created 2.x branch?

[rocketeerbkw]

Rebased on 2.x over at https://github.com/unicorn-fail/dreditor/pull/287