[webkit-reviews] review denied: [Bug 65511] Web Inspector: autocomplete combobox for CSS style property names/values. : [Attachment 104048] [PATCH] Comments addressed
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 16 10:58:33 PDT 2011
Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 65511: Web Inspector: autocomplete combobox for CSS style property
names/values.
https://bugs.webkit.org/show_bug.cgi?id=65511
Attachment 104048: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=104048&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104048&action=review
Too much copypaste.
> Source/WebCore/inspector/front-end/StylesSidebarPane.js:930
> + if (!curSection) {
Could you submit this as a separate change?
> Source/WebCore/inspector/front-end/StylesSidebarPane.js:952
> + if (!curSection) {
same other change
> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1139
> + this._selectorElement.scrollIntoViewIfNeeded(false);
same other change
> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2047
> + else if (moveDirection === "backward") {
same other change
> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2248
> + //this._clearAutoComplete();
Should this code be commented?
> Source/WebCore/inspector/front-end/SuggestBox.js:32
> + this.parentElement = inputElement.ownerDocument.body;
Please make these fields private
> Source/WebCore/inspector/front-end/SuggestBox.js:43
> + window.addEventListener("scroll", this._boundOnScroll, true);
No need for scroll / resize listeners.
> Source/WebCore/inspector/front-end/SuggestBox.js:85
> + const anchorBox = this.inputElement.boxInWindow(window,
this.parentElement);
Could you style it using CSS instead?
> Source/WebCore/inspector/front-end/SuggestBox.js:135
> + if (event.handled)
event.handled is not standard. how can this happen? The one setting it should
stop propagation instead as you do below.
> Source/WebCore/inspector/front-end/SuggestBox.js:146
> + this.tabKeyPressed(event);
These all should be private.
> Source/WebCore/inspector/front-end/SuggestBox.js:192
> + this.inputElement.pruneEmptyTextNodes();
This looks a lot like copypaste from the TextPrompt.
> Source/WebCore/inspector/front-end/SuggestBox.js:222
> + acceptAutoComplete: function(event)
This method looks exactly as the one in the TextPrompt. I think the way you
should implement it is to extend TextPrompt instead. It could be TextPrompt +
drop box.
> Source/WebCore/inspector/front-end/SuggestBox.js:378
> + _completionsReady: function(selection, completions)
Again giant copypaste?
More information about the webkit-reviews
mailing list