Only 'form associated' elements have a potential form owner.

Only 'form associated' elements have a potential form owner.

The spec tells us that only certain elements have the notion of a form
owner,

 http://www.whatwg.org/specs/web-apps/current-work/#form-associated-element
 http://www.whatwg.org/specs/web-apps/current-work/#form-owner

Implement that selective notion of 'form owner' over elements.

Internally, make use of a more precise form owner notion over elements
to slightly simplify form lookups (RadioNodeList + current form
selection in a frame.)

R=arv@chromium.org,tkent@chromium.org
BUG=324828
TEST=fast/forms/form-associated-element

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

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

[sof, 2013-12-10 14:50:10]

Please take a look when you next have a moment.

[This is the 3rd & final CL from https://codereview.chromium.org/107183004/ . It
depends on https://codereview.chromium.org/102263006/ before it can be
successfully tried & landed.]

[sof, 2013-12-10 17:26:05]

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLLabelEl...
File Source/core/html/HTMLLabelElement.h (left):

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLLabelEl...
Source/core/html/HTMLLabelElement.h:37: HTMLFormElement* form() const;
The removal of this method would have belonged better in the "fromOwner()" CL
(https://codereview.chromium.org/93723003/), but didn't end up there. Mea culpa.

No change in functionality.

[arv (Not doing code reviews), 2013-12-10 20:37:19]

LGTM

https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
Source/core/editing/FrameSelection.cpp:1743: HTMLFormElement* owner =
toHTMLElement(element)->formOwner();
You could do:

if (HTMLFormElement* owner = toHTMLElement(element)->formOwner())
  return owner;

I'm not sure which style is generally preferred.

[sof, 2013-12-10 21:04:53]

On 2013/12/10 20:37:19, arv wrote:
> LGTM
> 
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> File Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> Source/core/editing/FrameSelection.cpp:1743: HTMLFormElement* owner =
> toHTMLElement(element)->formOwner();
> You could do:
> 
> if (HTMLFormElement* owner = toHTMLElement(element)->formOwner())
>   return owner;
> 
> I'm not sure which style is generally preferred.

Yes, I can live with either form, but I do prefer null checking via such a
binding construct.

The suggestion in an earlier review to shy away from it,

  https://codereview.chromium.org/56973002/#msg2

is why I don't add new code with that idiom.

[adamk, 2013-12-10 21:11:00]

On 2013/12/10 21:04:53, sof wrote:
> On 2013/12/10 20:37:19, arv wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> > File Source/core/editing/FrameSelection.cpp (right):
> > 
> >
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> > Source/core/editing/FrameSelection.cpp:1743: HTMLFormElement* owner =
> > toHTMLElement(element)->formOwner();
> > You could do:
> > 
> > if (HTMLFormElement* owner = toHTMLElement(element)->formOwner())
> >   return owner;
> > 
> > I'm not sure which style is generally preferred.
> 
> Yes, I can live with either form, but I do prefer null checking via such a
> binding construct.
> 
> The suggestion in an earlier review to shy away from it,
> 
>   https://codereview.chromium.org/56973002/#msg2
> 
> is why I don't add new code with that idiom.

I think dslomov's opinion stated in that comment doesn't match common Blink (not
just WebKit) practice. It's neither encouraged nor forbidden by the style guide,
but it's very common even in new Blink code.

[sof, 2013-12-10 21:24:32]

On 2013/12/10 21:11:00, adamk wrote:
> On 2013/12/10 21:04:53, sof wrote:
> > On 2013/12/10 20:37:19, arv wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> > > File Source/core/editing/FrameSelection.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/106423005/diff/1/Source/core/editing/FrameSel...
> > > Source/core/editing/FrameSelection.cpp:1743: HTMLFormElement* owner =
> > > toHTMLElement(element)->formOwner();
> > > You could do:
> > > 
> > > if (HTMLFormElement* owner = toHTMLElement(element)->formOwner())
> > >   return owner;
> > > 
> > > I'm not sure which style is generally preferred.
> > 
> > Yes, I can live with either form, but I do prefer null checking via such a
> > binding construct.
> > 
> > The suggestion in an earlier review to shy away from it,
> > 
> >   https://codereview.chromium.org/56973002/#msg2
> > 
> > is why I don't add new code with that idiom.
> 
> I think dslomov's opinion stated in that comment doesn't match common Blink
(not
> just WebKit) practice. It's neither encouraged nor forbidden by the style
guide,
> but it's very common even in new Blink code.

Good, I'll try to pick up on where it fits with local style.

[tkent, 2013-12-10 23:19:27]

Please investigate test failures.

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:745: HTMLFormElement* HTMLElement::formOwner()
const
This function always calls a virtual function isFormAssociatedElement(), and
calls another virtual function virtualForm() if this is
isFormAssociatedElement(). It's inefficient.

Why don't you just make formOwner() virtual, and override it in form-associated
elements?

Unfortunately we already have FormAssociatedElement class and
isFormAssociatedElement() doesn't match to it. It's very confusing.

[sof, 2013-12-11 07:08:25]

On 2013/12/10 23:19:27, tkent wrote:
> Please investigate test failures.
> 

See comment made when sending out this CL re: dependencies.

With that dependent fix now landed, all green.

(If there's a convention/process to record these dependencies more clearly so
that this kind of workflow inefficiency don't happen again, please let me know.)

[sof, 2013-12-11 07:55:03]

Thanks for balking at how the methods were structured; I'd watched myself blind
at their lack of beauty. :)

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/106423005/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:745: HTMLFormElement* HTMLElement::formOwner()
const
On 2013/12/10 23:19:28, tkent wrote:
> This function always calls a virtual function isFormAssociatedElement(), and
> calls another virtual function virtualForm() if this is
> isFormAssociatedElement(). It's inefficient.
> 
> Why don't you just make formOwner() virtual, and override it in
form-associated
> elements?
> 
> Unfortunately we already have FormAssociatedElement class and
> isFormAssociatedElement() doesn't match to it. It's very confusing.

Agreed, too much indirection here.

As suggested, I've virtualized formOwner() instead, doing away with the explicit
is-form-associated predicate. Looks better?

(The reason there isn't an exact overlap with FormAssociatedElement and the
form-associated predicate in the spec is <label> and <img>.)

[tkent, 2013-12-11 08:52:12]

On 2013/12/11 07:08:25, sof wrote:
> On 2013/12/10 23:19:27, tkent wrote:
> > Please investigate test failures.
> > 
> 
> See comment made when sending out this CL re: dependencies.

I'm sorry for that.  I overlooked it.

[tkent, 2013-12-11 08:53:32]

The patch looks great. I'd say ＬＧＴＭ if try bots are green.

[sof, 2013-12-11 08:54:20]

On 2013/12/11 08:52:12, tkent wrote:
> On 2013/12/11 07:08:25, sof wrote:
> > On 2013/12/10 23:19:27, tkent wrote:
> > > Please investigate test failures.
> > > 
> > 
> > See comment made when sending out this CL re: dependencies.
> 
> I'm sorry for that.  I overlooked it.

It's failing to apply now though for the trybots due to a HTMLElement.h conflict
(insertAdjacentHTML related). Rebasing.

[tkent, 2013-12-11 09:28:04]

lgtm

[sof, 2013-12-11 09:30:56]

On 2013/12/11 09:28:04, tkent wrote:
> lgtm

Excellent, thanks :) Green again post-rebase.

