Recalc OPTION and OPTGROUP from recalcOwnStyle.

Recalc OPTION and OPTGROUP from recalcOwnStyle.

When recalc was done from willRecalcStyle, the StyleRecalcChange state for
OPTGROUP would always be NoChange for recalcOwnStyle and inherited properties
not propagated to OPTION children for LocalStyleChange in OPTGROUP.

R=esprehn
BUG=343536
NOTRY=true

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

[rune, 2014-02-13 23:57:35]

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
Source/core/html/HTMLOptionElement.cpp:300: updateNonRenderStyle();
Possibly, updateNonRenderStyle can be called unconditionally here and the
HTMLOptionElement::attach method can be removed.

[esprehn, 2014-02-14 00:46:20]

lgtm, but why is it safe to skip for reattach?

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptGrou...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptGrou...
Source/core/html/HTMLOptGroupElement.cpp:110: updateNonRenderStyle();
Why do you skip this for reattach?

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
Source/core/html/HTMLOptionElement.cpp:300: updateNonRenderStyle();
On 2014/02/13 23:57:35, rune - CET wrote:
> Possibly, updateNonRenderStyle can be called unconditionally here and the
> HTMLOptionElement::attach method can be removed.

I'm inclined to say we do that. This has been a source of a bunch of bugs, we
keep moving around the calls to updateNonRenderStyle(). Perhaps we should just
call it unconditionally.

[rune, 2014-02-14 09:42:08]

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptGrou...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptGrou...
Source/core/html/HTMLOptGroupElement.cpp:110: updateNonRenderStyle();
On 2014/02/14 00:46:20, esprehn wrote:
> Why do you skip this for reattach?

Done.

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
File Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/165433002/diff/1/Source/core/html/HTMLOptionE...
Source/core/html/HTMLOptionElement.cpp:300: updateNonRenderStyle();
On 2014/02/14 00:46:20, esprehn wrote:
> On 2014/02/13 23:57:35, rune - CET wrote:
> > Possibly, updateNonRenderStyle can be called unconditionally here and the
> > HTMLOptionElement::attach method can be removed.
> 
> I'm inclined to say we do that. This has been a source of a bunch of bugs, we
> keep moving around the calls to updateNonRenderStyle(). Perhaps we should just
> call it unconditionally.

Done.

[rune, 2014-02-14 10:00:45]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-14 10:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/165433002/140001

[commit-bot: I haz the power, 2014-02-14 11:31:40]

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

[commit-bot: I haz the power, 2014-02-14 11:31:40]

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

[rune, 2014-02-16 23:07:10]

I did remove the ::attach methods, which was a bad idea since the render style
would not be calculated at all for initial attach of option elements.

I am now calling updateNonRenderStyle() unconditionally from
customStyleForRenderer, as discussed, but I'm also taking advantage of
AttachContext::resolvedStyle in ::attach to avoid calculating render style twice
for option elements (as you pointed out could be an issue on #blink).

@esprehn: could you PTAL?

[esprehn, 2014-02-19 00:04:16]

https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:93: optGroupContext.resolvedStyle =
m_style.get();
Why do you need this branch? Shouldn't the createRendererForElementIfNeeded()
call inside ::attach() below call styleForRenderer() which will then call
customStyleForRenderer() which does this for you?

[rune, 2014-02-19 09:27:03]

On 2014/02/19 00:04:16, esprehn wrote:
>
https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
> File Source/core/html/HTMLOptGroupElement.cpp (right):
> 
>
https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
> Source/core/html/HTMLOptGroupElement.cpp:93: optGroupContext.resolvedStyle =
> m_style.get();
> Why do you need this branch? Shouldn't the createRendererForElementIfNeeded()
> call inside ::attach() below call styleForRenderer() which will then call
> customStyleForRenderer() which does this for you?

Yes, for the OPTGROUP case, you're right. I'll need to keep that branch for
OPTION since createRendererForElementIfNeeded will hit an early return when
OPTIONs are inside OPTGROUP since OPTGROUP won't have a renderer which makes
shouldCreateRenderer return false.

[rune, 2014-02-19 09:47:25]

https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
File Source/core/html/HTMLOptGroupElement.cpp (right):

https://codereview.chromium.org/165433002/diff/400001/Source/core/html/HTMLOp...
Source/core/html/HTMLOptGroupElement.cpp:93: optGroupContext.resolvedStyle =
m_style.get();
On 2014/02/19 00:04:16, esprehn wrote:
> Why do you need this branch? Shouldn't the createRendererForElementIfNeeded()
> call inside ::attach() below call styleForRenderer() which will then call
> customStyleForRenderer() which does this for you?

Done.

[rune, 2014-02-19 09:47:39]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-19 09:47:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/165433002/480001

[commit-bot: I haz the power, 2014-02-19 12:45:23]

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

[commit-bot: I haz the power, 2014-02-19 12:45:24]

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

[rune, 2014-02-19 12:57:56]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-19 12:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/165433002/480001

[commit-bot: I haz the power, 2014-02-19 12:58:27]

Change committed as 167414