760370 - Visually distinguish non-extensible objects

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

From a recent discussion in es-discuss:

https://mail.mozilla.org/pipermail/es-discuss/2012-May/022991.html

We could alter the display of frozen, sealed and non-extensible objects in a similar fashion to what we do for writable, enumerable and configurable properties. I imagine a single character at the right side of the object value, with a "float: right" alignment, that reads either F(rozen), S(ealed) or N(on-extensible). Other ideas are more than welcome.

Comment 1

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

Test page: http://htmlpad.org/debugger-fsn/

Comment 2

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

Making a note here:
We need protocol support. There's information only about the writable, enumerable, configurable properties in the messages.

Comment 3

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

Somehow I think it would be more useful to have this in-your-face, not hidden in a tooltip. You don't get such objects everywhere, and it's important to.. distinguish them.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Victor, fair enough.

Uploading my existing WIP now.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip

Breaking some mochitests, but hopefully I can sidestep this issue when switching from tooltips to something more in your face, as Victor suggests.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

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

Comment 7

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

Comment on attachment 750728 [details] [diff] [review]
v1

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

Exemplary, but take a look at the comments below.

::: browser/devtools/debugger/test/browser_dbg_propertyview-12.js
@@ +24,5 @@
> +}
> +
> +function testFSN() {
> +  gDebugger.DebuggerController.activeThread.addOneTimeListener("framesadded", function() {
> +    // XXX: Let the variables view do its thing and setup before we begin testing.

I don't entirely understand this. Do what thing? Although I'm not happy about it, I'm not against a thread dispatch since all other test do this, so ok! But this comment makes me think you might want to listen to Debugger:FetchedVariables instead, otherwise intermittents may occur. (see other tests).

@@ +65,5 @@
> +    },
> +    extensible: hasNoneTester,
> +    string: hasNoneTester,
> +    arguments: hasNoneTester,
> +    this: hasNoneTester

This brings back FP memories. Thank you.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2389,4 @@
>        tooltip.setAttribute("orient", "horizontal");
> +
> +      let labels = ["configurable", "enumerable", "writable", "native-getter",
> +                    "frozen", "sealed", "non-extensible"];

We.. should really localize these at some point. Care to file a followup bug?

@@ +2389,5 @@
>        tooltip.setAttribute("orient", "horizontal");
> +
> +      let labels = ["configurable", "enumerable", "writable", "native-getter",
> +                    "frozen", "sealed", "non-extensible"];
> +      labels.forEach(label => {

When did for..of become too mainstream?

@@ +2423,5 @@
>  
>      if (this.ownerView.eval) {
>        this._target.setAttribute("editable", "");
>      }
> +    if (!descriptor.null) {

descriptor.null is a hack that makes me really sad. I'll file a bug to change it to something better.

@@ +2433,5 @@
> +      }
> +      if (!descriptor.writable && !this.ownerView.getter && !this.ownerView.setter) {
> +        this._target.setAttribute("non-writable", "");
> +      }
> +      if (descriptor.value && typeof descriptor.value === "object") {

Nit: I'd use == since really, typeof always returns strings, but I'm not holding a gun.

::: browser/themes/linux/devtools/widgets.css
@@ +531,5 @@
>    text-shadow: 0 0 8px #fcc;
>  }
>  
> +.variable-or-property[non-extensible]:not([non-writable]) > .title:after {
> +  content: "N";

I'm not sure how I feel about this. These should probably be localized, but since tooltips aren't localized either, the followup bug I mentioned earlier would be ok.

Comment 8

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

(In reply to Victor Porof [:vp] from comment #7)
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2389,4 @@
> >        tooltip.setAttribute("orient", "horizontal");
> > +
> > +      let labels = ["configurable", "enumerable", "writable", "native-getter",
> > +                    "frozen", "sealed", "non-extensible"];
> 
> We.. should really localize these at some point. Care to file a followup bug?

Why do we need to localize values specified in the JavaScript language? I don't believe anyone thinks in their native language when reading code.

But maybe we should change non-extensible to extensible. It's the only negative term and the strikeout effect would amount to a double negative.

Comment 9

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

> Why do we need to localize values specified in the JavaScript language? I
> don't believe anyone thinks in their native language when reading code.
> 

Good point. If you think it's ok, then I won't be worried. Although "native-getter" seems to be a special case, switching to "WebIDL" sounds good and universal.

> But maybe we should change non-extensible to extensible. It's the only
> negative term and the strikeout effect would amount to a double negative.

Agreed.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> But maybe we should change non-extensible to extensible. It's the only negative
> term and the strikeout effect would amount to a double negative.

I think this could possibly be more confusing because

a) we don't show the tooltip when an object is extensible (which I can change, but the vast majority of objects are, and I think always showing it would be clutter)

b) it isn't double negative because it isn't strike-through'd

c) The "N" wouldn't stand for non-extensble anymore. I guess we could do an "E" with a strike-through.

Thoughts?

Comment 11

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

Now that I've actually played with the patch I see what you mean. It looks fine to me.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So can I [land-in-fx-team] this bad boy?

Comment 13

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

Yes please!

Comment 14

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

Or not! No updated patch yet. The only thing I mostly care about is using Debugger:FetchedVariables instead of framesadded, like I said in comment #7. I wouldn't mind the rest of the nits addressed, but they're.. just nits :)

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: V2

* Using Debugger:FetchedVariables in test

* Using for of loop instead of forEach

* Using == instead of === in string comparison

* Rebased on new devtools directory structure

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

Comment 16

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

Comment on attachment 755745 [details] [diff] [review]
V2

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

Ship it!

Comment 17

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

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

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/842928279e36