Bug 211051 – Make loadURLIntoChildFrame private and non-exported

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

Make loadURLIntoChildFrame private and non-exported to reduce the amount of public API functions that start loads. In order to do this loadURLIntoChildFrame is being called from SubframeLoader and SubframeLoader is made an inner class of FrameLoader. Because this simplifies createFrame (and make createFrame behave strictly like its name) url and referrer do not have to be passed.

[Rob Buis]

Created attachment 397627 [details]
Patch

[Darin Adler]

Comment on attachment 397627 [details]
Patch

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

> Source/WebCore/ChangeLog:49
> +        (WebCore::FrameLoader::SubframeLoader::SubframeLoader):
> +        (WebCore::FrameLoader::SubframeLoader::clear):
> +        (WebCore::FrameLoader::SubframeLoader::requestFrame):
> +        (WebCore::FrameLoader::SubframeLoader::resourceWillUsePlugin):
> +        (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable):
> +        (WebCore::FrameLoader::SubframeLoader::requestPlugin):
> +        (WebCore::FrameLoader::SubframeLoader::requestObject):
> +        (WebCore::FrameLoader::SubframeLoader::createJavaAppletWidget):
> +        (WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe):
> +        (WebCore::FrameLoader::SubframeLoader::loadSubframe):
> +        (WebCore::FrameLoader::SubframeLoader::shouldUsePlugin):
> +        (WebCore::FrameLoader::SubframeLoader::loadPlugin):
> +        (WebCore::FrameLoader::SubframeLoader::completeURL const):
> +        (WebCore::FrameLoader::SubframeLoader::shouldConvertInvalidURLsToBlank const):
> +        (WebCore::SubframeLoader::SubframeLoader): Deleted.
> +        (WebCore::SubframeLoader::clear): Deleted.
> +        (WebCore::SubframeLoader::requestFrame): Deleted.
> +        (WebCore::SubframeLoader::resourceWillUsePlugin): Deleted.
> +        (WebCore::SubframeLoader::pluginIsLoadable): Deleted.
> +        (WebCore::SubframeLoader::requestPlugin): Deleted.
> +        (WebCore::SubframeLoader::requestObject): Deleted.
> +        (WebCore::SubframeLoader::createJavaAppletWidget): Deleted.
> +        (WebCore::SubframeLoader::loadOrRedirectSubframe): Deleted.
> +        (WebCore::SubframeLoader::loadSubframe): Deleted.
> +        (WebCore::SubframeLoader::shouldUsePlugin): Deleted.
> +        (WebCore::SubframeLoader::loadPlugin): Deleted.
> +        (WebCore::SubframeLoader::completeURL const): Deleted.
> +        (WebCore::SubframeLoader::shouldConvertInvalidURLsToBlank const): Deleted.

I know the change log generation machinery makes this, but you can just delete it if you think it’s not helpful.

> Source/WebKit/ChangeLog:9
> +        Adapt createFrame to stricty create a sub Frame and
> +        not load anything.

Typo in strictly.

it‘s "subframe", not "sub Frame".

> Source/WebKitLegacy/mac/ChangeLog:9
> +        Adapt createFrame to stricty create a sub Frame and
> +        not load anything.

Ditto.

> Source/WebKitLegacy/win/ChangeLog:9
> +        Adapt createFrame to stricty create a sub Frame and
> +        not load anything.

Again.

