949556 - Add Firefox 26-28 backwards compatibility to Style Editor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Heather Arthur [:harth]]

Backwards compatibility was broken with CSS source maps change, see https://bugzilla.mozilla.org/show_bug.cgi?id=926014#c12.

Paul mentioned Gaia developers use nightly, so we should fix this ASAP.

Comment 1

[Heather Arthur [:harth]]

Attachment: Add Fronts to talk to old actors

This patch adds "StyleEditorActor/StyleEditorFront" and "OldStyleSheetActor/OldStyleSheetFront". These fronts are used when the old server API is detected. This is detected by testing for the existence of the newer 'styleSheetsActor' on the target.

I tried debugging Firefox 26, and all functionality is there.

try: https://tbpl.mozilla.org/?tree=Try&rev=3a190c0a71b4

Comment 2

[Heather Arthur [:harth]]

Comment on attachment 8347255 [details] [diff] [review]
Add Fronts to talk to old actors

Review of attachment 8347255 [details] [diff] [review]:
-----------------------------------------------------------------

Paul, this should make the Style Editor work with older actors. I tested with Firefox 26 myself, maybe you'd like to test with b2g as well.

Comment 3

[Heather Arthur [:harth]]

I should mention, "stylesheets.js" in this patch is just a file move, and has been reviewed completely, but of course feel free to look at it if you want to.

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 8347255 [details] [diff] [review]
Add Fronts to talk to old actors

I'll test this code later today, and r+ this patch if it works as expected.

>-  getStyleSheets: method(function() {
>-    let deferred = promise.defer();
>+  getBaseURI: method(function() {
>+    return this.document.baseURIObject.spec;
>+  }, {
>+    response: { baseURI: RetVal("string")}
>+  }),

Can you use `document.location.spec` instead of `baseURIObject` (which is not standard)?

>-    return deferred.promise;
>-  }, {
>-    request: {},
>-    response: { styleSheets: RetVal("array:stylesheet") }
>-  }),
>+    let documents = [this.document];
>+    var forms = [];
>+    for (let doc of documents) {
>+      let sheetForms = this._addStyleSheets(doc.styleSheets);
>+      forms = forms.concat(sheetForms);
>+      // Recursively handle style sheets of the documents in iframes.
>+      for (let iframe of doc.getElementsByTagName("iframe")) {
>+        documents.push(iframe.contentDocument);
>+      }
>+    }
>+
>+    events.emit(this, "document-load", forms);
>+  },

Assuming only <iframes> contain documents is not safe.

Use docshells. Here is a way to do it (not tested, don't trust this code too much):

> function visitDocShell(docshell) {
>     docshell.QueryInterface(Ci.nsIDocShell)
>             .QueryInterface(Ci.nsIDocShellTreeItem)
>             .QueryInterface(Ci.nsIDocShellTreeNode);
> 
>     let d = [];
>     for (let i = 0; i < docshell.childCount; i++) {
>         d.push(docshell.getChildAt(i));
>         d = d.concat(visitDocShell(child));
>     }
>     return [docshell].concat(d);
> }
> 
> let allDocShells = visitDocShell(window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation));

From a docshell, to get the document or the window, use docShell.contentViewer.

Comment 5

[Paul Rouget [:paul]]

Comment on attachment 8347255 [details] [diff] [review]
Add Fronts to talk to old actors

Error loading: resource://gre/modules/devtools/server/actors/styleeditor.js: TypeError: redeclaration of var promise - @resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/styleeditor.js:15
loadSubScript@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:48
DS_addActors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:302
debugger_start@chrome://browser/content/shell.js:1025
@chrome://browser/content/settings.js:216
onsuccess@chrome://browser/content/settings.js:51

Comment 6

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #5)
> Comment on attachment 8347255 [details] [diff] [review]
> Add Fronts to talk to old actors
> 
> Error loading: resource://gre/modules/devtools/server/actors/styleeditor.js:
> TypeError: redeclaration of var promise -
> @resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js ->
> resource://gre/modules/devtools/server/actors/styleeditor.js:15
> loadSubScript@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:48
> DS_addActors@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:302
> debugger_start@chrome://browser/content/shell.js:1025
> @chrome://browser/content/settings.js:216
> onsuccess@chrome://browser/content/settings.js:51

This is solved by Alex' patch in bug 926014.

Comment 7

[Paul Rouget [:paul]]

Heather, can you rebase your patch on top of Alex' patch from bug 926014? I'll then do more tests.

Comment 8

[Heather Arthur [:harth]]

Attachment: Add Fronts for old actors - with shell.js fix

This patch should work by itself. I had incorporated Alex's changes, but forgot to leave out the original styleeditor import in shell.js

If Alex's patch lands before this one, I'll rebase before I land.

Comment 9

[Heather Arthur [:harth]]

(In reply to Paul Rouget [:paul] from comment #4)

> >+  getBaseURI: method(function() {
> >+    return this.document.baseURIObject.spec;
> >+  }, {
> >+    response: { baseURI: RetVal("string")}
> >+  }),
> Can you use `document.location.spec` instead of `baseURIObject` (which is
> not standard)?

Turns out we're not using getBaseURI anywhere anymore, so I'll take the whole thing out.

> Use docshells. Here is a way to do it (not tested, don't trust this code too
> much):

Follow-up bug? to get it right.

Comment 10

[Heather Arthur [:harth]]

Attachment: Add Fronts for old actors - rebased

Rebased and minus baseURI code. Try:

https://tbpl.mozilla.org/?tree=Try&rev=dd8c5cc63dd3

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 8349198 [details] [diff] [review]
Add Fronts for old actors - rebased

Tested on FxOS 1.2, FxOS 1.3 and FxOS 1.4. It works.

Your patch won't apply correctly because of a missing ";" here:

> DebuggerServer.registerModule("devtools/server/actors/inspector")

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6f8a258f6c2c

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f8a258f6c2c