Closed Bug 960254 Opened 6 years ago Closed 6 years ago

Add ability to properly manage write access for DataSourceSurfaces

Categories

(Core :: Graphics, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

(Whiteboard: [qa-])

Attachments

(4 files)

Right now we have a GetData() caller on DataSourceSurfaces. This is not really what we want, in some cases you can write, in other cases you can't. An explicit Map/Unmap pair is much more like what we want.
Comment on attachment 8360653 [details] [diff] [review]
Part 1: Add Map/Unmap API to DataSourceSurface

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

::: gfx/2d/2D.h
@@ +361,5 @@
>     * Can return null if there was OOM allocating surface data.
>     */
>    virtual uint8_t *GetData() = 0;
>  
> +  /* [DEPTRECATED]

spelling is wrong.
Attachment #8360653 - Flags: review?(jmuizelaar) → review+
Attachment #8360656 - Flags: review?(jmuizelaar) → review+
Attachment #8360665 - Flags: review?(jmuizelaar) → review+
Attachment #8360666 - Flags: review?(jmuizelaar) → review+
Thanks. I really disliked MarkDirty() but I didn't have the right fix for it.
Shouldn't there be an RAII helper object for this?

Are we going to convert all users of Data/Stride to this?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #10)
> Shouldn't there be an RAII helper object for this?
> 
> Are we going to convert all users of Data/Stride to this?

Eventually I'd like to, yes. Also I have no objection to an RAII helper. Although personally I often to prefer explicit locking/unlocking because it makes us explicitly think about where we do the lock when we for example start adding a lot of branches that exit a function. On the other hand I've been guilty of forgetting to add an unlock in one of these branches regardless so I also see the value in using RAII helpers :). In this case all the locks/unlocks could occur close together so I didn't feel there was a lot of value in having a helper here.
Whiteboard: [qa-]
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Shouldn't there be an RAII helper object for this?

I think there should be. Bug 979853 comment 6 notes that we're already screwing up the Unmap() calls, and calling DataSourceSurfaceD2D::Unmap() seems guaranteed to crash debug builds. Probably best to discuss this in bug 980233 though.
You need to log in before you can comment on or make changes to this bug.