[css-display] Support display: contents pseudo-elements.

[css-display] Support display: contents pseudo-elements.

This commit moves part of the pseudo-element caching logic to Element, in order
to support PseudoIdBefore and PseudoIdAfter for display: contents.

BUG=657748
Signed-off-by: Emilio Cobos Álvarez <ecobos@igalia.com>;

[emilio, 2017-03-02 01:54:04]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:54:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 02:12:32]

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

[commit-bot: I haz the power, 2017-03-02 02:12:33]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-02 02:27:22]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 02:28:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 02:49:18]

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

[commit-bot: I haz the power, 2017-03-02 02:49:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[emilio, 2017-03-02 02:59:28]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 03:00:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 04:34:52]

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

[commit-bot: I haz the power, 2017-03-02 04:34:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-02 11:25:02]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 11:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 13:24:44]

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

[commit-bot: I haz the power, 2017-03-02 13:24:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[emilio, 2017-03-02 13:53:27]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 13:53:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-02 14:04:25]

ecobos@igalia.com changed reviewers:
+ rune@opera.com

[emilio, 2017-03-02 14:04:25]

Hi rune, if you had the time and could review this, that'd be great :)

I've kept the Gecko behavior for now for pseudo-elements with display: contents
(treating it as an inline), though the csswg is likely to change it.

In any case after this patch it's trivial to switch to whatever behavior they
resolve.

Thanks in advance!

[commit-bot: I haz the power, 2017-03-02 15:25:56]

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

[commit-bot: I haz the power, 2017-03-02 15:25:57]

Dry run: This issue passed the CQ dry run.

