[webkit-reviews] review requested: [Bug 44013] HTMLMediaElement should delay document load event : [Attachment 64894] updated patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
(In reply to comment #7)
> (From update of attachment 64466 [details])
> > +	 , 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".
> 
I ended up using m_loadEventDelayCount,

> > +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.
> 
I changed the method names to incrementLoadEventDelayCount and
decrementLoadEventDelayCount.

> > +	 // 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.
> 
I tried this, but the DocumentLoader is sometimes NULL when the element needs
to change the delay count (depending on the timing of the media engine state
changes), so I ended up leaving it on Document for now.