Implement basic parts of hit regions on canvas2d.

Source/core/html/canvas/CanvasRenderingContext2D.cpp

Index: Source/core/html/canvas/CanvasRenderingContext2D.cpp
diff --git a/Source/core/html/canvas/CanvasRenderingContext2D.cpp b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
index 41e63a591a501682e59282f86ccf09652b1d04ae..d6b53a632cd6818466250ef73d47d22214255dc7 100644
--- a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
+++ b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
@@ -37,7 +37,6 @@
 #include "bindings/v8/ExceptionMessages.h"
 #include "bindings/v8/ExceptionState.h"
 #include "bindings/v8/ExceptionStatePlaceholder.h"
-#include "core/accessibility/AXObjectCache.h"
 #include "core/css/CSSFontSelector.h"
 #include "core/css/parser/BisonCSSParser.h"
 #include "core/css/StylePropertySet.h"
@@ -86,6 +85,7 @@ static bool contextLostRestoredEventsEnabled()
 
 CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement* canvas, const Canvas2DContextAttributes* attrs, bool usesCSSCompatibilityParseMode)
     : CanvasRenderingContext(canvas)
+    , m_hitRegionManager(HitRegionManager::create())

[Rik, 2014/06/02 22:57:40]

do you want to do this lazily?

[zino, 2014/06/06 06:35:15]

Done.

     , m_usesCSSCompatibilityParseMode(usesCSSCompatibilityParseMode)
     , m_hasAlpha(!attrs || attrs->alpha())
     , m_isContextLost(false)
@@ -1245,6 +1245,7 @@ void CanvasRenderingContext2D::clearRect(float x, float y, float width, float he
         context->setCompositeOperation(CompositeSourceOver);
     }
     context->clearRect(rect);
+    m_hitRegionManager->removeHitRegionsInRect(rect, state().m_transform);
     if (saved)
         context->restore();
 
@@ -2357,4 +2358,68 @@ void CanvasRenderingContext2D::drawFocusRing(const Path& path)
     didDraw(dirtyRect);
 }
 
