Add drawing of line names to the grid overlay when enabled.
<rdar://problem/74409722>
Created attachment 420903 [details] WIP v0.1 - Auto-repeat and implicit names not working
Created attachment 420904 [details] Screenshot of WIP v0.1
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.
Created attachment 420990 [details] Patch v1.0
Created attachment 420991 [details] Screenshot of Patch v1.0 (Explicit and Auto-repeat Line Names)
Created attachment 420992 [details] Screenshot of Patch v1.0 (Implicit Line Names)
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.
Created attachment 420998 [details] Screenshot of Patch v1.0 (Line Numbers)
Created attachment 420999 [details] Screenshot of Patch v1.0 (Line Numbers and Names)
Created attachment 421000 [details] Patch v1.1 - Review note
Committed r273155: <https://commits.webkit.org/r273155> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421000 [details].
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.
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.
Reopening to attach new patch.
Created attachment 421050 [details] Followup (r273155) v1.0 - String performance notes
Comment on attachment 421050 [details] Followup (r273155) v1.0 - String performance notes r=me
Committed r273185: <https://commits.webkit.org/r273185> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421050 [details].
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.