Bug 69234

Summary: Web Inspector: Factor out object properties popup
Product: WebKit Reporter: Mikhail Naganov <mnaganov>
Component: Web Inspector (Deprecated)Assignee: Mikhail Naganov <mnaganov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 61179    
Attachments:
Description Flags
Patch
pfeldman: review-, mnaganov: commit-queue-
Comments addressed pfeldman: review+, mnaganov: commit-queue-

Description Mikhail Naganov 2011-10-02 20:55:47 PDT
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
Comment 1 Mikhail Naganov 2011-10-02 21:01:36 PDT
Created attachment 109434 [details]
Patch
Comment 2 Pavel Feldman 2011-10-03 02:05:58 PDT
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?
Comment 3 Mikhail Naganov 2011-10-03 14:38:46 PDT
(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();
Comment 4 Mikhail Naganov 2011-10-03 14:46:19 PDT
Created attachment 109532 [details]
Comments addressed
Comment 5 Pavel Feldman 2011-10-04 04:16:37 PDT
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.
Comment 6 Pavel Feldman 2011-10-04 07:42:37 PDT
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?
Comment 7 Pavel Feldman 2011-10-04 07:47:03 PDT
Comment on attachment 109532 [details]
Comments addressed

Please extract method and fix vcproj.
Comment 8 Mikhail Naganov 2011-10-04 09:04:37 PDT
(In reply to comment #7)
> (From update of attachment 109532 [details])
> Please extract method and fix vcproj.

OK. Comments addressed, will land.
Comment 9 Mikhail Naganov 2011-10-04 09:18:26 PDT
Manually committed http://trac.webkit.org/changeset/96599

2011-10-04  Mikhail Naganov  <mnaganov@chromium.org>

        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: