Bug 182686 - Basic OffscreenCanvas functionality
Summary: Basic OffscreenCanvas functionality
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on: 179191 182503 182685 202513 203884 203895 204347 204520
Blocks: 183720 202626 182921 183440
  Show dependency treegraph
 
Reported: 2018-02-11 23:35 PST by Zan Dobersek
Modified: 2019-11-25 06:41 PST (History)
26 users (show)

See Also:


Attachments
WIP patch (8.05 KB, patch)
2018-02-11 23:38 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (20.45 KB, patch)
2019-10-01 05:09 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.35 MB, patch)
2019-10-10 03:09 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.71 MB, patch)
2019-10-11 14:41 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.72 MB, patch)
2019-11-01 08:24 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.72 MB, patch)
2019-11-01 08:34 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.72 MB, patch)
2019-11-01 09:02 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.37 MB, patch)
2019-11-04 02:57 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.36 MB, patch)
2019-11-07 08:14 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.41 MB, patch)
2019-11-20 06:56 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.41 MB, patch)
2019-11-20 07:43 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (14.13 MB, application/zip)
2019-11-20 16:35 PST, Build Bot
no flags Details
Patch (1.41 MB, patch)
2019-11-21 04:34 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.42 MB, patch)
2019-11-21 09:25 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.38 MB, patch)
2019-11-25 02:49 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.38 MB, patch)
2019-11-25 03:13 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.38 MB, patch)
2019-11-25 03:20 PST, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2018-02-11 23:35:20 PST
Add minimum OffscreenCanvas functionality that gets the implementation working reasonably well on the W3C test suite.
Comment 1 Zan Dobersek 2018-02-11 23:38:46 PST
Created attachment 333583 [details]
WIP patch

Extends the CanvasBase functionality, roughly mirroring HTMLCanvasElement. Also implements getContext() to support the 2D context type.

Still depends on bugs #182503 and #182685, but also requires standalone layout tests.
Comment 2 Chris Lord 2019-10-01 05:09:03 PDT
Created attachment 379894 [details]
Patch
Comment 3 Chris Lord 2019-10-10 03:09:56 PDT
Created attachment 380624 [details]
Patch
Comment 4 Ryosuke Niwa 2019-10-11 02:44:51 PDT
Comment on attachment 380624 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.cpp:71
> +    if (isMainThread())
> +        return m_identifierValues[ident].get();
> +    return CSSPrimitiveValue::create(ident);

This is going to very confusing because most callosities of this function will be like:
CSSValuePool::singleton().createIdentifierValue

We should instead make CSSParser access CSSValuePool via its member; e.g. m_valuePool
and assert that CSSValuePool::singleton is only accessed in the main thread.

That way, value pool optimization will also work in worker,
which might be important in keeping memory usage in check.
Comment 5 Chris Lord 2019-10-11 04:57:14 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 380624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380624&action=review
> 
> > Source/WebCore/css/CSSValuePool.cpp:71
> > +    if (isMainThread())
> > +        return m_identifierValues[ident].get();
> > +    return CSSPrimitiveValue::create(ident);
> 
> This is going to very confusing because most callosities of this function
> will be like:
> CSSValuePool::singleton().createIdentifierValue
> 
> We should instead make CSSParser access CSSValuePool via its member; e.g.
> m_valuePool
> and assert that CSSValuePool::singleton is only accessed in the main thread.
> 
> That way, value pool optimization will also work in worker,
> which might be important in keeping memory usage in check.

I'd like to do something like this, but CSSParser is just a bunch of static functions, for the most part... Perhaps an alternative would be to have a CSSValuePool per-thread(?) But if we did that, I don't know where/how destruction would be handled.

