Running small test suite for Bug 63504 found that some content child pattern makes redundant RenderText node.
Created attachment 99797 [details] Patch
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 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 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');
(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.
Created attachment 99921 [details] Asking cq
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.
(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 on attachment 99921 [details] Asking cq Clearing flags on attachment: 99921 Committed r90536: <http://trac.webkit.org/changeset/90536>
All reviewed patches have been landed. Closing bug.
> > > 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!