Bug 137295 – Web Inspector: Move the computation that results in UI strings from JSC to the Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

Currently, there is some implied UI that says that Number == Double, but make it more clear that Number can be Int || Double.

[Radar WebKit Bug Importer]

<rdar://problem/18511800>

[Saam Barati]

This patch will now also include a computed property now being computed in the WebInspector instead of in JSC.

[Saam Barati]

Created attachment 239191 [details]
patch

[Geoffrey Garen]

Comment on attachment 239191 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239191&action=review

r=me

> Source/WebInspectorUI/UserInterface/Models/TypeSet.js:122
> +        // It's implied that type Integer is contained in type Number. Don't put 
> +        // both 'Integer' and 'Number' into the set because this could imply that 
> +        // Number means to Double instead of Double|Integer.
> +        if (typeSet.isNumber)
> +            this._primitiveTypeNames.push("Number");
> +        else if (typeSet.isInteger)
> +            this._primitiveTypeNames.push("Integer");

I wonder if this could be a feature of typeSet. It's a bit weird that typeSet responds "true" to "isInteger" for something that is, in fact, number. Or perhaps "is" should be renamed to "contains"?

[Saam Barati]

(In reply to comment #4)
> (From update of attachment 239191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239191&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Models/TypeSet.js:122
> > +        // It's implied that type Integer is contained in type Number. Don't put 
> > +        // both 'Integer' and 'Number' into the set because this could imply that 
> > +        // Number means to Double instead of Double|Integer.
> > +        if (typeSet.isNumber)
> > +            this._primitiveTypeNames.push("Number");
> > +        else if (typeSet.isInteger)
> > +            this._primitiveTypeNames.push("Integer");
> 
> I wonder if this could be a feature of typeSet. It's a bit weird that typeSet responds "true" to "isInteger" for something that is, in fact, number. Or perhaps "is" should be renamed to "contains"?

Yeah, I like renaming these fields to containsX. I'll open a  new bug to do this.

There is indeed some weirdness here.
Currently, TypeSet wont respond to isNumber as true when it's only an Integer, but even though it's weird, I think it should be changed to that.

Here are all the possible states now and how typeset responds, and how I think typeset should respond:

1. isInteger=false, isNumber=false:
  Everything here works fine.
2. isInteger=true, isNumber=false:
  Everything also works fine here, we've only ever seen an integer for this typeset.
3. isInteger=false, isNumber=true:
  This gets slightly weird. isNumber implies Int | Double, but isInteger is false. This doesn't really make sense conceptually,
  and I propose that we should never allow the TypeSet to get into this state.
4. isInteger=true, isNumber=true:
  This is also fine, and for this state, I propose that, when hovering over a type token, we don't show both Int and Number, but simply
  just show Number. This is what this patch implements.

My vote is to never allow state 3 to happen, even though this may be slightly weird.

All this weirdness derives from not being able to reliably distinguish between Double and Integer inside the DFG. Because the DFG may
coerce an Integer into a Double, if a value is profiled after this coercion, there is no reliable way to determine that this thing I'm profiling that is a Double used to be an Integer. So it's better to just say that a profiled Double is Double | Integer.

[Saam Barati]

Created attachment 239225 [details]
patch

Previous patch had a bug, isFunction shouldn't be considered for primitiveTypeNames.

[WebKit Commit Bot]

Comment on attachment 239225 [details]
patch

Clearing flags on attachment: 239225

Committed r174292: <http://trac.webkit.org/changeset/174292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.