[commit-bot: I haz the power, 2013-12-11 09:31:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/106423005/40001

[commit-bot: I haz the power, 2013-12-11 10:39:21]

Change committed as 163697

[Justin Novosad, 2013-12-11 16:33:13]

On 2013/12/11 10:39:21, I haz the power (commit-bot) wrote:
> Change committed as 163697

This change appears to be causing test fast/forms/form-associated-element.html
to timeout on mac.

[Justin Novosad, 2013-12-11 16:45:03]

Reverted with r163697

Failures:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...

[sof, 2013-12-11 16:54:02]

On 2013/12/11 16:45:03, junov wrote:
> Reverted with r163697
> 
> Failures:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...

ok, thanks - will address it. A dozen programmatically dispatched click events
taking that long to process?

[sof, 2013-12-11 20:52:06]

I switched the test over to using 'error' instead of 'click' events to test
scoping wrt "form owner" -- could you please have a look if
fast/forms/form-associated-element is now ok?

Running times (in ms) from trybot runs:

      Patchset 3    Patchset 4
win:     2795            52
mac:     4920            33
lin:     1813            34

I tried adjusting propagation of click events locally to see if that made much
of a difference on running times. It didn't, but using 'error' avoids the
slowness that 'click' runs into.

[tkent, 2013-12-11 22:47:23]

lgtm

[commit-bot: I haz the power, 2013-12-11 22:48:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/106423005/60001

[commit-bot: I haz the power, 2013-12-12 00:15:18]

Change committed as 163738