Bug 84291 – [BlackBerry] MediaPlayerPrivate should not reference HTMLMediaElement directly

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Max Feil]

It has been raised in https://bugs.webkit.org/show_bug.cgi?id=82930 that the BlackBerry version of MediaPlayerPrivate should not be referencing HTMLMediaElement directly. This should be done via MediaPlayer. I am creating this bug to address this concern. Here is was Eric Carlson wrote:

"Particularly unfortunate, IMO, is the code that casts MediaPlayer::mediaPlayerClient() to HTMLMediaElement to make calls on the media element directly. The engine constructor is passed a MediaPlayer* so it has an opaque interface to make calls to WebCore without having to do this. The MediaPlayerClient interface can be extended if you need something from the media element or something it knows about. Adding the necessary plumbing isn't a lot of work, and once it is there other ports can use it too. For example, to allow the AVFoundation based media engine to pass the correct UA string (https://bugs.webkit.org/show_bug.cgi?id=46241) we added mediaPlayerUserAgent to MediaPlayerClient so the engine can get the media element to fetch the UA string from the loader. MediaPlayerBlackBerry apparently needed the same functionality as well, but instead casts to an HTMLMediaElement so it can get a pointer to the frame loader to call userAgent() directly. Bug 46241 would have been much easier to fix if the MediaPlayerClient plumbing had already been there"

[John Griggs]

Created attachment 159679 [details]
Patch

[Build Bot]

Comment on attachment 159679 [details]
Patch

Attachment 159679 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13545528

[John Griggs]

Created attachment 159692 [details]
Patch

[WebKit Review Bot]

Attachment 159692 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/MediaPlayer.h:195:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Griggs]

Created attachment 159698 [details]
Patch

[Eric Carlson]

Comment on attachment 159698 [details]
Patch

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

Thank you for working on this! r- for now because of the relatively minor issues noted, but this is close.

> Source/WebCore/html/HTMLMediaElement.cpp:4398
> +    sprintf(attrString, "%d", width);
> +    setAttribute(widthAttr, attrString);

sprintf is unsafe so you should always use snprintf instead, but in this case you can use String::number :

    setAttribute(widthAttr, String::number(width));

> Source/WebCore/platform/graphics/MediaPlayer.h:195
> +    virtual void mediaPlayerSetSize(int , int) { }

mediaPlayerContentBoxRect returns a LayoutRect, why not have this take a LayoutSize?

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:775
> +    return m_webCorePlayer->mediaPlayerClient()->mediaPlayerOwningDocument()->view();

Accessing Document and FrameView from platform is also a layering violation. I see that you only use it to get to the HostWindow and the ClipRect, so why don't you add accessors for those instead?

[John Griggs]

Created attachment 159946 [details]
Patch

[John Griggs]

Eric - implemented all of your notes except for replacing the int arguments to mediaPlayerSetSize with a LayoutSize - there did not seem to be any point in constructing the object only to access it's individual parts (and have to cast them for type for String::number()) at the other end of the function call...

[John Griggs]

Eric - implemented all of your notes except for replacing the int arguments to mediaPlayerSetSize with a LayoutSize - there did not seem to be any point in constructing the object only to access it's individual parts (and have to cast them for type for String::number()) at the other end of the function call...

[Eric Carlson]

Comment on attachment 159946 [details]
Patch

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

Marking r+, but I would like to see the size methods change noted.

> Source/WebCore/platform/graphics/MediaPlayer.h:196
> +    virtual void mediaPlayerSetSize(int, int) { }

Every spatial "setSize" in WebCore (except one) takes a size class rather than a pair of unit values. Using ints is fine, but you should really pass the value as an IntSize:

    virtual void mediaPlayerSetSize(const IntSize&) { }

[John Griggs]

Comment on attachment 159946 [details]
Patch

Adding commit-queue ?

[Eric Carlson]

(In reply to comment #11)
> (From update of attachment 159946 [details])
> Adding commit-queue ?

Are you planning to address, or at least comment on, my request to change setSize?

[John Griggs]

Sorry - I'm new at this.  I assumed that the review+ meant that that would have to be handled in a separate bug.  If you want to do whatever is necessary to hold this bug, I will update the patch tomorrow.  Otherwise, feel free to open a new bug and assign to me.  Thanks.

[Eric Carlson]

(In reply to comment #13)
> Sorry - I'm new at this.  I assumed that the review+ meant that that would have to be handled in a separate bug.  If you want to do whatever is necessary to hold this bug, I will update the patch tomorrow.  Otherwise, feel free to open a new bug and assign to me.  Thanks.

Because you didn't mark the patch cq? when you put it up for review I assumed that you would have someone commit it manually (if you mark a patch r? and cq? the reviewer will typically change both when reviewing). I marked it r+ with a comment to indicate that I think the rest of the patch is OK modulo the changes noted, assuming that you would make those changes before committing. 

I think it makes sense to fix this one minor issue in this patch. If you update the patch and mark r? and cq? I will change both at the same time.

[John Griggs]

Created attachment 160154 [details]
Patch

[John Griggs]

Eric - Thanks for your patience.  I am still learning the ropes for both WebKit and RIM procedures.

[WebKit Review Bot]

Attachment 160154 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/HTMLMediaElement.h:428:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Griggs]

Created attachment 160157 [details]
Patch - fixed style issue from previous patch

[Eric Carlson]

Comment on attachment 160157 [details]
Patch - fixed style issue from previous patch

Thank you John!

[John Griggs]

My pleasure!  Thank you for your patience and assistance with the proper procedures.

[WebKit Review Bot]

Comment on attachment 160157 [details]
Patch - fixed style issue from previous patch

Clearing flags on attachment: 160157

Committed r126441: <http://trac.webkit.org/changeset/126441>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

Closing bug for patch that landed a long time ago. Thanks John!