Bug 69234 - Web Inspector: Factor out object properties popup
Summary: Web Inspector: Factor out object properties popup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks: 61179
  Show dependency treegraph
 
Reported: 2011-10-02 20:55 PDT by Mikhail Naganov
Modified: 2011-10-04 09:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.41 KB, patch)
2011-10-02 21:01 PDT, Mikhail Naganov
pfeldman: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Comments addressed (12.35 KB, patch)
2011-10-03 14:46 PDT, Mikhail Naganov
pfeldman: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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: