add url as additional argument to createMediaPlayer.

add url as additional argument to createMediaPlayer.
Since this touches API in WebKit, it takes 3 steps:
1. add a new function createMediaPlayer with url as additional argument (this patch);
2. corresponding change in WebKit;
3. remove the old createMediaPlayer (without url).

This is the second patch to allow render_view_impl to create different WebKit::WebMediaPlayer based on URL.
The new approach (https://docs.google.com/a/chromium.org/document/d/1lH9gzjUzA3L1pEPHWVOae6KpzB4IlIDOM-4lLbwGMgY/edit)
will have a new WebKit::WebMediaPlayer when URL is media stream (refer to prototype patch http://codereview.chromium.org/10382048/,
render_view_impl.cc shows how a different WebKit::WebMediaPlayer is created based on URL).

BUG=142988
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151922

[wjia(left Chromium), 2012-06-08 23:07:10]

Another trivial patch for url-based WebMediaPlayer.

[Ami GONE FROM CHROMIUM, 2012-06-09 02:45:14]

It would be a better demonstration that this is the right API if you filled in
the actual implementation of the new overloads.  Any reason not to do that in
this CL?

http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.h:511: const WebKit::WebURL& url);
Make this be the first param?

[wjia(left Chromium), 2012-06-11 21:30:57]

On 2012/06/09 02:45:14, Ami Fischman wrote:
> It would be a better demonstration that this is the right API if you filled in
> the actual implementation of the new overloads.  Any reason not to do that in
> this CL?
> 

There is a prototype patch http://codereview.chromium.org/10382048/ which shows
how the new overloads work. I am trying to land them in small pieces, especially
when it comes to WebKit API change.

>
http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
> File content/renderer/render_view_impl.h (right):
> 
>
http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
> content/renderer/render_view_impl.h:511: const WebKit::WebURL& url);
> Make this be the first param?

[wjia(left Chromium), 2012-06-11 21:31:07]

PTAL.

http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10537091/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.h:511: const WebKit::WebURL& url);
On 2012/06/09 02:45:14, Ami Fischman wrote:
> Make this be the first param?

Done.

[Ami GONE FROM CHROMIUM, 2012-06-12 03:46:48]

On 2012/06/11 21:30:57, wjia wrote:
> On 2012/06/09 02:45:14, Ami Fischman wrote:
> > It would be a better demonstration that this is the right API if you filled
in
> > the actual implementation of the new overloads.  Any reason not to do that
in
> > this CL?
> > 
> 
> There is a prototype patch http://codereview.chromium.org/10382048/ which
shows
> how the new overloads work. I am trying to land them in small pieces,
especially
> when it comes to WebKit API change.

Is there a reason why the new param is necessary instead of using
client->sourceURL() ?

[Ami GONE FROM CHROMIUM, 2012-06-12 16:17:36]

On 2012/06/12 03:46:48, Ami Fischman wrote:
> On 2012/06/11 21:30:57, wjia wrote:
> > On 2012/06/09 02:45:14, Ami Fischman wrote:
> > > It would be a better demonstration that this is the right API if you
filled
> in
> > > the actual implementation of the new overloads.  Any reason not to do that
> in
> > > this CL?
> > > 
> > 
> > There is a prototype patch http://codereview.chromium.org/10382048/ which
> shows
> > how the new overloads work. I am trying to land them in small pieces,
> especially
> > when it comes to WebKit API change.
> 
> Is there a reason why the new param is necessary instead of using
> client->sourceURL() ?

Oops - that's the MediaSource magical URL, not the URL of the media.

But that points out the flaw of this approach - the URL of the media resource
isn't known at the point createMediaPlayer() is called - media URLs are part of
the load algorithm and so you only at load() time do you know which URL is
actually being attempted.

I don't understand how you made this work; can you point me at the WebKit side
of your prototype patch?

[Ami GONE FROM CHROMIUM, 2012-06-21 17:40:00]

wjia: ping?
Also, you might be interested in https://bugs.webkit.org/show_bug.cgi?id=89514
which seems like the gstreamer folks solving a similar problem, in webkit.

[Ami GONE FROM CHROMIUM, 2012-07-05 23:21:57]

