Closed Bug 1119527 Opened 9 years ago Closed 9 years ago

Implement clearHitRegions

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: fs, Assigned: fs)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-blink] gfx-noted)

Attachments

(1 file, 1 obsolete file)

void ctx.clearHitRegions() was added to canvas in https://html5.org/r/8747

"When the clearHitRegions() method is invoked, the user agent must remove all the hit regions from the rendering context's scratch bitmap's hit region list."

MDN https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.clearHitRegions
Attached patch bug1119527.patch (obsolete) — Splinter Review
I usually only document and not implement features, but I thought I give this a shot. Feel free to tweak as needed or let me know what to do here.
Comment on attachment 8546293 [details] [diff] [review]
bug1119527.patch

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

Drive by feedback...

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3232,5 @@
>  
> +void
> +CanvasRenderingContext2D::ClearHitRegions()
> +{
> +  mHitRegionsOptions.ClearAndRetainStorage();

Why not just Clear()?

::: dom/canvas/test/test_hitregion_canvas.html
@@ +39,5 @@
>      ctx.removeHitRegion("b");
> +
> +    ctx.addHitRegion("x");
> +    ctx.addHitRegion("y");
> +    ctx.clearHitRegions();

I don't think there are regions x and y - I'd just add clearHitRegions() after all the removes - to test if clear works properly when there are no regions, but also copy the whole try{} and do clear instead of the three removes, to test the clear when there are valid regions around.
Whiteboard: [parity-blink] → [parity-blink] gfx-noted
Attached patch bug1119527.patchSplinter Review
Attachment #8546293 - Attachment is obsolete: true
(In reply to Milan Sreckovic [:milan] from comment #3)
> > +  mHitRegionsOptions.ClearAndRetainStorage();
> 
> Why not just Clear()?
> 

No idea, I found that in CanvasRenderingContext2D::Reset() and thought it would do the right thing.
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1037
Changed to just Clear() which seems to make more sense.

> I don't think there are regions x and y - I'd just add clearHitRegions()
> after all the removes - to test if clear works properly when there are no
> regions, but also copy the whole try{} and do clear instead of the three
> removes, to test the clear when there are valid regions around.

Done.
Attachment #8546729 - Flags: review?(gwright) → review+
A try run and this should be ready to go...
Assignee: nobody → fscholz
I have no try server access. Could someone push that for me? :)
If I interpret the try run correctly, it looks all good. Setting checkin-needed.
Keywords: checkin-needed
this need Dom peer review for the change in  WebIDL file dom/webidl/CanvasRenderingContext2D.webidl
Flags: needinfo?(fscholz)
Keywords: checkin-needed
Flags: needinfo?(fscholz)
Attachment #8546729 - Flags: review?(bzbarsky)
Comment on attachment 8546729 [details] [diff] [review]
bug1119527.patch

r=me
Attachment #8546729 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a0f3db72a62c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.