Closed Bug 1439006 Opened 4 years ago Closed 4 years ago

Allow multiple kinds of WebRenderUserData on a DisplayItem

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → jmuizelaar
Attachment #8951774 - Flags: review?(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.
(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.
Attachment #8951774 - Flags: review?(mstange)
Attachment #8951774 - Attachment is obsolete: true
Attachment #8959652 - Flags: review?(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
Attachment #8959652 - Flags: review?(mstange) → review+
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0200f9fc1ab
Allow multiple kinds of WebRenderUserData on a DisplayItem. r=mstange
https://hg.mozilla.org/mozilla-central/rev/c0200f9fc1ab
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1487120
You need to log in before you can comment on or make changes to this bug.