Bug 63977 - [ShadowContentElement] Redundant RenderText objects are created on the content boundaries
Summary: [ShadowContentElement] Redundant RenderText objects are created on the conten...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 63504
  Show dependency treegraph
 
Reported: 2011-07-05 23:45 PDT by Hajime Morrita
Modified: 2011-07-07 01:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (22.93 KB, patch)
2011-07-06 00:05 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Asking cq (22.89 KB, patch)
2011-07-06 20:36 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-07-05 23:45:21 PDT
Running small test suite for Bug 63504 found that
some content child pattern makes redundant RenderText node.
Comment 1 Hajime Morrita 2011-07-06 00:05:15 PDT
Created attachment 99797 [details]
Patch
Comment 2 Hajime Morrita 2011-07-06 00:08:43 PDT
Hi Dimitri, could you take a look?

After this fix, I'm planning to make the "flatten tree" concept clear in the code
by extracting a FlattenTreeTraverser or something.
Currently the concept is mixed into NodeRenderingContext, which is sad situation.
Comment 3 Dominic Cooney 2011-07-06 06:06:16 PDT
Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

Trivial comments inline.

> LayoutTests/ChangeLog:9
> +        One is for shadow tree which has one content elmement,

nit: elmement → element

> LayoutTests/ChangeLog:12
> +        Then the test makes 2 complete DOM trees (dom tree with shadow and

nit: dom → DOM
for consistency

> LayoutTests/ChangeLog:14
> +        compare their layout result.

I love this testing approach.

> LayoutTests/fast/dom/shadow/content-element-renderers-expected.txt:1
> +This test compares a shadow-based render tree with one for a refrence DOM tree.

nit: refrence → reference

> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
> +    document.getElementById("console").innerText += (message + "\n");

WebKit zeitgeist seems to prefer textContent over innerText?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:57
> +    //var enhancedHtml = html.replace(/<span\/>/g, "<span>#span</span>").replace(/<div\/>/g, "<div>#div</div>").replace(/<content\/>/g, "<content></content>");

Don’t need it? Delete it?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:58
> +    var enhancedHtml = html.replace(/<span\/>/g, "<span> </span>").replace(/<div\/>/g, "<div> </div>").replace(/<content\/>/g, "<content></content>").replace(/#text/g, " ");

I am curious… why the whitespace in span and div but not content?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:61
> +    var contentPlaceholder = root.getElementsByTagName("CONTENT")[0];

Is root.querySelector('content') equivalent and more succinct?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:83
> +    moveChildren(content, host);

Why do you create a blockquote in createTreeFrom(contentHtml) and then move the nodes into another blockquote that is host?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:93
> +    container.removeChild

Oops.

> LayoutTests/fast/dom/shadow/content-element-renderers.html:121
> +    

Delete this blank line.

> LayoutTests/fast/dom/shadow/content-element-renderers.html:133
> +    layoutTestController.dumpAsText();

Maybe add a description explaining that this test can not be run in the browser?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:136
> +

Delete this blank line.
Comment 4 Dimitri Glazkov (Google) 2011-07-06 08:20:24 PDT
Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

pls address the nits.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
>> +    document.getElementById("console").innerText += (message + "\n");
> 
> WebKit zeitgeist seems to prefer textContent over innerText?

yup -- and don't use +=, as abbreviated as it sounds, it's highly inefficient.

How's this:

var log = document.getElementById("console");
log.info = function(message)
{
    this.appendChild(document.createElement("div")).textContent = message;
}
// ...
log.info('foo');
Comment 5 Dimitri Glazkov (Google) 2011-07-06 08:21:08 PDT
(In reply to comment #2)
> Hi Dimitri, could you take a look?
> 
> After this fix, I'm planning to make the "flatten tree" concept clear in the code
> by extracting a FlattenTreeTraverser or something.
> Currently the concept is mixed into NodeRenderingContext, which is sad situation.

Let's design this first. I would like to understand the approach better before landing patches.
Comment 6 Hajime Morrita 2011-07-06 20:36:45 PDT
Created attachment 99921 [details]
Asking cq
Comment 7 Hajime Morrita 2011-07-06 20:38:29 PDT
Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

Hi Domini, Dimitri, thanks for reviewing! I updated the patch and asking cq to land it.

>> LayoutTests/ChangeLog:9
>> +        One is for shadow tree which has one content elmement,
> 
> nit: elmement → element

fixed.

>> LayoutTests/ChangeLog:12
>> +        Then the test makes 2 complete DOM trees (dom tree with shadow and
> 
> nit: dom → DOM
> for consistency

fixed.

>> LayoutTests/fast/dom/shadow/content-element-renderers-expected.txt:1
>> +This test compares a shadow-based render tree with one for a refrence DOM tree.
> 
> nit: refrence → reference

fixed.

>>> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
>>> +    document.getElementById("console").innerText += (message + "\n");
>> 
>> WebKit zeitgeist seems to prefer textContent over innerText?
> 
> yup -- and don't use +=, as abbreviated as it sounds, it's highly inefficient.
> 
> How's this:
> 
> var log = document.getElementById("console");
> log.info = function(message)
> {
>     this.appendChild(document.createElement("div")).textContent = message;
> }
> // ...
> log.info('foo');

Rewrote to Dimitri version.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:61
>> +    var contentPlaceholder = root.getElementsByTagName("CONTENT")[0];
> 
> Is root.querySelector('content') equivalent and more succinct?

No strong reason. Just because I'm from pre-CSS era...

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:83
>> +    moveChildren(content, host);
> 
> Why do you create a blockquote in createTreeFrom(contentHtml) and then move the nodes into another blockquote that is host?

It's because ShadowRoot doesn't support innerHTML but I'd like to define test data as HTML text.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:93
>> +    container.removeChild
> 
> Oops.

Whoa...

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:121
>> +    
> 
> Delete this blank line.

Fixed.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:133
>> +    layoutTestController.dumpAsText();
> 
> Maybe add a description explaining that this test can not be run in the browser?

Added.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:136
>> +
> 
> Delete this blank line.

Done.
Comment 8 Hajime Morrita 2011-07-06 20:39:29 PDT
(In reply to comment #5)
> (In reply to comment #2)
> > Hi Dimitri, could you take a look?
> > 
> > After this fix, I'm planning to make the "flatten tree" concept clear in the code
> > by extracting a FlattenTreeTraverser or something.
> > Currently the concept is mixed into NodeRenderingContext, which is sad situation.
> 
> Let's design this first. I would like to understand the approach better before landing patches.
Okay, I have no concrete idea yet. So I'll write a note for this.
Comment 9 WebKit Review Bot 2011-07-06 21:22:30 PDT
Comment on attachment 99921 [details]
Asking cq

Clearing flags on attachment: 99921

Committed r90536: <http://trac.webkit.org/changeset/90536>
Comment 10 WebKit Review Bot 2011-07-06 21:22:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Hajime Morrita 2011-07-07 01:15:12 PDT
> > > After this fix, I'm planning to make the "flatten tree" concept clear in the code
> > > by extracting a FlattenTreeTraverser or something.
> > > Currently the concept is mixed into NodeRenderingContext, which is sad situation.
> > 
> > Let's design this first. I would like to understand the approach better before landing patches.
> Okay, I have no concrete idea yet. So I'll write a note for this.

Hi Dimitri, I put the note on Bug 64072.
Although the idea is very rough, I'd love to hear your opinion!