[webkit-reviews] review denied: [Bug 54682] Improvement in code quality, memory usage, and performance in ContainerNode.cpp : [Attachment 82850] patch improving code quality, performance, and memory usage in ContainerNode.cpp

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied sangeetha.sugavanam at nokia.com's
request for review:
Bug 54682: Improvement in code quality, memory usage, and performance in
ContainerNode.cpp
https://bugs.webkit.org/show_bug.cgi?id=54682

Attachment 82850: patch improving code quality, performance, and memory usage
in ContainerNode.cpp
https://bugs.webkit.org/attachment.cgi?id=82850&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82850&action=review

> Source/WebCore/ChangeLog:5
> +	   Need a short description and bug URL 

Typically you'd remove this line.

> Source/WebCore/ChangeLog:6
> +I am improving code quality in the method getUpperLeftCorner() in the file
ContainerNode.cpp.  In this function, originally we were setting the value of
pointer p to the value of pointer o.  By having a careful look at the code, it
looks like we do not need to have the pointer p because it is not used
anywhere.  The bug url for this is https://bugs.webkit.org/show_bug?id=54682.  


Typically, you'd indent this line (and wrap the text).

> Source/WebCore/ChangeLog:8
> +	   No new tests. 

This line should go away.

> Source/WebCore/ChangeLog:9
> +There are no tests for this particular case because this is an improvement
in code quality which does not in anyway impact the behavior of the given
function.  

This line should be indented.

> Source/WebCore/ChangeLog:11
> +

In short see other ChangeLog entries and how they are formated.

> Source/WebCore/dom/ContainerNode.cpp:788
>      // What is this code really trying to do?

Love this comment.

> Source/WebCore/dom/ContainerNode.cpp:821
> +	   if (o->node() && o->node() == this && o->isText() && !o->isBR() &&
!toRenderText(o)->firstTextBox()) {

It is not true that p == o here. Note that if o has a first child, then o gets
moved to that, but p stays where it is, so this code is not the same as before.


If this code change passes all layout tests, it would be interesting to create
a layout test fails due to this change.