Bug 235422 – Web Inspector: Element tooltips in overlays should use same encodable/decodable Label type as grid overlays

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

Element tooltips are currently their own separate implementation of a tooltip/label from the ones we implemented for Grid overlays. We should share this implementation as we work towards further unifying the overlay drawing code paths across platforms.

[Radar WebKit Bug Importer]

<rdar://problem/87853106>

[Patrick Angle]

Created attachment 449676 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 449676 [details]
Patch v1.0

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

> Source/WebCore/inspector/InspectorOverlay.cpp:1106
> +    return InspectorOverlayLabel(WTFMove(labelContents), { labelX, labelY }, elementTitleBackgroundColor, arrowDirection, arrowEdgePosition).draw(context);

NIT: I'd make the `InspectorOverlayLabel` into a local so it's clearer that the return value of this function is the return value of `draw`.

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:49
> +

Style: unnecessary newline

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:54
> +: InspectorOverlayLabel({ Content(text) }, location, backgroundColor, arrowDirection, arrowEdgePosition)

Style: missing indent

> Source/WebCore/inspector/InspectorOverlayLabel.h:45
> +    enum class ArrowDirection {

```
enum class ArrowDirection : uint8_t {
```

> Source/WebCore/inspector/InspectorOverlayLabel.h:53
> +    enum class ArrowEdgePosition {

ditto (:45)

NIT: Rather than have this be two separate `enum class`, maybe we could combine them into a `struct`?
```
struct Arrow {
    enum class Direction : uint8_t {
        ...
    };

    enum class Alignment : uint8_t {
        ...
    };

    Direction direction;
    Alignment alignment;
};
```
This way, it's only one contained argument to provide to `InspectorOverlayLabel` instead of two related ones.

> Source/WebCore/inspector/InspectorOverlayLabel.h:64
> +        String text;
> +        Color textColor;

Can we make these `const`?  Or are they reassigned somewhere?

> Source/WebCore/inspector/InspectorOverlayLabel.h:66
> +        Content(String text, Color textColor = Color::black)

`const String&`?  `String&&`?
`const Color&`?  `Color&&`?

Also, is this actually necessary?  It seems like you're always instantiating with aggregate initialization instead of explicitly using this constructor.  I think it might be better to require all callsites to explicitly provide a `Color` instead of having this implicit behavior.

> Source/WebCore/inspector/InspectorOverlayLabel.h:106
> +    encoder << static_cast<uint32_t>(m_arrowDirection);

I don't think we want to do this.  You should be able to add support for encoding/decoding `enum class` by adding the following at the bottom of this file
```
namespace WTF {

template<> struct EnumTraits<WebCore::InspectorOverlayLabel::ArrowDirection> {
    using values = EnumValues<
        WebCore::InspectorOverlayLabel::ArrowDirection,
        WebCore::InspectorOverlayLabel::ArrowDirection::None,
        WebCore::InspectorOverlayLabel::ArrowDirection::Down,
        WebCore::InspectorOverlayLabel::ArrowDirection::Up,
        WebCore::InspectorOverlayLabel::ArrowDirection::Left,
        WebCore::InspectorOverlayLabel::ArrowDirection::Right
    >;
};

} // namespace WTF

```

> Source/WebCore/inspector/InspectorOverlayLabel.h:107
> +    encoder << static_cast<uint32_t>(m_arrowEdgePosition);

ditto (:106)

> Source/WebCore/inspector/InspectorOverlayLabel.h:114
> +    Vector<Content> contents;
> +    if (!decoder.decode(contents))
> +        return { };

Can we use the new style of decoding?
```
std::optional<Vector<Content>> contents;
decoder >> contents;
if (!contents)
    return std::nullopt;

...

return { {
    WTFMove(*contents),
    ...
} };
```

ditto below

[Patrick Angle]

Comment on attachment 449676 [details]
Patch v1.0

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

Thanks for the comments! As mentioned offline I'll also prepare a diff of the label drawing code to make that comparison easier as well as write up more detailed changelog info for those functions. I also clearly need to rebase to appease EWS.

>> Source/WebCore/inspector/InspectorOverlayLabel.h:53
>> +    enum class ArrowEdgePosition {
> 
> ditto (:45)
> 
> NIT: Rather than have this be two separate `enum class`, maybe we could combine them into a `struct`?
> ```
> struct Arrow {
>     enum class Direction : uint8_t {
>         ...
>     };
> 
>     enum class Alignment : uint8_t {
>         ...
>     };
> 
>     Direction direction;
>     Alignment alignment;
> };
> ```
> This way, it's only one contained argument to provide to `InspectorOverlayLabel` instead of two related ones.

That would work. Although the static `InspectorOverlayLabel::expectedSize()` only needs a direction, not an alignment since where along the edge the arrow is doesn't effect the overall shape of the label, which iirc is why these were separate previously. But maybe that method should get both anyways in case in the future it does affect sizing.

>> Source/WebCore/inspector/InspectorOverlayLabel.h:64
>> +        Color textColor;
> 
> Can we make these `const`?  Or are they reassigned somewhere?

They are not reassigned, so yeah `const` is a good idea here

>> Source/WebCore/inspector/InspectorOverlayLabel.h:66
>> +        Content(String text, Color textColor = Color::black)
> 
> `const String&`?  `String&&`?
> `const Color&`?  `Color&&`?
> 
> Also, is this actually necessary?  It seems like you're always instantiating with aggregate initialization instead of explicitly using this constructor.  I think it might be better to require all callsites to explicitly provide a `Color` instead of having this implicit behavior.

I was almost ready to say yes, but only three call sites rely on the implicit color (2x in InspectorOverlay::drawElementTitle, and 1x in InspectorOverlayLabel::InspectorOverlayLabel when a String is provided instead of `Content`s), so probably best just to explicitly set a color at those call sites instead, yeah

[Patrick Angle]

Created attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

[Patrick Angle]

Created attachment 450828 [details]
Diff of Moved Drawing Code

This diff shows the old InspectorOverlay methods for drawing diffed against the new InspectorOverlayLabel methods that replace them, since there are changes in their implementation.

[Devin Rousso]

Comment on attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

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

r=mews, neat stuff!  Thanks for uploading the other diff :)

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:43
> +InspectorOverlayLabel::InspectorOverlayLabel(Vector<Content>&& contents, const FloatPoint& location, const Color& backgroundColor, Arrow arrow)

NIT: I realize that it's a `struct` of two `enum`, but it's probably a good idea to make all these `const Arrow&` in case a new member is added.

ditto below

> Source/WebCore/inspector/InspectorOverlayLabel.cpp:221
> +            auto text = lines.at(i);

NIT: Any reason not to just `[i]` instead?

ditto below

[Patrick Angle]

I'm pretty sure this change will need rebased on top of Bug 236465 once it lands. In addition to Devin's review notes I also forgot to add the new header to the non-Xcode build file so that windows/gtk/wpe build.

[Patrick Angle]

Created attachment 451791 [details]
[fast-cq] Patch v1.2 - Address review nits, fix non-Cocoa builds

[Darin Adler]

Comment on attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

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

>> Source/WebCore/inspector/InspectorOverlayLabel.cpp:43
>> +InspectorOverlayLabel::InspectorOverlayLabel(Vector<Content>&& contents, const FloatPoint& location, const Color& backgroundColor, Arrow arrow)
> 
> NIT: I realize that it's a `struct` of two `enum`, but it's probably a good idea to make all these `const Arrow&` in case a new member is added.
> 
> ditto below

I would literally say the opposite. We should stop doing const& on everything by default, and only do it when needed.

[Patrick Angle]

Comment on attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

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

>>> Source/WebCore/inspector/InspectorOverlayLabel.cpp:43
>>> +InspectorOverlayLabel::InspectorOverlayLabel(Vector<Content>&& contents, const FloatPoint& location, const Color& backgroundColor, Arrow arrow)
>> 
>> NIT: I realize that it's a `struct` of two `enum`, but it's probably a good idea to make all these `const Arrow&` in case a new member is added.
>> 
>> ditto below
> 
> I would literally say the opposite. We should stop doing const& on everything by default, and only do it when needed.

What are the disadvantages to these parameters being `const&` here? Is it that (as opposed to passing by value) we could theoretically be modifying the passed value if some aspect of it was mutable even when `const`, or am I perhaps missing another aspect here? Also, if we don't normally want to be using `const&` here, is there some kind of threshold we should consider (large memory footprint, etc?) to switch from parameters being values to being `const&`?

[Darin Adler]

Comment on attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

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

>>>> Source/WebCore/inspector/InspectorOverlayLabel.cpp:43
>>>> +InspectorOverlayLabel::InspectorOverlayLabel(Vector<Content>&& contents, const FloatPoint& location, const Color& backgroundColor, Arrow arrow)
>>> 
>>> NIT: I realize that it's a `struct` of two `enum`, but it's probably a good idea to make all these `const Arrow&` in case a new member is added.
>>> 
>>> ditto below
>> 
>> I would literally say the opposite. We should stop doing const& on everything by default, and only do it when needed.
> 
> What are the disadvantages to these parameters being `const&` here? Is it that (as opposed to passing by value) we could theoretically be modifying the passed value if some aspect of it was mutable even when `const`, or am I perhaps missing another aspect here? Also, if we don't normally want to be using `const&` here, is there some kind of threshold we should consider (large memory footprint, etc?) to switch from parameters being values to being `const&`?

There are two disadvantages of using const&:

1) Poorer performance. By passing a small structure as const& we prevent it from being efficiently passed in the normal fashion, typically in registers, and instead pass a pointer to it. For StringView we measured this as slower, so if we used const StringView& we got slower code than just StringView.

2) Lifetime trickiness. This is a very small disadvantage, but it’s possible in some cases to pass a reference to something that is deleted.

