Bug 204740 – Start adding encoding support for DisplayList and some DisplayListItems

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Start adding encoding support for DisplayList and some DisplayListItems

[Tim Horton]

Created attachment 384602 [details]
Patch

[Tim Horton]

Created attachment 384603 [details]
Patch

[Alex Christensen]

Comment on attachment 384603 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1907
> +    // ApplyDeviceScaleFactor
> +    default:

I think it would be better to remove the default and all the comments with missing types.
Also, this would be done for you automatically if Item contained a Variant of many types instead of being inherited from.  That might be worse for other reasons, but it might be worth considering.

[Tim Horton]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 384603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384603&action=review
> 
> > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1907
> > +    // ApplyDeviceScaleFactor
> > +    default:
> 
> I think it would be better to remove the default and all the comments with
> missing types.

And add logging to each missing type instead?

> Also, this would be done for you automatically if Item contained a Variant
> of many types instead of being inherited from.  That might be worse for
> other reasons, but it might be worth considering.

I will tippytap it and see how it looks

[Said Abou-Hallawa]

Comment on attachment 384603 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:320
> +    Optional<float> x;
> +    decoder >> x;
> +    if (!x)
> +        return WTF::nullopt;
> +
> +    Optional<float> y;
> +    decoder >> y;
> +    if (!y)
> +        return WTF::nullopt;
> +
> +    return Translate::create(*x, *y);

I do not think we need to use these Optionals and these checks for something we do internally. Decoding should not fail unless we did something really bad while encoding the display list. I think we should not make let it go silently like that. Instead we should keep ASSERTing all the way. So I would expect a function like this to be like this:

    float x;
    float x;
    decoder >> x;
    decoder >> y;
    return Translate::create(x, y);

Decoder::operator>>() should always assert it is not at the end of the buffer and it has enough data to be consumed.

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 384603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384603&action=review
> 
> > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:320
> > +    Optional<float> x;
> > +    decoder >> x;
> > +    if (!x)
> > +        return WTF::nullopt;
> > +
> > +    Optional<float> y;
> > +    decoder >> y;
> > +    if (!y)
> > +        return WTF::nullopt;
> > +
> > +    return Translate::create(*x, *y);
> 
> I do not think we need to use these Optionals and these checks for something
> we do internally. Decoding should not fail unless we did something really
> bad while encoding the display list.

This is idiomatic IPC encoding code. 

> I think we should not make let it go
> silently like that.

It is not silent. The optionals bubble up, the message decode is marked as failed, and the *process is killed*. That is the opposite of silent. Again, this is idiomatic, we have zillions of encoders exactly like this. There's nothing about display lists that makes them special or deserving of special treatment.

> Instead we should keep ASSERTing all the way. So I would
> expect a function like this to be like this:
> 
>     float x;
>     float x;
>     decoder >> x;
>     decoder >> y;
>     return Translate::create(x, y);
> 
> Decoder::operator>>() should always assert it is not at the end of the
> buffer and it has enough data to be consumed.

No, this doesn't fit in with the existing model at all.

[Simon Fraser (smfr)]