[rune, 2017-03-06 10:13:33]

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1581: return isPseudoElement();
Why? Pseudo elements on which display applies can also be styled with
display:contents.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1817: ComputedStyle* newStyle) {
This method has a terrible name and an incomprehensible return value.

Looking closer at this method, I think it should just go away. It's relying on a
bug in the mask for PseudoId bits as well to trigger first-line changes.

Edit: I took a look at this code and I'm about to remove it. First CL here:
https://codereview.chromium.org/2729373003/

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1844: return true;
This method is buggy in the first place in that it only traverses pseudo ids
present in the old style. Returning in the middle of the loop may cause
necessary calls to be missed. ::first-line is not allowed on display:contents,
but if it was, firstLineStyleDidChange would have been skipped. This looks
fragile, but as I mentioned above, I don't think this method should be here, and
if the bit-mask is corrected for hasPseudoStyle, it will never be called.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:2075:
elementRareData()->clearComputedStyle();
Is the clearComputedStyle() here a fix made necessary for pseudo elements, or is
it an unrelated fix?

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3380: ComputedStyle*
Element::getCachedPseudoStyleOrCache(PseudoId pseudoId) const {
I don't understand the name of this method.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3414: // with an actual layout
object.
I was just thinking about this in relation to ::selection. Shouldn't this work?:

<style>
  .contents { display: contents }
  .contents::selection { background: green }
</style>
<div>
  <div class="contents">This text should have a green background while
selected.</div>
</div>

[emilio, 2017-03-06 16:52:09]

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1581: return isPseudoElement();
On 2017/03/06 10:13:33, rune wrote:
> Why? Pseudo elements on which display applies can also be styled with
> display:contents.

Well, that's definitely not what Gecko does fwiw. A Gecko frame tree dump of a
simple page with div::before { display: contents; content: "Foo"; } shows:

Block(div)(0)@7f4760a061c0 {0,0,35640,1140} [state=0000120800100210]
[content=7f4762b54430] [sc=7f47618fb9a8]<
  line 7f4760a066c8: count=1 state=inline,clean,prevmarginclean,not impacted,not
wrapped,before:nobr,after:nobr[0x100] {0,0,1740,1140} <
    Inline(_moz_generated_content_before)(-1)@7f4760a06548 {0,0,1740,1140}
[state=0000100800000240] [content=7f47618fe2c0] [sc=7f4760a062b8:before]<
      Text(0)"Foo"@7f4760a065c0 {0,0,1740,1140} [state=00000048b0600040]
[content=7f47618fe350] [sc=7f4760a063f8:-moz-text] [run=7f47618fe3e0][0,3,T] 
    >
  >
>

Which shows the _moz_generated_content_before box as an inline. I wonder if it's
observable? (At first I thought "of course it will", but then I couldn't find an
example as trivially as I thought, I should think about it a bit more I guess).

Also, note that for ::backdrop, the following applies:

> If its specified display property is contents, it computes to block.

(https://fullscreen.spec.whatwg.org/#::backdrop-pseudo-element)

Which other pseudos are you thinking about?

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1817: ComputedStyle* newStyle) {
On 2017/03/06 10:13:33, rune wrote:
> Looking closer at this method, I think it should just go away. It's relying on
a
> bug in the mask for PseudoId bits as well to trigger first-line changes.

lol, fun.

> Edit: I took a look at this code and I'm about to remove it. First CL here:
> https://codereview.chromium.org/2729373003/

Awesome! will rebase when that lands then :)

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3380: ComputedStyle*
Element::getCachedPseudoStyleOrCache(PseudoId pseudoId) const {
On 2017/03/06 10:13:33, rune wrote:
> I don't understand the name of this method.

It explicitly states what getCachedPseudoStyle did under the hood, which is
computing and caching the pseudo-element style if it wasn't cached already.

If you can think of a better name than getCachedPseudoStyleOrCache, or think
that that detail shouldn't be in the method name, I'm happy to change it.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3414: // with an actual layout
object.
On 2017/03/06 10:13:33, rune wrote:
> I was just thinking about this in relation to ::selection. Shouldn't this
work?:
> 
> <style>
>   .contents { display: contents }
>   .contents::selection { background: green }
> </style>
> <div>
>   <div class="contents">This text should have a green background while
> selected.</div>
> </div>

Note that this works with this patch, because I intentionally skip the check in
pseudoStyleForElementInternal (I think the alternative was returning null there,
but that would effectively disable ::selection and pretty much every other
pseudo-element for direct children of display: contents, which makes no sense).

Perhaps we could find a better name for this method, since this method only
affects pseudos for which we generate content. The comment should also be
updated to reflect this better.

[emilio, 2017-03-06 17:03:47]

Sorry, forgot to reply to this one.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:2075:
elementRareData()->clearComputedStyle();
On 2017/03/06 10:13:33, rune wrote:
> Is the clearComputedStyle() here a fix made necessary for pseudo elements, or
is
> it an unrelated fix?

I made it thinking on pseudos. It doesn't fix any other problems, as far as I
know, because the clearComputedStyle() in attachLayoutTree and detachLayoutTree
have our backs when display changes. Still worth making it explicit I think.

[rune, 2017-03-07 13:46:02]

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:1581: return isPseudoElement();
On 2017/03/06 16:52:09, emilio wrote:
> On 2017/03/06 10:13:33, rune wrote:
> > Why? Pseudo elements on which display applies can also be styled with
> > display:contents.
> 
> Well, that's definitely not what Gecko does fwiw. A Gecko frame tree dump of a
> simple page with div::before { display: contents; content: "Foo"; } shows:

> Which shows the _moz_generated_content_before box as an inline. I wonder if
it's
> observable? (At first I thought "of course it will", but then I couldn't find
an
> example as trivially as I thought, I should think about it a bit more I
guess).

This case below fails in Gecko. I think that's a bug:

<!DOCTYPE html>
<style>
    div::before {
        content: "PASS";
        display: contents;
        border: 10px solid red
    }
</style>
<p>You should see the word PASS and no red border below.</p>
<div></div>

I've made a pull request for the csswg test:
https://github.com/w3c/csswg-test/pull/1230

> Also, note that for ::backdrop, the following applies:
> 
> > If its specified display property is contents, it computes to block.
> 
> (https://fullscreen.spec.whatwg.org/#::backdrop-pseudo-element)
> 
> Which other pseudos are you thinking about?

Only pseudos on which the display property applies, which is ::before and
::after afaik.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:2075:
elementRareData()->clearComputedStyle();
On 2017/03/06 17:03:46, emilio wrote:
> On 2017/03/06 10:13:33, rune wrote:
> > Is the clearComputedStyle() here a fix made necessary for pseudo elements,
or
> is
> > it an unrelated fix?
> 
> I made it thinking on pseudos. It doesn't fix any other problems, as far as I
> know, because the clearComputedStyle() in attachLayoutTree and
detachLayoutTree
> have our backs when display changes. Still worth making it explicit I think.

Acknowledged.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3380: ComputedStyle*
Element::getCachedPseudoStyleOrCache(PseudoId pseudoId) const {
On 2017/03/06 16:52:09, emilio wrote:
> On 2017/03/06 10:13:33, rune wrote:
> > I don't understand the name of this method.
> 
> It explicitly states what getCachedPseudoStyle did under the hood, which is
> computing and caching the pseudo-element style if it wasn't cached already.
> 
> If you can think of a better name than getCachedPseudoStyleOrCache, or think
> that that detail shouldn't be in the method name, I'm happy to change it.

I read that name as "return the cached pseudo style or the cache itself", which
sounded weird. This is more like an "ensure"-type of method, so
ensureCachedPseudoStyle() perhaps. But, this method returns null for various
reasons. I'd like a method with a clearer contract, like an ensure-method that
would get the cached style if it's there, and construct one otherwise, as well
as caching it. Then callers of the ensure-method should check if we need to
create the style in the first place.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3383: return nullptr;
I don't think we use cache entries for internal pseudos. You should just DCHECK
pseudoId < FirstInternalPseudoId instead.

https://codereview.chromium.org/2727853002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:3414: // with an actual layout
object.
On 2017/03/06 16:52:09, emilio wrote:
> On 2017/03/06 10:13:33, rune wrote:
> > I was just thinking about this in relation to ::selection. Shouldn't this
> work?:
> > 
> > <style>
> >   .contents { display: contents }
> >   .contents::selection { background: green }
> > </style>
> > <div>
> >   <div class="contents">This text should have a green background while
> > selected.</div>
> > </div>
> 
> Note that this works with this patch, because I intentionally skip the check
in
> pseudoStyleForElementInternal (I think the alternative was returning null
there,
> but that would effectively disable ::selection and pretty much every other
> pseudo-element for direct children of display: contents, which makes no
sense).
> 
> Perhaps we could find a better name for this method, since this method only
> affects pseudos for which we generate content. The comment should also be
> updated to reflect this better.

I find it hard to understand what "allowed" means here. Why is it for instance
separated from the other checks for whether it's allowed, like ::first-letter
only being allowed on block-containers.

[emilio, 2017-03-07 21:58:56]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 21:59:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-07 22:07:10]

I've made display: contents work for pseudos instead of doing the same as Gecko
(haven't done a dry run yet, it's running).

To do this, I added a vector with the generated content we need to propagate
style for in PseudoElement. I _think_ it's safe (a generated LayoutObject can't
go away under the hood), but special care for that would be appreciated.

The reason it's done that way is that the generated content position in the
layout tree with respect to the parent is no longer known if the pseudo has
display: contents.

An alternative would be to traverse the layout children looking for the
anonymous items (from the beginning for PseudoIdBefore, from the end for
PseudoIdAfter), but to do that I need to expose node() for them too (or an
accessor like rawNode() or similar), which seemed quite inelegant.

A few review comments aren't yet addressed, I'll probably poke at them during
the rest of the week.

[commit-bot: I haz the power, 2017-03-07 22:26:16]

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

[commit-bot: I haz the power, 2017-03-07 22:26:18]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emilio, 2017-03-07 23:14:14]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 23:14:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 00:24:16]

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

[commit-bot: I haz the power, 2017-03-08 00:24:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-08 09:22:19]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 09:22:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 10:30:25]

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

