Last Comment Bug 816967 - Remotable Style Editor
: Remotable Style Editor
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: unspecified
: All All
-- normal with 2 votes (vote)
: Firefox 23
Assigned To: Heather Arthur [:harth]
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Depends on: 859569 866264 870004
Blocks: 816961
  Show dependency treegraph
Reported: 2012-11-30 06:09 PST by Ben Francis [:benfrancis]
Modified: 2013-05-14 16:51 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Remote Style Editor (208.59 KB, patch)
2013-04-22 19:44 PDT, Heather Arthur [:harth]
dcamp: review+
Details | Diff | Splinter Review
updated to comments (209.71 KB, patch)
2013-04-24 16:10 PDT, Heather Arthur [:harth]
fayearthur: review+
Details | Diff | Splinter Review
fix browser_webconsole_bug_782653_CSS_links_in_Style_Editor.js orange (1.74 KB, patch)
2013-04-25 12:46 PDT, Heather Arthur [:harth]
dcamp: review+
Details | Diff | Splinter Review

Description User image Ben Francis [:benfrancis] 2012-11-30 06:09:00 PST
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.
Comment 1 User image Panos Astithas [:past] 2012-11-30 06:23:56 PST
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.
Comment 2 User image Ben Francis [:benfrancis] 2012-11-30 06:49:25 PST
Cool, thanks. Renaming this bug accordingly.
Comment 3 User image Heather Arthur [:harth] 2013-04-22 19:44:27 PDT
Created attachment 740611 [details] [diff] [review]
Remote Style Editor

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.
Comment 4 User image Victor Porof [:vporof][:vp] 2013-04-23 00:54:11 PDT
ZOMG harth! \o/
Comment 5 User image Ben Francis [:benfrancis] 2013-04-23 12:52:52 PDT
Comment 6 User image Dave Camp (:dcamp) 2013-04-23 16:03:35 PDT
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);
Comment 7 User image Heather Arthur [:harth] 2013-04-24 16:10:37 PDT
Created attachment 741564 [details] [diff] [review]
updated to comments

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.
Comment 8 User image Heather Arthur [:harth] 2013-04-24 16:20:31 PDT
Comment 9 User image Heather Arthur [:harth] 2013-04-24 17:21:43 PDT
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]
Comment 10 User image Heather Arthur [:harth] 2013-04-25 10:05:02 PDT

fixed xpcshell failure.
Comment 11 User image Heather Arthur [:harth] 2013-04-25 12:46:47 PDT
Created attachment 741991 [details] [diff] [review]
fix browser_webconsole_bug_782653_CSS_links_in_Style_Editor.js orange

Landing created something that will become an orange. Here's the fix.
Comment 12 User image Heather Arthur [:harth] 2013-04-25 14:06:27 PDT

Note You need to log in before you can comment on or make changes to this bug.