352990 - Improve domi's Evaluate JavaScript

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Jason Barnabe (np)]

Switch the evaluate dialog to a real dialog instead of window class="dialog". Brings correct cross-platform button arrangement, less code, and other goodness.

"Inspect in new window" is confusing - unless you return a value nothing gets inspected anywhere. Also, if you uncheck it and you return a value, does that mean don't inspect it, or does it mean inspect it somewhere else? Switch to:
 Inspect return in
 (o) Existing window
  ( ) New window

If the expression has an error, the user isn't informed. Alert any error that happens in the expression and don't close the dialog.

If the expression changes the current subject, the change isn't reflected in the viewer. Make it so that if the expression doesn't return a value, assume it was a change to the subject and refresh the viewer.

Comment 1

[Jason Barnabe (np)]

Attachment: patch v1

cvs diff didn't include the stylesheets I added. I'll include them separately.

Comment 2

[Jason Barnabe (np)]

Attachment: added stylesheet, v1

This goes in
extensions/inspector/resources/skin/classic/viewers/jsObject/evaluateDialog.css
and
extensions/inspector/resources/skin/modern/viewers/jsObject/evaluateDialog.css

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 238843 [details] [diff] [review]
patch v1

>+function execute()

Could you please JavaDoc this function?  :)

Also, I'd like a screenshot of the new behavior, if you would please.

On the CSS stylesheet, it may be prudent to add a MPL/LGPL/GPL boilerplate to the beginning of it.

Comment 4

[timeless]

Comment on attachment 238843 [details] [diff] [review]
patch v1

>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js,v
>+      } else
>+        // the expression is likely to have changed the subject, so reload it.
>+        this.subject = this.subject;

please brace multiline else blocks.

Comment 5

[Jason Barnabe (np)]

Attachment: screenshot of the updated screen

Comment 6

[[e-mail]]

Comment on attachment 238844 [details]
added stylesheet, v1

If you're adding a width solely to make the description wrap then I have no objection to inline style on the document element but the width must be in font-relative units (currently em, until some layout peer should bother to sort out our width unit story, sigh).

Comment 7

[Jason Barnabe (np)]

Attachment: patch v2

The width was there before, but let's fix it anyway.

Comment 8

[[e-mail]]

Comment on attachment 238921 [details] [diff] [review]
patch v2

>+  skin/classic/inspector/viewers/jsObject/evaluateDialog.css          (resources/skin/classic/viewers/jsObject/evaluateDialog.css)
>+  skin/modern/inspector/viewers/jsObject/evaluateDialog.css          (resources/skin/modern/viewers/jsObject/evaluateDialog.css)
Not used.

>+    // alert the user of an error in their expression, and don't close
>+    alert(ex);
Use the prompt service...

>+        buttons="accept,cancel"
Nit: that's the default

>+        ondialogaccept="return execute()"
Nit: ;

>+        style="width: 30em">
Nit: ;

>+  <textbox id="txfExprInput" value=""/>
I know you only reindented but value="" is the default.

>+        // the expression is likely to have changed the subject, so reload it.
>+        this.subject = this.subject;
I'd prefer a separate menuitem to do this.

>+<!ENTITY evaluateDialog.title "Evaluate Expression">
>+<!ENTITY jsEval.desc "Enter a JavaScript expression.  The variable &quot;target&quot; is the property that is currently selected.">
> <!ENTITY jsExecute.label "Execute">
>-<!ENTITY jsCancel.label "Cancel">
>+<!ENTITY inspectReturn.label "Inspect return in">
>+<!ENTITY inspectReturnNew.label "New window">
>+<!ENTITY inspectReturnExisting.label "Existing window">
Aren't you supposed to do something about these new entites in other locales?

Comment 9

[Jason Barnabe (np)]

(In reply to comment #8)
> >+        this.subject = this.subject;
> I'd prefer a separate menuitem to do this.

A separate menuitem? Like, an entry on the context menu that says "Refresh"? I don't see that being very useful.

I wonder if there's some sort of event handler we could put on the object (assuming it's a node of some kind) that would tell us if it changed.

> Aren't you supposed to do something about these new entites in other locales?

No, when adding new strings, all we're supposed to do is make sure the locales missing bits aren't in the makefile.
http://groups.google.com/group/mozilla.dev.l10n/browse_frm/thread/50e5d03b7ea29dd7/d357ca5b80e90545?lnk=st&q=&rnum=2&hl=en#d357ca5b80e90545

Comment 10

[[e-mail]]

(In reply to comment #9)
>an entry on the context menu that says "Refresh"? I
>don't see that being very useful.
Well, I don't think refreshing based on the lack of a result is useful either.

Comment 11

[[e-mail]]

Comment on attachment 238921 [details] [diff] [review]
patch v2

>+      } else
>+        // the expression is likely to have changed the subject, so reload it.
>+        this.subject = this.subject;
I'd like this removed anyway. sr- as per previous comments.

Comment 12

[Jason Barnabe (np)]

Attachment: patch v3

Updated to comments.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

extensions/inspector/resources/locale/ru/viewers/jsObject.dtd 	1.2
extensions/inspector/resources/content/viewers/jsObject/evalExprDialog.js 1.6
extensions/inspector/resources/content/viewers/jsObject/evalExprDialog.xul 1.5
extensions/inspector/resources/locale/en-US/viewers/jsObject.dtd 1.4
<and other locale files>