[webkit-reviews] review granted: [Bug 33001] Web Inspector: Migrate to canvas-based text viewer / editor that scales. : [Attachment 46041] [PATCH] Same with support for CJK.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 33001: Web Inspector: Migrate to canvas-based text viewer / editor that
scales.
https://bugs.webkit.org/show_bug.cgi?id=33001

Attachment 46041: [PATCH] Same with support for CJK.
https://bugs.webkit.org/attachment.cgi?id=46041&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    setText: function(text)

Can this be a getter and setter? Why only a setter?


> +	       console.log("Repaint %d:%d", firstLine, lastLine);

I think we have WebInspector.log which will log to the current inspector's
console to make it easy to see.


> +	   this._replaceSelectionWith(e.clipboardData.getData("Text"));

What happens when there is no Text on the pastboard? Like an image. Maybe it
should bail and not replace the selection with nothing.


> +	   this._isMac = navigator.userAgent.indexOf("Mac OS") !== -1;
> +	   this._isWin = navigator.userAgent.indexOf("Windows") !== -1;
> +	   this._isLinux = navigator.userAgent.indexOf("Linux") !== -1;

It is good the editor is standalone, but I would like to see this use
WebInspector.platform. These classes already require the WebInspector object ro
exist for namespace.


> +	       else if (this._isLinux)
> +		   this._font = "10px Droid Sans Mono";

Does this ship with all Linux platforms? I thought this was a Google font.


> +WebInspector.TextCursor = function(cursorElement)
> +{
> +    this._visible = false;
> +    this._cursorElement = cursorElement;
> +}

Why do you use a DOM element for this? Why paint it in the canvas?


> +WebInspector.TextEditorModel.prototype = {
> +
> +    linesCount: function()

Remove extra empty line. Use a getter?


> +    columnsCount: function()

Getter?