[Webkit-unassigned] [Bug 53020] [Meta] Convert HTMLMediaElement to use the new shadow DOM

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53020





--- Comment #18 from Dimitri Glazkov (Google) <dglazkov at chromium.org>  2011-04-08 13:50:18 PST ---
(From update of attachment 88736)
View in context: https://bugs.webkit.org/attachment.cgi?id=88736&action=review

>> Source/WebCore/ChangeLog:16
>> +        are now using a set of higher fidelity set of hooks that notify MediaControls
> 
> Nit: you probably only want one "set" here

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:138
>> +    // FIXME: ONLY APPEND WHEN SUPPORTS SEEKBACK
> 
> Bug number (http://webkit.org/b/57163 maybe)?

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:143
>> +    // FIXME: ONLY APPEND WHEN SUPPORTS SEEKFORWARD
> 
> Ditto.

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:158
>> +    panel->appendChild(fullScreenButton.release(), ec, true);
> 
> Not all movies/themes support fullscreen, so it is probably worth a "FIXME:" and a bug number here too.

Done.

>> Source/WebCore/html/shadow/MediaControls.cpp:264
>> +    // FIXME: Make more efficient.
> 
> Might as well write up a bug now and include the number here.

Done. Bug 58157 filed.

>> Source/WebCore/html/shadow/MediaControls.cpp:366

> 
> SNIP

That is a good idea. Hid the fullscreen button.

>> Source/WebCore/rendering/MediaControlElements.cpp:87
>> +    // FIXME: Make more efficient.
> 
> Bug number?

Done.

>> Source/WebCore/rendering/MediaControlElements.cpp:94
>> +    // FIXME: Make more efficient.
> 
> Ditto.

Done.

>> Source/WebCore/rendering/MediaControlElements.cpp:305
>> +    style()->setProperty(displayString(), "none", ec);
> 
> Is there any reason to not use a static String for "none"?

Fixed.

>> Source/WebCore/rendering/MediaControlElements.cpp:633
>>  }
> 
> It’s not a good idea to call functions like this from inside the constructor when the object hasn’t been adopted by its first owner. It can lead to RefCounted assertions if anything ref/derefs. We used to do this throughout the media controls classes, but Darin fixed them a while ago. 
> 
> It looks like you already do this in the create() function anyway.

DOH! I totally thought I fixed it. Fixed now.

>> Source/WebCore/rendering/MediaControlElements.cpp:664
>> +        // FIXME: This is fired 3 times on every click. We should not be doing that.
> 
> Bug number?

Done.

>> Source/WebCore/rendering/MediaControlElements.cpp:701
>>  }
> 
> Shouldn't do this in the constructor.

Sorry. Removed.

>> Source/WebCore/rendering/MediaControlElements.h:79
>> +    // FIXME: These should be part of some InlineStylableElement.
> 
> Bug number?

FIXME removed, this is obsolete now.

>> Source/WebCore/rendering/MediaControlElements.h:157
>> +    // FIXME: These should be part of some InlineStylableElement.
> 
> Ditto.

Done.

>> Source/WebCore/rendering/MediaControlElements.h:229
>> +    // FIXME: PERHAPS MAKE EXPLICIT PLAY/PAUSE CALLS.
> 
> Ditto.

This was an old idea, comment obsolete now.

>> Source/WebCore/rendering/RenderMedia.cpp:54
>>  HTMLMediaElement* RenderMedia::mediaElement() const
> 
> Does this really need to move to RenderVideo? Bug number of so please.

No, not really -- early ideas. Comment obsolete now, removed :)

>> Source/WebCore/rendering/RenderMedia.cpp:91
>>  }
> 
> What does this do?

This is my debugging code! :) Removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.