1439006 - Allow multiple kinds of WebRenderUserData on a DisplayItem

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

Currently we can only have one type of WebRenderUserData on an Item. We already have a hash table of WebRenderUserData so it's not hard to include type in the hash to support one per type.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Allow multiple kinds of WebRenderUserData on a DisplayItem

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8951774 [details] [diff] [review]
Allow multiple kinds of WebRenderUserData on a DisplayItem

Review of attachment 8951774 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLCanvasElement.cpp
@@ +1089,5 @@
>    if (frame->HasProperty(nsIFrame::WebRenderUserDataProperty())) {
>      nsIFrame::WebRenderUserDataTable* userDataTable =
>        frame->GetProperty(nsIFrame::WebRenderUserDataProperty());
>      RefPtr<WebRenderUserData> data;
> +    userDataTable->Get(WebRenderUserDataKey(static_cast<uint32_t>(DisplayItemType::TYPE_CANVAS), WebRenderUserData::UserDataType::eCanvas), getter_AddRefs(data));

This is an extremely long line. How about

// nsDisplayCanvas does not override GetPerFrameKey(), so the
// perFrameKey is just the display item type as uint32.
uint32_t perFrameKey = static_cast<uint32_t>(DisplayItemType::TYPE_CANVAS);
userDataTable->Get(WebRenderUserData::UserDataType::eCanvas, perFrameKey, getter_AddRefs(data));

And make WebRenderUserDataTable a real struct instead of a typedef, with Get, GetOrInsert and Remove methods that operate on the underlaying hashtable and use WebRenderUserDataKey internally.

::: gfx/layers/wr/WebRenderCommandBuilder.h
@@ +166,5 @@
>      if (!userDataTable) {
>        return nullptr;
>      }
>  
> +    RefPtr<WebRenderUserData> data = userDataTable->Get(WebRenderUserDataKey(aPerFrameKey, T::Type()));

line break after =

::: gfx/layers/wr/WebRenderUserData.cpp
@@ +30,5 @@
>    nsIFrame::WebRenderUserDataTable* userDataTable =
>      aFrame->GetProperty(nsIFrame::WebRenderUserDataProperty());
>  
> +  userDataTable->Get(WebRenderUserDataKey(static_cast<uint32_t>(DisplayItemType::TYPE_VIDEO), WebRenderUserData::UserDataType::eImage),
> +                     getter_AddRefs(data));

Please add a comment like in the canvas case.

::: gfx/layers/wr/WebRenderUserData.h
@@ +96,5 @@
> +    return mFrameKey == other.mFrameKey && mType == other.mType;
> +  }
> +  PLDHashNumber Hash() const
> +  {
> +    return HashGeneric(mFrameKey, static_cast<std::underlying_type<decltype(mType)>::type>(mType));

Please write a comment that points to the bug about making HashGeneric support enum classes.

@@ +99,5 @@
> +  {
> +    return HashGeneric(mFrameKey, static_cast<std::underlying_type<decltype(mType)>::type>(mType));
> +  }
> +
> +  uint32_t mFrameKey;

Please call this mPerFrameKey. It's something that's used to key *in addition to* the frame address, not something that keys the frame.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #2)
> Comment on attachment 8951774 [details] [diff] [review]
> Allow multiple kinds of WebRenderUserData on a DisplayItem
> 
> Review of attachment 8951774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLCanvasElement.cpp
> @@ +1089,5 @@
> >    if (frame->HasProperty(nsIFrame::WebRenderUserDataProperty())) {
> >      nsIFrame::WebRenderUserDataTable* userDataTable =
> >        frame->GetProperty(nsIFrame::WebRenderUserDataProperty());
> >      RefPtr<WebRenderUserData> data;
> > +    userDataTable->Get(WebRenderUserDataKey(static_cast<uint32_t>(DisplayItemType::TYPE_CANVAS), WebRenderUserData::UserDataType::eCanvas), getter_AddRefs(data));
> 
> This is an extremely long line. How about
> 
> // nsDisplayCanvas does not override GetPerFrameKey(), so the
> // perFrameKey is just the display item type as uint32.
> uint32_t perFrameKey = static_cast<uint32_t>(DisplayItemType::TYPE_CANVAS);
> userDataTable->Get(WebRenderUserData::UserDataType::eCanvas, perFrameKey,
> getter_AddRefs(data));
> 
> And make WebRenderUserDataTable a real struct instead of a typedef, with
> Get, GetOrInsert and Remove methods that operate on the underlaying
> hashtable and use WebRenderUserDataKey internally.

It would be better just to have methods per type. All of the callers have statically know the type that they want so we should probably just encode it in the name.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Allow multiple kinds of WebRenderUserData on a DisplayItem

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8959652 [details] [diff] [review]
Allow multiple kinds of WebRenderUserData on a DisplayItem

Review of attachment 8959652 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderUserData.h
@@ +6,5 @@
>  
>  #ifndef GFX_WEBRENDERUSERDATA_H
>  #define GFX_WEBRENDERUSERDATA_H
>  
> +#include <type_traits>

unnecessary

@@ +32,5 @@
>  class WebRenderImageData;
>  class WebRenderFallbackData;
>  class WebRenderLayerManager;
>  
> +

unnecessary

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0200f9fc1ab
Allow multiple kinds of WebRenderUserData on a DisplayItem. r=mstange

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c0200f9fc1ab