Remove dependency on GURL from content/common/gpu

Remove dependency on GURL from content/common/gpu

BUG=586368

[Mark Dittmer, 2016-02-17 19:36:08]

Description was changed from

==========
Remove dependency on GURL from content/common/gpu

BUG=586368
==========

to

==========
Remove dependency on GURL from content/common/gpu

BUG=586368
==========

[Mark Dittmer, 2016-02-17 19:36:08]

markdittmer@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2016-02-18 18:29:28]

https://codereview.chromium.org/1703163002/diff/1/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:864:
GpuDataManagerImpl::GetInstance()->BlockDomainFrom3DAPIs(gurl, guilt);
Oh, I hadn't realized we used the URL to extract the domain. We then need to
make sure the round-trip through a string is non-lossy. Is that the case that
GURL(url.spec()) is always identical to url?

https://codereview.chromium.org/1703163002/diff/1/content/public/common/conte...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1703163002/diff/1/content/public/common/conte...
content/public/common/content_client.h:80: void SetActiveURL(const std::string&
url);
nit: avoid overloads, especially with one method virtual and the other one not.
SetActiveURLFromString?

OTOH, since gpu_command_buffer_stub.cc is not supposed to access the
ContentClient directly (won't exist in mus), maybe the string->URL conversion
should be in the GpuChildThread (will stay in content/), accessed via the
GpuChannelManagerDelegate that Fady introduces in
https://codereview.chromium.org/1711533002 , so you wouldn't need this here.

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_encoder_host.cc:508:
GpuChannelHost::kDefaultStreamPriority, attribs, "",
nit: std::string()

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/ppb...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/ppb...
content/renderer/pepper/ppb_graphics_3d_impl.cc:296:
GpuChannelHost::kDefaultStreamPriority, attribs, "", gpu_preference);
nit: std::string()

https://codereview.chromium.org/1703163002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:1042:
attributes.topDocumentURL.utf8(), limits,
Is this equivalent, string.utf8() vs blink::WebStringToGURL(string).spec()?
WebStringToGURL seems to treat 8-bit strings (possibly Latin-1) and utf-16
strings differently.

[Mark Dittmer, 2016-02-18 21:10:25]

Locally all but GURL/WebString and SetActiveURL overload. Patch forthcoming
(hopefully tomorrow morning EST).

https://codereview.chromium.org/1703163002/diff/1/content/browser/gpu/gpu_pro...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/browser/gpu/gpu_pro...
content/browser/gpu/gpu_process_host.cc:864:
GpuDataManagerImpl::GetInstance()->BlockDomainFrom3DAPIs(gurl, guilt);
On 2016/02/18 18:29:28, piman wrote:
> Oh, I hadn't realized we used the URL to extract the domain. We then need to
> make sure the round-trip through a string is non-lossy. Is that the case that
> GURL(url.spec()) is always identical to url?

If GURL is stored on either end, and spec() is used for extract-to-send, then we
should be fine. spec() checks for an invalid URL, and then returns the
canonicalized URL. We could use possibly_invalid_spec(), but that seems less
safe to me.

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_encoder_host.cc:508:
GpuChannelHost::kDefaultStreamPriority, attribs, "",
On 2016/02/18 18:29:28, piman wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/ppb...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/pepper/ppb...
content/renderer/pepper/ppb_graphics_3d_impl.cc:296:
GpuChannelHost::kDefaultStreamPriority, attribs, "", gpu_preference);
On 2016/02/18 18:29:28, piman wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/1703163002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1703163002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:1042:
attributes.topDocumentURL.utf8(), limits,
On 2016/02/18 18:29:28, piman wrote:
> Is this equivalent, string.utf8() vs blink::WebStringToGURL(string).spec()?
> WebStringToGURL seems to treat 8-bit strings (possibly Latin-1) and utf-16
> strings differently.

I'm having trouble reasoning about StringImpl and StringUTF8Adaptor to know for
sure. It's certainly the case that if topDocumentURL isn't guaranteed to be
canonical, then we need to pay the cost of
blink::WebStringToGURL(string).spec().

