Remove scoped styles.

Remove scoped styles.

Removes <style scoped>.

This was discussed and approved with LGTMs in blink-dev:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/R1x18ZLS5qQ/Bjuh_cENhlQJ

Chrome status dashboard entry:
http://www.chromestatus.com/features/5374137958662144

Note:
this is based on esprehn's original cl:
https://codereview.chromium.org/214693002/

BUG=379096
TEST=pass all layout tests

[kochi, 2014-06-03 04:47:12]

Hi Eliott, Takashi,

This is a CL to remove <style scoped>.

I applied Eliott's CL, and slightly fixed 
ScopedStyleResolver.cpp so that some tests pass
(fast/html/imports/import-style-{basic,tree-order,tree-order-dedup}.html failed)

Could you review?

[tasak, 2014-06-03 05:57:48]

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:95: static Node*
determineScopingNodeForStyleScoped(HTMLStyleElement* ownerElement,
StyleSheetContents* styleSheetContents)
Probably it would be better to rename this function. So
determineScopingNodeForStyleInShadow or something?

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:98:
ASSERT(ownerElement->isInShadowTree());
Now isScoped() has the same meaning as isInShadowTree().

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (left):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:51: return 0;
I think, this code is not related to style scoped.

I'm not sure whether we can now remove this code or not. In the past,
StyleResolver tried to append stylesheets without document. So this code is just
for filtering out those no-document stylesheets.

