[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 #20 from Tim Horton <timothy_horton at apple.com>  2012-06-21 00:50:05 PST ---
(From update of attachment 148346)
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.

> 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?

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

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

> 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)

> 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)

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

> LayoutTests/fast/regions/webkit-named-flow-content-nodes.html:21
> -                window.layoutTestController.dumpAsText();
> +            window.layoutTestController.dumpAsText();

I think you broke the indentation here.

> 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?

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