[webkit-reviews] review denied: [Bug 44013] HTMLMediaElement should delay document load event : [Attachment 64466] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 44013: HTMLMediaElement should delay document load event
https://bugs.webkit.org/show_bug.cgi?id=44013

Attachment 64466: proposed patch
https://bugs.webkit.org/attachment.cgi?id=64466&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    , m_elementsBlockingLoad(0)

The name for this data member is not great. It contains a count, not elements,
so it's not good that it's named "elements".

> +void Document::setDelayLoadEvent(bool delay)

I don't like the name for this. It makes it sound like it sets something named
"delay load event". But it doesn't. It actually bumps a count. And it's
important that it's clear from the caller's point of view that each call to
delay is balanced by a call to cancel the delay. I suggest having two functions
instead of one, maybe one with the names add/remove or register/unregister or
added/removed.

> +    if (!frame() || !frame()->loader())
> +	   return;

There is no need to check frame()->loader() for 0. Every frame always has a
loader.

I think you also need a comment that says that a document is either always in a
frame or never, to make it clear this does not dynamically change.

> +    // Used to allow element that loads data without using a FrameLoader to
delay the 'load' event.
> +    void setDelayLoadEvent(bool);
> +    bool isDelayingLoadEvent() const { return m_elementsBlockingLoad; }

It seems unfortunate to have to add a new mechanism for this that is unrelated
to the other loading, and also a bit far away. Perhaps this counter can go into
a loading-related object instead of the document. I'm thinking DocumentLoader
would be a good place for it. That way you wouldn't have to touch Document.h at
all.

> +    if (m_delayingTheLoadEvent)
> +	   setDelayLoadEvent(false);

There's no need for the if statement here. The setDelayLoadEvent already does
that check. Same issue in the HTMLMediaElement destructor.

> +    // The spec doesn't say to block the load event until we actually run
the asynchronous section
> +    // algorithm, but do it now because we won't start that until after the
timer fires and the 
> +    // event may have already fired by then.
> +    setDelayLoadEvent(true);
> +
>  }

You should remove that extra blank line here.

> +void HTMLMediaElement::setDelayLoadEvent(bool delay)

I think this should be named setShouldDelayLoadEvent.

> +    if (m_delayingTheLoadEvent == delay || !delay &&
!m_delayingTheLoadEvent)
> +	   return;

There's no need for the second part of this expression. It's already covered by
the first half.

> +    m_delayingTheLoadEvent = delay;

I think this should be named m_shouldDelayLoadEvent.

This looks good and is probably nearly OK to land already, but I had enough
minor nitpick comments that I think I'll say review- for now.