Bug 137295 - Web Inspector: Move the computation that results in UI strings from JSC to the Web Inspector
Summary: Web Inspector: Move the computation that results in UI strings from JSC to th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-01 01:14 PDT by Saam Barati
Modified: 2014-10-03 14:35 PDT (History)
5 users (show)

See Also:


Attachments
patch (12.05 KB, patch)
2014-10-03 01:31 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch (11.97 KB, patch)
2014-10-03 13:27 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2014-10-01 01:14:14 PDT
Currently, there is some implied UI that says that Number == Double, but make it more clear that Number can be Int || Double.
Comment 1 Radar WebKit Bug Importer 2014-10-01 01:14:28 PDT
<rdar://problem/18511800>
Comment 2 Saam Barati 2014-10-02 18:56:09 PDT
This patch will now also include a computed property now being computed in the WebInspector instead of in JSC.
Comment 3 Saam Barati 2014-10-03 01:31:54 PDT
Created attachment 239191 [details]
patch
Comment 4 Geoffrey Garen 2014-10-03 10:38:42 PDT
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"?
Comment 5 Saam Barati 2014-10-03 13:18:04 PDT
(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.
Comment 6 Saam Barati 2014-10-03 13:27:02 PDT
Created attachment 239225 [details]
patch

Previous patch had a bug, isFunction shouldn't be considered for primitiveTypeNames.
Comment 7 WebKit Commit Bot 2014-10-03 14:35:32 PDT
Comment on attachment 239225 [details]
patch

Clearing flags on attachment: 239225

Committed r174292: <http://trac.webkit.org/changeset/174292>
Comment 8 WebKit Commit Bot 2014-10-03 14:35:34 PDT
All reviewed patches have been landed.  Closing bug.