[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 #7 from Andrey Adaikin <aandrey at google.com>  2011-02-02 03:09:57 PST ---
(From update of attachment 80754)
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.

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