[webkit-reviews] review denied: [Bug 208322] Improve some media code : [Attachment 392068] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied  review:
Bug 208322: Improve some media code
https://bugs.webkit.org/show_bug.cgi?id=208322

Attachment 392068: Patch

https://bugs.webkit.org/attachment.cgi?id=392068&action=review




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 392068
  --> https://bugs.webkit.org/attachment.cgi?id=392068
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392068&action=review

>>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>>>> +void MediaControlsHost::ref()
>>> 
>>> The alternative to having MediaControlsHost having its lifetime tied
completely to the HTMLMediaElement and forwarding ref/deref calls would be to
use a WeakPtr and then add null checks to every function. I think this is a
better solution, but I suppose there is some risk of a reference cycle if I
don’t understand the ownership relationships correctly. Like what can hold a
reference to the media controls host object and is there a chance of a cycle.
>> 
>> HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript
that implements controls in the shadow DOM. The controls JS puts the
MediaControlsHost into a global variable. 
>> 
>> HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so
doesn't this create a retain cycle?
> 
> Sounds like it probably does create a retain cycle. I need to find some other
solution, because the old code has a potentially dangling pointer that I can’t
just leave as-is. I suppose I could use a WeakPtr and add a null check to every
use of m_mediaElement. Any other ideas about how to handle this back-pointer?

Sounds like this needs a WeakPtr and a bunch of null checks then.

>>> Source/WebCore/html/track/WebVTTParser.cpp:342
>>> +		     m_regionList.remove(i);
>> 
>> This is a O(n^2) algorithm that mutates the collection while iterating, so
it would miss regions right after the removed region.  removeAllMatching would
be faster and more correct.
> 
> Agreed that it’s O(n^2), but note that this breaks out of the loop when it
finds a region that matches; so there are no "missed regions" because this
removes only the first. All I did here was get rid of the second unnecessary
loop over the vector inside the removeFirst function. While it’s OK for someone
to change this to removeAllMatching I don’t think that change is necessary as
part of this patch. I also don’t think removeAllMatching would be significantly
faster.

Ah, I didn't read all the way to the break. This is fine.