> Source/WebCore/loader/FrameLoader.h:158
> +    // This is a slight misnomer. It handles the higher level logic of loading both subframes and plugins.
> +    class SubframeLoader {
> +        WTF_MAKE_NONCOPYABLE(SubframeLoader); WTF_MAKE_FAST_ALLOCATED;
> +    public:
> +        explicit SubframeLoader(Frame&);
> +
> +        void clear();
> +
> +        bool requestFrame(HTMLFrameOwnerElement&, const String& url, const AtomString& frameName, LockHistory = LockHistory::Yes, LockBackForwardList = LockBackForwardList::Yes);
> +        bool requestObject(HTMLPlugInImageElement&, const String& url, const AtomString& frameName,
> +            const String& serviceType, const Vector<String>& paramNames, const Vector<String>& paramValues);
> +
> +        RefPtr<Widget> createJavaAppletWidget(const IntSize&, HTMLAppletElement&, const Vector<String>& paramNames, const Vector<String>& paramValues);
> +
> +        bool containsPlugins() const { return m_containsPlugins; }
> +
> +        bool resourceWillUsePlugin(const String& url, const String& mimeType);
> +
> +    private:
> +        bool requestPlugin(HTMLPlugInImageElement&, const URL&, const String& serviceType, const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback);
> +        Frame* loadOrRedirectSubframe(HTMLFrameOwnerElement&, const URL&, const AtomString& frameName, LockHistory, LockBackForwardList);
> +        RefPtr<Frame> loadSubframe(HTMLFrameOwnerElement&, const URL&, const String& name, const String& referrer);
> +        bool loadPlugin(HTMLPlugInImageElement&, const URL&, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback);
> +
> +        bool shouldUsePlugin(const URL&, const String& mimeType, bool hasFallback, bool& useFallback);
> +        bool pluginIsLoadable(const URL&, const String& mimeType);
> +
> +        URL completeURL(const String&) const;
> +
> +        bool shouldConvertInvalidURLsToBlank() const;
> +
> +        bool m_containsPlugins;
> +        Frame& m_frame;
> +    };

This could still be in its own header. Here we would just write:

    class SubframeLoader;

In the SubframeLoader.h header we would defined:

    class FrameLoader::SubframeLoader {

Everything else would be the same.

> Source/WebCore/loader/SubframeLoader.cpp:65
>      : m_containsPlugins(false)

Seems like this should be initialized in the class definition.

[Rob Buis]

Created attachment 397652 [details]
Patch

[Rob Buis]

Created attachment 397655 [details]
Patch

[Rob Buis]

Created attachment 397661 [details]
Patch

[Rob Buis]

Created attachment 397673 [details]
Patch

[EWS]

Committed r260751: <https://trac.webkit.org/changeset/260751>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397673 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62447689>

[Rob Buis]

Comment on attachment 397627 [details]
Patch

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

>> Source/WebCore/ChangeLog:49
>> +        (WebCore::SubframeLoader::shouldConvertInvalidURLsToBlank const): Deleted.
> 
> I know the change log generation machinery makes this, but you can just delete it if you think it’s not helpful.

Ah, forgot to do that before landing! Will try to remember next time.

>> Source/WebKit/ChangeLog:9
>> +        not load anything.
> 
> Typo in strictly.
> 
> it‘s "subframe", not "sub Frame".

Done.

>> Source/WebKitLegacy/mac/ChangeLog:9
>> +        not load anything.
> 
> Ditto.

Done.

>> Source/WebKitLegacy/win/ChangeLog:9
>> +        not load anything.
> 
> Again.

Done.

>> Source/WebCore/loader/FrameLoader.h:158
>> +    };
> 
> This could still be in its own header. Here we would just write:
> 
>     class SubframeLoader;
> 
> In the SubframeLoader.h header we would defined:
> 
>     class FrameLoader::SubframeLoader {
> 
> Everything else would be the same.

Done.

>> Source/WebCore/loader/SubframeLoader.cpp:65
>>      : m_containsPlugins(false)
> 
> Seems like this should be initialized in the class definition.

Done.

[Darin Adler]

Comment on attachment 397673 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:428
> +    void loadURLIntoChildFrame(const URL&, const String& referer, Frame*);

We should fix this spelling error at some point. The word is "referrer" and "referer" is a misspelling. A misspelling that is part of the web platform now because the HTTP header field name has to remain misspelled for compatibility, but it need not be anywhere else other than in string constants for that field’s name.