[commit-bot: I haz the power, 2017-03-08 10:30:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-08 14:22:59]

On 2017/03/07 22:07:10, emilio wrote:
> I've made display: contents work for pseudos instead of doing the same as
Gecko
> (haven't done a dry run yet, it's running).
> 
> To do this, I added a vector with the generated content we need to propagate
> style for in PseudoElement. I _think_ it's safe (a generated LayoutObject
can't
> go away under the hood), but special care for that would be appreciated.

Of course, they can go away when ::first-line/letter are at play, so tracking
them as raw pointers is not viable.

> An alternative would be to traverse the layout children looking for the
> anonymous items (from the beginning for PseudoIdBefore, from the end for
> PseudoIdAfter), but to do that I need to expose node() for them too (or an
> accessor like rawNode() or similar), which seemed quite inelegant.

This is also not viable, because the anonymous LayoutObject's node pointer
points to the document.

Looking at the usage of m_node, I think it's doable to make the m_node pointer
in anonymous generated content point to the pseudo instead of the doc.

That should be straight-forward, because node() already returns nullptr for
anonymous items, and the rest of uses seem straight-forward and not dependent on
whether the node is the document or not.

That would allow us to find generated content appropriately. What do you think
rune?

[emilio, 2017-03-08 15:45:28]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[emilio, 2017-03-08 15:45:50]

