Bug 211656 – Streamline MarkupAccumulator to improve efficiency a bit

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Streamline MarkupAccumulator to improve efficiency a bit

[Darin Adler]

Created attachment 398910 [details]
Patch

[Darin Adler]

Comment on attachment 398910 [details]
Patch

Lost a quote mark. Oops.

[Darin Adler]

Created attachment 398925 [details]
Patch

[Darin Adler]

Created attachment 398926 [details]
Patch

[Anders Carlsson]

Comment on attachment 398926 [details]
Patch

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

> Source/WebCore/editing/MarkupAccumulator.h:75
> +    template<typename ...StringTypes> void append(StringTypes... strings) { m_markup.append(strings...); }

Any reason why StringTypes isn't a forwarding reference, and why you're not using std::forward in the call?

[Darin Adler]

Comment on attachment 398926 [details]
Patch

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

>> Source/WebCore/editing/MarkupAccumulator.h:75
>> +    template<typename ...StringTypes> void append(StringTypes... strings) { m_markup.append(strings...); }
> 
> Any reason why StringTypes isn't a forwarding reference, and why you're not using std::forward in the call?

No reason. I will do what you suggest.

[Darin Adler]

Also, I will need to fix the crash seen on mac-debug!

[Darin Adler]

Ah, got the null check wrong.

[Darin Adler]

Committed r261437: <https://trac.webkit.org/changeset/261437>

[Radar WebKit Bug Importer]

<rdar://problem/63054009>

[Sam Weinig]

Comment on attachment 398926 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:407
> +// Stopgap until C++20 adds std::ranges::reverse_view.
> +template<typename Collection> struct ReverseView {
> +    Collection& collection;
> +    decltype(collection.rbegin()) begin() const { return collection.rbegin(); }
> +    decltype(collection.rend()) end() const { return collection.rend(); }
> +    decltype(collection.size()) size() const { return collection.size(); }
> +    ReverseView(Collection& collection)
> +        : collection(collection)
> +    {
> +    }
> +};

We really should just import a copy of our own as we did with Optional and Variant.

That said, we actually have the equivalent (I think) functionality already by way of makeReversedRange() in <wtf/IteratorRange.h>.

[Darin Adler]

(In reply to Sam Weinig from comment #11)
> We really should just import a copy of our own as we did with Optional and
> Variant.

OK.

I was just working on replacing WTF::Optional with std::optional.

> That said, we actually have the equivalent (I think) functionality already
> by way of makeReversedRange() in <wtf/IteratorRange.h>.

I’ll try using that.

[Sam Weinig]

(In reply to Darin Adler from comment #12)
> (In reply to Sam Weinig from comment #11)
> > We really should just import a copy of our own as we did with Optional and
> > Variant.
> 
> OK.
> 
> I was just working on replacing WTF::Optional with std::optional.

Sure. That's the right thing to do now. All I meant that I we brought those in before we had a copy in the standard library we could use.

[Darin Adler]

(In reply to Sam Weinig from comment #13)
> (In reply to Darin Adler from comment #12)
> > I was just working on replacing WTF::Optional with std::optional.
> 
> Sure. That's the right thing to do now. All I meant that I we brought those
> in before we had a copy in the standard library we could use.

Sure, I understood that. I was sort of observing the flow of the tides in and back out.