Alternatively, canvas only needs style parsing for colours and fonts - perhaps we could just have a separate CanvasCSSParser provided by Document/WorkerGlobalScope that provides only those functions?
Comment 6 Chris Lord 2019-10-11 08:32:51 PDT
(In reply to Chris Lord from comment #5)
> I'd like to do something like this, but CSSParser is just a bunch of static
> functions, for the most part... Perhaps an alternative would be to have a
> CSSValuePool per-thread(?) But if we did that, I don't know where/how
> destruction would be handled.
> 
> Alternatively, canvas only needs style parsing for colours and fonts -
> perhaps we could just have a separate CanvasCSSParser provided by
> Document/WorkerGlobalScope that provides only those functions?

What I've gone with is for CSSParser (and fast-path equivalent) to have a prototype that accepts a given CSSValuePool, and WorkerGlobalScope has a CSSValuePool member. I've also added a main-thread assert to CSSValuePool::singleton. This does mean that CSSValuePool's members are no longer LazyNeverDestroyed, however. I'll tackle fonts in later patches, but this seems like a reasonably simple solution for colours. Patch incoming.
Comment 7 Chris Lord 2019-10-11 14:41:12 PDT
Created attachment 380787 [details]
Patch
Comment 8 Chris Lord 2019-10-11 14:46:08 PDT
I did what I said in comment #6. It's a deceptively large-looking patch, but it's essentially just adding an extra parameter to the static CSS property parsing functions to allow a CSSValuePool to be passed through. Along with the assert in singleton(), this seems a pretty decent way of stopping off-main-thread access - It certainly caught a few things I missed on my initial run. This still requires the patches in bug #182503 and bug #182685.
Comment 9 Chris Lord 2019-11-01 08:24:42 PDT
Created attachment 382582 [details]
Patch
Comment 10 Chris Lord 2019-11-01 08:34:41 PDT
Created attachment 382583 [details]
Patch
Comment 11 Chris Lord 2019-11-01 09:02:59 PDT
Created attachment 382585 [details]
Patch
Comment 12 Antti Koivisto 2019-11-01 09:16:11 PDT
Comment on attachment 382585 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:487
> -static RefPtr<CSSFontFeatureValue> consumeFontFeatureTag(CSSParserTokenRange& range)
> +static RefPtr<CSSFontFeatureValue> consumeFontFeatureTag(CSSValuePool& cssValuePool, CSSParserTokenRange& range)

We are not going to uglify the code by adding CSSValuePool argument everywhere. Please find alternative solutions, like using thread global data.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:689
> +static RefPtr<CSSValue> consumeFontVariantEastAsian(CSSValuePool& cssValuePool, CSSParserTokenRange& range)

We are not going to start passing
Comment 13 Antti Koivisto 2019-11-01 09:21:54 PDT
Perhaps you could make many of the static function members and so have a general solution for context passing.
Comment 14 Chris Lord 2019-11-04 02:57:06 PST
Created attachment 382725 [details]
Patch
Comment 15 Chris Lord 2019-11-04 02:59:03 PST
(In reply to Antti Koivisto from comment #12)
> Comment on attachment 382585 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382585&action=review
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:487
> > -static RefPtr<CSSFontFeatureValue> consumeFontFeatureTag(CSSParserTokenRange& range)
> > +static RefPtr<CSSFontFeatureValue> consumeFontFeatureTag(CSSValuePool& cssValuePool, CSSParserTokenRange& range)
> 
> We are not going to uglify the code by adding CSSValuePool argument
> everywhere. Please find alternative solutions, like using thread global data.

Yes, fair enough - I've removed all of these changes and just used ThreadGlobalData instead.
Comment 16 Ryosuke Niwa 2019-11-04 13:38:20 PST
This patch is too massive. Please split it into smaller pieces.
Comment 17 Chris Lord 2019-11-04 16:02:22 PST
(In reply to Ryosuke Niwa from comment #16)
> This patch is too massive. Please split it into smaller pieces.

The majority of the patch is just test expectation changes - Given OffscreenCanvas is behind a flag and Mac/iOS aren't affected, I guess this isn't so important to land simultaneously.

I guess I could just split it per test subdirectory, does that sound ok?
Comment 18 Ryosuke Niwa 2019-11-04 20:22:03 PST
Comment on attachment 382725 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2019-10-08  Zan Dobersek  <zdobersek@igalia.com>  and  Chris Lord  <clord@igalia.com>

Additional attribution should happen in the description, not this line.
Ditto for other change logs.

> Source/WebCore/platform/graphics/ImageSource.cpp:-56
> -    ASSERT(isMainThread());

This assert should still exist when offscreen canvas is disabled.

> Source/WebCore/platform/graphics/ImageSource.h:43
> -class ImageSource : public ThreadSafeRefCounted<ImageSource, WTF::DestructionThread::Main>, public CanMakeWeakPtr<ImageSource> {
> +class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeakPtr<ImageSource> {

This isn't right. As far as I understand, destruction happening in a non-main thread would corrupt data in macOS / iOS.
Comment 19 Chris Lord 2019-11-05 03:27:05 PST
(In reply to Ryosuke Niwa from comment #18)
> Comment on attachment 382725 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382725&action=review
> 
> > Source/WebCore/ChangeLog:1
> > +2019-10-08  Zan Dobersek  <zdobersek@igalia.com>  and  Chris Lord  <clord@igalia.com>
> 
> Additional attribution should happen in the description, not this line.
> Ditto for other change logs.

It's done elsewhere like this in ChangeLogs and I've had previous patches approved and committed with this format, could you be more specific with how you'd like it changed?

> > Source/WebCore/platform/graphics/ImageSource.cpp:-56
> > -    ASSERT(isMainThread());
> 
> This assert should still exist when offscreen canvas is disabled.
> 
> > Source/WebCore/platform/graphics/ImageSource.h:43
> > -class ImageSource : public ThreadSafeRefCounted<ImageSource, WTF::DestructionThread::Main>, public CanMakeWeakPtr<ImageSource> {
> > +class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeakPtr<ImageSource> {
> 
> This isn't right. As far as I understand, destruction happening in a
> non-main thread would corrupt data in macOS / iOS.

I'm going to go over this and see if it's actually necessary or could be worked around in a different way.
Comment 20 Chris Lord 2019-11-05 07:00:05 PST
> > > Source/WebCore/platform/graphics/ImageSource.cpp:-56
> > > -    ASSERT(isMainThread());
> > 
> > This assert should still exist when offscreen canvas is disabled.
> > 
> > > Source/WebCore/platform/graphics/ImageSource.h:43
> > > -class ImageSource : public ThreadSafeRefCounted<ImageSource, WTF::DestructionThread::Main>, public CanMakeWeakPtr<ImageSource> {
> > > +class ImageSource : public ThreadSafeRefCounted<ImageSource>, public CanMakeWeakPtr<ImageSource> {
> > 
> > This isn't right. As far as I understand, destruction happening in a
> > non-main thread would corrupt data in macOS / iOS.
> 
> I'm going to go over this and see if it's actually necessary or could be
> worked around in a different way.

So going over and testing, I remember what this issue was - drawShadowLayer ends up triggering ImageBuffer::copyImage, which at least on cairo ends up creating an ImageSource using a native image buffer (and is safe to use off-main-thread).

I ran into this in bug 202797 and ended up just creating an image buffer and using getPremultipliedImageData/putByteArray instead of copyImage to work around it... I'll see if that makes sense here, or find a better fix. I'll look at what happens on other backends too, I wonder if this is a general problem or specific to the cairo drawing backend.
Comment 21 Chris Lord 2019-11-05 07:42:23 PST
> I ran into this in bug 202797 and ended up just creating an image buffer and
> using getPremultipliedImageData/putByteArray instead of copyImage to work
> around it... I'll see if that makes sense here, or find a better fix. I'll
> look at what happens on other backends too, I wonder if this is a general
> problem or specific to the cairo drawing backend.

So the only reason copyImage was being called is because it's used to get the native image pointer in cairo's GraphicsContext for clipToImageBuffer(). I'm testing just putting a Cairo-specific native image accessor on ImageBuffer, and if that works I expect we can just remove the changes to ImageSource altogether.
Comment 22 Chris Lord 2019-11-05 07:55:48 PST
ok, no, unfortunately not as simple as that. That was one of the reasons, but there are other reasons too... I'm not sure that this can be worked around so easily, but still will see what I can do.
Comment 23 Chris Lord 2019-11-06 02:44:12 PST
(In reply to Chris Lord from comment #22)
> ok, no, unfortunately not as simple as that. That was one of the reasons,
> but there are other reasons too... I'm not sure that this can be worked
> around so easily, but still will see what I can do.

Of course, a night's sleep makes everything simpler. It is as simple as that (hopefully, pending tests) - reworking the Cairo backend a tiny bit so that it doesn't use copyImage unnecessarily fixes this issue and makes the changes to ImageSource completely unnecessary. I'll open up a separate bug for that change.
Comment 24 Chris Lord 2019-11-07 08:14:17 PST
Created attachment 383051 [details]
Patch
Comment 25 Chris Lord 2019-11-07 08:17:39 PST
I didn't change the ChangeLog attribution as it exists like this elsewhere in the file already. If you'd really like that to be different, if you could be more specific about the format I should follow that would be appreciated.

Likewise, I haven't split the test expectation changes away from this patch to make it smaller as it would result in a bunch of unexpected passes on Gtk/WPE until the expectations are updated and it seems counter-intuitive to me to check in changes that knowingly break things (there's no escaping that the test expectation changes here are going to be large).

Test expectation changes aside, I'd say this patch is quite small now. If you could be more specific with any feedback if it's still too large, that would again be much appreciated.
Comment 26 Antti Koivisto 2019-11-18 02:18:41 PST
Comment on attachment 383051 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.h:79
> +    static RefPtr<CSSValue> parseSingleValue(CSSValuePool&, CSSPropertyID, const String&, const CSSParserContext& = strictCSSParserContext());

This has no implementation nor callers.

> Source/WebCore/css/parser/CSSParser.h:89
> +    static Color parseColor(CSSValuePool&, const String&, bool strict = false);

Neither does this.
Comment 27 Antti Koivisto 2019-11-18 02:20:59 PST
We talked about this and I suggested factoring the (apparently small) subset of CSS parser needed for this feature into a separate type. This will make it easier to audit that what we are doing is actually thread safe with reasonable confidence.
Comment 28 Chris Lord 2019-11-19 09:06:50 PST
After talking on IRC, the plan for this patch is to isolate the part of CSS parsing that it needs (just colour parsing at this point), add a thread-safe API for that, rather than enabling the whole of CSS parsing off-main-thread.

My initial plan is to augment and enable the parser fast-path for off-main-thread use, the augmentation of which I'm doing in bug 204347. This does mean that some malformed colour formats will not work in OffscreenCanvas, but I think we can deal with these separately, later.

Currently I have truncated rgb/rgba working and I plan on adding hsl/hsla. This will mean that the most common ways of specifying colour will work at least, while just a handful of malformed specifications (that for some reason are tested for?) won't work.
Comment 29 Chris Lord 2019-11-20 06:56:25 PST
Created attachment 383961 [details]
Patch
Comment 30 Chris Lord 2019-11-20 07:43:05 PST
Created attachment 383964 [details]
Patch
Comment 31 Chris Lord 2019-11-20 07:58:02 PST
Discovered that relying on the fast-path wasn't really going to cut it and after discussing on IRC, decided that the way forward would be to make the necessary parts of the parser thread-safe under a different name-space to differentiate them.

So this patch makes parseColor and functions it depends on thread-safe (essentially by making CSSValuePool usage optional in those functions), and makes CanvasStyle use the thread-safe version of consumeColor directly.

As this was all under CSSPropertyParserHelpers, there is now a CSSPropertyParserHelpers::ThreadSafe namespace, under which contains only functions that are safe to call off-main-thread.
Comment 32 Build Bot 2019-11-20 16:35:56 PST
Comment on attachment 383964 [details]
Patch

Attachment 383964 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13270081

New failing tests:
fast/css/test-setting-canvas-color.html
Comment 33 Build Bot 2019-11-20 16:35:59 PST
Created attachment 384002 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 34 Chris Lord 2019-11-21 04:34:27 PST
Created attachment 384045 [details]
Patch
Comment 35 Antti Koivisto 2019-11-21 07:07:15 PST
Comment on attachment 384045 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.cpp:120
>          return namedColor;
>      
>      // Try the fast path to parse hex and rgb.
> -    RefPtr<CSSValue> value = CSSParserFastPaths::parseColor(string, strict ? HTMLStandardMode : HTMLQuirksMode);
> -    
> +    CSSValuePool* valuePool = isMainThread() ? &CSSValuePool::singleton() : nullptr;
> +    auto parseMode = strict ? HTMLStandardMode : HTMLQuirksMode;
> +    RefPtr<CSSValue> value = CSSParserFastPaths::parseColor(string, parseMode, valuePool);
> +
>      // If that fails, try the full parser.
> -    if (!value)
> -        value = parseSingleValue(CSSPropertyColor, string, strictCSSParserContext());
> +    if (!value) {
> +        CSSTokenizer tokenizer(string);
> +        CSSParserTokenRange range(tokenizer.tokenRange());
> +        range.consumeWhitespace();
> +        value = CSSPropertyParserHelpers::ThreadSafe::consumeColor(range, parseMode, valuePool);
> +        if (!range.atEnd())
> +            value = nullptr;
> +    }
>      if (!value || !value->isPrimitiveValue())
>          return Color();

Maybe you could provide a separate CSSParser::parseColorThreadSafea and avoid branching on isMainThread?

Are we assuming CSSParserFastPaths is entirely thread safe?

> Source/WebCore/html/canvas/CanvasStyle.cpp:55
>  {
>      Color color = CSSParser::parseColor(colorString);
> -    if (color.isValid())
> +    if (color.isValid() || !isMainThread())
>          return color;

Can we just branch based on if this is an offscreen canvas? Thread checks are not nice.
Comment 36 Chris Lord 2019-11-21 07:39:18 PST
(In reply to Antti Koivisto from comment #35)
> Comment on attachment 384045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384045&action=review
> 
> > Source/WebCore/css/parser/CSSParser.cpp:120
> >          return namedColor;
> >      
> >      // Try the fast path to parse hex and rgb.
> > -    RefPtr<CSSValue> value = CSSParserFastPaths::parseColor(string, strict ? HTMLStandardMode : HTMLQuirksMode);
> > -    
> > +    CSSValuePool* valuePool = isMainThread() ? &CSSValuePool::singleton() : nullptr;
> > +    auto parseMode = strict ? HTMLStandardMode : HTMLQuirksMode;
> > +    RefPtr<CSSValue> value = CSSParserFastPaths::parseColor(string, parseMode, valuePool);
> > +
> >      // If that fails, try the full parser.
> > -    if (!value)
> > -        value = parseSingleValue(CSSPropertyColor, string, strictCSSParserContext());
> > +    if (!value) {
> > +        CSSTokenizer tokenizer(string);
> > +        CSSParserTokenRange range(tokenizer.tokenRange());
> > +        range.consumeWhitespace();
> > +        value = CSSPropertyParserHelpers::ThreadSafe::consumeColor(range, parseMode, valuePool);
> > +        if (!range.atEnd())
> > +            value = nullptr;
> > +    }
> >      if (!value || !value->isPrimitiveValue())
> >          return Color();
> 
> Maybe you could provide a separate CSSParser::parseColorThreadSafea and
> avoid branching on isMainThread?
> 
> Are we assuming CSSParserFastPaths is entirely thread safe?
> 
> > Source/WebCore/html/canvas/CanvasStyle.cpp:55
> >  {
> >      Color color = CSSParser::parseColor(colorString);
> > -    if (color.isValid())
> > +    if (color.isValid() || !isMainThread())
> >          return color;
> 
> Can we just branch based on if this is an offscreen canvas? Thread checks
> are not nice.

These both sound good, will get to this imminently.
Comment 37 Chris Lord 2019-11-21 07:57:26 PST
(In reply to Chris Lord from comment #36)
> (In reply to Antti Koivisto from comment #35)
> > Comment on attachment 384045 [details]

> > Are we assuming CSSParserFastPaths is entirely thread safe?

for parseColor, yes - at least, from my inspection, all it does is basic string manipulation. The only thing that wasn't safe to do off-main-thread was use CSSValuePool::singleton(). The rest of the file looks similar, but I haven't inspected it like parseColor as we aren't using it.

Something interesting I found, when FastPath::parseColor fails, CSSParser calls parseSingleValue, which calls CSSFastPath::maybeParseValue, which calls parseColor again - so we were actually calling this twice (at least it looked that way) for every colour that required the full parser. This patch no longer calls maybeParseValue, so fixes that (assuming I'm not misinterpreting what was happening). I assume not just calling maybeParse in the first place was to avoid unnecessary checks for things that aren't colours.

This reminds me, I think this patch doesn't add the main-thread assert on CSSValuePool::singleton() like earlier versions did, so I'll make sure to add that again.
Comment 38 Chris Lord 2019-11-21 09:25:17 PST
Created attachment 384061 [details]
Patch
Comment 39 Antti Koivisto 2019-11-22 07:21:47 PST
Comment on attachment 384061 [details]
Patch

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

> Source/WebCore/css/parser/CSSParser.cpp:100
>  Color CSSParser::parseColor(const String& string, bool strict)
> +{
> +    return parseColorThreadSafe(string, &CSSValuePool::singleton(), strict);
> +}
> +
> +Color CSSParser::parseColorThreadSafe(const String& string, CSSValuePool* valuePool, bool strict)

Function that takes valuePool is clearly not thread safe, no matter what the name says.

You should just write a separate thread safe function and leave the existing parseColor alone.
Comment 40 Chris Lord 2019-11-25 02:49:16 PST
Created attachment 384285 [details]
Patch
Comment 41 Chris Lord 2019-11-25 03:13:40 PST
Created attachment 384286 [details]
Patch
Comment 42 Chris Lord 2019-11-25 03:20:57 PST
Created attachment 384287 [details]
Patch
Comment 43 Antti Koivisto 2019-11-25 05:52:49 PST
Comment on attachment 384287 [details]
Patch

Ok, looks good. r=me
Comment 44 WebKit Commit Bot 2019-11-25 06:40:00 PST
Comment on attachment 384287 [details]
Patch

Clearing flags on attachment: 384287

Committed r252856: <https://trac.webkit.org/changeset/252856>
Comment 45 WebKit Commit Bot 2019-11-25 06:40:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Radar WebKit Bug Importer 2019-11-25 06:41:30 PST
<rdar://problem/57472685>