[webkit-reviews] review granted: [Bug 51723] Avoid manual ref/deref in AsyncScriptRunner by using PendingScript : [Attachment 77642] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tony Gentilcore
<tonyg at chromium.org>'s request for review:
Bug 51723: Avoid manual ref/deref in AsyncScriptRunner by using PendingScript
https://bugs.webkit.org/show_bug.cgi?id=51723

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

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

I’m going to say review+ but I do wonder why the PendingScript changes here
were needed at all.

> WebCore/ChangeLog:23
> +	   * dom/PendingScript.cpp:
> +	   (WebCore::PendingScript::PendingScript):
> +	   * dom/PendingScript.h:

No explanation of why you changed this class to no longer inline its
constructors. It doesn’t seem required by the rest of the patch. And you didn’t
touch the assignment operator. I’m a bit puzzled. Why?

> WebCore/dom/AsyncScriptRunner.cpp:48
>      for (size_t i = 0; i < m_scriptsToExecuteSoon.size(); ++i) {
> -	   m_scriptsToExecuteSoon[i].first->element()->deref(); // Balances
ref() in executeScriptSoon().
>	   m_document->decrementLoadEventDelayCount();
>      }

No braces needed now that this is a 1-line for loop body.

> WebCore/dom/AsyncScriptRunner.cpp:61
> +    PendingScript pendingScript;
> +    pendingScript.adoptElement(element);

Should we make a suitable constructor. It seems it would be better to construct
the PendingScript ready to go rather than empty-constructing it and then
setting the two pieces separately.

Also, the function name adoptElement is not good! This function doesn’t “adopt”
an element. In WebKit we call a function “adopt” when it takes ownership of
something in a unusual way, such as when a String takes over storage formerly
owned by a Vector or a smart pointer takes a reference in the function
adoptRef.

> WebCore/dom/PendingScript.h:44
>  class PendingScript : public CachedResourceClient {

The dual nature of the PendingScript class is not great. We treat these as data
objects that we can copy around, but they are also CachedResourceClient objects
that can perform loading. It seems impossible to correctly copy one while its
loading.

> WebCore/dom/PendingScript.h:47
> +    PendingScript(const PendingScript& other);

No need for the argument name “other” here.