[Webkit-unassigned] [Bug 53299] Web Inspector: Use DIVs instead of TABLE in TextViewer

[bugzilla-daemon at webkit.org]

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


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80444|review?                     |review-
               Flag|                            |




--- Comment #2 from Pavel Feldman <pfeldman at chromium.org>  2011-01-31 07:44:50 PST ---
(From update of attachment 80444)
View in context: https://bugs.webkit.org/attachment.cgi?id=80444&action=review

This change needs further splitting. Otherwise, it is hard to follow.

> Source/WebCore/inspector/front-end/Settings.js:34
> +    sourceEditorEnabled: false,

Is this change relevant?

> Source/WebCore/inspector/front-end/SourceFrame.js:-212
> -        if (this._canEditScripts)

Where did this go?

> Source/WebCore/inspector/front-end/SourceFrame.js:853
> +        if (!Preferences.canEditScriptSource || !this._isScript || Preferences.sourceEditorEnabled)

I think it is too early to introduce sourceEditorEnable switch.

> Source/WebCore/inspector/front-end/TextViewer.js:-44
> -    this.element.addEventListener("beforecopy", this._beforeCopy.bind(this), false);

Where did these go?

> Source/WebCore/inspector/front-end/TextViewer.js:46
> +    var mainElm = this._mainPanel.element;

Do not abbreviate ("mainElement")

> Source/WebCore/inspector/front-end/TextViewer.js:47
> +    mainElm.addEventListener("scroll", this._syncScroll.bind(this), false);

It would be nice to encapsulate these since they serve same goal. Also, you might want to bind once.

> Source/WebCore/inspector/front-end/TextViewer.js:52
> +    // FIXME: Remove old live editing functionality and Preferences.sourceEditorEnabled flag.

ditto (too early)

> Source/WebCore/inspector/front-end/TextViewer.js:75
> +        this._mainPanel.addDecoration(lineNumber, decoration);

Why is this code getting more complex?

> Source/WebCore/inspector/front-end/TextViewer.js:84
> +        this._mainPanel.removeDecoration(lineNumber, decoration);

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:112
> +    _handleKeyDown: function()

Did this method change? It is nearly impossible to track changes in this diff. Can we please split it into a series of refactorings?

> Source/WebCore/inspector/front-end/TextViewer.js:236
> +//==================================================================================================

No comments like this in WebKit please.

> Source/WebCore/inspector/front-end/TextViewer.js:237
> +// WebInspector.TextEditorChunkedPanel

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:421
> +//==================================================================================================

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:458
> +//==================================================================================================

ditto

> Source/WebCore/inspector/front-end/TextViewer.js:-341
> -        var offset = 0;

Why did this change?

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