Summary: | Web Inspector: Move the computation that results in UI strings from JSC to the Web Inspector | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
Component: | Web Inspector | Assignee: | Saam Barati <saam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Saam Barati
2014-10-01 01:14:14 PDT
This patch will now also include a computed property now being computed in the WebInspector instead of in JSC. Created attachment 239191 [details]
patch
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"? (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. Created attachment 239225 [details]
patch
Previous patch had a bug, isFunction shouldn't be considered for primitiveTypeNames.
Comment on attachment 239225 [details] patch Clearing flags on attachment: 239225 Committed r174292: <http://trac.webkit.org/changeset/174292> All reviewed patches have been landed. Closing bug. |