816967 - Remotable Style Editor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Ben Francis [:benfrancis]]

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

[Panos Astithas (he/him) [:past] (please ni?)]

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

[Ben Francis [:benfrancis]]

Cool, thanks. Renaming this bug accordingly.

Comment 3

[Heather Arthur [:harth]]

Attachment: 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

[Victor Porof [:vporof][:vp]]

ZOMG harth! \o/

Comment 5

[Ben Francis [:benfrancis]]

\o/

Comment 6

[Dave Camp (:dcamp)]

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

[Heather Arthur [:harth]]

Attachment: 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

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/26caa9ab5c35

Comment 9

[Heather Arthur [:harth]]

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]

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

Comment 10

[Heather Arthur [:harth]]

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

fixed xpcshell failure.

Comment 11

[Heather Arthur [:harth]]

Attachment: 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

[Heather Arthur [:harth]]

http://hg.mozilla.org/integration/fx-team/rev/3919cfc6dab4

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a5e74dbe5327
https://hg.mozilla.org/mozilla-central/rev/3919cfc6dab4