[webkit-reviews] review denied: [Bug 97800] Allow ports to override text track rendering style : [Attachment 167418] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 97800: Allow ports to override text track rendering style
https://bugs.webkit.org/show_bug.cgi?id=97800

Attachment 167418: Patch
https://bugs.webkit.org/attachment.cgi?id=167418&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167418&action=review


> Source/WebKit/mac/ChangeLog:24
> +2012-10-05  Eric Carlson  <eric.carlson at apple.com>
> +
> +	   Allow ports to override text track rendering style
> +	   https://bugs.webkit.org/show_bug.cgi?id=97800
> +	   <rdar://problem/12044964>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).
> +
> +	   * WebCoreSupport/WebSystemInterface.mm:
> +	   (InitWebCoreSystemInterface): Initialize new WKSI function pointers.

> +
> +2012-09-27  Eric Carlson  <eric.carlson at apple.com>
> +
> +	   Need a short description (OOPS!).
> +	   Need the bug URL (OOPS!).
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).
> +
> +	   * WebCoreSupport/WebSystemInterface.mm:
> +	   (InitWebCoreSystemInterface):

Double Changelog!

> Source/WebCore/html/HTMLMediaElement.cpp:557
> +#if ENABLE(VIDEO_TRACK)
> +    if (document()->page())
> +	  
document()->page()->theme()->registerForCaptionPreferencesChangedCallbacks(this
);
> +#endif

Are caption preferences only observable when in the render tree? In other
words, why is attach the right place for this, rather than say,
insertedIntoDocument()?

> Source/WebCore/html/shadow/MediaControlElements.cpp:1315
> +void MediaControlTextTrackContainerElement::userCaptionPreferencesChanged()

Does this change captions for all pages in the page group?  If so, why does
this need to be down in MediaControlTextTrackContainerElement. Should it be up
in PageGroup instead?

> Source/WebCore/html/shadow/MediaControlElements.cpp:1317
> +    DEFINE_STATIC_LOCAL(KURL, captionsStyleSheetURL, (ParsedURLString,
"user-captions-override:01F6AF12-C3B0-4F70-AF5E-A3E00234DC23"));

I think a comment explaining the weird URL would be useful.