On 2017/03/08 14:22:59, emilio wrote:
> On 2017/03/07 22:07:10, emilio wrote:
> > I've made display: contents work for pseudos instead of doing the same as
> Gecko
> > (haven't done a dry run yet, it's running).
> > 
> > To do this, I added a vector with the generated content we need to propagate
> > style for in PseudoElement. I _think_ it's safe (a generated LayoutObject
> can't
> > go away under the hood), but special care for that would be appreciated.
> 
> Of course, they can go away when ::first-line/letter are at play, so tracking
> them as raw pointers is not viable.
> 
> > An alternative would be to traverse the layout children looking for the
> > anonymous items (from the beginning for PseudoIdBefore, from the end for
> > PseudoIdAfter), but to do that I need to expose node() for them too (or an
> > accessor like rawNode() or similar), which seemed quite inelegant.
> 
> This is also not viable, because the anonymous LayoutObject's node pointer
> points to the document.
> 
> Looking at the usage of m_node, I think it's doable to make the m_node pointer
> in anonymous generated content point to the pseudo instead of the doc.
> 
> That should be straight-forward, because node() already returns nullptr for
> anonymous items, and the rest of uses seem straight-forward and not dependent
on
> whether the node is the document or not.
> 
> That would allow us to find generated content appropriately. What do you think
> rune?

I pushed a WIP version of this in this CL.

[commit-bot: I haz the power, 2017-03-08 15:45:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 15:55:56]

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

[commit-bot: I haz the power, 2017-03-08 15:55:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-08 16:21:08]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 16:21:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 17:21:45]

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

[commit-bot: I haz the power, 2017-03-08 17:21:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-08 19:53:19]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 19:53:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emilio, 2017-03-08 20:24:37]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 20:24:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 21:07:14]

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

[commit-bot: I haz the power, 2017-03-08 21:07:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-09 16:04:41]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 16:05:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 17:33:10]

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

[commit-bot: I haz the power, 2017-03-09 17:33:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emilio, 2017-03-09 19:05:57]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 19:06:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 21:03:59]

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

[commit-bot: I haz the power, 2017-03-09 21:04:00]

Dry run: This issue passed the CQ dry run.

[emilio, 2017-03-09 22:33:03]

This is green now and should be landable modulo the first-letter TODO (and a lot
of tests too, of course).

I'd want to get some feedback on it, though. Does this approach seem reasonable?
Are there any other approaches that could be taken? Thanks!

[rune, 2017-03-13 10:29:50]

Generally, looks sane to me. You should be able to split out a few CLs making
reviewing easier. Also, more elaborate description of what your changing would
be good.

createAnonymous change could be split out, right?

Also, just introducing Element::pseudoStyle() looks like a CL on its own.

In particular, I haven't looked at the first-letter code in detail.

