Bug 200270 – [FTW] Refactor Direct2D code to follow Cairo's model to support modern WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

Refactor the Direct2D code to separate the core drawing routines into helper functions that can be used by GraphicsContext and GraphicsContextImpl.

[Brent Fulgham]

Created attachment 375169 [details]
Patch

[EWS Watchlist]

Attachment 375169 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:68:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 375171 [details]
Patch

[Dean Jackson]

Comment on attachment 375171 [details]
Patch

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

rs=me assuming you don't land those settings changes.

> Source/WebCore/page/Settings.yaml:148
> -  initial: false
> +  initial: true
>    onChange: setNeedsRecalcStyleInAllFrames
>  showRepaintCounter:
> -  initial: false
> +  initial: true
>    onChange: setNeedsRecalcStyleInAllFrames
>  visibleDebugOverlayRegions:
>    type: DebugOverlayRegions
>    initial: 0
>  showTiledScrollingIndicator:
> -  initial: false
> +  initial: true

I really think you don't want this!!

[Brent Fulgham]

Created attachment 375172 [details]
Patch

[Alex Christensen]

Comment on attachment 375172 [details]
Patch

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

> Source/WebCore/platform/graphics/win/BackingStoreBackendDirect2D.h:57
> +    void* m_surfaceBackingData;

This needs a default initializer, and maybe it should go in the impl.

> Source/WebCore/platform/graphics/win/BackingStoreBackendDirect2DImpl.cpp:43
> +static COMPtr<IWICBitmap> createDirect2DImageSurfaceWithFastMalloc(const IntSize& size, double deviceScaleFactor, void*& backingStoreData)

I think it would be nicer if this returned a std::pair<COMPtr<IWICBitmap>, void*>

> Source/WebCore/platform/graphics/win/BackingStoreBackendDirect2DImpl.h:47
> +    void* m_scrollSurfaceBackingData;

{ nullptr }

> Source/WebCore/platform/graphics/win/GraphicsContextPlatformPrivateDirect2D.h:78
> +    PlatformContextDirect2D* platformContext() { return &m_platformContext; }

Why not just return a PlatformContextDirect2D& ?

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.cpp:89
> +\    // We must call savePlatformState at least once before we can use image masking,

You probably don't want this \

> Source/WebCore/platform/graphics/win/PlatformContextDirect2D.h:123
> +    State* m_state;

{ nullptr }

> Source/WebCore/platform/win/DragImageWin.cpp:176
> +    ASSERT(0);

notImplemented()?

[Brent Fulgham]

Comment on attachment 375172 [details]
Patch

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

>> Source/WebCore/platform/win/DragImageWin.cpp:176
>> +    ASSERT(0);
> 
> notImplemented()?

No! This should be uncommented. I'll fix.

[Brent Fulgham]

Created attachment 375182 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 375182 [details]
Patch for landing

Clearing flags on attachment: 375182

Committed r248020: <https://trac.webkit.org/changeset/248020>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53731210>