[Webkit-unassigned] [Bug 133395] Web Inspector: debugger should be able to show variable types

Mon Jun 23 13:10:00 PDT 2014

https://bugs.webkit.org/show_bug.cgi?id=133395

Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #233394|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #17 from Filip Pizlo <fpizlo at apple.com>  2014-06-23 13:10:21 PST ---
(From update of attachment 233394)
View in context: https://bugs.webkit.org/attachment.cgi?id=233394&action=review

Looks pretty good, just minor nits.

> Source/JavaScriptCore/jsc.cpp:942
> +
> +

Remove

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:57
> +    //TODO: Should this functor be called with one operand?

Space between // and TODO.

And yes, this should keep the input alive.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1926
> +                //TODO: handle other op.type here

Space between // and TODO

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1933
> +                    //TODO: implement

Ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1969
> +            //String name = nameForRegister(virtualRegister);
> +            //printf("LINKING: op_profile_types_with_high_fidelity has uniqueID:'%llu' and name:'%s', line:'%u', column:'%u'\n", globalID, name.ascii().data(), location->line, location->column); 

Remove this.  Or, create a "static const bool verbose = false" and use that as a guard.  Also, don't use printf() - use dataLog() instead.

> Source/JavaScriptCore/bytecode/TypeLocation.h:36
> +    enum HighFidelityGlobalIDFlags {
> +        HighFidelityNeedsUniqueIDGeneration = -1,
> +        HighFidelityNoGlobalIDExists = -2
> +    };

Unindent the enum.

> Source/JavaScriptCore/bytecode/TypeLocation.h:51
> +    //uint64_t globalVariableID;

Remove.

> Source/JavaScriptCore/bytecode/TypeLocation.h:53
> +    int64_t globalVariableID;
> +    uint64_t globalInstructionID;

Prepend "m_" to field names.

> Source/JavaScriptCore/bytecode/TypeLocation.h:54
> +    //size_t unlinkedByteCodeOffset;

Unique.

> Source/JavaScriptCore/bytecode/TypeLocation.h:57
> +    intptr_t sourceID;
> +    unsigned line;
> +    unsigned column;

Prepend "m_" to field names.

> Source/JavaScriptCore/bytecode/TypeLocation.h:58
> +    TypeSet* typeSet;

Use a std::unique_ptr<TypeSet> if this is owned by TypeLocation.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1120
> +    //op_profile_types_with_high_fidelity value_reg location hasGlobalID

Turn this comment into a sentence or remove it.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1123
> +    instructions().append(0);

You could add a comment at the end of this line saying what this zero means.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1129
> +    //instructions().append(offset);

Remove.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1884
> +            //TODO: maybe emit expression info???

Space between // and TODO.  Only one ?.  Turn it into a sentence (i.e. capitalize Maybe).

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:195
> +                                                                 int in_startLine, int in_startColumn, int in_endLine, int in_endColumn, String* out_types) 

Bad indentation, you should make this line be only 4 spaces indented.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1471
> +    // put_to_scope_with_profile scope, id, value, ResolveModeAndType, Structure, Operand, TypeLocation*

Same comment as before - turn this into a sentence or remove it.  For example, you could make this say:

// The format of this instruction is: put_to_scope_with_profile scope, id, value, ResolveModeAndType, Structure, Operand, TypeLocation*

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.h:2
> + * Copyright (C) 2008, 2014 Apple Inc. All Rights Reserved.

Only say 2014.

> Source/JavaScriptCore/runtime/SymbolTable.h:419
> +        //use -1 as a flag to indicate we need to produce a unique ID because we need the VM to do this. 
> +        //When linking, we can produce this unique ID as we have access to VM

Complete sentences in comments, capitalize the first word in the sentence, and a space between // and use, and between // and When.

> Source/JavaScriptCore/runtime/SymbolTable.h:472
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:485
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:500
> -        
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:507
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:522
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/SymbolTable.h:527
> -    
> +

Revert.

> Source/JavaScriptCore/runtime/TypeSet.cpp:63
> +    else if (v.isGetterSetter())
> +        ret = TypeGetterSetter;

Remove.

> Source/JavaScriptCore/runtime/TypeSet.cpp:65
> +    //else if (v.isCustomGetterSetter())
> +    //    ret = TypeCustomGetterSetter;

Remove.

> Source/JavaScriptCore/runtime/TypeSet.cpp:74
> +//structure may be null

Space between // and structure, turn it into a sentence.  Or remove it.

> Source/JavaScriptCore/runtime/TypeSet.cpp:139
> +    if (m_seenTypes & TypeGetterSetter)
> +         seen.append("GetterSetter ");
> +    if (m_seenTypes & TypeCustomGetterSetter)
> +         seen.append("CustomGetterSetter ");
> +    //if (m_seenTypes & TypeObject)
> +    //     seen.append("Object ");

Remove.

> Source/JavaScriptCore/runtime/TypeSet.h:53
> +    class JSValue;
> +    class Structure;
> +
> +    enum RuntimeType {
> +        TypeNothing            = 0x0,
> +        TypeFunction           = 0x1,
> +        TypeUndefined          = 0x2,
> +        TypeNull               = 0x4,
> +        TypeBoolean            = 0x8,
> +        TypeMachineInt         = 0x10,
> +        TypeNumber             = 0x20,
> +        TypeString             = 0x40,
> +        TypePrimitive          = 0x80,
> +        TypeGetterSetter       = 0x100,
> +        TypeCustomGetterSetter = 0x200,
> +        TypeObject             = 0x400,
> +    };
> +

Unindent.

> Source/JavaScriptCore/runtime/TypeSet.h:54
> +

Remove this blank line.

> Source/JavaScriptCore/runtime/TypeSet.h:62
> +

Remove this blank line.

> Source/JavaScriptCore/runtime/VM.cpp:335
> +    //TODO: conditionally allocate all this stuff based on whether the inspector is running

Space between // and TODO.

> Source/JavaScriptCore/runtime/VM.cpp:943
> +    //printf("Getting type for variable given sourceID:'%ld' line:'%u'\n", sourceID, startLine);

Remove or guard with a verbose flag.

> Source/JavaScriptCore/runtime/VM.cpp:971
> +        printf("[Line, Column]::[%u, %u] Local:'%s' Global:'%s'\n", location->line, location->column, 
> +                                               profiler->getLocalTypesForVariableInRange(location->line, location->column, location->line, location->column, "", location->sourceID).ascii().data(),
> +                                               profiler->getGlobalTypesForVariableInRange(location->line, location->column, location->line, location->column, "", location->sourceID).ascii().data());

Use dataLog().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.