Bug 211656 - Streamline MarkupAccumulator to improve efficiency a bit
Summary: Streamline MarkupAccumulator to improve efficiency a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-08 19:27 PDT by Darin Adler
Modified: 2020-05-09 16:34 PDT (History)
28 users (show)

See Also:

Attachments
Patch (40.44 KB, patch)
2020-05-08 19:49 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (40.53 KB, patch)
2020-05-09 06:35 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (40.52 KB, patch)
2020-05-09 07:30 PDT, Darin Adler 		andersca: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-05-08 19:27:26 PDT 
Streamline MarkupAccumulator to improve efficiency a bit
Comment 1 Darin Adler 2020-05-08 19:49:07 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-05-08 23:47:28 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-05-09 06:35:40 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-05-09 07:30:25 PDT 
Created attachment 398926 [details]
Patch
Comment 5 Anders Carlsson 2020-05-09 08:55:14 PDT 
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?
Comment 6 Darin Adler 2020-05-09 08:55:54 PDT 
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.
Comment 7 Darin Adler 2020-05-09 08:56:28 PDT 
Also, I will need to fix the crash seen on mac-debug!
Comment 8 Darin Adler 2020-05-09 08:57:55 PDT 
Ah, got the null check wrong.
Comment 9 Darin Adler 2020-05-09 09:04:05 PDT 
Committed r261437: <https://trac.webkit.org/changeset/261437>
Comment 10 Radar WebKit Bug Importer 2020-05-09 09:05:17 PDT 
<rdar://problem/63054009>
Comment 11 Sam Weinig 2020-05-09 15:48:12 PDT 
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>.
Comment 12 Darin Adler 2020-05-09 16:22:05 PDT 
(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.
Comment 13 Sam Weinig 2020-05-09 16:31:20 PDT 
(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.
Comment 14 Darin Adler 2020-05-09 16:34:53 PDT 
(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.