+void CanvasRenderingContext2D::addHitRegion(ExceptionState& exceptionState)
+{
+    addHitRegion(Dictionary(), exceptionState);
+}
+
+void CanvasRenderingContext2D::addHitRegion(const Dictionary& options, ExceptionState& exceptionState)
+{
+    HitRegionOptions passOptions;
+
+    bool idIsNull = !(options.getWithUndefinedOrNullCheck("id", passOptions.id)) || passOptions.id.isEmpty();

[dmazzoni, 2014/06/03 17:26:10]

"null" isn't quite accurate since it could be empty or undefined. How about
reversing it to "hasId" and "hasControl"?

[zino, 2014/06/06 06:35:16]

Done.

+    bool controlIsNull = !options.getWithUndefinedOrNullCheck("control", passOptions.control) || !passOptions.control;

[fs, 2014/06/03 11:47:01]

controlIsNull seems redundant with passOptions.control. (Ditto for idIsNull vs.
passOptions.id.isEmpty())

+
+    if (passOptions.control && !passOptions.control->isDescendantOf(canvas())) {
+        passOptions.control = nullptr;

[Rik, 2014/06/02 22:57:40]

why are you doing this?
It's ok for a control to start out outside the canvas and then moved to the
canvas later.
Better not to check for isdescendant here.

[zino, 2014/06/06 06:35:15]

Done.

+        controlIsNull = true;
+    }
+
+    if (idIsNull && controlIsNull) {
+        exceptionState.throwDOMException(NotSupportedError, "Both id and control are null or undefined or invalid type.");

[fs, 2014/06/03 11:47:01]

Suggest you drop " or undefined or invalid type". (If 'id' is specified and
undefined, the default value - null - will be used. Similarly I think an
incorrect type should've been handled by the ES -> IDL dictionary conversion
[which isn't implemented yet in Blink.)

[dmazzoni, 2014/06/03 17:26:10]

What about an empty string?

[fs, 2014/06/04 08:13:40]

Empty strings are coerced to null explicitly (addHitRegion() step 5, "If the
arguments object's id member is an empty string, let it be null instead.")

[zino, 2014/06/06 06:35:15]

You're right but dictionary.get() can't do check null or undefined.
So, following codes work abnormally.

context.addHitRegion({
    id : null    // expected - empty string, actual - ???
});

context.addHitRegion({
    id : undefined    // expected - empty string, actual - 'undefined'
});

There was a similar case. please refer to
'https://codereview.chromium.org/290143007'

[fs, 2014/06/09 11:06:26]

You're not using "plain" get() though, but rather getWithUndefinedOrNullCheck()
- which explicitly filters out null/undefined (i.e. giving nullable treatment).
In both of these cases getWithUndefinedOrNullCheck() will return false (and
don't modify its out-parameter), so semantics-wise you're fine as long as you
pass an isEmpty() String(). I.e. there'll be no ToString operation on
null/undefined - which there would've been with "plain" get() (and with "plain"
get you'd have 'null' [the string] in the first case.)
 
Since that is using lower-level primitives it's not directly comparable.
(Tangent: It's also incorrect wrt how it treats null values, since only
'undefined' will cause default-value handling [WebIDL 4.2.20 step 5.1.4])

[zino, 2014/06/09 20:00:08]

Thank you for detailed explanations :)
BTW, I was wondering whether you'd like this to be fixed or not.

[fs, 2014/06/10 07:25:47]

I think fixing it would reduce clutter - and in the case of the exception
message align more to where we want to be. So I suppose the exception message
could stay for now if there's a test for it (which I believe there is.)

[zino, 2014/06/10 08:21:35]

You seems to want the exception message to be modified.
Is my understanding correct?
If so, I've already fixed since patch set 4.

I was wondering about below your comment.
----------------------------------------------------------------------------------------------
controlIsNull seems redundant with passOptions.control. (Ditto for idIsNull vs.
passOptions.id.isEmpty())
----------------------------------------------------------------------------------------------
As you mentioned, getWithUndefinedOrNullCheck() will return false and doesn't
modify its out-parameter.
So, I think we can do early null check without creating of empty string object
because of short-circuit.
Do I need to modify this more?

[fs, 2014/06/10 09:05:49]

I'm fine with the exception message in its current (PS4+) form.
What I meant was that you could do just:

options.getWithUndefinedOrNullCheck("id", passOptions.id);
options.getWithUndefinedOrNullCheck("control", passOptions.control);
if (passOptions.id.isEmpty() && !passOptions.control)
    ...

which is very marginally (if at all - the controls-check is likely cheaper since
it avoids coercion into a boolean) worse, and IMHO easier to read. I welcome
others to chime in though.

+        return;
+    }
+
+    if (m_path.isEmpty()) {

[fs, 2014/06/03 11:47:01]

IIRC, Path::isEmpty means "does not have any segments". The W3 ED seems to say
"If the specified path has no pixels, ...", which suppose could be interpreted
this way, but could also be interpreted as "does not intersect any (CSS) pixel
centers". Rik?

[zino, 2014/06/06 06:35:15]

Done.

+        exceptionState.throwDOMException(NotSupportedError, "The specified path has no pixels.");
+        return;
+    }
+
+    passOptions.path = m_path;
+    passOptions.path.transform(state().m_transform);
+
+    addHitRegionInternal(passOptions, exceptionState);
+}
+
+void CanvasRenderingContext2D::addHitRegionInternal(const HitRegionOptions& options, ExceptionState& exceptionState)
+{
+    // Remove previous region (with id or control)
+    m_hitRegionManager->removeHitRegionById(options.id);
+    m_hitRegionManager->removeHitRegionByControl(options.control.get());
+
+    RefPtrWillBeRawPtr<HitRegion> hitRegion = HitRegion::create(options);
+    hitRegion->updateAccessibility(canvas());
+    m_hitRegionManager->addHitRegion(hitRegion.release());
+}
+
+void CanvasRenderingContext2D::removeHitRegion(const String& id)
+{
+    m_hitRegionManager->removeHitRegionById(id);
+}
+
+void CanvasRenderingContext2D::clearHitRegions()
+{
+    m_hitRegionManager->removeAllHitRegions();
+}
+
+unsigned CanvasRenderingContext2D::hitRegionsCount() const
+{
+    return m_hitRegionManager->getHitRegionsCount();
+}
+
+HitRegion* CanvasRenderingContext2D::hitRegionAtPoint(const LayoutPoint& point)
+{
+    return m_hitRegionManager->getHitRegionAtPoint(point);
+}
+
 } // namespace WebCore