I'll keep reading the code, but is there someone we could CC that would know the
answer to this?

[Mark Dittmer, 2016-02-18 21:10:33]

Locally all but GURL/WebString and SetActiveURL overload. Patch forthcoming
(hopefully tomorrow morning EST).

[piman, 2016-02-18 23:06:32]

piman@chromium.org changed reviewers:
+ brettw@chromium.org

[piman, 2016-02-18 23:06:32]

+brettw, probably knows the answer to the question about WebStringToGURL above
(is string.utf8() equivalent to blink::WebStringToGURL(string).spec()) ?

[Mark Dittmer, 2016-02-19 14:58:46]

Fixed issue with overload using delegate pattern. Hope to hear from brettw@
about (no?) need for WebStringToGURL. Then I think everything will be addressed.

https://codereview.chromium.org/1703163002/diff/1/content/public/common/conte...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1703163002/diff/1/content/public/common/conte...
content/public/common/content_client.h:80: void SetActiveURL(const std::string&
url);
On 2016/02/18 18:29:28, piman wrote:
> nit: avoid overloads, especially with one method virtual and the other one
not.
> SetActiveURLFromString?
> 
> OTOH, since gpu_command_buffer_stub.cc is not supposed to access the
> ContentClient directly (won't exist in mus), maybe the string->URL conversion
> should be in the GpuChildThread (will stay in content/), accessed via the
> GpuChannelManagerDelegate that Fady introduces in
> https://codereview.chromium.org/1711533002 , so you wouldn't need this here.

Refactored call into SetActiveURL to plumb through a delegate (same one as in
https://codereview.chromium.org/1711533002/). No more overload.

[Fady Samuel, 2016-02-19 18:38:36]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-02-19 18:38:37]

https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:29: #include
"content/public/common/content_client.h"
You can remove this include now, right?

[brettw, 2016-02-19 22:11:17]

If we have URLs, we should always be using URL objects, why are we doing this?

Getting a WebURL, converting to a string, sending over IPC as a string, an
re-parsing as a GURL is something that I think should not happen.

[Fady Samuel, 2016-02-19 22:16:20]

fsamuel@chromium.org changed reviewers:
+ ben@chromium.org

[Fady Samuel, 2016-02-19 22:16:21]

+ben@ for thoughts: you suggested the GPU service should not know about GURL.

[Ben Goodger (Google), 2016-02-19 22:38:05]

On 2016/02/19 22:16:21, Fady Samuel wrote:
> +ben@ for thoughts: you suggested the GPU service should not know about GURL.

So I think I don't really have context here. I made some statement about it
seeming odd that the GPU stuff knows about URLs, but that was made in abstract,
without any knowledge of what this code actually does.

Here are a few thoughts:

- if this code is intended to run in the Mus service, then the URL will have to
be passed via mojom. There's no "URL" data type, so conventionally it'd be
passed as a string. Converting to/from strings is unavoidable in this case.
- ... you might ask whether or not Mus should know about such URL-based policy
checking at all. maybe it shouldn't. Maybe it should only expose the control API
and let some other layer handle URL checking.

Rather than just hacking code around, I think it's worthwhile to think about
what the roles/responsibilities of the various components should be.

[Fady Samuel, 2016-02-19 23:19:26]

On 2016/02/19 22:38:05, Ben Goodger (Google) wrote:
> On 2016/02/19 22:16:21, Fady Samuel wrote:
> > +ben@ for thoughts: you suggested the GPU service should not know about
GURL.
> 
> So I think I don't really have context here. I made some statement about it
> seeming odd that the GPU stuff knows about URLs, but that was made in
abstract,
> without any knowledge of what this code actually does.
> 
> Here are a few thoughts:
> 
> - if this code is intended to run in the Mus service, then the URL will have
to
> be passed via mojom. There's no "URL" data type, so conventionally it'd be
> passed as a string. Converting to/from strings is unavoidable in this case.
> - ... you might ask whether or not Mus should know about such URL-based policy
> checking at all. maybe it shouldn't. Maybe it should only expose the control
API
> and let some other layer handle URL checking.
> 
> Rather than just hacking code around, I think it's worthwhile to think about
> what the roles/responsibilities of the various components should be.

To the GPU service, the URL is an opaque identifier for a context. The client
cares about this identifier for example to block misbehaving URLs from using
WebGL in the future.

I think it doesn't make sense for GPU service code because this is a policy that
may not apply for other clients.

In Mus+Ash, a privileged client (e.g. browser) could be told about which of its
children are misbehaving. However, the GPU service doesn't do anything
interesting with the URL itself I believe.

[brettw, 2016-02-20 00:08:49]

On 2016/02/19 23:19:26, Fady Samuel wrote:
> To the GPU service, the URL is an opaque identifier for a context. The client
> cares about this identifier for example to block misbehaving URLs from using
> WebGL in the future.

I guess I don't understand the bigger picture. This code you're changing is in
content, not the GPU service.

[brettw, 2016-02-20 00:12:35]

On 2016/02/20 00:08:49, brettw wrote:
> On 2016/02/19 23:19:26, Fady Samuel wrote:
> > To the GPU service, the URL is an opaque identifier for a context. The
client
> > cares about this identifier for example to block misbehaving URLs from using
> > WebGL in the future.
> 
> I guess I don't understand the bigger picture. This code you're changing is in
> content, not the GPU service.

Also, a lot of this discussion could have been avoided if the CL or bug had
something more than "do X" where "X" is something that's actively discouraged in
Chrome.

[Fady Samuel, 2016-02-20 00:30:41]

Apologies for the confusion. The goal here is to pull content/common/gpu
out of content into gpu/ipc. As a first step we're trying to remove
dependencies on other parts of content from content/common/gpu code. GURL
has its ParamTraits in content. [1] Thus, the two options are

1. Move ParamTraits<GURL> outside of content into a common place.
2. Remove dependency on GURL.

AFAICT, there's no deep need for GURL (other than passing it back to the
client and for debugging) within the GPU service so by avoiding use of GURL
in content/common/gpu, we can avoid needing that ParamTrait.

