Bug 222006 – Web Inspector: Implement Grid Overlay "Show line names" drawing

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

[Patrick Angle]

Add drawing of line names to the grid overlay when enabled.

[Radar WebKit Bug Importer]

<rdar://problem/74409722>

[Patrick Angle]

Created attachment 420903 [details]
WIP v0.1 - Auto-repeat and implicit names not working

[Patrick Angle]

Created attachment 420904 [details]
Screenshot of WIP v0.1

[Patrick Angle]

Comment on attachment 420903 [details]
WIP v0.1 - Auto-repeat and implicit names not working

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

> Source/WebCore/inspector/InspectorOverlay.cpp:1340
> +    auto orderedGridLineNames = direction == GridTrackSizingDirection::ForColumns ? renderStyle->orderedNamedGridColumnLines() : renderStyle->orderedNamedGridRowLines();

A breadcrumb for future me: I think I need to copy these values, not reference them, otherwise I’m actually adding the other grid line name maps to this map.

[Patrick Angle]

Created attachment 420990 [details]
Patch v1.0

[Patrick Angle]

Created attachment 420991 [details]
Screenshot of Patch v1.0 (Explicit and Auto-repeat Line Names)

[Patrick Angle]

Created attachment 420992 [details]
Screenshot of Patch v1.0 (Implicit Line Names)

[Blaze Burg]

Comment on attachment 420990 [details]
Patch v1.0

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

r=me

For posterity, please attach a screenshot showing line numbers, and line numbers + labels.

> Source/WebCore/inspector/InspectorOverlay.cpp:1341
> +    auto addOrAppendLineNames = [&](unsigned index, Vector<String> newNames) {

It would be clearer as "appendLineNames" since it always does an append instead of a splice.

[Patrick Angle]

Created attachment 420998 [details]
Screenshot of Patch v1.0 (Line Numbers)

[Patrick Angle]

Created attachment 420999 [details]
Screenshot of Patch v1.0 (Line Numbers and Names)

[Patrick Angle]

Created attachment 421000 [details]
Patch v1.1 - Review note

[EWS]

Committed r273155: <https://commits.webkit.org/r273155>

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

[Darin Adler]

Comment on attachment 421000 [details]
Patch v1.1 - Review note

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

> Source/WebCore/inspector/InspectorOverlay.cpp:1341
> +    auto appendLineNames = [&](unsigned index, Vector<String> newNames) {

This copies the entire Vector and bumps the reference count on the all the strings. Should take const Vector<String>& instead for better efficiency.

> Source/WebCore/inspector/InspectorOverlay.cpp:1484
> +            lineLabelParts.append(String::number(i + 1));
> +            lineLabelParts.append(String::number(-static_cast<int>(columnPositions.size() - i)));

Using StringBuilder we can build up something like this without allocating and destroying temporary strings. But it might be tricky to refactor to take advantage of that.

> Source/WebCore/inspector/InspectorOverlay.cpp:1494
> +                lineLabel.append(thinSpace);
> +                lineLabel.append(bullet);
> +                lineLabel.append(thinSpace);
> +                lineLabel.append(lineLabelParts[i]);

This is a *very* inefficient way to build up a string. WTF has StringBuilder, which does this kind of thing far more efficiently, and also lets you append more than one thing with a single function call:

    lineLabel.append(thinSpace, bullet, thinSpace, lineLabelParts[I]);

We should probably consider removing the WTF::String::append function because it’s tempting to use it even though it is almost absurdly inefficient.

> Source/WebCore/inspector/InspectorOverlay.cpp:1560
> +        Vector<String> lineLabelParts;
>          if (gridOverlay.config.showLineNumbers) {
> +            lineLabelParts.append(String::number(i + 1));
> +            lineLabelParts.append(String::number(-static_cast<int>(rowPositions.size() - i)));
> +        }
> +        if (gridOverlay.config.showLineNames && rowLineNames.contains(i))
> +            lineLabelParts.appendVector(rowLineNames.get(i));
> +        if (lineLabelParts.size()) {
> +            auto lineLabel = lineLabelParts[0];
> +            for (size_t i = 1; i < lineLabelParts.size(); ++i) {
> +                lineLabel.append(thinSpace);
> +                lineLabel.append(bullet);
> +                lineLabel.append(thinSpace);
> +                lineLabel.append(lineLabelParts[i]);
> +            }

Same comments as above. Also, this code seems repetitive enough that we should find a way to use a helper function instead of use repeating all the code twice.

[Patrick Angle]

Comment on attachment 421000 [details]
Patch v1.1 - Review note

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:1560
>> +            }
> 
> Same comments as above. Also, this code seems repetitive enough that we should find a way to use a helper function instead of use repeating all the code twice.

Working to address these notes today with the exception of spinning the line label drawing code off into a helper; BJ and I discussed this internally and decided we felt it would make the drawing code more complex for only two repetitions.

[Patrick Angle]

Reopening to attach new patch.

[Patrick Angle]

Created attachment 421050 [details]
Followup (r273155) v1.0 - String performance notes

[Blaze Burg]

Comment on attachment 421050 [details]
Followup (r273155) v1.0 - String performance notes

r=me

[EWS]

Committed r273185: <https://commits.webkit.org/r273185>

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

[Darin Adler]

Comment on attachment 421000 [details]
Patch v1.1 - Review note

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

>>> Source/WebCore/inspector/InspectorOverlay.cpp:1560
>>> +            }
>> 
>> Same comments as above. Also, this code seems repetitive enough that we should find a way to use a helper function instead of use repeating all the code twice.
> 
> Working to address these notes today with the exception of spinning the line label drawing code off into a helper; BJ and I discussed this internally and decided we felt it would make the drawing code more complex for only two repetitions.

After you land, I’ll submit a patch that shows you how this can be done in a way that makes the code easier to understand, not more complex. You are welcome to land it like this.