[webkit-reviews] review requested: [Bug 31207] Web Inspector: Datatip should be a tree : [Attachment 42693] proposed patch 2

[bugzilla-daemon at webkit.org]

Keishi Hattori <casey.hattori at gmail.com> has asked  for review:
Bug 31207: Web Inspector: Datatip should be a tree
https://bugs.webkit.org/show_bug.cgi?id=31207

Attachment 42693: proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=42693&action=review

------- Additional Comments from Keishi Hattori <casey.hattori at gmail.com>
(In reply to comment #10)

Thanks for taking a look.

> 1) Please add new js file into WebCore/WebCore.gypi and
> WebCore.vcproj/WebCore.vcproj

Done.

> 2) You should layout each subsequent expansion separately so that say third
> expansion would expand to 'above' the anchor if needed.
> 3) You don't limit size of the tip with visible area and provide no scrollers

> when tip does not fit
> 4) If user moves mouse around and expands things, he gets them overlapping
> (will attach screen). You should only show single 'path' to a property. Even
> further - you should remove each 'tip' in the path on mouse leaving it.

Subclassed ObjectPropertiesSection so it expands one property at a time and
positions the element within the window.

> I also don't like that we don't reuse Popup's code that was positioning wrt
> anchor. Even though you don't need Popup behavior, you could try reusing
popup

Popup and Datatip have different behavior and different anchor points so not
much of the code can be shared.

> positioning. When we were doing Popup, we decided to use object with named
> properties { x, y, width, height } for rect, not the class. Not that I liked
> the idea, but now we definitely have a big overlap in utilities around rect
> intersections, etc in different terms. Please merge / reuse code either way.
> Ideal way would be to extract positioning from popup, migrate it to rect and
> reuse in both places? We can do it later while we polish the datatip though,
so
> FIXME should be enough though.

I think a Rect class is really useful used in combination with
getBoundingClientRect(), so I added a FIXME comment to rewrite Popup.