Devirtualize supportsSpatialNavigationFocus

Devirtualize supportsSpatialNavigationFocus

Devirtualize supportsSpatialNavigationFocus, there is no need to use virtual here. Removing this also decreases the binary size.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177974

[rwlbuis, 2014-07-10 21:00:42]

PTAL

[pdr., 2014-07-10 21:19:24]

This looks good in general.

Two minor questions.

https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2186: || (isSVGElement() &&
(hasEventListeners(EventTypeNames::focus)
I don't think the indentation is correct here, or it's at least hard to read.
Can you refactor this so it's easy to tell that focus/blur/etc only apply to the
isSVGElement() case?


SVGElement doesn't seem to be special here. Should blur/focus/etc apply to all
elements and not just SVGElement?

[rwlbuis, 2014-07-10 22:37:53]

On 2014/07/10 21:19:24, pdr wrote:
> This looks good in general.
> 
> Two minor questions.
> 
> https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp#...
> Source/core/dom/Element.cpp:2186: || (isSVGElement() &&
> (hasEventListeners(EventTypeNames::focus)
> I don't think the indentation is correct here, or it's at least hard to read.
> Can you refactor this so it's easy to tell that focus/blur/etc only apply to
the
> isSVGElement() case?
> 
> 
> SVGElement doesn't seem to be special here. Should blur/focus/etc apply to all
> elements and not just SVGElement?

So I made version two where we always do that and it is green. I don't know if
it is correct though and Ed is probably OOO. Maybe I can find something in the
spec...

[fs, 2014-07-11 08:15:29]

On 2014/07/10 22:37:53, rwlbuis wrote:
> On 2014/07/10 21:19:24, pdr wrote:
> > This looks good in general.
> > 
> > Two minor questions.
> > 
> > https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp
> > File Source/core/dom/Element.cpp (right):
> > 
> >
>
https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp#...
> > Source/core/dom/Element.cpp:2186: || (isSVGElement() &&
> > (hasEventListeners(EventTypeNames::focus)
> > I don't think the indentation is correct here, or it's at least hard to
read.
> > Can you refactor this so it's easy to tell that focus/blur/etc only apply to
> the
> > isSVGElement() case?
> > 
> > 
> > SVGElement doesn't seem to be special here. Should blur/focus/etc apply to
all
> > elements and not just SVGElement?
> 
> So I made version two where we always do that and it is green. I don't know if
> it is correct though and Ed is probably OOO. Maybe I can find something in the
> spec...

Since this input into the spatial navigation heuristic, I don't think there's
any spec. coverage of this - if any it'd be in 1.2 tiny (ref below). I'm
including the spec-citation + additions from Presto (which I believe is what ed
based the extend list in SVGElement on.) Enjoy.

/*
 * Focusable=auto (should evaluate to true if one of these conditions hold):
 *	- the a element
 *	- the text and textArea elements with editable set to true
 *	- elements that are the target of an animation whose begin or end criteria
 *	  is triggered by the following user interface events: focusin, focusout,
activate
 *	- elements that are the target or observer of a listener element that is
registered
 *	  on one of the following user interface events: focusin, focusout, activate
 *
 *	Also we allow events: focus, blur. Those events are not in the spec, but are
handled by
 *  Mozilla.
 *
 * Reference: http://www.w3.org/TR/SVGMobile12/interact.html#focus
 */

Playing it safe probably means leaving in the isSVGElement().

[pdr., 2014-07-11 17:46:54]

On 2014/07/11 08:15:29, fs wrote:
> On 2014/07/10 22:37:53, rwlbuis wrote:
> > On 2014/07/10 21:19:24, pdr wrote:
> > > This looks good in general.
> > > 
> > > Two minor questions.
> > > 
> > >
https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp
> > > File Source/core/dom/Element.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/382043002/diff/1/Source/core/dom/Element.cpp#...
> > > Source/core/dom/Element.cpp:2186: || (isSVGElement() &&
> > > (hasEventListeners(EventTypeNames::focus)
> > > I don't think the indentation is correct here, or it's at least hard to
> read.
> > > Can you refactor this so it's easy to tell that focus/blur/etc only apply
to
> > the
> > > isSVGElement() case?
> > > 
> > > 
> > > SVGElement doesn't seem to be special here. Should blur/focus/etc apply to
> all
> > > elements and not just SVGElement?
> > 
> > So I made version two where we always do that and it is green. I don't know
if
> > it is correct though and Ed is probably OOO. Maybe I can find something in
the
> > spec...
> 
> Since this input into the spatial navigation heuristic, I don't think there's
> any spec. coverage of this - if any it'd be in 1.2 tiny (ref below). I'm
> including the spec-citation + additions from Presto (which I believe is what
ed
> based the extend list in SVGElement on.) Enjoy.
> 
> /*
>  * Focusable=auto (should evaluate to true if one of these conditions hold):
>  *	- the a element
>  *	- the text and textArea elements with editable set to true
>  *	- elements that are the target of an animation whose begin or end criteria
>  *	  is triggered by the following user interface events: focusin, focusout,
> activate
>  *	- elements that are the target or observer of a listener element that is
> registered
>  *	  on one of the following user interface events: focusin, focusout,
activate
>  *
>  *	Also we allow events: focus, blur. Those events are not in the spec, but
are
> handled by
>  *  Mozilla.
>  *
>  * Reference: http://www.w3.org/TR/SVGMobile12/interact.html#focus
>  */
> 
> Playing it safe probably means leaving in the isSVGElement().

I agree as well.

LGTM with the current patch.

[rwlbuis, 2014-07-11 18:28:33]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-07-11 18:29:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rob.buis@samsung.com/382043002/40001

[commit-bot: I haz the power, 2014-07-11 19:28:31]

Change committed as 177974