Bug 205876 – Move allowPlugins to FrameLoader

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

[Rob Buis]

Move allowPlugins to Frame.

[Rob Buis]

Created attachment 387008 [details]
Patch

[Rob Buis]

Created attachment 395846 [details]
Patch

[Darin Adler]

Comment on attachment 395846 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Move allowPlugins to Frame since the FrameLoader
> +        ends up delegating to the setting on the Frame.
> +        This makes for less delegation and less dependency
> +        on FrameLoader.

What I don’t like about this is that we still have a long term plan of cutting the Frame class down to the absolute minimum, with each consideration about frames in a purpose-specific logic. I consider everything in Frame except for the basics to be there only for legacy reasons, and propose that we move it out.

This is just undoing what we did when we created the FrameLoader in the first place. The whole point was to cut the giant Frame class down to size and head in the direction of separate objects with relationships that could be simplified over time.

Seems fine to move this out of SubframeLoader, but not great to move it out of FrameLoader.

[Rob Buis]

Created attachment 396275 [details]
Patch

[Rob Buis]

Created attachment 396287 [details]
Patch

[Rob Buis]

Created attachment 396300 [details]
Patch

[Darin Adler]

Comment on attachment 396300 [details]
Patch

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

This change seems OK, but I think we are lacking in vision about the direction we intend for the loader set of objects.

Long ago I had this concept:

1) Get loading out of Frame itself, moving it to a loader object, which became FrameLoader, to help Frame not be a "god object".

2) Have the FrameLoader only contain code about loading before we have a specific document loaded, and put the loading of the document be managed by DocumentLoader. One FrameLoader, a succession of DocumentLoader objects. Minimize data members in FrameLoader that keep getting reset as a new document arrives, move those into DocumentLoader.

3) Can’t remember where SubframeLoader came from and what its relationship with FrameLoader and DocumentLoader would be, but perhaps it’s about things that load inside the frame as opposed to the main document in the frame?

Another small mess is that there are things in Frame and FrameLoader (hopefully not in DocumentLoader) that should really only be done for the top level main frame, and not for subframes. A while back I tried to make MainFrame a derived class different from Frame and move things that should only be done for the main frame into MainFrame, and also move things from Page to MainFrame, but that never got far and eventually Chris Dumez removed my MainFrame class.

Again, motion between these classes of functions or data makes more sense when in the context of our overall goals. Something that might make things prettier in the short term might instead be evidence that there is code that should be moving *in* to one of the loader classes rather than code that should be easier to call on the object that is closest at hand.

> Source/WebCore/loader/FrameLoader.h:335
> +    WEBCORE_EXPORT bool allowPlugins();

In Apple’s Cocoa naming guidelines, they try to name functions like this so they are clearly a boolean predicate, and not ambiguously possibly a command to the frame loader saying "you shall allow plug-ins".

In WebKit we have often followed suit.

That’s why we use named like "shouldAllowPlugins" or "arePluginsEnabled". And it seems like we could improve the name here.

> Source/WebCore/loader/SubframeLoader.cpp:157
> -    if ((!allowPlugins() && !MIMETypeRegistry::isApplicationPluginMIMEType(mimeType)))
> +    if ((!m_frame.settings().arePluginsEnabled() && !MIMETypeRegistry::isApplicationPluginMIMEType(mimeType)))

Is this good? If we are wiling to do the check directly then why do we need an allowPlugins function at all?

[Rob Buis]

Created attachment 396408 [details]
Patch

[Rob Buis]

Created attachment 396412 [details]
Patch

[Rob Buis]

Comment on attachment 396300 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.h:335
>> +    WEBCORE_EXPORT bool allowPlugins();
> 
> In Apple’s Cocoa naming guidelines, they try to name functions like this so they are clearly a boolean predicate, and not ambiguously possibly a command to the frame loader saying "you shall allow plug-ins".
> 
> In WebKit we have often followed suit.
> 
> That’s why we use named like "shouldAllowPlugins" or "arePluginsEnabled". And it seems like we could improve the name here.

Makes sense, I think arePluginsEnabled is good here as it asks the Setting with the same name.

[Rob Buis]

Thanks for the detailed description of the design that went into this! I will attempt to do more patches for FrameLoader/DocumentLoader as it has some overlap with Fetch.

(In reply to Darin Adler from comment #7)
> Comment on attachment 396300 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396300&action=review
> 

> 3) Can’t remember where SubframeLoader came from and what its relationship
> with FrameLoader and DocumentLoader would be, but perhaps it’s about things
> that load inside the frame as opposed to the main document in the frame?

FWIW SubframeLoader.h says:
It handles the higher level logic of loading both subframes and plugins.

[Rob Buis]

Comment on attachment 396300 [details]
Patch

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

>> Source/WebCore/loader/SubframeLoader.cpp:157
>> +    if ((!m_frame.settings().arePluginsEnabled() && !MIMETypeRegistry::isApplicationPluginMIMEType(mimeType)))
> 
> Is this good? If we are wiling to do the check directly then why do we need an allowPlugins function at all?

It did look stange to me so I simply replaced && with || and that broke a lot of stuff. So it seems that arePluginsEnabled is enough to stop requesting the plugin, *unless* we deal with an application plugin. I chose a different logic form to hopefully make it more clear.

[EWS]

Committed r260239: <https://trac.webkit.org/changeset/260239>

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

[Radar WebKit Bug Importer]

<rdar://problem/61924336>