Bug 61114

Created attachment 94051 [details] Patch

(In reply to comment #1) I like this clean-up very much! :) As a follow-up, it'd be great if we could move createRenderer into the factory as well.

Comment on attachment 94051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94051&action=review I am not convinced that we should be passing NodeRendererFactory to rendererIsNeeded. This leads to awkwardness like recreating it in CharacterData. Also, a Factory that's used as a context is more of a builder. Perhaps we could separate these two instead? NodeRendererFactory holds NodeRenderingContext, which is passed to rendererIsNeeded? > Source/WebCore/ChangeLog:139 > + (WebCore::SVGViewElement::rendererIsNeeded): If you're not putting any data here, might as well clip the long list. > Source/WebCore/WebCore.vcproj/WebCore.vcproj:45400 > + <File You need a change do DOMAllinOne.cpp to make this build on platform/win. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:2629 > - 892CF210134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 892CF208134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h */; }; > + 892CF210134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 892CF208134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h */; }; This seems like an unrelated change. Perhaps we first need to sort the project file and then rebase this patch? > Source/WebCore/dom/CharacterData.cpp:185 > + if ((!renderer() || !rendererIsNeeded(NodeRendererFactory(this, renderer()->style()))) && attached()) { This instantiation seems unfortunate. We now do extra work where none was done before.

Roland, Dimitri, thank you for taking a look! (In reply to comment #2) > (In reply to comment #1) > > I like this clean-up very much! :) As a follow-up, it'd be great if we could move createRenderer into the factory as well. createRenderer() is polymorphic so it's hard to move even it can be wrapped by the builder. In that viewpoint, the builder might be really a builder-mediator. but the name is too long ;-) (In reply to comment #3) > (From update of attachment 94051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94051&action=review > > I am not convinced that we should be passing NodeRendererFactory to rendererIsNeeded. This leads to awkwardness like recreating it in CharacterData. Also, a Factory that's used as a context is more of a builder. Perhaps we could separate these two instead? NodeRendererFactory holds NodeRenderingContext, which is passed to rendererIsNeeded? Sure. I'll make a separate context object. For CharacterData, it' been actually broken by the content-element change because the rendererIfNeeded() depends parentNode() but it shouldn't. That's really unfortunate. But I don't come up with the fix at this time.

Created attachment 94176 [details] Patch

Updated patch, introducing NodeRenderingContext. > > Source/WebCore/ChangeLog:139 > > + (WebCore::SVGViewElement::rendererIsNeeded): > > If you're not putting any data here, might as well clip the long list. Makes sense. removed excepting filenames. > > > Source/WebCore/WebCore.vcproj/WebCore.vcproj:45400 > > + <File > > You need a change do DOMAllinOne.cpp to make this build on platform/win. > Oops. Added. > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:2629 > > - 892CF210134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 892CF208134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h */; }; > > + 892CF210134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h in Headers */ = {isa = PBXBuildFile; fileRef = 892CF208134C8BB300AAEDA1 /* JSStorageInfoQuotaCallback.h */; }; > > This seems like an unrelated change. Perhaps we first need to sort the project file and then rebase this patch? Hmm. Now its gone git rebase removed these. > > > Source/WebCore/dom/CharacterData.cpp:185 > > + if ((!renderer() || !rendererIsNeeded(NodeRendererFactory(this, renderer()->style()))) && attached()) { > > This instantiation seems unfortunate. We now do extra work where none was done before. Add ResolveType to skip extra work. I think we need some work for Bug 61111, But I believe we have some way to minimize the impact.

Created attachment 94190 [details] updated bug description

Comment on attachment 94190 [details] updated bug description View in context: https://bugs.webkit.org/attachment.cgi?id=94190&action=review > Source/WebCore/ChangeLog:759 > > -2011-05-18 MORITA Hajime <morrita@google.com> > - > - Unreviewed attempt to fix clang build. > - > - * rendering/InlineTextBox.h: > - > 2011-05-18 Nat Duca <nduca@chromium.org> You shouldn't remove your history.

> You shouldn't remove your history. Oops. Thanks for the catch...

Created attachment 94192 [details] Patch

Comment on attachment 94192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94192&action=review Just one more pass. This is looks awesome. Thanks for working on this! > Source/WebCore/dom/CharacterData.cpp:185 > + if ((!renderer() || !rendererIsNeeded(NodeRenderingContext(this, renderer()->style(), NodeRenderingContext::ResolveNone))) && attached()) { Looks like this is the only time you use both ResolveNone and pass style in. Maybe just have a different constructor, instead of adding another enum? > Source/WebCore/dom/NodeRenderingContext.cpp:76 > + if (resolve == ResolveNone) > + return; > + > + ContainerNode* parent = m_node->parentOrHostNode(); > + if (!parent) > + return; > + > + if (parent->isShadowBoundary()) { > + m_location = LocationShadowChild; > + m_parentNodeForRenderingAndStyle = parent->shadowHost(); > + return; > + } > + > + m_location = LocationLightChild; > + > + if (parent->isElementNode()) { > + m_visualParentShadowRoot = toElement(parent)->shadowRoot(); > + > + if (m_visualParentShadowRoot) { > + if (ContainerNode* contentContainer = m_visualParentShadowRoot->activeContentContainer()) { > + m_phase = AttachContentForwarded; > + m_parentNodeForRenderingAndStyle = NodeRenderingContext(contentContainer).parentNodeForRenderingAndStyle(); > + return; > + } > + > + m_phase = AttachContentLight; > + m_parentNodeForRenderingAndStyle = parent; > + return; > + } > + } > + > + m_parentNodeForRenderingAndStyle = parent; I expected that Context would be a dumb storage of the data, and factory would be stuffing it with the right values, but I guess we can do this in a separate pass. > Source/WebCore/dom/NodeRenderingContext.h:29 > +#include "Node.h" Do we need this include? > Source/WebCore/dom/NodeRenderingContext.h:50 > + Node* node() const { return m_node; } Darin told me once before that having a method definition and declaration in the same line is bad for readability. I can't find his original comment now, but basically: * keep the declarations free of definitions * if you need a function inlined, put the definition right after of the class declaration, like http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.h&l=727&exact_package=chromium

Created attachment 94361 [details] Patch

Comment on attachment 94192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94192&action=review Thank you for reviewing again, Dimitri! Updated patch is here. >> Source/WebCore/dom/CharacterData.cpp:185 >> + if ((!renderer() || !rendererIsNeeded(NodeRenderingContext(this, renderer()->style(), NodeRenderingContext::ResolveNone))) && attached()) { > > Looks like this is the only time you use both ResolveNone and pass style in. Maybe just have a different constructor, instead of adding another enum? Good poing. Splited this path out to another construtor. >> Source/WebCore/dom/NodeRenderingContext.cpp:76 >> + m_parentNodeForRenderingAndStyle = parent; > > I expected that Context would be a dumb storage of the data, and factory would be stuffing it with the right values, but I guess we can do this in a separate pass. I'm not sure It might be better to hide m_phase and m_location inside the context class. In that sense, "context" might be "resolver". But the name sounds too active. >> Source/WebCore/dom/NodeRenderingContext.h:29 >> +#include "Node.h" > > Do we need this include? Good catch! removed. >> Source/WebCore/dom/NodeRenderingContext.h:50 >> + Node* node() const { return m_node; } > > Darin told me once before that having a method definition and declaration in the same line is bad for readability. I can't find his original comment now, but basically: > > * keep the declarations free of definitions > * if you need a function inlined, put the definition right after of the class declaration, like http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Node.h&l=727&exact_package=chromium Hmm. So I kicked inline functions out from the class declarations.

Comment on attachment 94361 [details] Patch Attachment 94361 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8723765

Created attachment 94395 [details] Another update to make a bot happy

Comment on attachment 94395 [details] Another update to make a bot happy ok. Please make sure to let the patch cook on bots before landing. This one didn't apply.

Committed r87125: <http://trac.webkit.org/changeset/87125>