Bug 56969 - Move media controls subtree creation into one method.
Summary: Move media controls subtree creation into one method.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 53020
  Show dependency tree / graph
 
Reported: 2011-03-23 15:34 PDT by Dimitri Glazkov (Google)
Modified: 2011-03-24 09:58 PDT (History)
2 users (show)

See Also:

Attachments
Patch (13.26 KB, patch)
2011-03-23 15:37 PDT, Dimitri Glazkov (Google) 		darin: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-03-23 15:34:33 PDT 
Move media controls subtree creation into one method.
Comment 1 Dimitri Glazkov (Google) 2011-03-23 15:37:57 PDT 
Created attachment 86711 [details]
Patch
Comment 2 Darin Adler 2011-03-23 16:18:32 PDT 
Comment on attachment 86711 [details]
Patch

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

> Source/WebCore/rendering/MediaControlElements.cpp:800
> +    setAttribute(precisionAttr, "float");

It’s not a great idea to call functional like this inside the constructor where the object still hasn’t been adopted by its first owner. It can lead to RefCounted assertions if anything ref/derefs. I took these out of the class intentionally a while back, and you are undoing that.

The work should be either in a create function or at the call site rather than in the constructor.

> Source/WebCore/rendering/MediaControlElements.cpp:864
> +    setAttribute(precisionAttr, "float");
> +    setAttribute(maxAttr, "1");
> +    setAttribute(valueAttr, String::number(mediaElement->volume()));

Ditto.
Comment 3 Dimitri Glazkov (Google) 2011-03-23 16:23:33 PDT 
(In reply to comment #2)
> (From update of attachment 86711 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86711&action=review
> 
> > Source/WebCore/rendering/MediaControlElements.cpp:800
> > +    setAttribute(precisionAttr, "float");
> 
> It’s not a great idea to call functional like this inside the constructor where the object still hasn’t been adopted by its first owner. It can lead to RefCounted assertions if anything ref/derefs. I took these out of the class intentionally a while back, and you are undoing that.
> 
> The work should be either in a create function or at the call site rather than in the constructor.
> 
> > Source/WebCore/rendering/MediaControlElements.cpp:864
> > +    setAttribute(precisionAttr, "float");
> > +    setAttribute(maxAttr, "1");
> > +    setAttribute(valueAttr, String::number(mediaElement->volume()));
> 
> Ditto.

Sure, I'll move these to create func.
Comment 4 Dimitri Glazkov (Google) 2011-03-24 09:58:25 PDT 
Committed r81872: <http://trac.webkit.org/changeset/81872>