Summary: | Track plugin in visibility in the plugin process. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephanie Lewis <slewis> | ||||||||
Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, buildbot, commit-queue, rniwa, slewis | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Stephanie Lewis
2014-03-24 12:45:26 PDT
Attachment 227682 [details] did not pass style-queue:
ERROR: Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
ERROR: Source/WebKit2/PluginProcess/WebProcessConnection.cpp:342: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 2 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 227682 [details] patch Attachment 227682 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6113792429654016 New failing tests: media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html Created attachment 227688 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Layout test failures seem unrelated. Unexpected flakiness: text-only failures (2) fast/repaint/obscured-background-no-repaint.html [ Failure Pass ] transitions/3d/interrupted-transition.html [ Failure Pass ] Regressions: Unexpected image-only failures (1) mathml/presentation/tokenElements-background-color.html [ ImageOnlyFailure ] Regressions: Unexpected crashes (1) media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html [ Crash ] The crash is slowness in MathML 746 WebCore::RenderMathMLOperator::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) (in WebCore) + 915 [0x10ac45de3] RenderMathMLOperator.cpp:1587 Can't reproduce the layout test failures. Comment on attachment 227682 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=227682&action=review I don't think this handles the case where a web process crashes. > Source/WebKit2/PluginProcess/PluginProcess.cpp:243 > +void PluginProcess::webProcessesWithVisiblePluginsChanged(bool isVisible) > +{ > + isVisible ? m_visiblePluginsActivity.increment() : m_visiblePluginsActivity.decrement(); > +} I think having two functions, one for "didBecomeHidden" and one for "didBecomeVisible" is more clear. > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:342 > + if (oldState != m_visiblePluginInstanceIDs.isEmpty()) { > + PluginProcess::shared().webProcessesWithVisiblePluginsChanged(isVisible); > + } Single line if bodies shouldn't have braces. Created attachment 227803 [details]
take 2
The PluginProcess does the right thing if the WebProcess crashes. A cleanup function on the WebConnection is called that destroys all the pluginControllers -> who remove themselves from the connection -> which cleans up the visible plugins, and eventually the connection is destroyed altogether.
Attachment 227803 [details] did not pass style-queue:
ERROR: Source/WebKit2/PluginProcess/PluginProcess.cpp:239: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 227803 [details] take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=227803&action=review > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:331 > + bool oldState = m_visiblePluginInstanceIDs.isEmpty(); Can you ASSERT(!m_visiblePluginInstanceIDs.contains(pluginInstanceID))? > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:346 > + if (m_visiblePluginInstanceIDs.contains(pluginInstanceID)) > + m_visiblePluginInstanceIDs.remove(pluginInstanceID); remove is just a no-op if the set doesn't contain the plug-in instance so there's no need to do a .contains check here. Can you assert that m_visiblePluginInstanceIDs.contains(pluginInstanceID) always returns true here? > Source/WebKit2/PluginProcess/WebProcessConnection.h:58 > + void pluginDidBecomeVisible(unsigned pluginInstanceId); > + void pluginDidBecomeHidden(unsigned pluginInstanceId); Capital d in ID. > Source/WebKit2/PluginProcess/mac/PluginControllerProxyMac.mm:109 > + printf("Visibility changed to %d for plugin %llu from %d\n", isVisible, m_pluginInstanceID, getpid()); Please remove this. > Source/WebKit2/PluginProcess/mac/PluginControllerProxyMac.mm:111 > + isVisible ? m_connection->pluginDidBecomeVisible(m_pluginInstanceID) : m_connection->pluginDidBecomeHidden(m_pluginInstanceID); I think an if statement is cleaner than a ternary operator here. (In reply to comment #9) > (From update of attachment 227803 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227803&action=review > > > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:331 > > + bool oldState = m_visiblePluginInstanceIDs.isEmpty(); > > Can you ASSERT(!m_visiblePluginInstanceIDs.contains(pluginInstanceID))? > It's valid to receive a plugin did hide message for a plugin we don't track as visible. We get a plugin visibility changed message shortly after the plugin has been initialized which could be visible or hidden. |