The generated content changes are due to the fact that they won't necessarily be
children of a pseudo element layout object anymore, I guess?

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1581: return isPseudoElement();
What layout object is needed and created for the ::before element in "::before {
display:contents; content: "PASS"}"?

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3195: return cached;
This and the code below duplicates the code in Element::pseudoStyle(). Couldn't
you call pseudoStyle() here instead?

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.h:625: bool
canHaveGeneratedPseudo(PseudoId) const;
Do these methods need to be public? All of them?

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3080: const
ComputedStyle* parentStyle) const {
So, longer term, we'd remove this method as well, getting the pseudo style for
::first-line, ::selection, etc from Element as well?

[rune, 2017-03-13 10:36:00]

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutQuote.h (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutQuote.h:96: // Livedness is the same
as m_node, so this is safe.
"Lifetime is the same as for LayoutObject::m_node"

[emilio, 2017-03-13 13:28:02]

On 2017/03/13 10:29:50, rune wrote:
> Generally, looks sane to me. You should be able to split out a few CLs making
> reviewing easier. Also, more elaborate description of what your changing would
> be good.
>
> createAnonymous change could be split out, right?

Yeah, I'll try to split out all the createAnonymous changes, and pseudoStyle
too, on their own CLs.

Thanks for taking a look! :)

[emilio, 2017-03-13 13:28:31]

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1581: return isPseudoElement();
On 2017/03/13 10:29:50, rune wrote:
> What layout object is needed and created for the ::before element in "::before
{
> display:contents; content: "PASS"}"?

Whoops, this is leftover from the previous approach. It'll arrive to
LayoutObject::createObject and return null there (no LayoutObject needed). This
part can be reverted completely.

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3195: return cached;
On 2017/03/13 10:29:50, rune wrote:
> This and the code below duplicates the code in Element::pseudoStyle().
Couldn't
> you call pseudoStyle() here instead?

No, I can't. We use pseudoStyleForElement instead of getUncachedPseudoStyle,
which makes a difference when calling getComputedStyle(foo, ":internal-pseudo").

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/PseudoElement.h (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PseudoElement.h:62:
std::vector<LayoutObject*> m_generatedContent;
This can also go away now.

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2727853002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3080: const
ComputedStyle* parentStyle) const {
On 2017/03/13 10:29:50, rune wrote:
> So, longer term, we'd remove this method as well, getting the pseudo style for
> ::first-line, ::selection, etc from Element as well?

Yes, I think that'd be saner.

[emilio, 2017-04-04 01:04:06]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 01:04:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 03:15:25]

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

[commit-bot: I haz the power, 2017-04-04 03:15:26]

Dry run: This issue passed the CQ dry run.

[rune, 2017-04-04 14:23:41]

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3422: bool
Element::canHaveGeneratedPseudo(PseudoId pseudoId) const {
canHavePseudoElement() or canGeneratePseudoElement() sounds better to me.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FirstLetterPseudoElement.cpp:306:
remainingLength);
I don't understand this. What is an ownerPseudoElement? Is it something specific
to first-letter?

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PseudoElement.cpp:154: static bool
isGeneratedContentLike(LayoutObject* layoutObject) {
Why not call it isGeneratedContent?

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PseudoElement.cpp:202: :
child->previousInPreOrder(layoutParent);
Why do you need to to walk descendants here?

In general, this code is hard to follow. I think it needs some documentation.
Are anonymous table layout objects handled here? How do you know the difference
between an anonymous table wrapper for generated content and one for normal dom
nodes? Are they marked as being generated for the pseudo element as well?

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:548: // NB: This also
handles anonymous generated content for a given
What is anonymous generated content?

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:553: bool
isPseudoElementGeneratedContentFor(const PseudoElement&) const;
This name is incomprehensible. It means that this layout object is generated
content for the passed in pseudo element (::before or ::after)?

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:2075: void
setPseudoForAnonymous(PseudoElement& pseudo) {
This also needs a better name if possible.

[emilio, 2017-04-04 15:09:18]

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:3422: bool
Element::canHaveGeneratedPseudo(PseudoId pseudoId) const {
On 2017/04/04 14:23:41, rune wrote:
> canHavePseudoElement() or canGeneratePseudoElement() sounds better to me.

Acknowledged.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FirstLetterPseudoElement.cpp:306:
remainingLength);
On 2017/04/04 14:23:41, rune wrote:
> I don't understand this. What is an ownerPseudoElement? Is it something
specific
> to first-letter?

So this is specific to first-letter because first-letter does this hacky thing
where it splits a text fragment into two. If that fragment happens to be
generated content inside a display: contents pseudo-element, we lost track of
the only hint that allows us to find them.

Perhaps I can take advantage of generated content always creating a text
fragment and mutate it instead, which may be cleaner and remove a bunch of
complexity. Let me try that.

Also, if we can guarantee that those LayoutObjects don't go away, we could just
track generated content in a std::vector<LayoutObject*> inside the
pseudo-element when it's a display: contents pseudo, but that sounds flacky.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/PseudoElement.cpp (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PseudoElement.cpp:154: static bool
isGeneratedContentLike(LayoutObject* layoutObject) {
On 2017/04/04 14:23:41, rune wrote:
> Why not call it isGeneratedContent?

Sounds good to me, yeah.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/PseudoElement.cpp:202: :
child->previousInPreOrder(layoutParent);
On 2017/04/04 14:23:41, rune wrote:
> Why do you need to to walk descendants here?
> 
> In general, this code is hard to follow. I think it needs some documentation.
> Are anonymous table layout objects handled here? How do you know the
difference
> between an anonymous table wrapper for generated content and one for normal
dom
> nodes? Are they marked as being generated for the pseudo element as well?

The idea is to handle them, yeah. Right now I don't think they're marked as
such, let me add tests for those.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:548: // NB: This also
handles anonymous generated content for a given
On 2017/04/04 14:23:41, rune wrote:
> What is anonymous generated content?

It's my ability to badly word comments. I meant anonymous layout objects for
generated ::before or after (that's why we look directly at m_node and not
node()). Will reword if this ends up being needed.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:553: bool
isPseudoElementGeneratedContentFor(const PseudoElement&) const;
On 2017/04/04 14:23:41, rune wrote:
> This name is incomprehensible. It means that this layout object is generated
> content for the passed in pseudo element (::before or ::after)?

Yeah, that's the idea. I'm terrible at naming, so I'll try to find a better name
if I can't do the ::first-line simplification for ::before and ::after.

https://codereview.chromium.org/2727853002/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:2075: void
setPseudoForAnonymous(PseudoElement& pseudo) {
On 2017/04/04 14:23:41, rune wrote:
> This also needs a better name if possible.

Acknowledged.