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.
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.
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
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?
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.
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?
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
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".
Created attachment 88764 [details] Patch for landing
(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. ;-)
Comment on attachment 88764 [details] Patch for landing Clearing flags on attachment: 88764 Committed r83252: <http://trac.webkit.org/changeset/83252>
All reviewed patches have been landed. Closing bug.