Don't updateRenderTree in the middle of a DOM insertion.

Don't updateRenderTree in the middle of a DOM insertion.

After [1] landed, this CL is now a revert of using a flag for postponing
scrolling, and scroll from didNotifySubtreeInsertion instead. The original
description for this fix below. The crasher was fixed by [1].
    
[1] https://codereview.chromium.org/783983005

The original description:

When inserting an OPTION element with the selected attribute set, we
synchronously scroll to that OPTION to make it visible. A prerequisite for
scrolling is to have the render tree up-to-date. At this point only parts
of the inserted fragment had its inDocument() flag set which caused a crash
when a nullptr was returned for the computed style of an input spinner.

Ideally, we should not scroll synchronously from the dom insertion code.
This CL moves scrolling from insertedInto() to didNotifySubtreeInsertion().
The inDocument() flags are up-to-date when didNotifySubtreeInsertion()
is called.

An ASSERT is added to Element::recalcStyle to catch other cases of calling
updateRenderTree in the middle of a dom insertion.

R=esprehn@chromium.org
BUG=438615
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187437

[rune, 2014-12-10 14:41:07]

PTAL.

It took me forever to figure out I needed a (text-)node before the <form> in the
test-case to trigger the crasher.

[rune, 2014-12-12 15:44:44]

ping

[dglazkov, 2014-12-12 15:59:57]

dglazkov@chromium.org changed reviewers:
+ dglazkov@chromium.org

[dglazkov, 2014-12-12 15:59:57]

lgtm

[rune, 2014-12-13 01:57:19]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-13 01:58:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791533004/1

[commit-bot: I haz the power, 2014-12-13 04:33:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-13 04:33:30]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)

[chrishtr, 2014-12-13 05:33:01]

On 2014/12/13 at 04:33:30, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)

Is it not web compatible to just dump this behavior?

Also, the phrasing of sentence 2, paragraph 3 of the CL description is
backwards.

[rune, 2014-12-15 10:24:37]

On 2014/12/13 at 05:33:01, chrishtr wrote:
> On 2014/12/13 at 04:33:30, commit-bot wrote:
> > Try jobs failed on following builders:
> >   mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)
> 
> Is it not web compatible to just dump this behavior?

You mean not to scroll at all when options are selected, or selected options are
inserted into the dom?

> Also, the phrasing of sentence 2, paragraph 3 of the CL description is
backwards.

Not sure I understood. Have split it into two sentences. Better now?

[rune, 2014-12-16 23:49:54]

On 2014/12/15 at 10:24:37, rune wrote:
> On 2014/12/13 at 05:33:01, chrishtr wrote:
> > On 2014/12/13 at 04:33:30, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)
> > 
> > Is it not web compatible to just dump this behavior?
> 
> You mean not to scroll at all when options are selected, or selected options
are inserted into the dom?
> 
> > Also, the phrasing of sentence 2, paragraph 3 of the CL description is
backwards.
> 
> Not sure I understood. Have split it into two sentences. Better now?

chrishtr@ ping?

[esprehn, 2014-12-16 23:57:33]

lgtm

[rune, 2014-12-17 00:22:50]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-17 00:23:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791533004/1

[commit-bot: I haz the power, 2014-12-17 00:23:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 00:23:49]

Failed to apply patch for Source/core/html/HTMLOptionElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLOptionElement.cpp
  Hunk #1 succeeded at 210 with fuzz 2 (offset -120 lines).
  Hunk #2 FAILED at 348.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/html/HTMLOptionElement.cpp.rej

Patch:       Source/core/html/HTMLOptionElement.cpp
Index: Source/core/html/HTMLOptionElement.cpp
diff --git a/Source/core/html/HTMLOptionElement.cpp
b/Source/core/html/HTMLOptionElement.cpp
index
90a3895b670fb248d30a38285853dbce40a046f8..9328d7064818ebb8b8f2af0d70dff15f0017e9eb
100644
--- a/Source/core/html/HTMLOptionElement.cpp
+++ b/Source/core/html/HTMLOptionElement.cpp
@@ -330,6 +330,12 @@ bool HTMLOptionElement::isDisabledFormControl() const
 
 Node::InsertionNotificationRequest
HTMLOptionElement::insertedInto(ContainerNode* insertionPoint)
 {
+    HTMLElement::insertedInto(insertionPoint);
+    return InsertionShouldCallDidNotifySubtreeInsertions;
+}
+
+void HTMLOptionElement::didNotifySubtreeInsertionsToDocument()
+{
     if (HTMLSelectElement* select = ownerSelectElement()) {
         select->setRecalcListItems();
         // Do not call selected() since calling updateListItemSelectedStates()
@@ -342,8 +348,6 @@ Node::InsertionNotificationRequest
HTMLOptionElement::insertedInto(ContainerNode
             select->scrollToSelection();
         }
     }
-
-    return HTMLElement::insertedInto(insertionPoint);
 }
 
 void HTMLOptionElement::removedFrom(ContainerNode* insertionPoint)

[rune, 2014-12-17 14:05:49]

Ended up in a conflict with [1]. I think this CL is cleaner/clearer, so it's now
removing the flag that was introduced by [1]. I'm afraid it needs another round
of review now.

[1] https://codereview.chromium.org/783983005

[rune, 2014-12-18 06:23:45]

rune@opera.com changed reviewers:
+ keishi@chromium.org

[rune, 2014-12-18 06:23:46]

keishi@ do you have any objections with regards to landing this approach instead
of the flag you added?

[keishi, 2014-12-18 06:31:50]

On 2014/12/18 06:23:46, rune wrote:
> keishi@ do you have any objections with regards to landing this approach
instead
> of the flag you added?

Nope. I was just trying to avoid using didNotifySubtreeInsertionsToDocument but
if that's the only way, great.

[rune, 2014-12-18 06:37:40]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-12-18 06:38:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/791533004/20001

[commit-bot: I haz the power, 2014-12-18 08:49:16]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187437

[chrishtr, 2014-12-20 18:11:27]

On 2014/12/15 at 10:24:37, rune wrote:
> On 2014/12/13 at 05:33:01, chrishtr wrote:
> > On 2014/12/13 at 04:33:30, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)
> > 
> > Is it not web compatible to just dump this behavior?
> 
> You mean not to scroll at all when options are selected, or selected options
are inserted into the dom?

I mean the need to scroll at all in either scenario. It's magic behavior, right?

Sorry for not replying earlier. For some reason I never got cc'd on this issue
despite replying to it..

> 
> > Also, the phrasing of sentence 2, paragraph 3 of the CL description is
backwards.
> 
> Not sure I understood. Have split it into two sentences. Better now?

[rune, 2014-12-22 08:40:04]

On 2014/12/20 at 18:11:27, chrishtr wrote:
> On 2014/12/15 at 10:24:37, rune wrote:
> > On 2014/12/13 at 05:33:01, chrishtr wrote:
> > > On 2014/12/13 at 04:33:30, commit-bot wrote:
> > > > Try jobs failed on following builders:
> > > >   mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35288)
> > > 
> > > Is it not web compatible to just dump this behavior?
> > 
> > You mean not to scroll at all when options are selected, or selected options
are inserted into the dom?
> 
> I mean the need to scroll at all in either scenario. It's magic behavior,
right?
> 
> Sorry for not replying earlier. For some reason I never got cc'd on this issue
despite replying to it..

It's a user experience thing, I think. If you through some user interaction in a
page adds a selected option to a select, it would probably be confusing to the
user if that option is added to some non-visible position in the multi-select. I
was not able to find anything in the HTML spec about it, but I'm not that used
to navigate that spec, so it might be something somewhere.