Using GURL in place of std::string for destination_url

Using GURL in place of std::string for destination_url

Currently destination_url uses GURL type only for IPC.
This patch makes all the layers use GURL.

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

[amogh.bihani, 2014-05-20 13:15:28]

PTAL.

[xhwang, 2014-05-21 16:59:00]

Thank you for taking this! Generally looking pretty good. Just a few comments.

https://codereview.chromium.org/297703002/diff/10001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/297703002/diff/10001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:399: routing_id(),
cdm_id, session_id, message, GURL::EmptyGURL()));
You could still use a "verified_gurl" so that you only need to have one place to
call Send(), which IMHO is a bit cleaner...

https://codereview.chromium.org/297703002/diff/10001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/297703002/diff/10001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:613: session_id, message,
GURL(default_url_string->value()));
hmm, the |default_url_string| may not be a valid URL. Maybe we should do the
same check as we do in BrowserMediaPlayerManager, i.e. if

!destination_url.is_valid() && !destination_url.is_empty()

we spit a WARNING and then use an empty GURL.

+ddorwin to confirm this.

https://codereview.chromium.org/297703002/diff/10001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/297703002/diff/10001/media/base/android/media...
media/base/android/media_drm_bridge.cc:436: GURL destination_gurl =
GURL(ConvertJavaStringToUTF8(env, j_destination_url));
Can you move the GURL verification in
BrowserMediaPlayerManager::OnSessionMessage() here? It seems better to do the
checking when we convert it from string to GURL.

[ddorwin, 2014-05-21 17:51:56]

https://codereview.chromium.org/297703002/diff/10001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/297703002/diff/10001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:613: session_id, message,
GURL(default_url_string->value()));
On 2014/05/21 16:59:01, xhwang wrote:
> hmm, the |default_url_string| may not be a valid URL. Maybe we should do the
> same check as we do in BrowserMediaPlayerManager, i.e. if
> 
> !destination_url.is_valid() && !destination_url.is_empty()

Link:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> 
> we spit a WARNING and then use an empty GURL.
> 
> +ddorwin to confirm this.

Doing the same thing here seems fine and is consistent. The other case is more
important because we're passing it across an IPC. Note that we still need to
perform the same checks closer to Blink so that we can cause the event attribute
to be null (and in case another path somehow missed this check).

"Implementations may validate and/or normalize the URL, which could result in a
value that is different from the original or null."
 -
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...

[amogh.bihani, 2014-05-22 11:59:49]

Thanks for the review :)
I have made the changes.

https://codereview.chromium.org/297703002/diff/10001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/297703002/diff/10001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:399: routing_id(),
cdm_id, session_id, message, GURL::EmptyGURL()));
On 2014/05/21 16:59:01, xhwang wrote:
> You could still use a "verified_gurl" so that you only need to have one place
to
> call Send(), which IMHO is a bit cleaner...

I have made the change. I did this as I was thinking about saving some memory by
not introducing a varaible.

[xhwang, 2014-05-22 18:44:52]

lgtm % comments. Thanks for fixing this!

https://codereview.chromium.org/297703002/diff/30001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/297703002/diff/30001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:396: if
(!verified_gurl.is_valid() && !verified_gurl.is_empty()) {
This seems redundant on Android. But I think keeping it here is actually a good
idea because this will also be used by other platforms in the future. Since this
is the browser process, we need to be careful in handling error cases.

No changes required here :)

https://codereview.chromium.org/297703002/diff/30001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/297703002/diff/30001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:612: GURL verified_gurl =
GURL(default_url_string->value());
I am surprised to see that we still have "default URL" used in various places.
We should replace all of them with "destination URL". Could you please help put
a TODO here?

[amogh.bihani, 2014-05-23 04:12:26]

+r: avi@ for content/renderer/pepper/.

PTAL.

[Avi (use Gerrit), 2014-05-23 04:33:12]

content/renderer/pepper/ LGTM

[amogh.bihani, 2014-05-23 04:36:29]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-23 04:37:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/70001

[amogh.bihani, 2014-05-23 05:58:40]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-23 06:00:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/90001

[commit-bot: I haz the power, 2014-05-23 09:30:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 10:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 10:12:14]

Try jobs failed on following builders:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)

[amogh.bihani, 2014-05-26 12:29:09]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-26 12:30:16]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/130001

[commit-bot: I haz the power, 2014-05-26 15:13:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-26 16:10:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-26 16:10:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[amogh.bihani, 2014-05-27 06:51:18]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-27 06:51:36]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/150001

[amogh.bihani, 2014-05-27 09:28:21]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-05-27 09:58:17]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-27 09:58:34]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/170001

[amogh.bihani, 2014-05-27 10:09:39]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-05-27 10:11:14]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-27 10:11:38]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/190001

[amogh.bihani, 2014-05-27 11:07:07]

The CQ bit was unchecked by amogh.bihani@samsung.com

[amogh.bihani, 2014-05-27 11:13:16]

I have added url_lib dependency in media_cdm_adapter.gyp for GURL, but still I
am getting Linking error for windows/mac_chromium_compile_dbg in ClearkeyCdm

Undefined symbols for architecture i386:
  "GURL::spec() const", 

Can you please help me figure out what am I missing here?
spec() is not in any #ifdef, so how is it platform dependent. Linux works fine
but windows and mac don't.

[xhwang, 2014-05-27 16:33:44]

https://codereview.chromium.org/297703002/diff/190001/media/media_cdm_adapter...
File media/media_cdm_adapter.gyp (right):

https://codereview.chromium.org/297703002/diff/190001/media/media_cdm_adapter...
media/media_cdm_adapter.gyp:22: '../url/url.gyp:url_lib',
Can you try to add this dependency on "clearkeycdm" target at:

https://code.google.com/p/chromium/codesearch#chromium/src/media/media_cdm.gy...

[ddorwin, 2014-05-27 17:00:13]

https://codereview.chromium.org/297703002/diff/190001/media/media_cdm_adapter...
File media/media_cdm_adapter.gyp (right):

https://codereview.chromium.org/297703002/diff/190001/media/media_cdm_adapter...
media/media_cdm_adapter.gyp:22: '../url/url.gyp:url_lib',
On 2014/05/27 16:33:45, xhwang wrote:
> Can you try to add this dependency on "clearkeycdm" target at:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/media_cdm.gy...

And it shouldn't be here.

The issue is that clearkeycdm reuses aes_decryptor, which now hwas the GURL
dependency. However, this adapter is independent of that code (and thus
media_keys.h and GURL).

[amogh.bihani, 2014-05-28 06:38:32]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-05-28 06:39:20]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/297703002/230001

[commit-bot: I haz the power, 2014-05-28 13:18:20]

Change committed as 273197