Closed Bug 816967 Opened 11 years ago Closed 10 years ago

Remotable Style Editor


(DevTools :: Style Editor, defect)

Not set


(Not tracked)

Firefox 23


(Reporter: benfrancis, Assigned: harth)



(Keywords: dev-doc-needed, Whiteboard: [blocking-development][b2g])


(3 files)

Gaia apps do not work in Firefox at the moment, some of them (e.g. the system app) may not work for a very long time, if ever.

Having an inspector & style editor that work with B2G Desktop would make developing these apps hugely less painful.
The remotable inspector is bug 805526, but I don't think we have a bug on file for a remotable Style Editor, so this could be it.
Cool, thanks. Renaming this bug accordingly.
Component: Developer Tools → Developer Tools: Style Editor
Summary: Port the Firefox Inspector & Style Editor to B2G Desktop → Remotable Style Editor
Assignee: nobody → fayearthur
Whiteboard: blocking-development → [blocking-development][b2g]
Depends on: 859569
This patch makes the Style Editor remotable.

The patch gets rid of StyleEditorChrome and the old structure, and replaces it with a few objects: StyleEditorDebuggee represents the document and wraps the calls to the StyleEditorActor on the server, it emits events like 'stylesheets-cleared' and 'stylesheets-added'. StyleSheet represents one style sheet in the document, and wraps communication with its respective StyleSheetActor on the server. StyleEditorUI builds and maintains the UI for the Style Editor tool. It listens to events from the StyleEditorDebuggee. Finally StyleSheetEditor controls the editor for a stylesheet, and listens to events from its StyleSheet object.

This patch regresses bug 826982, so the style editor no longer shows a notification when there are unsaved changes. This is because of a bug with nsIRequest and data urls, a bug to be filed. This feature could be added back in a new bug, however.
Attachment #740611 - Flags: review?(dcamp)
ZOMG harth! \o/
Comment on attachment 740611 [details] [diff] [review]
Remote Style Editor

Review of attachment 740611 [details] [diff] [review]:

::: toolkit/devtools/styleeditor/dbg-styleeditor-actors.js
@@ +82,5 @@
> +  _window: null,
> +
> +  actorPrefix: "styleEditor",
> +
> +  grip: function()

This should probably be 'form'.

@@ +100,5 @@
> +
> +    this._sheets.clear();
> +
> +    this.conn.removeActorPool(this.actorPool);
> +    this._actorPool = null;

do you need to remove this._actorPool?

@@ +223,5 @@
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "styleSheetsAdded",
> +      styleSheets: actors
> +    });

Due to some lameness in how the dbg-client.jsm is implemented, there's a potential race condition here.  If this is hit after a client sends a request but before we process the request, they'll get this packet as the answer.

You can add 'styleSheetsAdded' (and any other notification types) to the list of UnsolicitedNotifications in dbg-client.jsm for now until we solve that lameness more permanently.

@@ +499,5 @@
> +
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "sourceLoad-" + this.actorID,
> +      source: this.text

In other areas displaying long strings (like sources) we use a long string actor to avoid sending giant gobs of data over the protocol in one fell swoop.  I don't think it's worth holding up this patch over, but we might want a followup bug here.

@@ +738,5 @@
> +  _notifyStyleApplied: function()
> +  {
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "styleApplied-" + this.actorID

Is the duplication of this.actorID in the type to make it easier to attach to notifications only for the correct client object?

If so, it'd be nice to leave that out of the on-the-wire protocol.  We could do something in dbg-client.jsm to make it work more generally:

 // Only try to notify listeners on events, not responses to requests
 // that lack a packet type.
 if (aPacket.type) {
   this.notify(aPacket.type, aPacket);
+  this.notify(aPacket.type + '-' + aPacket.from, aPacket);
Attachment #740611 - Flags: review?(dcamp) → review+
Thanks for the catches, Dave. I'll file follow up bugs for the long string actor, and the events with actor IDs.

This patch addresses the other comments, and passes try server.
Attachment #741564 - Flags: review+
Whiteboard: [blocking-development][b2g] → [blocking-development][b2g][fixed-in-fx-team]
Backed out for XPCShell test failure:

TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://global/content/devtools/dbg-styleeditor-actors.js :: <TOP_LEVEL> :: line 15" data: no]
Whiteboard: [blocking-development][b2g][fixed-in-fx-team] → [blocking-development][b2g]

fixed xpcshell failure.
Whiteboard: [blocking-development][b2g] → [blocking-development][b2g][fixed-in-fx-team]
Landing created something that will become an orange. Here's the fix.
Attachment #741991 - Flags: review?(dcamp)
Attachment #741991 - Flags: review?(dcamp) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [blocking-development][b2g][fixed-in-fx-team] → [blocking-development][b2g]
Target Milestone: --- → Firefox 23
Depends on: 866264
Depends on: 870004
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.