[webkit-reviews] review granted: [Bug 235422] Web Inspector: Element tooltips in overlays should use same encodable/decodable Label type as grid overlays : [Attachment 450827] Patch v1.1 - Address review comments so far, rebase

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 10 14:19:16 PST 2022


Devin Rousso <drousso at apple.com> has granted 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 450827: Patch v1.1 - Address review comments so far, rebase

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 450827
  --> https://bugs.webkit.org/attachment.cgi?id=450827
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


More information about the webkit-reviews mailing list