Bug 53897 – [chromium] WebPageSerializerImpl doesn't serialize sub-frames correctly

• Component: New Bugs
• Status: RESOLVED FIXED

Exported sub-frames are not referenced correctly by it's parent frame. Exported sub-frames also is using the wrong path for it's resources.

This bug is related to http://code.google.com/p/chromium/issues/detail?id=25303

Created attachment 81439 [details]
patch for WebPageSerializerImpl.cpp

Fixed resource paths in sub-frames. Also made sure sub-frames are referenced correctly from parent frame.

[Patrick R. Gansterer]

Comment on attachment 81439 [details]
patch for WebPageSerializerImpl.cpp

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

Created attachment 81442 [details]
Fixed formatting and removed const cast

(In reply to comment #2)
> (From update of attachment 81439 [details])
> 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.

Fixed.

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

Fixed.

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

Removed the const_cast and made fromFrameOwnerElement take a const argument instead.

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

Fixed.

[Patrick R. Gansterer]

View in context: https://bugs.webkit.org/attachment.cgi?id=81442&action=review

Did you used svn-create-patch to generate this patch? Seams you patch has a problem to apply clearly: See the bubbles beside your patch. They should be green, like at your last patch.
FYI: "Tools/Scripts/webkit-patch upload" can help a load when creating a patch

> Source/WebKit/chromium/ChangeLog:5
> +        Fixed resource paths in sub-frames. Also made sure sub-frames are referenced correctly from parent frame. https://bugs.webkit.org/show_bug.cgi?id=53897

We usually write a short description in the first line after the "reviewed by" and in the next line the bug# and after an additional empty line comes a "detailed description". See the example at http://webkit.org/coding/contributing.html#changelogs.

Created attachment 81561 [details]
Fixed ChangeLog

[Adam Barth]

Comment on attachment 81561 [details]
Fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=81561&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1963
> -WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(Element* element)
> +WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(const Element* element)

Why is this change related to the problem you're trying to solve?  We almost never have "const Element*".

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

Why do we need to call KURL if we're trying to get a String in the end?

[Adam Barth]

Those are really more questions than anything...

(In reply to comment #7)
> (From update of attachment 81561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81561&action=review
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1963
> > -WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(Element* element)
> > +WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(const Element* element)
> 
> Why is this change related to the problem you're trying to solve?  We almost never have "const Element*".

openTagToString is passed a const Element*. So I either had to remove the constness, cast it away or make fromFrameOwnerElement take a const argument. 
I thought the latter was the cleaner solution.

> 
> > Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:331
> > +                        String completeURL = subFrame ? KURL(subFrame->url()) : param->document->completeURL(attrValue);
> 
> Why do we need to call KURL if we're trying to get a String in the end?

That's how it was done previously in that file, e.g in the constructor and completeURL(). I just stuck to the same style.

[Adam Barth]

Comment on attachment 81561 [details]
Fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=81561&action=review

>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1963
>>> -WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(Element* element)
>>> +WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(const Element* element)
>> 
>> Why is this change related to the problem you're trying to solve?  We almost never have "const Element*".
> 
> openTagToString is passed a const Element*. So I either had to remove the constness, cast it away or make fromFrameOwnerElement take a const argument. 
> I thought the latter was the cleaner solution.

It's better to remove the constness.  "const Element" doesn't make any sense.

>>> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:331
>>> -                        String completeURL = param->document->completeURL(attrValue);
>>> +                        WebFrameImpl* subFrame = WebFrameImpl::fromFrameOwnerElement(element);
>>> +                        String completeURL = subFrame ? KURL(subFrame->url()) : param->document->completeURL(attrValue);
>> 
>> Why do we need to call KURL if we're trying to get a String in the end?
> 
> That's how it was done previously in that file, e.g in the constructor and completeURL(). I just stuck to the same style.

That probably wrong.  We should probably fix it.

Created attachment 81915 [details]
Patch

(In reply to comment #10)
> (From update of attachment 81561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81561&action=review
> 
> >>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1963
> >>> -WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(Element* element)
> >>> +WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(const Element* element)
> >> 
> >> Why is this change related to the problem you're trying to solve?  We almost never have "const Element*".
> > 
> > openTagToString is passed a const Element*. So I either had to remove the constness, cast it away or make fromFrameOwnerElement take a const argument. 
> > I thought the latter was the cleaner solution.
> 
> It's better to remove the constness.  "const Element" doesn't make any sense.

Removed the constness from the function argument.

> 
> >>> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:331
> >>> -                        String completeURL = param->document->completeURL(attrValue);
> >>> +                        WebFrameImpl* subFrame = WebFrameImpl::fromFrameOwnerElement(element);
> >>> +                        String completeURL = subFrame ? KURL(subFrame->url()) : param->document->completeURL(attrValue);
> >> 
> >> Why do we need to call KURL if we're trying to get a String in the end?
> > 
> > That's how it was done previously in that file, e.g in the constructor and completeURL(). I just stuck to the same style.
> 
> That probably wrong.  We should probably fix it.

Changed it to grab the url directly rather than calling KURL.

Is everything looking ok?

[Darin Fisher (:fishd, Google)]

Comment on attachment 81915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81915&action=review

> Source/WebKit/chromium/ChangeLog:8
> +        Serialize sub-frames correctly when using 'save page as'.

it would be helpful if this ChangeLog actually described the bug
you are fixing.  what was incorrect about the way sub-frames were
serialized?  one shouldn't need to open the bug report to figure
out why a change is being made.

Created attachment 88236 [details]
Better ChangeLog description

[Darin Fisher (:fishd, Google)]

Comment on attachment 88236 [details]
Better ChangeLog description

View in context: https://bugs.webkit.org/attachment.cgi?id=88236&action=review

> Source/WebKit/chromium/ChangeLog:15
> +        sub-directory appended to the path of resources located in the same sub-directory.

appended -> prepended

Created attachment 88760 [details]
Updated ChangeLog

Comment on attachment 88760 [details]
Updated ChangeLog

Changed "appended" to "prepended".

[Patrick R. Gansterer]

Created attachment 88764 [details]
Patch for landing

[Patrick R. Gansterer]

(In reply to comment #18)
> (From update of attachment 88760 [details])
> Changed "appended" to "prepended".

FYI: If a patch got a "r+" you can replace the "Reviewed by NOBODY (OOPS!)." with the correct name or you shouldn't remove the "r+" flag from "old" attachment. Otherwise the commit-queue can't find the correct reviewer and and a reviewer must set the r+ flag again.
Also there was an unneeded space after the "saved." in line12. ;-)

[WebKit Commit Bot]

Comment on attachment 88764 [details]
Patch for landing

Clearing flags on attachment: 88764

Committed r83252: <http://trac.webkit.org/changeset/83252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.