[webkit-reviews] review canceled: [Bug 235422] Web Inspector: Element tooltips in overlays should use same encodable/decodable Label type as grid overlays : [Attachment 449676] Patch v1.0

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has canceled Patrick Angle
<pangle at apple.com>'s request for review:
Bug 235422: Web Inspector: Element tooltips in overlays should use same
encodable/decodable Label type as grid overlays
https://bugs.webkit.org/show_bug.cgi?id=235422

Attachment 449676: Patch v1.0

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




--- Comment #4 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 449676
  --> https://bugs.webkit.org/attachment.cgi?id=449676
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