Output Element defaultValue should be based on textContent

Output Element defaultValue should be based on textContent

Default value based on textContent if flag is in mode default.

R=tkent, keishi1
BUG=345788
TEST=Check default value based on textContent.

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

[keishi, 2014-03-03 02:00:12]

Thanks. lgtm.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
File
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js
(right):

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:124:
var output1 = document.createElement('output');
nit: Maybe use the variable "output" to match previous tests.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:126:
shouldBeTrue('output1.defaultValue == \'A\'');
Use shouldBeEqualToString.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:126:
shouldBeTrue('output1.defaultValue == \'A\'');
Maybe we should test output1.value too just in case.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:128:
shouldBeTrue('output1.defaultValue == \'B\'');
Ditto.

[Habib Virji, 2014-03-04 11:11:17]

Updated as per review comments.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
File
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js
(right):

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:124:
var output1 = document.createElement('output');
On 2014/03/03 02:00:12, keishi1 wrote:
> nit: Maybe use the variable "output" to match previous tests.

Done.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:126:
shouldBeTrue('output1.defaultValue == \'A\'');
On 2014/03/03 02:00:12, keishi1 wrote:
> Use shouldBeEqualToString.

Done.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:126:
shouldBeTrue('output1.defaultValue == \'A\'');
On 2014/03/03 02:00:12, keishi1 wrote:
> Maybe we should test output1.value too just in case.

Done.

https://codereview.chromium.org/178253006/diff/1/LayoutTests/fast/dom/HTMLOut...
LayoutTests/fast/dom/HTMLOutputElement/script-tests/htmloutputelement-value.js:128:
shouldBeTrue('output1.defaultValue == \'B\'');
On 2014/03/03 02:00:12, keishi1 wrote:
> Ditto.

Done.

[Habib Virji, 2014-03-04 11:34:21]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-03-04 11:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/178253006/20001

[commit-bot: I haz the power, 2014-03-04 12:09:41]

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

[commit-bot: I haz the power, 2014-03-04 12:09:42]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[keishi, 2014-03-04 13:07:49]

On 2014/03/04 12:09:42, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

I'm sorry I don't have OWNER privileges. tkent could you take a look?

[tkent, 2014-03-04 22:55:21]

https://codereview.chromium.org/178253006/diff/20001/Source/core/html/HTMLOut...
File Source/core/html/HTMLOutputElement.cpp (right):

https://codereview.chromium.org/178253006/diff/20001/Source/core/html/HTMLOut...
Source/core/html/HTMLOutputElement.cpp:124: setDefaultValue(value());
Updating m_defaultValue in the defaultValue getter smells bad thing.

This code means we are failing to keep m_defaultValue up-to-date. Why
m_defaultValue is not up-to-date while we hook childrenChanged()?

[Habib Virji, 2014-03-05 14:22:43]

Updated code 

The scenario that was failing is 
- This was not setting A as defaultValue: <output name=o>A</output></form>
- This was working fine: document.forms.f.o.textContent = 'B';

m_defaultValue but was not getting set especially when it is empty. ResetImpl
changes are due to if condition in childrenChange was relying on that return for
defaultValue not to be overwritten. 

Test updated to test above scenario.

https://codereview.chromium.org/178253006/diff/20001/Source/core/html/HTMLOut...
File Source/core/html/HTMLOutputElement.cpp (right):

https://codereview.chromium.org/178253006/diff/20001/Source/core/html/HTMLOut...
Source/core/html/HTMLOutputElement.cpp:124: setDefaultValue(value());
On 2014/03/04 22:55:21, tkent wrote:
> Updating m_defaultValue in the defaultValue getter smells bad thing.
> 
> This code means we are failing to keep m_defaultValue up-to-date. Why
> m_defaultValue is not up-to-date while we hook childrenChanged()?
> 

Done.
Yes that was true, childrenChanged was not updating m_defaultValue because of
return statement just before it which appears to be related to resetImpl. In
resetImpl, as per spec defaultFlag should be set but setting the flag before
updating child was updating default value too. That's the reason flag is now 
set later and return statement is removed to allow setDefaultValue.

[tkent, 2014-03-05 23:46:01]

Looks like we can remove m_isSetTextContentInProgress completely, and can
replace setTextContextInternal with setTextContent.

[Habib Virji, 2014-03-06 15:05:40]

On 2014/03/05 23:46:01, tkent wrote:
> Looks like we can remove m_isSetTextContentInProgress completely, and can
> replace setTextContextInternal with setTextContent.

Thanks. 

Yes m_isSetTextContentInProgress is not needed and has been removed and also
defaultValue setting does work fine.

[tkent, 2014-03-07 01:32:53]

lgtm.  The code becomes simpler.  Great!

[tkent, 2014-03-07 01:32:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:33:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/178253006/80001

[commit-bot: I haz the power, 2014-03-07 01:47:10]

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

[commit-bot: I haz the power, 2014-03-07 01:47:11]

Try jobs failed on following builders: mac_blink_compile

[tkent, 2014-03-07 03:30:37]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-07 03:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/178253006/80001

[commit-bot: I haz the power, 2014-03-07 04:33:46]

Change committed as 168703