Fady

On Fri, Feb 19, 2016 at 7:12 PM <brettw@chromium.org> wrote:

>
> On 2016/02/20 00:08:49, brettw wrote:
> > On 2016/02/19 23:19:26, Fady Samuel wrote:
> > > To the GPU service, the URL is an opaque identifier for a context. The
> client
> > > cares about this identifier for example to block misbehaving URLs from
using
> > > WebGL in the future.
> >
> > I guess I don't understand the bigger picture. This code you're changing is
in
> > content, not the GPU service.
>
> Also, a lot of this discussion could have been avoided if the CL or bug had
> something more than "do X" where "X" is something that's actively discouraged
in
> Chrome.
> https://codereview.chromium.org/1703163002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-02-20 00:31:32]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

On Fri, Feb 19, 2016 at 7:30 PM Fady Samuel <fsamuel@chromium.org> wrote:

> Apologies for the confusion. The goal here is to pull content/common/gpu
> out of content into gpu/ipc. As a first step we're trying to remove
> dependencies on other parts of content from content/common/gpu code. GURL
> has its ParamTraits in content. [1] Thus, the two options are
>
> 1. Move ParamTraits<GURL> outside of content into a common place.
> 2. Remove dependency on GURL.
>
> AFAICT, there's no deep need for GURL (other than passing it back to the
> client and for debugging) within the GPU service so by avoiding use of GURL
> in content/common/gpu, we can avoid needing that ParamTrait.
>
> Fady
>
> On Fri, Feb 19, 2016 at 7:12 PM <brettw@chromium.org> wrote:
>
>>
>> On 2016/02/20 00:08:49, brettw wrote:
>> > On 2016/02/19 23:19:26, Fady Samuel wrote:
>> > > To the GPU service, the URL is an opaque identifier for a context. The
>> client
>> > > cares about this identifier for example to block misbehaving URLs from
using
>> > > WebGL in the future.
>> >
>> > I guess I don't understand the bigger picture. This code you're changing is
in
>> > content, not the GPU service.
>>
>> Also, a lot of this discussion could have been avoided if the CL or bug had
>> something more than "do X" where "X" is something that's actively discouraged
in
>> Chrome.
>> https://codereview.chromium.org/1703163002/
>>
>>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Dittmer, 2016-02-20 00:55:20]

