Bug 69234 – Web Inspector: Factor out object properties popup

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mikhail Naganov]

The yellow onhover popup that is used in the debugger can be reused. Per pfeldman's comment to issue 61179, extracting it as a separate class. https://bugs.webkit.org/show_bug.cgi?id=61179#c15

[Mikhail Naganov]

Created attachment 109434 [details]
Patch

[Pavel Feldman]

Comment on attachment 109434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109434&action=review

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:45
> +    onPropertiesReceived: function(properties)

Client should override updateProperties instead.

> Source/WebCore/inspector/front-end/Popover.js:296
> +    panelElement.addEventListener("scroll", this.hidePopover.bind(this), true);

You should only react on the scroll events in the path to the root, right?

> Source/WebCore/inspector/front-end/Popover.js:327
> +                var section = new WebInspector.ObjectPropertiesSection(result);

Popover.js should not depend on ObjectPropertiesSection.js since it breaks partial front-end compilability.

> Source/WebCore/inspector/front-end/SourceFrame.js:-654
> -            this._popoverHelper.hidePopover();

Why did this change?

[Mikhail Naganov]

(In reply to comment #2)
> (From update of attachment 109434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109434&action=review
> 
> > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:45
> > +    onPropertiesReceived: function(properties)
> 
> Client should override updateProperties instead.
>

Done.
 
> > Source/WebCore/inspector/front-end/Popover.js:296
> > +    panelElement.addEventListener("scroll", this.hidePopover.bind(this), true);
> 
> You should only react on the scroll events in the path to the root, right?
>

You mean the bubbling phase? No. If scrolling is handled in the bubbling phase then what happens -- first, the page is scrolled, with the popup is left on its position, then the event is dispatched, and only then popup hides -- not pretty.

I moved this code from SourceFrame.js:295, 645-650.
 
> > Source/WebCore/inspector/front-end/Popover.js:327
> > +                var section = new WebInspector.ObjectPropertiesSection(result);
> 
> Popover.js should not depend on ObjectPropertiesSection.js since it breaks partial front-end compilability.
>

Extracted into a separate file.
 
> > Source/WebCore/inspector/front-end/SourceFrame.js:-654
> > -            this._popoverHelper.hidePopover();
> 
> Why did this change?

Because PopoverHelper also handles this event and does the same (disableOnClick is true):

    _mouseDown: function(event)
    {
        if (this._disableOnClick)
            this.hidePopover();

[Mikhail Naganov]

Created attachment 109532 [details]
Comments addressed

[Pavel Feldman]

Comment on attachment 109532 [details]
Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=109532&action=review

r- for .js + i need to apply this change. it is not clear to me whether this regresses anything :(

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:69384
> +					RelativePath="..\inspector\front-end\ObjectPopoverHelper.js.js"

.js.js -> .js

> Source/WebCore/inspector/front-end/SourceFrame.js:-653
> -        if (this._popoverHelper)

I'll need to apply the change to check whether something regresses.

[Pavel Feldman]

Comment on attachment 109532 [details]
Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=109532&action=review

> Source/WebCore/inspector/front-end/ObjectPopoverHelper.js:39
> +    _showObjectPopover: function(element, popover)

Btw, this nesting looks ugly. Could you extract the method below and call a bound one?

[Pavel Feldman]

Comment on attachment 109532 [details]
Comments addressed

Please extract method and fix vcproj.

[Mikhail Naganov]

(In reply to comment #7)
> (From update of attachment 109532 [details])
> Please extract method and fix vcproj.

OK. Comments addressed, will land.

[Mikhail Naganov]

Manually committed http://trac.webkit.org/changeset/96599

2011-10-04  Mikhail Naganov  <[e-mail]>

        Web Inspector: Factor out object properties popup.
        https://bugs.webkit.org/show_bug.cgi?id=69234

        Also, for HTML elements, show a non-empty id value in the element name.

        Reviewed by Pavel Feldman.

        * WebCore.gypi:
        * WebCore.vcproj/WebCore.vcproj:
        * inspector/front-end/ObjectPopoverHelper.js: Added.
        * inspector/front-end/SourceFrame.js: Extracted from here.
        (WebInspector.SourceFrame.prototype._initializeTextViewer):
        (WebInspector.SourceFrame.prototype._mouseDown):
        (WebInspector.SourceFrame.prototype._onShowPopover.showObjectPopover):
        (WebInspector.SourceFrame.prototype._onShowPopover):
        * inspector/front-end/WebKit.qrc:
        * inspector/front-end/inspector.html: