Bug 81279 – [chromium] Updating WebGraphicsContext3D MemoryAllocation callback to accept a struct with have backbuffer suggestion.

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

[Michal Mocny]

[chromium] Updating WebGraphicsContext3D MemoryAllocation callback to accept a struct with have backbuffer suggestion.

[Michal Mocny]

Created attachment 132137 [details]
Patch

[Michal Mocny]

Comment on attachment 132137 [details]
Patch

This is just providing a hint to the compositor about when to drop the backbuffer.  The drop extension, and the compositor memory allocation callback coming soon.

[James Robinson]

Comment on attachment 132137 [details]
Patch

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

> Source/Platform/chromium/public/WebGraphicsContext3D.h:31
> +

spurious newline here

> Source/Platform/chromium/public/WebGraphicsContext3D.h:135
> +        virtual void onMemoryAllocationChanged(const WebGraphicsMemoryAllocation&);

passing by value is probably just as (or more) efficient.

can you leave a FIXME here indicating that the other version is going away and that this will become pure virtual once the chromium side lands?

> Source/Platform/chromium/public/WebGraphicsMemoryAllocation.h:34
> +    unsigned int gpuResourceSizeInBytes;

here and elsewhere: we spell this type "unsigned", not "unsigned int"

> Source/Platform/chromium/public/WebGraphicsMemoryAllocation.h:50
> +inline bool operator==(const WebGraphicsMemoryAllocation& a, const WebGraphicsMemoryAllocation& b)

are these necessary?

[James Robinson]

Comment on attachment 132137 [details]
Patch

Other than nits this API is fine with me if it's OK with Nat.

[Michal Mocny]

Created attachment 132140 [details]
Patch

[Michal Mocny]

Fixed nits.

[James Robinson]

Comment on attachment 132140 [details]
Patch

Looks good.  I think Nat's unavailable for a few days, but we can refine when he gets back if there's more feedback.

[Nat Duca]

Comment on attachment 132140 [details]
Patch

Isn't there an ExtensionsChromium change as well?

[Michal Mocny]

Yes I did miss adding to ExtensionsChromium, so I will add it as part of the change to have compositor register a callback.

[WebKit Review Bot]

Comment on attachment 132140 [details]
Patch

Rejecting attachment 132140 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
locationChangedCallbackAdapter:GraphicsContext3DChromium.cpp(.data.rel.ro._ZTVN7WebCore55GraphicsContext3DMemoryAllocationChangedCallbackAdapterE+0x18): error: undefined reference to 'WebKit::WebGraphicsContext3D::WebGraphicsMemoryAllocationChangedCallbackCHROMIUM::onMemoryAllocationChanged(WebKit::WebGraphicsMemoryAllocation)'
collect2: ld returned 1 exit status
make: *** [out/Debug/DumpRenderTree] Error 1

Full output: http://queues.webkit.org/results/11962166

[Michal Mocny]

Created attachment 132287 [details]
Patch

[Michal Mocny]

Fixed the build issue and added the Extension3DChromium change.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail] or [e-mail] before submitting because this patch contains changes to the Chromium platform API.

[Michal Mocny]

Created attachment 132333 [details]
Patch

[Michal Mocny]

Comment on attachment 132333 [details]
Patch

Added a default implementation for allocation callback taking the struct.

[WebKit Review Bot]

Comment on attachment 132333 [details]
Patch

Clearing flags on attachment: 132333

Committed r111216: <http://trac.webkit.org/changeset/111216>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.