Here is the threshold I think we should consider for moving from passing by value to const&:

1) Anything that involves reference counting, non-trivial copy constructors and destructors. In that case const& is almost certainly more efficient, and we also consider moving with && sometimes too.

2) Even if no non-trivial copy/destroy operation is is involved, a structure that is much larger than a pointer, say as big as three or four pointers, is big enough that passing const& may help performance. Since we are mostly optimizing for 64-bit systems and typically 32-bit integers, that roughly means structures that contain 3 or 4 pointers, or 6 or more int-sized things. If we are taking 8-bit enumerations, we’d need to have like 16 or more.

[Patrick Angle]

Comment on attachment 450827 [details]
Patch v1.1 - Address review comments so far, rebase

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

>>>>> Source/WebCore/inspector/InspectorOverlayLabel.cpp:43
>>>>> +InspectorOverlayLabel::InspectorOverlayLabel(Vector<Content>&& contents, const FloatPoint& location, const Color& backgroundColor, Arrow arrow)
>>>> 
>>>> NIT: I realize that it's a `struct` of two `enum`, but it's probably a good idea to make all these `const Arrow&` in case a new member is added.
>>>> 
>>>> ditto below
>>> 
>>> I would literally say the opposite. We should stop doing const& on everything by default, and only do it when needed.
>> 
>> What are the disadvantages to these parameters being `const&` here? Is it that (as opposed to passing by value) we could theoretically be modifying the passed value if some aspect of it was mutable even when `const`, or am I perhaps missing another aspect here? Also, if we don't normally want to be using `const&` here, is there some kind of threshold we should consider (large memory footprint, etc?) to switch from parameters being values to being `const&`?
> 
> There are two disadvantages of using const&:
> 
> 1) Poorer performance. By passing a small structure as const& we prevent it from being efficiently passed in the normal fashion, typically in registers, and instead pass a pointer to it. For StringView we measured this as slower, so if we used const StringView& we got slower code than just StringView.
> 
> 2) Lifetime trickiness. This is a very small disadvantage, but it’s possible in some cases to pass a reference to something that is deleted.
> 
> Here is the threshold I think we should consider for moving from passing by value to const&:
> 
> 1) Anything that involves reference counting, non-trivial copy constructors and destructors. In that case const& is almost certainly more efficient, and we also consider moving with && sometimes too.
> 
> 2) Even if no non-trivial copy/destroy operation is is involved, a structure that is much larger than a pointer, say as big as three or four pointers, is big enough that passing const& may help performance. Since we are mostly optimizing for 64-bit systems and typically 32-bit integers, that roughly means structures that contain 3 or 4 pointers, or 6 or more int-sized things. If we are taking 8-bit enumerations, we’d need to have like 16 or more.

That makes a lot of sense. Thank you for the explanation!

[Patrick Angle]

Created attachment 451911 [details]
[fast-cq] Patch v1.3 - Address additional feedback

[EWS]

Committed r289769 (247241@main): <https://commits.webkit.org/247241@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451911 [details].