Bug 61114 – [Refactoring] Pass NodeRendererFactory to Node::rendererIsNeeded()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

This change is a preparation for Bug 61111, and other possible problems around renderer creation inside <content>

[Hajime Morrita]

Created attachment 94051 [details]
Patch

[Roland Steiner]

(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.

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 94176 [details]
Patch

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 94190 [details]
updated bug description

[Kent Tamura]

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  <[e-mail]>
> -
> -        Unreviewed attempt to fix clang build.
> -
> -        * rendering/InlineTextBox.h:
> -
>  2011-05-18  Nat Duca  <[e-mail]>

You shouldn't remove your history.

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 94192 [details]
Patch

[Dimitri Glazkov (Google)]

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

[Hajime Morrita]

Created attachment 94361 [details]
Patch

[Hajime Morrita]

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.

[Early Warning System Bot]

Comment on attachment 94361 [details]
Patch

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

[Hajime Morrita]

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

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

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