Bug 53299 – Web Inspector: Use DIVs instead of TABLE in TextViewer

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Adaikin]

Migrate TextViewer from TABLE to DIV.
  TextViewer is now build on top of a TABLE, so that one row contains the gutter's line number and the text content.
  Pros: gutter and content rows' heights are always in sync.
  Cons: gutter disappears when you scroll from left to the right, empty TABLE rows are not selected in the UI and native copy of a text selection to the clipboard will contain gutter's line numbers (we intercept copy and beforecopy events to prevent that).
  We should migrate from TABLE to DIV and decouple gutter and main panels. Things to take into account:
  - Implement on top of TextViewer, hide new editor functionality under a flag.
  - Live Editing should still work.
  - Sync gutter and content rows' heights.
  - Should be no significant performance regression.

[Andrey Adaikin]

Created attachment 80444 [details]
Patch

[Pavel Feldman]

Comment on attachment 80444 [details]
Patch

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?

[Andrey Adaikin]

Created attachment 80734 [details]
Patch

[Andrey Adaikin]

Comment on attachment 80444 [details]
Patch

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

[Andrey Adaikin]

Created attachment 80754 [details]
Patch

[Pavel Feldman]

Comment on attachment 80754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80754&action=review

Few minor nits, otherwise looks good.

> Source/WebCore/inspector/front-end/TextViewer.js:41
>      this.element.addEventListener("keydown", this._handleKeyDown.bind(this), false);

Why is this registered not in the this._mainPanel?

> Source/WebCore/inspector/front-end/TextViewer.js:48
> +    this._mainPanel.syncScrollListener = this._syncScroll.bind(this);

Can we pass these as constructor parameters instead? (they are not re-assignable, right?)

> Source/WebCore/inspector/front-end/TextViewer.js:166
> +        if (lineNumber >= this._textModel.linesCount)

Can this actually happen?

> Source/WebCore/inspector/front-end/TextViewer.js:572
> +    this.element.addEventListener("DOMCharacterDataModified", this._handleDomUpdates.bind(this), false);

should be handleDOMUpdates. Again, do you want to reuse the bound instance?

> Source/WebCore/inspector/front-end/TextViewer.js:898
> +                this._syncDecorationsForLineListener(lineRow.lineNumber);

This does not look robust. Are you sure you need it?

> Source/WebCore/inspector/front-end/TextViewer.js:930
> +        this.element.appendChild(document.createElement("br"));

Will this affect drag'n'drop / copy?

[Andrey Adaikin]

Comment on attachment 80754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80754&action=review

>> Source/WebCore/inspector/front-end/TextViewer.js:41
>>      this.element.addEventListener("keydown", this._handleKeyDown.bind(this), false);
> 
> Why is this registered not in the this._mainPanel?

done. also, seems like this event can be dropped out as well (not doing this in the CL)

>> Source/WebCore/inspector/front-end/TextViewer.js:48
>> +    this._mainPanel.syncScrollListener = this._syncScroll.bind(this);
> 
> Can we pass these as constructor parameters instead? (they are not re-assignable, right?)

done

>> Source/WebCore/inspector/front-end/TextViewer.js:166
>> +        if (lineNumber >= this._textModel.linesCount)
> 
> Can this actually happen?

theoretically yes, provided that this method is called asynchronously

>> Source/WebCore/inspector/front-end/TextViewer.js:572
>> +    this.element.addEventListener("DOMCharacterDataModified", this._handleDomUpdates.bind(this), false);
> 
> should be handleDOMUpdates. Again, do you want to reuse the bound instance?

http://webkit.org/coding/coding-style.html: Lower-case the first letter, including all letters in an acronym, in a variable or function name.

reused the bound instance.

>> Source/WebCore/inspector/front-end/TextViewer.js:898
>> +                this._syncDecorationsForLineListener(lineRow.lineNumber);
> 
> This does not look robust. Are you sure you need it?

Yes. I added the following comment:
  // Wait until this event is processed and only then sync the sizes. This is necessary in
  // case of the DOMNodeRemoved event, because it is dispatched before the removal takes place.
(see http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMNodeRemoved)

>> Source/WebCore/inspector/front-end/TextViewer.js:930
>> +        this.element.appendChild(document.createElement("br"));
> 
> Will this affect drag'n'drop / copy?

Nope. In fact, this is exactly what the browser would do, if you make an empty line in a content editable DIVs.

[Andrey Adaikin]

Created attachment 80907 [details]
Patch

[WebKit Review Bot]

Attachment 80907 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

>>>> should be handleDOMUpdates. Again, do you want to reuse the bound instance?

>> http://webkit.org/coding/coding-style.html: Lower-case the first letter,
>> including all letters in an acronym, in a variable or function name.

should be:
handleDOMUpdates or
domUpdatesHandler

I think your quote refers to the second case.

[Andrey Adaikin]

(In reply to comment #10)
> >>>> should be handleDOMUpdates. Again, do you want to reuse the bound instance?
> 
> >> http://webkit.org/coding/coding-style.html: Lower-case the first letter,
> >> including all letters in an acronym, in a variable or function name.
> 
> should be:
> handleDOMUpdates or
> domUpdatesHandler
> 
> I think your quote refers to the second case.

done

[Andrey Adaikin]

Created attachment 80911 [details]
Patch

[WebKit Review Bot]

Attachment 80911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Podivilov]

Committed r77375: <http://trac.webkit.org/changeset/77375>