[webkit-reviews] review denied: [Bug 53897] [chromium] WebPageSerializerImpl doesn't serialize sub-frames correctly : [Attachment 81439] patch for WebPageSerializerImpl.cpp

[bugzilla-daemon at webkit.org]

Patrick R. Gansterer <paroga at paroga.com> has denied public at fuzzac.com's request
for review:
Bug 53897: [chromium] WebPageSerializerImpl doesn't serialize sub-frames
correctly
https://bugs.webkit.org/show_bug.cgi?id=53897

Attachment 81439: patch for WebPageSerializerImpl.cpp
https://bugs.webkit.org/attachment.cgi?id=81439&action=review

------- Additional Comments from Patrick R. Gansterer <paroga at paroga.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81439&action=review

Every patch needs a ChangeLog. See http://webkit.org/coding/contributing.html
for mor information.

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:330
>			   // Get the absolute link
> -			   String completeURL =
param->document->completeURL(attrValue);
> +			   // handle iframe and frame tags

We usually write whole sentences. Should read: Handle iframe and frame tags.
Does this comment add any extra value? IMHO it does not, so we can remove it.
See webkit-dev thread for the actual discussion about comments:
https://lists.webkit.org/pipermail/webkit-dev/2011-January/015767.html

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:333
> +			   WebFrameImpl* subFrame = 
> +			       WebFrameImpl::fromFrameOwnerElement(
> +						  
const_cast<Element*>(element));

Is this const_cast really necessary?

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:336
> +			   String completeURL = subFrame ? 
> +			       KURL(subFrame->url()) :
> +			       param->document->completeURL(attrValue);

Can you write this in one line only. Not need for 3 lines :-) Same for the
subFrame above