[webkit-reviews] review denied: [Bug 182471] Web Inspector: Styles: Control-Space should show completion popover : [Attachment 333059] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Nikita Vasilyev <nvasilyev at apple.com>'s
request for review:
Bug 182471: Web Inspector: Styles: Control-Space should show completion popover
https://bugs.webkit.org/show_bug.cgi?id=182471

Attachment 333059: Patch

https://bugs.webkit.org/attachment.cgi?id=333059&action=review




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 333059
  --> https://bugs.webkit.org/attachment.cgi?id=333059
Patch

I don't like your changes. Why thread acceptEmptyPrefix everywhere? It would be
better to make a different factory method on WI.CSSKeywordCompletions that
returns an object with all completions, and use it.

This is not possible right now because completionProvider is a function, not a
delegate/object. I don't understand the completionProvider design. It seems
like this should just be a different delegate that provides completion objects
to various widgets. As it's written now, it's just a callback so it can't have
different methods (ie., completionsForProperty(), completionsForValue()). Not
only does this make the code hard to read (i.e., what does
this._completionProvider() actually do?) but it scatters the responsibility for
figuring out what type completions to provide. It should be obvious at the
calcite.