[Webkit-unassigned] [Bug 89000] [CSS Regions] Fix the lifecycle for the flow objects and their renderers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89000





--- Comment #21 from Andrei Bucur <abucur at adobe.com>  2012-06-21 01:32:37 PST ---
(In reply to comment #20)
> (From update of attachment 148346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148346&action=review
> 
> So, this patch is way beyond me, but I had a few little nitpicks:
> 
> > Source/WebCore/CMakeLists.txt:667
> > +    dom/WebKitNamedFlowCollection.cpp
> 
> It seems vaguely odd to me that the filenames are WebKit-prefixed as well, but it seems like this isn't even remotely the first one, so I guess it's OK. Class names, too.
I thought the decision to prefix the NamedFlow class had some good reasons behind it so I've made the same thing for the collection.

> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:54468
> > +				<FileConfiguration
> > +					Name="Debug|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Release|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Debug_Cairo_CFLite|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Release_Cairo_CFLite|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Debug_All|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +				<FileConfiguration
> > +					Name="Production|Win32"
> > +					ExcludedFromBuild="true"
> > +					>
> > +					<Tool
> > +						Name="VCCLCompilerTool"
> > +					/>
> > +				</FileConfiguration>
> > +			</File>
> 
> I wonder if you meant for all this VCCLCompilerTool stuff to be here?
I hand-crafted this part to be similar with the configuration for the WebKitNamedFlow.* files :). You're saying that they are not actually required?

> 
> > Source/WebCore/dom/Document.cpp:1129
> > +    // It's possible to have pendiding styles not applied that affect the existing flows.
> 
> s/pendiding/pending/, I think.

Will do, thx!
> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:106
> > +        for (NamedFlowContentNodes::const_iterator it = m_parentFlowThread->contentNodes().begin(); it != m_parentFlowThread->contentNodes().end(); ++it) {
> 
> Could grab contentNodes() just once and make this line somewhat less long.
Yes.

> 
> > Source/WebCore/dom/WebKitNamedFlow.cpp:107
> > +            Node* node = const_cast<Node*>(*it);
> 
> Can you reassure my innate fear of const_cast in this case? (i.e. explain why it's safe)
This code was inherited from the old version :). However, I think the const_cast should be safe because the original Node* HashSet on the renderer doesn't have any const-ness attached to it.

> 
> > Source/WebCore/dom/WebKitNamedFlow.h:62
> > +        STATE_CREATED,
> > +        STATE_NULL
> 
> I think we usually use UpperCamelCase for enum members. And maybe nicer names, too. (FlowCreatedState, FlowNullState, maybe? I don't know)
Sounds good.

> 
> > Source/WebCore/rendering/FlowThreadController.cpp:72
> > +    // Sanity check for the inexistence of a named flow in the "CREATED" state with the same name.
> 
> "inexistence" is not a particularly common word in English (to the extent that my spell-check doesn't even accept it). I would go with "absence" or something like that.
Yea, this is why I had the feeling I've never heard that word before. I'll replace it.

> 
> > LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:21
> > -                window.layoutTestController.dumpAsText();
> > +            window.layoutTestController.dumpAsText();
> 
> I think you broke the indentation here.
Yep :).

> 
> > LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:25
> > +            // It should not be possible to get the NamedFlow object for an inexistent flow.
> 
> Ditto on the inexistent thing.
:)

> 
> > LayoutTests/fast/regions/webkit-named-flow-invalid-name.html:27
> > -            assert(namedFlow != null, "Named flow for flow should not be null");
> > +            assert(namedFlow == null, "Named flow for flow should not be null");
> 
> Did you mean to change the assert comment string here?
I just wanted to confuse people reading that. J/k I'll update it :).

Thanks for the review! Update coming later today.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.