[Webkit-unassigned] [Bug 75454] Web Inspector: Add utility changes for Spectrum colorpicker

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=75454


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121132|review?                     |review-
               Flag|                            |




--- Comment #9 from Pavel Feldman <pfeldman at chromium.org>  2012-01-05 01:44:28 PST ---
(From update of attachment 121132)
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

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