Plugins created for generating a snapshot (when plugin snapshotting is enabled), should run in a process with muted audio so that the user does not hear it.
<rdar://problem/12481743>
Created attachment 172915 [details] Patch
Comment on attachment 172915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172915&action=review > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:49 > +void PluginProcessManager::getPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, bool snapshotProcess, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) In general, I wonder if this boolean ought to be part of the structs used to initialize the plugin, instead of as a separate parameter in all of these functions. I also think that the variable/param names should be more in the line of "mutesAudio" or "isAudioMuted".
Comment on attachment 172915 [details] Patch Here are some high-level comments: I agree with Jon that we shouldn't hard-code "pluginSnapshotProcess" In the plug-in process, but instead use "muteAudio". I think we should use a separate PluginProcessManager member function for getting a snapshot plug-in process instead of adding a boolean. GetSitesWithData and ClearSiteData don't need to be sent to the snapshot plug-in process.
(In reply to comment #3) > (From update of attachment 172915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172915&action=review > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:49 > > +void PluginProcessManager::getPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, bool snapshotProcess, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > > In general, I wonder if this boolean ought to be part of the structs used to initialize the plugin, instead of as a separate parameter in all of these functions. I also think that the variable/param names should be more in the line of "mutesAudio" or "isAudioMuted". (In reply to comment #3) > (From update of attachment 172915 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172915&action=review > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:49 > > +void PluginProcessManager::getPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, bool snapshotProcess, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > > In general, I wonder if this boolean ought to be part of the structs used to initialize the plugin, instead of as a separate parameter in all of these functions. I also think that the variable/param names should be more in the line of "mutesAudio" or "isAudioMuted". Since the plugin is initialized after getting a connection to the process, it is necessary to pass the snapshot/non snapshot information to the plugin process manager. Will fix the variable and param names for PluginProcess to be along the lines of "mutesAudio" or "isAudioMuted".
Created attachment 173433 [details] Patch
(In reply to comment #4) > (From update of attachment 172915 [details]) > Here are some high-level comments: > > I agree with Jon that we shouldn't hard-code "pluginSnapshotProcess" In the plug-in process, but instead use "muteAudio". > I changed the process creation parameter to shouldMuteAudio. However trying to change the name of the parameter in other places such as PluginProcessProxy seemed to hamper readability. For instance changing the parameter of PluginProcessCrashed message from "snapshotProcess" to say "audioMuted" could also mean that a non snapshot process crashed while auto was muted. I think "snapshotProcess" makes it clear about which process crashed. So, I did not change the parameter name in other places. > I think we should use a separate PluginProcessManager member function for getting a snapshot plug-in process instead of adding a boolean. Added a separate getSnapshotPluginProcess method to PluginProcessManager and for symmetry to PluginProcessConnectionManager too. > > GetSitesWithData and ClearSiteData don't need to be sent to the snapshot plug-in process. Fixed. This also eliminated all changes to PluginSiteDataManager. Besides the above fixes, I also fixed a bug in PluginProcessConnectionManager::removePluginProcessConnection, where I was removing the connection in both snapshot and non snapshot connection maps instead of only removing from the map corresponding to the type of connection.
(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 172915 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172915&action=review > > > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:49 > > > +void PluginProcessManager::getPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, bool snapshotProcess, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > > > > In general, I wonder if this boolean ought to be part of the structs used to initialize the plugin, instead of as a separate parameter in all of these functions. I also think that the variable/param names should be more in the line of "mutesAudio" or "isAudioMuted". > > (In reply to comment #3) > > (From update of attachment 172915 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172915&action=review > > > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:49 > > > +void PluginProcessManager::getPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, bool snapshotProcess, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > > > > In general, I wonder if this boolean ought to be part of the structs used to initialize the plugin, instead of as a separate parameter in all of these functions. I also think that the variable/param names should be more in the line of "mutesAudio" or "isAudioMuted". > > Since the plugin is initialized after getting a connection to the process, it is necessary to pass the snapshot/non snapshot information to the plugin process manager. Will fix the variable and param names for PluginProcess to be along the lines of "mutesAudio" or "isAudioMuted". Jon, I didn't fully understand your comment earlier on using the struct used to initialize the plugin. I looked at the code again and found the struct Plugin::parameters you are referring to. I could add the bool to it, but it is not used on the receiving side. Only the PluginProxy is aware of it. So, i think it doesn't quite belong there.
Comment on attachment 173433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173433&action=review Looks great overall but needs some minor fixes. > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:310 > + AudioObjectPropertyAddress theAddress = {kAudioHardwarePropertyProcessIsAudible, kAudioObjectPropertyScopeGlobal, kAudioObjectPropertyElementMaster}; Please add spaces after the { and before the }, and use a more descriptive name than "theAddress". How about, propertyAddress. > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:311 > + UInt32 theData = 0; Same thing here. Just "data" or maybe "processIsAudibleProperty". > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:313 > + CRASH(); Do we really want to crash in this case? > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:86 > +PluginProcessProxy* PluginProcessManager::pluginProcessWithPath(const String& pluginPath, bool snapshotProcess) You should call this isSnapshotProcess since otherwise it sounds like you're snapshotting the process itself. Same with all other occurrences of "bool snapshotProcess" in the patch.
Created attachment 176065 [details] Patch
(In reply to comment #9) > (From update of attachment 173433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173433&action=review > > Looks great overall but needs some minor fixes. > > > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:310 > > + AudioObjectPropertyAddress theAddress = {kAudioHardwarePropertyProcessIsAudible, kAudioObjectPropertyScopeGlobal, kAudioObjectPropertyElementMaster}; > > Please add spaces after the { and before the }, and use a more descriptive name than "theAddress". How about, propertyAddress. > > > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:311 > > + UInt32 theData = 0; > > Same thing here. Just "data" or maybe "processIsAudibleProperty". Added spaces and renamed the local variables to propertyAddress and propertyData. > > > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:313 > > + CRASH(); > > Do we really want to crash in this case? Yeah, this was probably too drastic. The call to set the property is not expected to fail. So, i replaced the CRASH with a ASSERT_NOT_REACHED. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:86 > > +PluginProcessProxy* PluginProcessManager::pluginProcessWithPath(const String& pluginPath, bool snapshotProcess) > > You should call this isSnapshotProcess since otherwise it sounds like you're snapshotting the process itself. Same with all other occurrences of "bool snapshotProcess" in the patch. You are right, fixed all occurrences to read "isSnapshotProcess". Besides the above changes, I renamed PluginProcessConnectionManager::m_pathsAndRegularProcessConnections to PluginProcessConnectionManager::m_pathsAndNonSnapshotProcessConnections since it complements PluginProcessConnectionManager::m_pathsAndSnapshotProcessConnections better.
Comment on attachment 176065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176065&action=review Overall, r=me, but would suggest considering the nits mentioned. > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:314 > + ASSERT_NOT_REACHED(); This could be expressed without using an if statement. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:53 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); For readability, I'd much rather see us using named constants like "CreateSnapshotProcess, CreatePlugInProcess" rather than a boolean argument. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:90 > + if ((pluginProcessProxy->pluginInfo().path == pluginPath) && (pluginProcessProxy->isSnapshotProcess() == isSnapshotProcess)) The grouping parentheses are unnecessary here. > Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:46 > + static PassRefPtr<PluginProcessConnection> create(PluginProcessConnectionManager* pluginProcessConnectionManager, const String& pluginPath, bool isSnapshotProcess, CoreIPC::Connection::Identifier connectionIdentifier, bool supportsAsynchronousPluginInitialization) Extra space.
Created attachment 176102 [details] Patch
(In reply to comment #12) > (From update of attachment 176065 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176065&action=review > > Overall, r=me, but would suggest considering the nits mentioned. > > > Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:314 > > + ASSERT_NOT_REACHED(); > > This could be expressed without using an if statement. > I used the if statement to avoid a unused variable error. But, looks like it made the code obtuse. Removed the if statement and replaced with simple ASSERT() and UNUSED_PARAM(). > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:53 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > > For readability, I'd much rather see us using named constants like "CreateSnapshotProcess, CreatePlugInProcess" rather than a boolean argument. I agree that named constants are more readable, but in this case i improved the code by using inline comments both here and other occurrences of literal "true" or "false" in the patch. PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:90 > > + if ((pluginProcessProxy->pluginInfo().path == pluginPath) && (pluginProcessProxy->isSnapshotProcess() == isSnapshotProcess)) > > The grouping parentheses are unnecessary here. > Fixed here and in PluginProcessConnectionManager::getProcessConnection > > Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:46 > > + static PassRefPtr<PluginProcessConnection> create(PluginProcessConnectionManager* pluginProcessConnectionManager, const String& pluginPath, bool isSnapshotProcess, CoreIPC::Connection::Identifier connectionIdentifier, bool supportsAsynchronousPluginInitialization) > > Extra space. Thanks for catching this. I couldn't pin point the extra space straight away, even after looking at the diff again.
Created attachment 176110 [details] Patch
(In reply to comment #15) > Created an attachment (id=176110) [details] > Patch Only change from previous patch is the use of ASSERT_UNUSED() macro, pointed out by Jon Lee, instead of a separate ASSERT() and UNUSED_PARAM() macros in muteAudio().
Comment on attachment 176110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176110&action=review r=me with the comments removed. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:53 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); This is not standard webkit style. Please remove the comment. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:62 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, true /* isSnapshotProcess */); Ditto. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:76 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); Ditto. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:82 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); Ditto. > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp:54 > + return getProcessConnection(pluginPath, false /* isSnapshotProcess */); Ditto. > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp:59 > + return getProcessConnection(pluginPath, true /* isSnapshotProcess */); Ditto.
Created attachment 176348 [details] Patch
(In reply to comment #17) > (From update of attachment 176110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176110&action=review > > r=me with the comments removed. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:53 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); > > This is not standard webkit style. Please remove the comment. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:62 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, true /* isSnapshotProcess */); > > Ditto. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:76 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); > > Ditto. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:82 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); > > Ditto. > > > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp:54 > > + return getProcessConnection(pluginPath, false /* isSnapshotProcess */); > > Ditto. > > > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp:59 > > + return getProcessConnection(pluginPath, true /* isSnapshotProcess */); > > Ditto. Removed comments.
Comment on attachment 176348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176348&action=review > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:62 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > + pluginProcess->getPluginProcessConnection(reply); > +} > + > +void PluginProcessManager::getSnapshotPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > +{ > + ASSERT(!pluginPath.isNull()); > + > + PluginModuleInfo plugin = pluginInfoStore.infoForPluginWithPath(pluginPath); > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, true); As Jon previously requested, could you please used named enums here instead of bool flags? I had to lookup what this bool meant to see what the code was doing. If you'd named enums I wouldn't have had to look it up... and that's one reason why we used them! :) > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:76 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:82 > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:86 > +PluginProcessProxy* PluginProcessManager::pluginProcessWithPath(const String& pluginPath, bool isSnapshotProcess) Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:97 > +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(const PluginModuleInfo& plugin, bool isSnapshotProcess) Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:102 > + RefPtr<PluginProcessProxy> pluginProcess = PluginProcessProxy::create(this, plugin, isSnapshotProcess); Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:68 > + PluginProcessProxy* getOrCreatePluginProcess(const PluginModuleInfo&, bool isSnapshotProcess); > + PluginProcessProxy* pluginProcessWithPath(const String& pluginPath, bool isSnapshotProcess); Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:55 > + return adoptRef(new PluginProcessProxy(PluginProcessManager, pluginInfo, isSnapshotProcess)); Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:58 > +PluginProcessProxy::PluginProcessProxy(PluginProcessManager* PluginProcessManager, const PluginModuleInfo& pluginInfo, bool isSnapshotProcess) Same here. > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:67 > + , m_isSnapshotProcess(isSnapshotProcess) The member variable itself could be a bool, except then... > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:177 > + contexts[i]->sendToAllProcesses(Messages::WebProcess::PluginProcessCrashed(m_pluginInfo.path, m_isSnapshotProcess)); You'd have to convert it back to an enum here. > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:208 > + parameters.shouldMuteAudio = m_isSnapshotProcess; The parameter member could be an enum also, and we have encodeEnum and decodeEnum in CoreIPC for that reason. > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.h:65 > + static PassRefPtr<PluginProcessProxy> create(PluginProcessManager*, const PluginModuleInfo&, bool isSnapshotProcess); At this point I'll stop commenting on how it should be an enum.
BTW, I know you replied to Jon with: (In reply to comment #14) > (In reply to comment #12) > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:53 > > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > > > > For readability, I'd much rather see us using named constants like "CreateSnapshotProcess, CreatePlugInProcess" rather than a boolean argument. > > I agree that named constants are more readable, but in this case i improved the code by using inline comments both here and other occurrences of literal "true" or "false" in the patch. > PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false /* isSnapshotProcess */); We much prefer code to comments, and named enums are impossible to get wrong whereas bools plus a mixture are easy to be misunderstood. What we're asking for here is in the coding style guidelines #10 - http://www.webkit.org/coding/coding-style.html
> whereas bools plus a mixture are easy to be misunderstood. I meant "a mixture of bools plus comments are easy to be misunderstood"
Created attachment 176425 [details] Patch
(In reply to comment #20) > (From update of attachment 176348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176348&action=review > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:62 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > > + pluginProcess->getPluginProcessConnection(reply); > > +} > > + > > +void PluginProcessManager::getSnapshotPluginProcessConnection(const PluginInfoStore& pluginInfoStore, const String& pluginPath, PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply> reply) > > +{ > > + ASSERT(!pluginPath.isNull()); > > + > > + PluginModuleInfo plugin = pluginInfoStore.infoForPluginWithPath(pluginPath); > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, true); > > As Jon previously requested, could you please used named enums here instead of bool flags? > > I had to lookup what this bool meant to see what the code was doing. If you'd named enums I wouldn't have had to look it up... and that's one reason why we used them! :) > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:76 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:82 > > + PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin, false); > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:86 > > +PluginProcessProxy* PluginProcessManager::pluginProcessWithPath(const String& pluginPath, bool isSnapshotProcess) > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:97 > > +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(const PluginModuleInfo& plugin, bool isSnapshotProcess) > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:102 > > + RefPtr<PluginProcessProxy> pluginProcess = PluginProcessProxy::create(this, plugin, isSnapshotProcess); > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:68 > > + PluginProcessProxy* getOrCreatePluginProcess(const PluginModuleInfo&, bool isSnapshotProcess); > > + PluginProcessProxy* pluginProcessWithPath(const String& pluginPath, bool isSnapshotProcess); > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:55 > > + return adoptRef(new PluginProcessProxy(PluginProcessManager, pluginInfo, isSnapshotProcess)); > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:58 > > +PluginProcessProxy::PluginProcessProxy(PluginProcessManager* PluginProcessManager, const PluginModuleInfo& pluginInfo, bool isSnapshotProcess) > > Same here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:67 > > + , m_isSnapshotProcess(isSnapshotProcess) > > The member variable itself could be a bool, except then... > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:177 > > + contexts[i]->sendToAllProcesses(Messages::WebProcess::PluginProcessCrashed(m_pluginInfo.path, m_isSnapshotProcess)); > > You'd have to convert it back to an enum here. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:208 > > + parameters.shouldMuteAudio = m_isSnapshotProcess; > > The parameter member could be an enum also, and we have encodeEnum and decodeEnum in CoreIPC for that reason. > > > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.h:65 > > + static PassRefPtr<PluginProcessProxy> create(PluginProcessManager*, const PluginModuleInfo&, bool isSnapshotProcess); > > At this point I'll stop commenting on how it should be an enum. Thanks for the detailed feedback. Added a PluginProcess::Type enum and it definitely improved the code overall. This lead to quite a few other changes, so please review the patch anew.
Comment on attachment 176425 [details] Patch Attachment 176425 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15023339
I won't be able to review for awhile. I know Jon is familiar with the patch and can hopefully easily help guide someone else to review.
Comment on attachment 176425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176425&action=review > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.h:65 > + HashMap<std::pair<String, unsigned>, RefPtr<CoreIPC::Connection> > m_pathsAndConnections; Shouldn't the second parameter be defined as PluginProcess::Type? > Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:72 > + , m_displayState(displayState) It's a layering violation to store the display state. You should use the PluginProcess::Type enum instead. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:538 > + return PluginProxy::create(pluginPath, pluginElement->displayState()); It's not correct to send the display state over to the proxy. The plugin should not know anything about the display state since its existence is fully controlled by the PluginView widget. The logic for determining which process to use should be done here instead.
Created attachment 176661 [details] Patch
Attachment 176661 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/PluginProcess/PluginProcess.h:127: More than one command on the same line [whitespace/newline] [4] Source/WebKit2/PluginProcess/PluginProcess.h:128: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #27) > (From update of attachment 176425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176425&action=review > > > Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.h:65 > > + HashMap<std::pair<String, unsigned>, RefPtr<CoreIPC::Connection> > m_pathsAndConnections; > > Shouldn't the second parameter be defined as PluginProcess::Type? I backed out of that when the compiler threw a crap ton of errors yesterday. But, I am so glad you questioned me on it, because on looking deeper I found that the default HashTraits for "unsigned" don't allow 0 to be stored, since they treat it as an empty value. I fixed this by starting the enum values at 1. Also added the necessary DefaultHash and IsInteger templates to fix compiler errors. > > > Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:72 > > + , m_displayState(displayState) > > It's a layering violation to store the display state. You should use the PluginProcess::Type enum instead. > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:538 > > + return PluginProxy::create(pluginPath, pluginElement->displayState()); > > It's not correct to send the display state over to the proxy. The plugin should not know anything about the display state since its existence is fully controlled by the PluginView widget. The logic for determining which process to use should be done here instead. Yeah, you are right. Fixed by replacing with PluginProcess::Type.
(In reply to comment #29) > Attachment 176661 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > Source/WebKit2/PluginProcess/PluginProcess.h:127: More than one command on the same line [whitespace/newline] [4] > Source/WebKit2/PluginProcess/PluginProcess.h:128: More than one command on the same line [whitespace/newline] [4] > Total errors found: 2 in 23 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. These are false positives. I could avoid these by splitting the simple templates into multiple lines, but WTF itself uses a single line to define these simple templates. So, I wish to follow the same style for these.
Comment on attachment 176661 [details] Patch r=me
(In reply to comment #31) > (In reply to comment #29) > > Attachment 176661 [details] [details] did not pass style-queue: > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > > Source/WebKit2/PluginProcess/PluginProcess.h:127: More than one command on the same line [whitespace/newline] [4] > > Source/WebKit2/PluginProcess/PluginProcess.h:128: More than one command on the same line [whitespace/newline] [4] > > Total errors found: 2 in 23 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > These are false positives. I could avoid these by splitting the simple templates into multiple lines, but WTF itself uses a single line to define these simple templates. So, I wish to follow the same style for these. Filed https://bugs.webkit.org/show_bug.cgi?id=103607, on check-webkit-style.
Comment on attachment 176661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176661&action=review > Source/WebKit2/WebProcess/WebProcess.messages.in:72 > + PluginProcessCrashed(String pluginProcess, unsigned processType) DispatchOnConnectionQueue This should be an explicitly sized integer type, uint32_t.
Created attachment 176867 [details] Patch
(In reply to comment #34) > (From update of attachment 176661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176661&action=review > > > Source/WebKit2/WebProcess/WebProcess.messages.in:72 > > + PluginProcessCrashed(String pluginProcess, unsigned processType) DispatchOnConnectionQueue > > This should be an explicitly sized integer type, uint32_t. Fixed the type of processType parameter for PluginProcessCrashed (WebProcess) and GetPluginProcessConnection (WebProcessProxy) to uint32_t. Also, for consistency changed DefaultHash template specialization of PluginProcess::Type to use DefaultHash<uint32_t>::Hash. No other changes in the latest patch.
Attachment 176867 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/PluginProcess/PluginProcess.h:127: More than one command on the same line [whitespace/newline] [4] Source/WebKit2/PluginProcess/PluginProcess.h:128: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #37) > Attachment 176867 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > Source/WebKit2/PluginProcess/PluginProcess.h:127: More than one command on the same line [whitespace/newline] [4] > Source/WebKit2/PluginProcess/PluginProcess.h:128: More than one command on the same line [whitespace/newline] [4] > Total errors found: 2 in 23 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Same false positives as before.
Comment on attachment 176867 [details] Patch Clearing flags on attachment: 176867 Committed r136193: <http://trac.webkit.org/changeset/136193>
All reviewed patches have been landed. Closing bug.