Comment on attachment 384603 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:101
> +    if ((m_changeFlags.contains(GraphicsContextState::CompositeOperationChange) || m_changeFlags.contains(GraphicsContextState::BlendModeChange))

Can this be m_changeFlags.containsAny({ GraphicsContextState::CompositeOperationChange, GraphicsContextState::BlendModeChange })

> Source/WebCore/platform/graphics/GraphicsContext.cpp:164
> +    if (flags.contains(GraphicsContextState::CompositeOperationChange) || flags.contains(GraphicsContextState::BlendModeChange)) {

Ditto

> Source/WebCore/platform/graphics/GraphicsContext.cpp:239
> +    if (m_changeFlags.contains(GraphicsContextState::CompositeOperationChange) || m_changeFlags.contains(GraphicsContextState::BlendModeChange))

Ditto

> Source/WebCore/platform/graphics/displaylists/DisplayList.h:134
> +        if (!item)
> +            continue;

Log here?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:174
> +    template<class Encoder> void encodeForDrawingItem(Encoder&) const;
> +    template<class Decoder> static bool decodeForDrawingItem(Decoder&, DrawingItem&);

Names are confusing.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:504
> +    if (*changeFlags & GraphicsContextState::StrokeColorChange) {

stateChange.m_changeFlags.contains()

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:513
> +    if (*changeFlags & GraphicsContextState::FillColorChange) {

.contains()

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:522
> +    if (*changeFlags & GraphicsContextState::AlphaChange) {

.contains()

[Tim Horton]

Created attachment 384694 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384694 [details]
Patch

Rejecting attachment 384694 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 384694, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=384694&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=204740&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 384694 from bug 204740.
Fetching: https://bugs.webkit.org/attachment.cgi?id=384694
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 11 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #3 FAILED at 29536.
Hunk #4 succeeded at 29908 (offset -2 lines).
Hunk #5 FAILED at 30049.
Hunk #6 FAILED at 30160.
Hunk #7 FAILED at 30552.
Hunk #8 FAILED at 31175.
Hunk #9 FAILED at 31207.
Hunk #10 FAILED at 31243.
Hunk #11 FAILED at 31704.
Hunk #12 FAILED at 32498.
Hunk #13 FAILED at 32827.
10 out of 13 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file Source/WebCore/platform/graphics/GraphicsContext.cpp
patching file Source/WebCore/platform/graphics/GraphicsContext.h
patching file Source/WebCore/platform/graphics/Pattern.h
patching file Source/WebCore/platform/graphics/displaylists/DisplayList.h
patching file Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp
patching file Source/WebCore/platform/graphics/displaylists/DisplayListItems.h
patching file Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp
patching file Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h
patching file Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/13285420

[Tim Horton]

Created attachment 384714 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384714 [details]
Patch

Clearing flags on attachment: 384714

Committed r253046: <https://trac.webkit.org/changeset/253046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57591573>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/253046/webkit

broke testing bot Mac debug wk2 https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/builds/6152

Testing is exiting early with crashes

example:
ASSERTION FAILED: Enumerator is not a positive power of two.
hasOneBitSet(static_cast<StorageType>(option))
/Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/usr/local/include/wtf/OptionSet.h(88) :

[Tim Horton]

(In reply to Truitt Savell from comment #14)
> It looks like the changes in https://trac.webkit.org/changeset/253046/webkit
> 
> broke testing bot Mac debug wk2
> https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/
> builds/6152
> 
> Testing is exiting early with crashes
> 
> example:
> ASSERTION FAILED: Enumerator is not a positive power of two.
> hasOneBitSet(static_cast<StorageType>(option))
> /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/usr/local/include/
> wtf/OptionSet.h(88) :

Weird! I'll fix it.

[Tim Horton]

(In reply to Tim Horton from comment #15)
> (In reply to Truitt Savell from comment #14)
> > It looks like the changes in https://trac.webkit.org/changeset/253046/webkit
> > 
> > broke testing bot Mac debug wk2
> > https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/
> > builds/6152
> > 
> > Testing is exiting early with crashes
> > 
> > example:
> > ASSERTION FAILED: Enumerator is not a positive power of two.
> > hasOneBitSet(static_cast<StorageType>(option))
> > /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/usr/local/include/
> > wtf/OptionSet.h(88) :
> 
> Weird! I'll fix it.

Have a fix, just building to test.

[Tim Horton]

(In reply to Tim Horton from comment #16)
> (In reply to Tim Horton from comment #15)
> > (In reply to Truitt Savell from comment #14)
> > > It looks like the changes in https://trac.webkit.org/changeset/253046/webkit
> > > 
> > > broke testing bot Mac debug wk2
> > > https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/
> > > builds/6152
> > > 
> > > Testing is exiting early with crashes
> > > 
> > > example:
> > > ASSERTION FAILED: Enumerator is not a positive power of two.
> > > hasOneBitSet(static_cast<StorageType>(option))
> > > /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/usr/local/include/
> > > wtf/OptionSet.h(88) :
> > 
> > Weird! I'll fix it.
> 
> Have a fix, just building to test.

Fixed in https://trac.webkit.org/changeset/253058/webkit