Bug 93138

Created attachment 156535 [details] Patch

What do you think about this, Adam and Jochen (since I think both you know things about SubframeLoader)? I think it makes sense to extract these tests out into a separate method, but the structure as it stands isn't terribly clear to me. Is there a good reason to have both requestPlugin and loadPlugin? Perhaps we could simply merge them?

Comment on attachment 156535 [details] Patch Makes sense. Usually we phrase these sorts of functions in terms of what's allowed rather than what's blocked to avoid having too many confusing double negatives.

Comment on attachment 156535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156535&action=review > Source/WebCore/loader/SubframeLoader.cpp:116 > + if (document() && document()->securityOrigin()->isLocal() && !settings->isJavaEnabledForLocalFiles()) For a followup patch, I wonder if we should be getting the document from the pluginElement. That way we'll always find it and we can remove these null checks.

(In reply to comment #3) > (From update of attachment 156535 [details]) > Makes sense. Usually we phrase these sorts of functions in terms of what's allowed rather than what's blocked to avoid having too many confusing double negatives. There's already `allowPlugins`, which (I think) is more concerned with content settings. I'm not sure it makes a lot of sense to have `allowPlugins?` directly followed by `isPluginAllowed?`. Conceptually, it's clear ("Are plugins allowed, in general?" is a different question to "Is this specific plugin allowed?"), but it doesn't read well. I'd originally planned to push the `isSandboxed` check into `allowPlugins` (since it falls into the former question), but stopped since it would have the side-effect of notifying `didNotAllowPlugins`, which I think Chromium does something interesting with. Any thoughts on that split?

> There's already `allowPlugins`, which (I think) is more concerned with content settings. I'm not sure it makes a lot of sense to have `allowPlugins?` directly followed by `isPluginAllowed?`. Conceptually, it's clear ("Are plugins allowed, in general?" is a different question to "Is this specific plugin allowed?"), but it doesn't read well. > > I'd originally planned to push the `isSandboxed` check into `allowPlugins` (since it falls into the former question), but stopped since it would have the side-effect of notifying `didNotAllowPlugins`, which I think Chromium does something interesting with. > > Any thoughts on that split? I think it's better to keep the sandbox check separate. That's what we do in other cases. Perhaps shouldLoadPlugin(...) is a good name for the function?

(In reply to comment #6) > > There's already `allowPlugins`, which (I think) is more concerned with content settings. I'm not sure it makes a lot of sense to have `allowPlugins?` directly followed by `isPluginAllowed?`. Conceptually, it's clear ("Are plugins allowed, in general?" is a different question to "Is this specific plugin allowed?"), but it doesn't read well. > > > > I'd originally planned to push the `isSandboxed` check into `allowPlugins` (since it falls into the former question), but stopped since it would have the side-effect of notifying `didNotAllowPlugins`, which I think Chromium does something interesting with. > > > > Any thoughts on that split? > > I think it's better to keep the sandbox check separate. That's what we do in other cases. Perhaps shouldLoadPlugin(...) is a good name for the function? Makes sense. However, there's already `shouldUsePlugin`, which determines whether the plugin rendering process should be kicked off for a particular resource. I've been sitting here about 5 minutes typing new suggestions for names, and then backspacing over them. :)

(In reply to comment #7) > (In reply to comment #6) > > > There's already `allowPlugins`, which (I think) is more concerned with content settings. I'm not sure it makes a lot of sense to have `allowPlugins?` directly followed by `isPluginAllowed?`. Conceptually, it's clear ("Are plugins allowed, in general?" is a different question to "Is this specific plugin allowed?"), but it doesn't read well. > > > > > > I'd originally planned to push the `isSandboxed` check into `allowPlugins` (since it falls into the former question), but stopped since it would have the side-effect of notifying `didNotAllowPlugins`, which I think Chromium does something interesting with. > > > > > > Any thoughts on that split? > > > > I think it's better to keep the sandbox check separate. That's what we do in other cases. Perhaps shouldLoadPlugin(...) is a good name for the function? > > Makes sense. However, there's already `shouldUsePlugin`, which determines whether the plugin rendering process should be kicked off for a particular resource. > > I've been sitting here about 5 minutes typing new suggestions for names, and then backspacing over them. :) pluginIsLoadable?

Lets not get too hung up on the name. Pick you favorite. :)

Created attachment 156539 [details] Patch

(In reply to comment #9) > Lets not get too hung up on the name. Pick you favorite. :) `pluginIsLoadable` is the least bad name I've come up with. CQ? :)

Comment on attachment 156539 [details] Patch Clearing flags on attachment: 156539 Committed r124704: <http://trac.webkit.org/changeset/124704>

All reviewed patches have been landed. Closing bug.