[webkit-reviews] review granted: [Bug 135142] Web Inspector: Create a UI for displaying JavaScript type information : [Attachment 237186] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 135142: Web Inspector: Create a UI for displaying JavaScript type
information
https://bugs.webkit.org/show_bug.cgi?id=135142

Attachment 237186: patch
https://bugs.webkit.org/attachment.cgi?id=237186&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=237186&action=review


r=me. A few questions worth considering.

> Source/WebInspectorUI/ChangeLog:26
> +	   This class is responsible for annotaing producing the inline

Typo: "annotaing" => "annotating"

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:63
> +    var showTypesImageSize = (WebInspector.Platform.isLegacyMacOS ? 15 :
16);

Style: No need for the ()s.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:155
> +	   if (this._typeTokenScrollHandler)
> +	       this._disableScrollEventsForTypeTokenAnnotator();

Is this necessary? I would expect that after "close" is called everything will
get garbage collected.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1278
> -	   this._popover = this._popover || new WebInspector.Popover(this);
> +	  
content.classList.add(WebInspector.SourceCodeTextEditor.PopoverDebuggerContentS
tyleClassName);
> +
> +	   if (this._popover)
> +	       this._dismissPopover();
> +	   this._popover = new WebInspector.Popover(this);

Is the dismiss and new-assignment needed? Or can we stick with the old code and
just reuse the popover?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1544
> +	   this._typeTokenScrollHandler =
this._makeTypeTokenScrollEventHandler();

Nit: console.assert(!this._typeTokenScrollHandler);

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1550
> +	   this.removeScrollHandler(this._typeTokenScrollHandler);

Nit: console.assert(this._typeTokenScrollHandler);