Bug 63295 - Remove some unneeded functions from FrameLoader
Summary: Remove some unneeded functions from FrameLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-23 15:15 PDT by Darin Adler
Modified: 2011-06-24 17:09 PDT (History)
3 users (show)

See Also:


Attachments
Patch (18.22 KB, patch)
2011-06-23 17:54 PDT, Darin Adler
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2011-06-23 15:15:45 PDT
Remove some unneeded functions from FrameLoader
Comment 1 Darin Adler 2011-06-23 17:54:01 PDT
Created attachment 98445 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-23 18:09:35 PDT
Comment on attachment 98445 [details]
Patch

Attachment 98445 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8934306
Comment 3 WebKit Review Bot 2011-06-23 19:49:38 PDT
Comment on attachment 98445 [details]
Patch

Attachment 98445 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8935230
Comment 4 WebKit Review Bot 2011-06-23 20:27:29 PDT
Comment on attachment 98445 [details]
Patch

Attachment 98445 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8935238
Comment 5 Darin Adler 2011-06-23 20:39:11 PDT
Comment on attachment 98445 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:-283
> -    bool suppressOpenerInNewFrame() const { return m_suppressOpenerInNewFrame; }

Looks like this is actually Chromium-only rather than unused.
Comment 6 Adam Barth 2011-06-23 21:33:17 PDT
Comment on attachment 98445 [details]
Patch

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

R+, modulo compile errors, of course.

> Source/WebCore/loader/FrameLoader.cpp:1570
> -    stopLoadingSubframes(clearProvisionalItemPolicy);
> +    for (RefPtr<Frame> child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
> +        child->loader()->stopAllLoaders(clearProvisionalItemPolicy);

Inlining this function is very helpful.

> Source/WebCore/loader/FrameLoader.h:159
> -    // FIXME: Move this method to ResourceLoader with the rest of the ResourceError accessors.
>      ResourceError cancelledError(const ResourceRequest&) const;

It seems like this function should be removed.  Are there clients of this method that aren't better off calling the ResourceLoader version?
Comment 7 Darin Adler 2011-06-24 10:25:23 PDT
Comment on attachment 98445 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.h:159
>>      ResourceError cancelledError(const ResourceRequest&) const;
> 
> It seems like this function should be removed.  Are there clients of this method that aren't better off calling the ResourceLoader version?

I thought the same thing, believed the FIXME removed it, and then learned how this error is different from the other resource errors.

There are multiple places where a cancelled error has to be synthesized when there is no resource loader. In WebCore, this happens in DocumentLoader::stopLoading, FrameLoader::requestFromDelegate, and possibly in PluginStream::stop.

While there may be some good way to deal with this, simply removing the function isn’t clearly the correct step forward, which is why I removed the FIXME.
Comment 8 Adam Barth 2011-06-24 10:39:19 PDT
Ok.  That explains why it was harder to move than the other ones.  :)
Comment 9 Darin Adler 2011-06-24 17:09:46 PDT
Committed r89715: <http://trac.webkit.org/changeset/89715>