Bug 207134

Summary: Make HostWindow be the creator of the remote ImageBuffer
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, cdumez, commit-queue, darin, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, jer.noble, jsbell, kondapallykalyan, noam, pdr, philipj, ryuan.choi, sam, schenney, sergio, tommyw, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 207109    
Bug Blocks: 204955, 207198    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for review
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2020-02-03 11:01:47 PST 
RenderingBackend will be responsible for creating the ImageBuffer. The plan is to have RemoteRenderingBackend which inherits RenderingBackend and lives in WebKit be responsible for creating the RemoteImageBuffer. The goal for RenderingBackend and RemoteRenderingBackend is to interact with RemoteImageBuffer seamlessly in the caller side.
Comment 1 Said Abou-Hallawa 2020-02-03 11:06:40 PST 
Created attachment 389540 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-02-03 14:29:03 PST 
Created attachment 389570 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-02-03 15:17:51 PST 
Created attachment 389579 [details]
Patch
Comment 4 Said Abou-Hallawa 2020-02-03 15:47:11 PST 
Created attachment 389584 [details]
Patch  for review
Comment 5 Said Abou-Hallawa 2020-02-21 17:14:49 PST 
Created attachment 391435 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-02-21 17:35:36 PST 
Created attachment 391438 [details]
Patch
Comment 7 Sam Weinig 2020-02-21 18:02:03 PST 
Comment on attachment 391438 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +
> +        RenderingBackend will be responsible for creating the ImageBuffer. In a
> +        following patch RemoteRenderingBackend will be introduced. It'll inherit
> +        RenderingBackend and live in WebKit. It will be responsible for creating
> +        the RemoteImageBuffer. The goal for RenderingBackend and RemoteRenderingBackend
> +        is to interact with RemoteImageBuffer seamlessly in the caller side.

What is the added value of this additional interface over doing this via HostWindow?
Comment 8 Sam Weinig 2020-02-21 18:05:11 PST 
(In reply to Sam Weinig from comment #7)
> Comment on attachment 391438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391438&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +
> > +        RenderingBackend will be responsible for creating the ImageBuffer. In a
> > +        following patch RemoteRenderingBackend will be introduced. It'll inherit
> > +        RenderingBackend and live in WebKit. It will be responsible for creating
> > +        the RemoteImageBuffer. The goal for RenderingBackend and RemoteRenderingBackend
> > +        is to interact with RemoteImageBuffer seamlessly in the caller side.
> 
> What is the added value of this additional interface over doing this via
> HostWindow?

To be more specific, the alternative I imagine is adding the createImageBuffer() virtual functions to HostWindow, and having Chrome forward those to createImageBuffer() virtual functions on the ChromeClient.
Comment 9 Said Abou-Hallawa 2020-02-24 16:11:11 PST 
Created attachment 391593 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-02-24 17:32:43 PST 
Created attachment 391615 [details]
Patch
Comment 11 Said Abou-Hallawa 2020-02-24 17:59:36 PST 
Created attachment 391616 [details]
Patch
Comment 12 Darin Adler 2020-02-24 22:08:16 PST 
Comment on attachment 391616 [details]
Patch

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

> Source/WebCore/platform/HostWindow.h:28
> +#include "ColorSpace.h"

Typically we can get away with a forward declaration of an enum, if it’s an enum class.

> Source/WebCore/platform/HostWindow.h:29
> +#include "RenderingMode.h"

Ditto.
Comment 13 Said Abou-Hallawa 2020-02-25 08:56:23 PST 
Created attachment 391656 [details]
Patch
Comment 14 Said Abou-Hallawa 2020-02-25 09:30:40 PST 
Created attachment 391662 [details]
Patch
Comment 15 WebKit Commit Bot 2020-02-25 10:49:07 PST 
Comment on attachment 391662 [details]
Patch

Clearing flags on attachment: 391662

Committed r257363: <https://trac.webkit.org/changeset/257363>
Comment 16 WebKit Commit Bot 2020-02-25 10:49:09 PST 
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2020-02-25 10:50:17 PST 
<rdar://problem/59770658>
Comment 18 Said Abou-Hallawa 2020-02-25 17:47:44 PST 
*** Bug 204955 has been marked as a duplicate of this bug. ***
Comment 19 Said Abou-Hallawa 2020-02-25 17:51:29 PST 
*** Bug 207198 has been marked as a duplicate of this bug. ***
Comment 20 Said Abou-Hallawa 2020-02-26 17:22:56 PST 
Comment on attachment 391438 [details]
Patch

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

>>> Source/WebCore/ChangeLog:12
>>> +        is to interact with RemoteImageBuffer seamlessly in the caller side.
>> 
>> What is the added value of this additional interface over doing this via HostWindow?
> 
> To be more specific, the alternative I imagine is adding the createImageBuffer() virtual functions to HostWindow, and having Chrome forward those to createImageBuffer() virtual functions on the ChromeClient.

Why adding createImageBuffer() to HostWindow and not adding the RenderingBackend to PlatformStrategies?
Comment 21 Sam Weinig 2020-02-27 12:13:19 PST 
(In reply to Said Abou-Hallawa from comment #20)
> Comment on attachment 391438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391438&action=review
> 
> >>> Source/WebCore/ChangeLog:12
> >>> +        is to interact with RemoteImageBuffer seamlessly in the caller side.
> >> 
> >> What is the added value of this additional interface over doing this via HostWindow?
> > 
> > To be more specific, the alternative I imagine is adding the createImageBuffer() virtual functions to HostWindow, and having Chrome forward those to createImageBuffer() virtual functions on the ChromeClient.
> 
> Why adding createImageBuffer() to HostWindow and not adding the
> RenderingBackend to PlatformStrategies?

PlatformStrategies have not been a good approach due their creation of a singleton and reliance on processes not wanting multiple strategies at once. Use of the per-page HostWindow avoids that problem.