https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:29: #include
"content/public/common/content_client.h"
On 2016/02/19 18:38:36, Fady Samuel wrote:
> You can remove this include now, right?

Done.

[Fady Samuel, 2016-02-20 01:15:55]

On 2016/02/20 00:55:20, Mark Dittmer wrote:
>
https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/1703163002/diff/40001/content/common/gpu/gpu_...
> content/common/gpu/gpu_command_buffer_stub.cc:29: #include
> "content/public/common/content_client.h"
> On 2016/02/19 18:38:36, Fady Samuel wrote:
> > You can remove this include now, right?
> 
> Done.

As discussed offline with brettw@, I feel like we're potentially conflating two
things here:

1. Decoupling content/common/gpu from content.
2. Whether the GPU service should know about URLs at all.

I think it might make sense to move GURL to url/ipc (or some other common place)
since other services will likely want those ParamTraits anyway.

We should, in parallel, discuss in a separate bug, whether it makes sense for
the GPU service to know about URLs at all.

[brettw, 2016-02-20 01:25:06]

On 2016/02/20 01:15:55, Fady Samuel wrote:
> As discussed offline with brettw@, I feel like we're potentially conflating
two
> things here:
> 
> 1. Decoupling content/common/gpu from content.
> 2. Whether the GPU service should know about URLs at all.
> 
> I think it might make sense to move GURL to url/ipc (or some other common
place)
> since other services will likely want those ParamTraits anyway.
> 
> We should, in parallel, discuss in a separate bug, whether it makes sense for
> the GPU service to know about URLs at all.

Yes, my feeling is that we'll need to move ParamTraits<GURL> out of content no
matter what given the increasing number of separate services we're planning to
add.

My opinions on the rest:

  - Something called a "url" should be a GURL. It does some extra IPC checking,
prevents many types of mistakes, makes the intent clear, and avoids unnecessary
conversions, etc.

  - GPU *could* have an "embedder data" thing that Chrome happens to send a URL
through. This seems like unnecessary complexity to me. I don't think it will be
used for anything else, and the benefit of not depending on url seems quite low
for the reduction in clarity. If we find that embedders need other stuff here,
we should have a more robust system of embedder data (passing a pickle?).

[Mark Dittmer, 2016-02-22 20:06:20]

On 2016/02/20 01:25:06, brettw wrote:
> On 2016/02/20 01:15:55, Fady Samuel wrote:
> > As discussed offline with brettw@, I feel like we're potentially conflating
> two
> > things here:
> > 
> > 1. Decoupling content/common/gpu from content.
> > 2. Whether the GPU service should know about URLs at all.
> > 
> > I think it might make sense to move GURL to url/ipc (or some other common
> place)
> > since other services will likely want those ParamTraits anyway.
> > 
> > We should, in parallel, discuss in a separate bug, whether it makes sense
for
> > the GPU service to know about URLs at all.
> 
> Yes, my feeling is that we'll need to move ParamTraits<GURL> out of content no
> matter what given the increasing number of separate services we're planning to
> add.
> 
> My opinions on the rest:
> 
>   - Something called a "url" should be a GURL. It does some extra IPC
checking,
> prevents many types of mistakes, makes the intent clear, and avoids
unnecessary
> conversions, etc.
> 
>   - GPU *could* have an "embedder data" thing that Chrome happens to send a
URL
> through. This seems like unnecessary complexity to me. I don't think it will
be
> used for anything else, and the benefit of not depending on url seems quite
low
> for the reduction in clarity. If we find that embedders need other stuff here,
> we should have a more robust system of embedder data (passing a pickle?).

Created https://codereview.chromium.org/1722773002 to, instead, move GURL
ParamTraits to url/. brettw@, piman@ PTAL.