Bug 23210 - Make it easier for ports to define custom UI for media controls
Summary: Make it easier for ports to define custom UI for media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-09 11:49 PST by Eric Carlson
Modified: 2009-03-02 11:51 PST (History)
0 users

See Also:


Attachments
proposed patch (7.38 KB, patch)
2009-01-09 11:50 PST, Eric Carlson
eric: review+
Details | Formatted Diff | Diff
Change media controls theming to be overrides instead of complete ruleset (15.10 KB, patch)
2009-01-13 05:35 PST, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Updated patch with changelog and #ifdefs in RenderTheme (16.97 KB, patch)
2009-01-13 06:21 PST, Tor Arne Vestbø
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2009-01-09 11:49:05 PST
It should be easier for ports to define their own media controls UI. Specifically we should not require the CSS to be in a single static variable, and we should assume that hit testing a control element's bounding box is sufficient.
Comment 1 Eric Carlson 2009-01-09 11:50:05 PST
Created attachment 26566 [details]
proposed patch
Comment 2 Eric Carlson 2009-01-09 12:05:04 PST
err, I meant to say "we should NOT assume that hit testing a control element's bounding box is
sufficient"
Comment 3 Eric Seidel (no email) 2009-01-09 13:13:05 PST
Comment on attachment 26566 [details]
proposed patch

I would expect this to be either:
const String& or String.  I'm not sure what const String would do for you.

The preferred webkit style is no else after return:
+    if (renderer() && renderer()->style()->hasAppearance())
+        return theme()->hitTestMediaControlPart(renderer(), absPoint);
+    else
+        return false;

No need for "o" here:
+    virtual bool hitTestMediaControlPart(RenderObject* o, const IntPoint& absPoint);

Otherwise it looks fine.  If you're a commiter, go ahead and fix the nits and land, no need to see another patch.
Comment 4 Eric Carlson 2009-01-11 17:11:44 PST
Committed revision 39782
Comment 5 Tor Arne Vestbø 2009-01-13 05:35:35 PST
Created attachment 26665 [details]
Change media controls theming to be overrides instead of complete ruleset
Comment 6 Tor Arne Vestbø 2009-01-13 05:35:48 PST
Re-opening as I have a patch to how this works. Talked to Eric about this on IRC.
Comment 7 Tor Arne Vestbø 2009-01-13 05:36:44 PST
Talked to Eric Carlson that is :) 
Comment 8 Tor Arne Vestbø 2009-01-13 06:21:04 PST
Created attachment 26666 [details]
Updated patch with changelog and #ifdefs in RenderTheme
Comment 9 Simon Hausmann 2009-01-13 06:31:38 PST
Comment on attachment 26666 [details]
Updated patch with changelog and #ifdefs in RenderTheme

Looks good to me!