Bug 130694 – Track plugin in visibility in the plugin process.

• Component: Plug-ins
• Status: RESOLVED FIXED

[Stephanie Lewis]

Created attachment 227682 [details]
patch

Track plugin in visibility in the plugin process.  This allows the Plugin Process to handle its own app sleep assertions and sleep more often.
<rdar://problem/16061257> PluginProcess should AppNap when no plugins on active tab.

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Stephanie Lewis]

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

[Stephanie Lewis]

Can't reproduce the layout test failures.

[Anders Carlsson]

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.

[Stephanie Lewis]

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.

[WebKit Commit Bot]

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.

[Anders Carlsson]

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.

[Stephanie Lewis]

(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.

[Stephanie Lewis]

Landed http://trac.webkit.org/changeset/166441