1119527 - Implement clearHitRegions

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Florian Scholz (Open Web Docs)]

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

Comment 1

[Florian Scholz (Open Web Docs)]

Attachment: bug1119527.patch

Comment 2

[Florian Scholz (Open Web Docs)]

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 3

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 4

[Florian Scholz (Open Web Docs)]

Attachment: bug1119527.patch

Comment 5

[Florian Scholz (Open Web Docs)]

(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.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

A try run and this should be ready to go...

Comment 7

[Florian Scholz (Open Web Docs)]

I have no try server access. Could someone push that for me? :)

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Sure! https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f665990af13

Comment 9

[Florian Scholz (Open Web Docs)]

If I interpret the try run correctly, it looks all good. Setting checkin-needed.

Comment 10

[Carsten Book [:Tomcat]]

this need Dom peer review for the change in  WebIDL file dom/webidl/CanvasRenderingContext2D.webidl

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546729 [details] [diff] [review]
bug1119527.patch

r=me

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f3db72a62c

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#Interfaces.2FAPIs.2FDOM
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D.clearHitRegions