[webkit-reviews] review denied: [Bug 75454] Web Inspector: Add utility changes for Spectrum colorpicker : [Attachment 121132] Patch to provide utility functionality necessary for the Spectrum colorpicker implementation (Update 1)

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Brian Grinstead
<briangrinstead at gmail.com>'s request for review:
Bug 75454: Web Inspector: Add utility changes for Spectrum colorpicker
https://bugs.webkit.org/show_bug.cgi?id=75454

Attachment 121132: Patch to provide utility functionality necessary for the
Spectrum colorpicker implementation (Update 1)
https://bugs.webkit.org/attachment.cgi?id=121132&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121132&action=review


> Source/WebCore/inspector/front-end/Popover.js:117
> +	   const scrollerWidth = this._hideOverflow ? 0 : 11;

Nit: contants should not be conditional: scrollerWidth is always 11, you should
use ? in the call site.

> Source/WebCore/inspector/front-end/Popover.js:124
> +	   const totalWidth = this._parentElement ?
this._parentElement.clientWidth : window.innerWidth;

this._parentElement is always be there, no need to check.

> Source/WebCore/inspector/front-end/Popover.js:130
> +	   // If a parent element has been specified, position within that
element instead

Now that I looked at the runtime, I think this is wrong. We never want popovers
to be limited with the ancestor elements. When there is not enough real estate,
we'd like them to be rendered say "to the left" of the anchor, which is outside
the styles sidebar. So you should not need this change. Sorry for bringing it
up so late.

> Source/WebCore/inspector/front-end/Popover.js:135
> +	       console.log(relativeOffset, anchorBox.x, anchorBox.y)

No logging should be used in the production.

> Source/WebCore/inspector/front-end/utilities.js:911
> +function stopPropagation(e) {

{ should be on the next line