On 2012/06/21 17:40:00, Ami Fischman wrote:
> wjia: ping?
> Also, you might be interested in https://bugs.webkit.org/show_bug.cgi?id=89514
> which seems like the gstreamer folks solving a similar problem, in webkit.

Time to close this issue?

[wjia(left Chromium), 2012-07-06 05:22:00]

On 2012/07/05 23:21:57, Ami Fischman wrote:
> On 2012/06/21 17:40:00, Ami Fischman wrote:
> > wjia: ping?
> > Also, you might be interested in
https://bugs.webkit.org/show_bug.cgi?id=89514
> > which seems like the gstreamer folks solving a similar problem, in webkit.
> 
> Time to close this issue?

Sorry for the late reply. Just came back from vacation and trying to clean up
email backlog. This patch is still needed.

I think https://bugs.webkit.org/show_bug.cgi?id=89514 is targeting a different
case. In a debugging session of media stream,
WebMediaPlayerClientImpl::supportsType is not called. It seems that chrome is
using a different code path.

The goal is to create different WebMediaPlayer based on URL. When
WebMediaPlayerClientImpl::load() is called (in
WebKit/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp), it will call
chrome's createMediaPlayer(), then calls WebMediaPlayerImpl::load() immediately.
So when RenderViewImpl::createMediaPlayer() is called, the URL is known.

[Ami GONE FROM CHROMIUM, 2012-07-06 15:45:28]

On 2012/07/06 05:22:00, wjia wrote:
> On 2012/07/05 23:21:57, Ami Fischman wrote:
> > On 2012/06/21 17:40:00, Ami Fischman wrote:
> > > wjia: ping?
> > > Also, you might be interested in
> https://bugs.webkit.org/show_bug.cgi?id=89514
> > > which seems like the gstreamer folks solving a similar problem, in webkit.
> > 
> > Time to close this issue?
> 
> I think https://bugs.webkit.org/show_bug.cgi?id=89514 is targeting a different
> case. In a debugging session of media stream,
> WebMediaPlayerClientImpl::supportsType is not called. It seems that chrome is
> using a different code path.

Yes; my point was that chrome (or chromium code inside WebKit) *could* use a
similar approach (note that that webkit CL just ignores the new param for all
platforms except gstreamer; I'm saying you could hook into that new param in the
chromium codepaths and stop ignoring it, and add calls to supportsType to use
it).

I guess the thing to do is to upload the webkit-side of this CL and get an r+
from a webkit reviewer that your API change is acceptable (and that there isn't
a snazzier way to do things webkit-side).

[scherkus (not reviewing), 2012-08-15 22:53:57]

lgtm w/ nits

https://chromiumcodereview.appspot.com/10537091/diff/36003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/content/renderer/r...
content/renderer/render_view_impl.cc:2365: // TODO(wjia): remove the version
without url when WebKit change is done.
can you annotate which webkit bug?

http://webk.it/XXXXX

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/support/web...
File webkit/support/webkit_support.h (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/support/web...
webkit/support/webkit_support.h:78: // TODO(wjia): remove this function after
WebKit patch is landed.
ditto

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/tools/test_...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/tools/test_...
webkit/tools/test_shell/test_webview_delegate.cc:638: // TODO(wjia): remove the
version without url when WebKit change is done.
ditto

[wjia(left Chromium), 2012-08-15 23:14:35]

Thanks, Andrew!

adding Brett for owner's stamp.

https://chromiumcodereview.appspot.com/10537091/diff/36003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/content/renderer/r...
content/renderer/render_view_impl.cc:2365: // TODO(wjia): remove the version
without url when WebKit change is done.
On 2012/08/15 22:53:57, scherkus wrote:
> can you annotate which webkit bug?
> 
> http://webk.it/XXXXX

Done.

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/support/web...
File webkit/support/webkit_support.h (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/support/web...
webkit/support/webkit_support.h:78: // TODO(wjia): remove this function after
WebKit patch is landed.
On 2012/08/15 22:53:57, scherkus wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/tools/test_...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

https://chromiumcodereview.appspot.com/10537091/diff/36003/webkit/tools/test_...
webkit/tools/test_shell/test_webview_delegate.cc:638: // TODO(wjia): remove the
version without url when WebKit change is done.
On 2012/08/15 22:53:57, scherkus wrote:
> ditto

Done.

[jamesr, 2012-08-16 18:19:03]

lgtm