[webkit-reviews] review denied: [Bug 136197] DOM timer throttling for hidden plugins : [Attachment 237044] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 136197: DOM timer throttling for hidden plugins
https://bugs.webkit.org/show_bug.cgi?id=136197

Attachment 237044: Fix
https://bugs.webkit.org/attachment.cgi?id=237044&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237044&action=review


The PlugIn vs. Plugin spelling thing is starting to annoy me.

review- because this doesn’t yet compile on non-Mac

> Source/WebCore/bindings/js/JSPluginElementFunctions.cpp:90
> +    if (Page* page = pluginElement.document().page()) {
> +	   for (auto executingScheduledAction :
page->executingScheduledActions())
> +	      
executingScheduledAction->scriptDidInteractWithPlugin(pluginElement);
> +    }

I suggest we move this code into a class member function of ScheduledAction
that takes an HTMLPlugInElement. It would be nice to concentrate the knowledge
of the details of this mechanism more thoroughly in ScheduledAction rather than
distributing the code across classes.

This should be auto* or auto& instead of auto so it’s clear to the reader that
there is no copying happening here.

> Source/WebCore/bindings/js/ScheduledAction.cpp:135
> +    if (executionRecord.didInvisibleInaudibleScriptInteractWithPlugin())
> +	   m_shouldThrottle = true;

I’m confused about the rule here. The rule here is that if the script interacts
with an invisible and inaudible plug-in it gets throttled. That seems a little
backwards to me. What if it interacts with both visible plug-ins and invisible
ones? This will cause that sort of script to be throttled. I think we need a
why comment explaining the rule here since the rule doesn’t explain itself.

The rule this code currently implements is a heuristic in multiple ways. It’s
based on fetching the plug-in object for use by script; I’m not entirely clear
what operation that corresponds to.

It’s not that far from a global rule. It seems we could implement almost the
same rule without all the data structures. Just a number that is bumped
whenever we interact with these invisible inaudible plug-ins and then this
function could fetch that number before and after. If it has changed, then set
m_shouldThrottle to true. Really unsure what data structure is needed here.

> Source/WebCore/bindings/js/ScheduledAction.cpp:167
> +    ExecutingScheduledActionRecord* result =
m_page->popExecutingScheduledAction();

I like auto* in places like this.

> Source/WebCore/bindings/js/ScheduledAction.cpp:174
> +    Widget* widget = pluginElement.pluginWidget();
> +    if (widget && !widget->isVisible() && widget->isPluginViewBase() &&
toPluginViewBase(widget)->audioHardwareActivity() ==
AudioHardwareActivityType::IsInactive)

Should this entire predicate be a member of the HTMLPlugInElement class? Maybe
named isInvisibleAndInaudible? It’s a little ugly to have this digging around
in the plug-in widget be here rather than in HTMLPlugInElement itself. But on
the other hand, it’s also maybe nice not to have to touch HTMLPlugInElement.

Perhaps the PluginViewBase function should be a new isInaudible function rather
than exposing the existing audioHardwareActivity function. Currently, the
audioHardwareActivity function for plug-ins is entirely a concept local to
WebKit2; it’s a little awkward to have it poke out into PluginViewBase like
this, but I can’t think of anything better.

> Source/WebCore/bindings/js/ScheduledAction.cpp:175
> +	   m_invisibleInaudibleScriptDidInteractWithPlugin = true;

Is it the script or the plug-in that is invisible and inaudible?

> Source/WebCore/bindings/js/ScheduledAction.h:78
> +    class ExecutingScheduledActionRecord
> +    {

Not sure the name “record” is quite the name for this programming pattern.

Our coding style puts the brace on the same line as the class name.

Normally for this we would use WTF_NONCOPYABLE since it’s not intrinsically not
copyable (no references) but we wouldn’t correctly handle copying it. If we
change this to hold a Ref<MainFrame> instead of a Page*, that won’t be
necessary.

If you take my suggestion about concentrating the code more in ScheduleAction,
we should be able to keep this class outside of the header entirely.

> Source/WebCore/bindings/js/ScheduledAction.h:87
> +	   Page* m_page;

What guarantees that this pointer won’t outlive the Page? I don’t think
anything does! We could use MainFrame instead; that’s something we can ref().

> Source/WebCore/page/DOMTimer.cpp:49
> +static const int minIntervalForThrottledScheduledAction = 1000;

Need a “why” comment somewhere about why this is an appropriate value.

> Source/WebCore/page/Page.h:110
> +typedef Vector<ExecutingScheduledActionRecord*, 2>
ExecutingScheduledActionStack;

I don’t think we should have a typedef for this. We should not need to utter
the type more than once.

> Source/WebCore/page/Page.h:437
> +    void pushExecutingScheduledAction(ExecutingScheduledActionRecord*
record)

How about we use a single global Vector for this in ScheduledAction? It seems a
shame to have to put these things into Page.h. I think we could make
ScheduledAction do this without involving the page. One downside is that we’d
have to iterate over all of these comparing page pointers (or probably main
frame pointers if you take my advice above), but I am not sure this is a
problem. Being O(all scripts executing) does not seem significantly worse than
O(scripts executing in this page).

> Source/WebCore/page/Page.h:447
> +	   ASSERT(m_executingScheduledActions.size());
> +	   ExecutingScheduledActionRecord* result =
m_executingScheduledActions.last();
> +	   m_executingScheduledActions.removeLast();
> +	   return result;

Vector already has all of this code, including the assert, in fact it’s a
RELEASE_ASSERT. We should just write:

    return m_executingScheduledActions.takeLast();

> Source/WebCore/page/Page.h:450
> +    const ExecutingScheduledActionStack executingScheduledActions() const {
return m_executingScheduledActions; }

The type here should be const auto&; we don’t want to copy the stack when
getting it. Or do we? If we do, I think we should name the function something
different.

> Source/WebCore/page/Page.h:612
> +    ExecutingScheduledActionStack m_executingScheduledActions;

Could consider a singly linked list instead of a vector, since we never iterate
the vector without also visiting each of the items in it.

> Source/WebCore/plugins/PluginViewBase.h:78
> +    virtual AudioHardwareActivityType audioHardwareActivity() const { return
AudioHardwareActivityType::Unknown; }

The override of this in PluginView.h should get a the virtual keyword and
override keyword on it. I see how to do this for the WebKit2 version, but not
for the version in WebCore itself. It seems, in fact, that we would want this
be pure virtual. Unless perhaps this is simply “only implemented for WebKit2 at
this time”?

I suggest removing the includes of AudioHardwareListener.h elsewhere that are
no longer needed now that we have added this.