[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





--- Comment #4 from Andrey Adaikin <aandrey at google.com>  2011-02-01 03:11:26 PST ---
(From update of attachment 80444)
View in context: https://bugs.webkit.org/attachment.cgi?id=80444&action=review

>> Source/WebCore/inspector/front-end/Settings.js:34
>> +    sourceEditorEnabled: false,
> 
> Is this change relevant?

I removed this flag for now

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

This is a dead code, I just could not pass by :) revert it?

>> 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.

reverted

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

We do not have to override beforecopy/copy now - the native copy works just fine. It did not before, because we had TABLE and text selection would include line numbers as well. The only thing is decorations: they also will be included in the text selection, if they are selected (although I use CSS style "-webkit-user-selection: none"). Is this OK?

>> Source/WebCore/inspector/front-end/TextViewer.js:46
>> +    var mainElm = this._mainPanel.element;
> 
> Do not abbreviate ("mainElement")

done

>> 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.

done

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

done

>> Source/WebCore/inspector/front-end/TextViewer.js:75
>> +        this._mainPanel.addDecoration(lineNumber, decoration);
> 
> Why is this code getting more complex?

Because, for example, we want to add the "webkit-execution-line" class name to both gutter and main panel lines/chunks.

>> 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?

this particular method did not change

>> Source/WebCore/inspector/front-end/TextViewer.js:236
>> +//==================================================================================================
> 
> No comments like this in WebKit please.

done

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

done

>> Source/WebCore/inspector/front-end/TextViewer.js:-341
>> -        var offset = 0;
> 
> Why did this change?

This was changed only slightly and still preserved in the _repaintAll method (see above).

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