[webkit-reviews] review granted: [Bug 239952] MotionMark design subtest doesn't look right with GPUP DOM rendering enabled : [Attachment 458854] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Cameron McCormack
(:heycam) <heycam at apple.com>'s request for review:
Bug 239952: MotionMark design subtest doesn't look right with GPUP DOM
rendering enabled
https://bugs.webkit.org/show_bug.cgi?id=239952

Attachment 458854: Patch

https://bugs.webkit.org/attachment.cgi?id=458854&action=review




--- Comment #8 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 458854
  --> https://bugs.webkit.org/attachment.cgi?id=458854
Patch

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

> Source/WebCore/ChangeLog:7
> +	   Reviewed by NOBODY (OOPS!).

Can't we write an API test for this patch? Maybe one similar to
BifurcatedGraphicsContextTests.Text

> Source/WebCore/platform/graphics/GraphicsContext.cpp:596
> +GraphicsContextState GraphicsContext::stateWithChangesCleared() const

I think this function should be in GraphicsContextState.

Can't we name this function such that it reveals its purpose? Can't it be named
something like GraphicsContextState::cloneForRecording()?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:599
> +    state.didApplyChanges();

This call is misleading. We did not apply the changes of the state to call its
didApplyChanges().

If this function is moved to GraphicsContextState then you can set
m_changeFlags directly there. But if this function stays in GraphicsContext,
then I would recommend adding a new function like clearChangeFlags() which can
replace didApplyChanges() or coexist with it.

> Source/WebCore/platform/graphics/GraphicsContext.h:160
> +    GraphicsContextState stateWithChangesCleared() const;

I think it's better to move this function to GraphicsContextState. But if you
prefer it to be here, I think a better name would be 'stateForRecording()'? And
I think it can return 'const GraphicsContextState'

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:55
> +    ASSERT(!state.changes());
>      m_stateStack.append({ state, initialCTM, initialCTM.mapRect(initialClip)
});

Another approach is to clone the state here:

m_stateStack.append({ state.cloneForRecording(), initialCTM,
initialCTM.mapRect(initialClip) });