It would be better to make another CL if we want to remove the above code?

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:54: if
(styleElement.isInShadowTree())
If we keep isScoped method in HTMLStyleElement, it would be better to use
isScoped() here?

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
File Source/core/html/HTMLStyleElement.h (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.h:46: bool isScoped() const { return m_scoped;
}
I think, it would be beter to do:
(a) return isInShadowTree() or,
(b) remove this method and replace other code (using isScope()) with
isInShadowTree().

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.h:79: bool m_scoped;
We still need m_scoped? I think, we could replace this with isInShadowTree().

[kochi, 2014-06-03 07:47:04]

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
File Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:95: static Node*
determineScopingNodeForStyleScoped(HTMLStyleElement* ownerElement,
StyleSheetContents* styleSheetContents)
On 2014/06/03 05:57:48, tasak wrote:
> Probably it would be better to rename this function. So
> determineScopingNodeForStyleInShadow or something?

Done.

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/invalida...
Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp:98:
ASSERT(ownerElement->isInShadowTree());
On 2014/06/03 05:57:48, tasak wrote:
> Now isScoped() has the same meaning as isInShadowTree().

Reduced to one ASSERT().

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (left):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:51: return 0;
On 2014/06/03 05:57:48, tasak wrote:
> I think, this code is not related to style scoped.
> 
> I'm not sure whether we can now remove this code or not. In the past,
> StyleResolver tried to append stylesheets without document. So this code is
just
> for filtering out those no-document stylesheets.
> 
> It would be better to make another CL if we want to remove the above code?

Thanks for the explanation.
I'll separate changes in this file and make another CL.

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/css/resolver...
Source/core/css/resolver/ScopedStyleResolver.cpp:54: if
(styleElement.isInShadowTree())
On 2014/06/03 05:57:48, tasak wrote:
> If we keep isScoped method in HTMLStyleElement, it would be better to use
> isScoped() here?

As I removed isScoepd(), isInShadowTree() is fine here.

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
File Source/core/html/HTMLStyleElement.h (right):

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.h:46: bool isScoped() const { return m_scoped;
}
On 2014/06/03 05:57:48, tasak wrote:
> I think, it would be beter to do:
> (a) return isInShadowTree() or,
> (b) remove this method and replace other code (using isScope()) with
> isInShadowTree().

I'll take the (b) approach.
Less "scope" terminology usage will make the code more readable.

https://codereview.chromium.org/310443002/diff/40001/Source/core/html/HTMLSty...
Source/core/html/HTMLStyleElement.h:79: bool m_scoped;
On 2014/06/03 05:57:48, tasak wrote:
> We still need m_scoped? I think, we could replace this with isInShadowTree().

Removed.

[kochi, 2014-06-03 08:38:18]

Removed HasScopedHTMLStyleChildFlag from Node in patch set 4.
Please take another look.

[kochi, 2014-06-04 07:31:36]

On 2014/06/03 08:38:18, Takayoshi Kochi wrote:
> Removed HasScopedHTMLStyleChildFlag from Node in patch set 4.
> Please take another look.

Elliott, could you review, or should I complete Takashi's review first?

[esprehn, 2014-06-04 08:13:04]

On 2014/06/04 07:31:36, Takayoshi Kochi wrote:
> On 2014/06/03 08:38:18, Takayoshi Kochi wrote:
> > Removed HasScopedHTMLStyleChildFlag from Node in patch set 4.
> > Please take another look.
> 
> Elliott, could you review, or should I complete Takashi's review first?

I'll look in the morning.

[kochi, 2014-06-04 08:14:29]

On 2014/06/04 08:13:04, esprehn wrote:
> On 2014/06/04 07:31:36, Takayoshi Kochi wrote:
> > On 2014/06/03 08:38:18, Takayoshi Kochi wrote:
> > > Removed HasScopedHTMLStyleChildFlag from Node in patch set 4.
> > > Please take another look.
> > 
> > Elliott, could you review, or should I complete Takashi's review first?
> 
> I'll look in the morning.

Thanks!!

[tasak, 2014-06-05 04:36:39]

lgtm.

I expect, once we remove style scoped, we can make Blink much simpler.
e.g. we can remove StyleSheetScopingNodeList. We can replace ScopedStyleResolver
and ScopedStyleTree with TreeScope (TreeScope will have active author
stylesheets).

[esprehn, 2014-06-05 05:23:00]

lgtm, but please update the node flags comment before landing.

https://codereview.chromium.org/310443002/diff/80001/Source/core/dom/Node.h
File Source/core/dom/Node.h (left):

https://codereview.chromium.org/310443002/diff/80001/Source/core/dom/Node.h#o...
Source/core/dom/Node.h:728: HasScopedHTMLStyleChildFlag = 1 << 30,
You should update the comment below this to show there's a new free bit.

[kochi, 2014-06-05 05:56:21]

Thanks for your reviews!

I'll rebase and submit to CQ.

https://codereview.chromium.org/310443002/diff/80001/Source/core/dom/Node.h
File Source/core/dom/Node.h (left):

https://codereview.chromium.org/310443002/diff/80001/Source/core/dom/Node.h#o...
Source/core/dom/Node.h:728: HasScopedHTMLStyleChildFlag = 1 << 30,
On 2014/06/05 05:23:01, esprehn wrote:
> You should update the comment below this to show there's a new free bit.

Done.

[kochi, 2014-06-05 06:07:11]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-06-05 06:07:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kochi@chromium.org/310443002/200001

[kochi, 2014-06-05 06:20:08]

The CQ bit was unchecked by kochi@chromium.org

[kochi, 2014-06-05 06:24:34]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-06-05 06:25:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kochi@chromium.org/310443002/210001

[commit-bot: I haz the power, 2014-06-05 08:28:39]

Change committed as 175550

[sergeyv, 2014-06-05 09:38:14]

A revert of this CL has been created in
https://codereview.chromium.org/317143002/ by sergeyv@chromium.org.

The reason for reverting is: Speculative revert: looks like this change broke
compilation
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....

[sergeyv, 2014-06-05 09:38:32]

A revert of this CL has been created in
https://codereview.chromium.org/313173010/ by sergeyv@chromium.org.

The reason for reverting is: Speculative revert: looks like this change broke
compilation
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....

[tasak, 2014-06-05 09:46:32]

On 2014/06/05 09:38:32, sergeyv wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/313173010/ by mailto:sergeyv@chromium.org.
> 
> The reason for reverting is: Speculative revert: looks like this change broke
> compilation
>
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....

I'm sorry.

We fogot to remove the following line in
//src/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:
StyleScoped status=experimental

[kochi, 2014-06-05 09:57:57]

On 2014/06/05 09:46:32, tasak wrote:
> On 2014/06/05 09:38:32, sergeyv wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/313173010/ by mailto:sergeyv@chromium.org.
> > 
> > The reason for reverting is: Speculative revert: looks like this change
broke
> > compilation
> >
>
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....
> 
> I'm sorry.
> 
> We fogot to remove the following line in
> //src/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:
> StyleScoped status=experimental

sergeyv, thanks for taking care of.

It's strange, that RuntimeEnabledFeatures.in should have been changed properly.
Will take a look.

[tasak, 2014-06-05 10:52:06]

On 2014/06/05 09:57:57, Takayoshi Kochi wrote:
> On 2014/06/05 09:46:32, tasak wrote:
> > On 2014/06/05 09:38:32, sergeyv wrote:
> > > A revert of this CL has been created in
> > > https://codereview.chromium.org/313173010/ by mailto:sergeyv@chromium.org.
> > > 
> > > The reason for reverting is: Speculative revert: looks like this change
> broke
> > > compilation
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....
> > 
> > I'm sorry.
> > 
> > We fogot to remove the following line in
> > //src/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:
> > StyleScoped status=experimental
> 
> sergeyv, thanks for taking care of.
> 
> It's strange, that RuntimeEnabledFeatures.in should have been changed
properly.
> Will take a look.

Yeah, RuntimeEnabledFeatures.in was updated.

The compile error said:

gen/blink/InternalRuntimeFlags.h: In member function 'bool
WebCore::InternalRuntimeFlags::styleScopedEnabled()':
gen/blink/InternalRuntimeFlags.h:110:40: error: 'styleScopedEnabled' is not a
member of 'WebCore::RuntimeEnabledFeatures'
ninja: build stopped: subcommand failed.

However, we should not use gen/Blink/InternalRuntimeFlags.h. Instead, we should
use:

gen/blink/core/InternalRuntimeFlags.h

[kochi, 2014-06-05 11:01:09]

On 2014/06/05 10:52:06, tasak wrote:
> On 2014/06/05 09:57:57, Takayoshi Kochi wrote:
> > On 2014/06/05 09:46:32, tasak wrote:
> > > On 2014/06/05 09:38:32, sergeyv wrote:
> > > > A revert of this CL has been created in
> > > > https://codereview.chromium.org/313173010/ by
mailto:sergeyv@chromium.org.
> > > > 
> > > > The reason for reverting is: Speculative revert: looks like this change
> > broke
> > > > compilation
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/GPU%20Mac%20Builder%20%2....
> > > 
> > > I'm sorry.
> > > 
> > > We fogot to remove the following line in
> > > //src/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:
> > > StyleScoped status=experimental
> > 
> > sergeyv, thanks for taking care of.
> > 
> > It's strange, that RuntimeEnabledFeatures.in should have been changed
> properly.
> > Will take a look.
> 
> Yeah, RuntimeEnabledFeatures.in was updated.
> 
> The compile error said:
> 
> gen/blink/InternalRuntimeFlags.h: In member function 'bool
> WebCore::InternalRuntimeFlags::styleScopedEnabled()':
> gen/blink/InternalRuntimeFlags.h:110:40: error: 'styleScopedEnabled' is not a
> member of 'WebCore::RuntimeEnabledFeatures'
> ninja: build stopped: subcommand failed.
> 
> However, we should not use gen/Blink/InternalRuntimeFlags.h. Instead, we
should
> use:
> 
> gen/blink/core/InternalRuntimeFlags.h

Thanks Takashi,
It looks like the failed builders needs clobbering.

Let me try resubmit.

[kochi, 2014-06-05 11:01:28]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-06-05 11:02:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kochi@chromium.org/310443002/210001

[commit-bot: I haz the power, 2014-06-05 11:20:22]

Change committed as 175555

[Peter Beverloo, 2014-06-05 12:10:19]

A revert of this CL has been created in
https://codereview.chromium.org/315173002/ by peter@chromium.org.

The reason for reverting is: Broke every incremental compile bot on the
waterfall:

InternalRuntimeFlags.h: In member function 'bool
WebCore::InternalRuntimeFlags::styleScopedEnabled()':
InternalRuntimeFlags.h:110:40: error: 'styleScopedEnabled' is not a member of
'WebCore::RuntimeEnabledFeatures'
     bool styleScopedEnabled() { return
RuntimeEnabledFeatures::styleScopedEnabled(); }

While a clobber build indeed fixes the problem, it also breaks local
builds, which increases the cost of this CL significantly. Let's try
to find a proper solution in the bug.

BUG=381111.

[kochi, 2014-06-06 04:56:26]

On 2014/06/05 12:10:19, Peter Beverloo wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/315173002/ by mailto:peter@chromium.org.
> 
> The reason for reverting is: Broke every incremental compile bot on the
> waterfall:
> 
> InternalRuntimeFlags.h: In member function 'bool
> WebCore::InternalRuntimeFlags::styleScopedEnabled()':
> InternalRuntimeFlags.h:110:40: error: 'styleScopedEnabled' is not a member of
> 'WebCore::RuntimeEnabledFeatures'
>      bool styleScopedEnabled() { return
> RuntimeEnabledFeatures::styleScopedEnabled(); }
> 
> While a clobber build indeed fixes the problem, it also breaks local
> builds, which increases the cost of this CL significantly. Let's try
> to find a proper solution in the bug.
> 
> BUG=381111.

This failure was analyzed by Nils (nbarth), Takashi (tasak) and me,
http://code.google.com/p/chromium/issues/detail?id=381111#c4

Anyways clobbering was required to land this CL at that point,
because stale generated file could have been there, after
Blink r175513.

https://codereview.chromium.org/316343003/
adds clobber build for Chromium (unfortunately we don't have the
counterpart in Blink at this point).  As Blink r175513 was already
rolled in to Chromium, landing the CL above will fix every developer's
incremental builds, then it should be safe to land this CL.

[kochi, 2014-06-07 01:21:37]

As nbarth's landmine CL landed on Chromium at r275406,
Retrying CQ to see how it goes.

[kochi, 2014-06-07 01:21:46]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-06-07 01:23:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kochi@chromium.org/310443002/210001

[commit-bot: I haz the power, 2014-06-07 03:29:21]

Change committed as 175720

[dcheng, 2014-06-08 04:15:23]

A revert of this CL has been created in
https://codereview.chromium.org/324593003/ by dcheng@chromium.org.

The reason for reverting is: Causes several browser tests to fail on ChromiumOS
(dbg):
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[kochi, 2014-06-09 09:58:46]

On 2014/06/08 04:15:23, dcheng wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/324593003/ by mailto:dcheng@chromium.org.
> 
> The reason for reverting is: Causes several browser tests to fail on
ChromiumOS
> (dbg):
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

Sorry for making this CL fail again.
I'm now on it and will update the CL at
https://codereview.chromium.org/325663003/ .