Reduce string copies in GURL creation

Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).

Committed: https://crrev.com/dfbcc3b7926990bbf5232112261f2ad9d8616e79
Cr-Commit-Position: refs/heads/master@{#370274}

[brettw, 2016-01-07 22:49:10]

Description was changed from

==========
GURL string piece constructors, WebString converter.

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.
==========

to

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.
==========

[brettw, 2016-01-07 23:04:52]

docs and export

[brettw, 2016-01-07 23:06:01]

brettw@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[brettw, 2016-01-07 23:06:02]

I imagine the change in WebString will be the most contentious, I'm open to
alternatives but this seemed least bad.

[brettw, 2016-01-07 23:06:08]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-07 23:06:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/20001

[brettw, 2016-01-07 23:14:08]

Description was changed from

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.
==========

to

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).
==========

[commit-bot: I haz the power, 2016-01-07 23:25:04]

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

[commit-bot: I haz the power, 2016-01-07 23:25:05]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2016-01-07 23:36:15]

Mac

[brettw, 2016-01-07 23:36:24]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-07 23:37:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/40001

[commit-bot: I haz the power, 2016-01-07 23:54:53]

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

[commit-bot: I haz the power, 2016-01-07 23:54:53]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[esprehn, 2016-01-07 23:56:24]

https://codereview.chromium.org/1568073002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebString.h (right):

https://codereview.chromium.org/1568073002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebString.h:170: BLINK_COMMON_EXPORT const
WebLChar* data8OrNull() const;
Yeah I don't want to expose this.  All this patch does is point out even harder
that chromium should be using WTF::String. That would have meant no copies in
the first place. :)

You could add a GURL converter for WebURL?

[brettw, 2016-01-08 00:08:01]

On 2016/01/07 23:56:24, esprehn wrote:
>
https://codereview.chromium.org/1568073002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebString.h (right):
> 
>
https://codereview.chromium.org/1568073002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebString.h:170: BLINK_COMMON_EXPORT const
> WebLChar* data8OrNull() const;
> Yeah I don't want to expose this.  All this patch does is point out even
harder
> that chromium should be using WTF::String. That would have meant no copies in
> the first place. :)
>
> You could add a GURL converter for WebURL?

We already have an implicit WebURL->GURL converter. But then all of these call
sites become WebString->WebURL->GURL which is an extra copy in each of these
cases.

If you really want me to keep the current extra copies, I will do my original
plan and replace the call sites with GURL(base::string16(WebString)) which round
trips everything through 16-bit.

[esprehn, 2016-01-08 02:12:50]

I think what you want to do is add a WebStringAdapter that has toStringPiece()
and toStringPiece16(), and internally has a StringUTF8Adaptor as a member to
keep the buffer alive for the StringPiece case.

https://codereview.chromium.org/1568073002/diff/40001/content/public/child/ur...
File content/public/child/url_conversion.cc (right):

https://codereview.chromium.org/1568073002/diff/40001/content/public/child/ur...
content/public/child/url_conversion.cc:14: reinterpret_cast<const
char*>(web_string.data8OrNull()),
I'm not sure if this is right, the 8 bit data is not valid utf8, that's only
true when the string contains only ASCII I think. See StringUTF8Adaptor.

[brettw, 2016-01-08 18:21:12]

On 2016/01/08 02:12:50, esprehn wrote:
> I think what you want to do is add a WebStringAdapter that has toStringPiece()
> and toStringPiece16(), and internally has a StringUTF8Adaptor as a member to
> keep the buffer alive for the StringPiece case.

I thought about this approach in the past when doing some StringPiece16 stuff
where we want a 16-bit StringPiece out of a WebString. That case is a bit
different because probably most WebStrings are 8-bit and so the 16-bit getter
will more often be a copy, so having an internal piece getter won't be as much
savings.

So I want to be explicit about our decision and reasoning here. We're going to
add an extra member for a UTF-8 StringPiece getter on WebString, but not a
UTF-16 based most WebStrings will be 8-bit so the extra bloat in WebString is
worth the benefit. Do you agree with this statement?



>
https://codereview.chromium.org/1568073002/diff/40001/content/public/child/ur...
> File content/public/child/url_conversion.cc (right):
> 
>
https://codereview.chromium.org/1568073002/diff/40001/content/public/child/ur...
> content/public/child/url_conversion.cc:14: reinterpret_cast<const
> char*>(web_string.data8OrNull()),
> I'm not sure if this is right, the 8 bit data is not valid utf8, that's only
> true when the string contains only ASCII I think. See StringUTF8Adaptor.

Yeah, I forgot about that.

[brettw, 2016-01-08 19:08:18]

On 2016/01/08 02:12:50, esprehn wrote:
> I think what you want to do is add a WebStringAdapter that has toStringPiece()
> and toStringPiece16(), and internally has a StringUTF8Adaptor as a member to
> keep the buffer alive for the StringPiece case.

Actually, I'm not sure how this adaptor would work. I'm not super familiar with
the bindings so I may be missing something.

It looks like the pattern is that I would make a WebStringUTF8Adaptor (I guess
this would be a friend of WebString to access the internals) and that has a
heap-allocated WTF::StringUTF8Adaptor inside it. But then we're still
heap-allocating a WTF type for no-op string conversions, which seems like it's
not worth doing all this for. Is there another approach?

[brettw, 2016-01-08 20:19:18]

I would do a completely new implementation of the UTF-8 adaptor (it's only a few
lines) to solve the above problem. This would still require a copy for the
non-8-bit case which is not the case for my current patch (since GURL can take
8- or 16-bit input). It may be unusual enough for strings that are URLs to be
16-bit that we don't care.

[esprehn, 2016-01-08 20:23:05]

WebStringAdapter shouldn't need to heap allocate the Adapter, but you could
write your own I guess.

asStringPiece() says:

if (is8Bit and ascii)
  return StringPiece(characters8, length)
else {
  m_utf8Buffer = ...;
  return StringPiece(m_utf8Buffer.data, m_utf8Buffer.length)
}

asStringPiece16()

has the same thing but for the 16 bit case.

It's the users job to create the WebStringAdapter on the stack to keep the
buffers alive. No new methods or members are needed on WebString (we can't make
it bigger).

[brettw, 2016-01-08 21:23:30]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 21:24:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/60001

[commit-bot: I haz the power, 2016-01-08 21:28:11]

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

[commit-bot: I haz the power, 2016-01-08 21:28:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2016-01-08 21:53:19]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 21:55:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/80001

[commit-bot: I haz the power, 2016-01-08 22:24:37]

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

[commit-bot: I haz the power, 2016-01-08 22:24:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2016-01-08 22:28:59]

PTAL. The only way I could get everything to work without exposing WTF to
content without introducing extra copies was to write my own adaptor and have it
store a reference to a CStringBuffer.

[brettw, 2016-01-08 22:37:58]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 22:39:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/100001

[michaeln, 2016-01-08 22:52:55]

https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
(right):

https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:51:
wtfString.length());
drive by: Do you want the adapter to keep the WTF::String's internal buffer
alive for the lifetime of the adapter?

[brettw, 2016-01-08 23:11:21]

https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
(right):

https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:51:
wtfString.length());
On 2016/01/08 22:52:55, michaeln wrote:
> drive by: Do you want the adapter to keep the WTF::String's internal buffer
> alive for the lifetime of the adapter?

Maybe, I'm not sure what you mean. It's expected the string will outlive the
adaptor. What I do here is derived from the current StringUTF8Adaptor. What are
you suggesting doing?

[commit-bot: I haz the power, 2016-01-08 23:15:12]

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

[commit-bot: I haz the power, 2016-01-08 23:15:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[michaeln, 2016-01-08 23:49:13]

On 2016/01/08 23:11:21, brettw wrote:
>
https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
> (right):
> 
>
https://codereview.chromium.org/1568073002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:51:
> wtfString.length());
> On 2016/01/08 22:52:55, michaeln wrote:
> > drive by: Do you want the adapter to keep the WTF::String's internal buffer
> > alive for the lifetime of the adapter?
> 
> Maybe, I'm not sure what you mean. It's expected the string will outlive the
> adaptor. What I do here is derived from the current StringUTF8Adaptor. What
are
> you suggesting doing?

I hadn't seen the existing StringUTF8Adaptor class was the basis for this. It
just caught my eye that if the string did not outlive the adapter,
asStringPiece() would be ripe for uaf problems. Is there anything that prevents
the adapter from not outliving the string its adapting? There are no comments
about that expectation. Maybe worth a comment or actually adding a ref to the
underlying string impl?

[brettw, 2016-01-08 23:56:41]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-01-08 23:56:56]

I added a comment about lifetime.

[commit-bot: I haz the power, 2016-01-08 23:58:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/120001

[commit-bot: I haz the power, 2016-01-09 00:30:37]

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

[commit-bot: I haz the power, 2016-01-09 00:30:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2016-01-11 17:53:48]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-11 17:54:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/140001

[commit-bot: I haz the power, 2016-01-11 18:05:20]

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

[commit-bot: I haz the power, 2016-01-11 18:05:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2016-01-12 22:34:27]

haraken: It looks like esprehn is out for a bit. I think I addressed all of his
concerns as best as is technically possible and the Blink API changes should be
relatively clean and simple.

Are you OK reviewing this? It looks like a lot of stuff, but needs thought are
the few changes in third_party/WebKit/*. The rest is just updating the call
sites.

[haraken, 2016-01-12 23:44:37]

(I'm not an owner of public/ though.)

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
(right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:2: *
Copyright (C) 2016 Google Inc. All rights reserved.

You can use the 3-line copyright.

// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:48: if
(wtfString.is8Bit() && wtfString.containsOnlyASCII()) {

Instead of implementing the conversion logic here, can we reuse
StringUTF8Adaptor
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It would be better to make WebStringUTF8Adaptor a think wrapper of
StringUTF8Adaptor.

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebStringUTF8Adaptor.h (right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebStringUTF8Adaptor.h:2: * Copyright (C)
2016 Google Inc. All rights reserved.

Ditto. You can use the 3-line copyright.

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebStringUTF8Adaptor.h:54:
WebStringUTF8Adaptor(const WebString&);

Is it intentional that we don't have an explicit on the constructor?

[esprehn, 2016-01-13 02:02:32]

lgtm w/ the friends removed. I'd rather we moved the GURL converter into blink
though. :)

https://codereview.chromium.org/1568073002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebStringUTF8Adaptor.h (right):

https://codereview.chromium.org/1568073002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebStringUTF8Adaptor.h:63:
WebPrivatePtr<WTF::CStringBuffer> m_utf8Buffer;
CString ?

https://codereview.chromium.org/1568073002/diff/140001/content/public/child/u...
File content/public/child/url_conversion.h (right):

https://codereview.chromium.org/1568073002/diff/140001/content/public/child/u...
content/public/child/url_conversion.h:22: CONTENT_EXPORT GURL
WebStringToGURL(const blink::WebString& web_string);
I sort of wonder if this shouldn't be in blink platform instead, it doesn't
really feel like it belongs here.

If this was just in blink platform we also wouldn't need WebStringUTF8Adaptor at
all. We probably _should_ have it, for other places in the codebase, but you
wouldn't need it for this usage.

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebString.h (right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebString.h:177: friend class
WebStringUTF8Adaptor;
you don't need friends.

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebStringUTF8Adaptor.h (right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebStringUTF8Adaptor.h:54:
WebStringUTF8Adaptor(const WebString&);
explicit

[brettw, 2016-01-13 05:40:43]

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
(right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:48: if
(wtfString.is8Bit() && wtfString.containsOnlyASCII()) {
The reason I didn't do this was that I can't include StringUTF8Adaptor.h in the
header so can't have it as a member. And I definitely didn't want to
heap-allocate an adapter which would eliminate the point of this change
(avoiding heap allocations in common conversion cases).

I could make UTF8StringAdaptor expose its internal buffer via a getter, use it
in the .cpp file on the stack, and take a reference to that buffer as
m_utf8Buffer on this class.

But it seemed like exposing the internal buffer on the existing adapter violates
the entire abstraction that the adapter provides, and the code is only 4 lines
of basic string manipulation. I thought it was not worth the benefit to do this.
If you prefer the alternative, I'm fine doing that, and I'm happy to hear any
other suggestions you might have.

[haraken, 2016-01-13 05:55:39]

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp
(right):

https://codereview.chromium.org/1568073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebStringUTF8Adaptor.cpp:48: if
(wtfString.is8Bit() && wtfString.containsOnlyASCII()) {
On 2016/01/13 05:40:43, brettw wrote:
> The reason I didn't do this was that I can't include StringUTF8Adaptor.h in
the
> header so can't have it as a member. And I definitely didn't want to
> heap-allocate an adapter which would eliminate the point of this change
> (avoiding heap allocations in common conversion cases).
> 
> I could make UTF8StringAdaptor expose its internal buffer via a getter, use it
> in the .cpp file on the stack, and take a reference to that buffer as
> m_utf8Buffer on this class.
> 
> But it seemed like exposing the internal buffer on the existing adapter
violates
> the entire abstraction that the adapter provides, and the code is only 4 lines
> of basic string manipulation. I thought it was not worth the benefit to do
this.
> If you prefer the alternative, I'm fine doing that, and I'm happy to hear any
> other suggestions you might have.

Just help me understand: Why can't we include wtf/StringUTF8Adaptor.h in
public/platform/? public/platform/ already includes String, Vector and a bunch
of fundamental classes in wtf/. I think StringUTF8Adaptor could be a part of
it...

[brettw, 2016-01-13 06:31:38]

> Just help me understand: Why can't we include wtf/StringUTF8Adaptor.h in
> public/platform/? public/platform/ already includes String, Vector and a bunch
> of fundamental classes in wtf/. I think StringUTF8Adaptor could be a part of
> it...

I don't see any such includes (I could have missed something). As an example,
WebString includes one wtf header under an INSIDE_BLINK guard, and some base
headers under the opposite condition. The functions that rely on the
corresponding types are all inlined. This way, Blink can create WebStrings from
Strings and such, and Content can see base types, but neither side ever sees the
other side's headers or types. WebVector is an even more extreme example, where
neither STL/base nor WTF types are used in any way.

[haraken, 2016-01-13 07:14:42]

On 2016/01/13 06:31:38, brettw wrote:
> > Just help me understand: Why can't we include wtf/StringUTF8Adaptor.h in
> > public/platform/? public/platform/ already includes String, Vector and a
bunch
> > of fundamental classes in wtf/. I think StringUTF8Adaptor could be a part of
> > it...
> 
> I don't see any such includes (I could have missed something). As an example,
> WebString includes one wtf header under an INSIDE_BLINK guard, and some base
> headers under the opposite condition. The functions that rely on the
> corresponding types are all inlined. This way, Blink can create WebStrings
from
> Strings and such, and Content can see base types, but neither side ever sees
the
> other side's headers or types. WebVector is an even more extreme example,
where
> neither STL/base nor WTF types are used in any way.

Ah, thanks, now I understand! LGTM.

[brettw, 2016-01-13 18:52:47]

I made a new file in platform for the conversion function. This removes the
WebStringUTF8Adaptor.

This is probably a more clean place for the code, and also allows it to be used
in media (which I had to do explicit conversions through string16 instead of
using the helper function).

Since this has access to String directly, it can also get the UTF-16 string out
which is a nice-to-have. It uses StringUTF8Adaptor for the 8-bit case to handle
Latin-1, which will be the only case now where a string conversion is required.

[brettw, 2016-01-13 18:52:54]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 18:53:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/180001

[commit-bot: I haz the power, 2016-01-13 18:58:55]

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

[commit-bot: I haz the power, 2016-01-13 18:58:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[brettw, 2016-01-13 19:20:23]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 19:21:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/200001

[commit-bot: I haz the power, 2016-01-13 19:49:45]

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

[commit-bot: I haz the power, 2016-01-13 19:49:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[haraken, 2016-01-13 23:59:37]

Looks much nicer! LGTM

[haraken, 2016-01-13 23:59:49]

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/URLConversion.h (right):

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/URLConversion.h:19: 

Unnecessary empty line.

[esprehn, 2016-01-14 01:47:59]

lgtm, btw this still does a needless string copy inside GURL because of using
the std library.

  // The actual text of the URL, in canonical ASCII form.
  std::string spec_;

if we just used wtf::String everywhere that would be a String and there'd be no
copy. GURL would also be smaller.

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/URLConversion.cpp (right):

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/URLConversion.cpp:14: GURL
WebStringToGURL(const WebString& webStr)
webString

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/URLConversion.cpp:16: WTF::String
str = webStr;
no need for WTF:: prefix.

also wtfString, don't abbreviate in blink please.

https://codereview.chromium.org/1568073002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/URLConversion.cpp:20: return
GURL(utf8.asStringPiece());
Note that this isn't safe unless the GURL copies the string, it does though
(spec_ is a std::string), the StringPiece is only valid as long as the Adapter.

[brettw, 2016-01-17 18:03:33]

On 2016/01/14 01:47:59, esprehn wrote:
> lgtm, btw this still does a needless string copy inside GURL because of using
> the std library.
> 
>   // The actual text of the URL, in canonical ASCII form.
>   std::string spec_;
> 
> if we just used wtf::String everywhere that would be a String and there'd be
no
> copy. GURL would also be smaller.

WTFString can't be used across threads which GURL needs to support. Given that
it clearly can't work for this use case, continuing to argue this is hurting
your overall argument for increased use of WTF.

[brettw, 2016-01-19 21:28:05]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 21:29:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/220001

[commit-bot: I haz the power, 2016-01-19 22:05:47]

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

[commit-bot: I haz the power, 2016-01-19 22:05:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2016-01-19 22:37:21]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-19 22:38:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/240001

[brettw, 2016-01-19 22:39:58]

Note: I added an empty check to the beginning of the new WebStringToGURL
function since String.is8Bit doesn't check for null and will crash in the null
case otherwise.

[commit-bot: I haz the power, 2016-01-19 23:18:35]

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

[commit-bot: I haz the power, 2016-01-19 23:18:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2016-01-19 23:52:33]

The CQ bit was checked by brettw@chromium.org

[brettw, 2016-01-19 23:52:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1568073002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2016-01-19 23:53:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568073002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568073002/260001

[ojan, 2016-01-20 00:09:56]

On 2016/01/17 at 18:03:33, brettw wrote:
> On 2016/01/14 01:47:59, esprehn wrote:
> > lgtm, btw this still does a needless string copy inside GURL because of
using
> > the std library.
> > 
> >   // The actual text of the URL, in canonical ASCII form.
> >   std::string spec_;
> > 
> > if we just used wtf::String everywhere that would be a String and there'd be
no
> > copy. GURL would also be smaller.
> 
> WTFString can't be used across threads which GURL needs to support. Given that
it clearly can't work for this use case, continuing to argue this is hurting
your overall argument for increased use of WTF.

I don't think it would be very difficult to make a threadsafe version of
wtf::String (i.e. wtf::ThreadSafeString that wraps the same StringImpl, but uses
threadsafe refcounting) to use in cases where thread safety is needed. I just
bring this up because I think that was implicit in Elliot's conception of how it
would work to use wtf::String everywhere.

[commit-bot: I haz the power, 2016-01-20 01:49:24]

Description was changed from

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).
==========

to

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).
==========

[commit-bot: I haz the power, 2016-01-20 01:49:26]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-01-20 01:51:13]

Description was changed from

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).
==========

to

==========
Reduce string copies in GURL creation

Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).

Committed: https://crrev.com/dfbcc3b7926990bbf5232112261f2ad9d8616e79
Cr-Commit-Position: refs/heads/master@{#370274}
==========

[commit-bot: I haz the power, 2016-01-20 01:51:15]

Patchset 14 (id:??) landed as
https://crrev.com/dfbcc3b7926990bbf5232112261f2ad9d8616e79
Cr-Commit-Position: refs/heads/master@{#370274}

[Anand Mistry (off Chromium), 2016-01-20 02:54:51]

On 2016/01/20 01:51:15, commit-bot: I haz the power wrote:
> Patchset 14 (id:??) landed as
> https://crrev.com/dfbcc3b7926990bbf5232112261f2ad9d8616e79
> Cr-Commit-Position: refs/heads/master@{#370274}

Currently breaks ChromiumOS bot. May break more as bots catch up:
FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/chrome/browser/chromeos/chromeos/arc_intent_helper_bridge_impl.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -DENABLE_MDNS=1 -DENABLE_NOTIFICATIONS
-DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DDONT_EMBED_BUILD_METADATA
-DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_ASH=1 -DUSE_AURA=1
-DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_CLIPBOARD_AURAX11=1
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_GLIB=1 -DUSE_OPENSSL=1 -DUSE_NSS_CERTS=1
-DUSE_X11=1 -DENABLE_WEBRTC=1 -DENABLE_EXTENSIONS=1
-DENABLE_CONFIGURATION_POLICY -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_APP_LIST=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_AUTOFILL_DIALOG=1 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_HIDPI=1
-DENABLE_TOPCHROME_MD=1 -DENABLE_BACKGROUND=1 -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD
-DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=257955-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DCOMPONENT_BUILD -DOS_CHROMEOS -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GLIBCXX_DEBUG=1
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0
-DSK_SUPPORT_GPU=1 -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DPROTOBUF_USE_DLLS
-DGL_GLEXT_PROTOTYPES -DTOOLKIT_VIEWS=1 -DMOJO_USE_SYSTEM_IMPL
-DMOJO_USE_SYSTEM_IMPL -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0
-DGTEST_HAS_RTTI=0 -DMOJO_USE_GLES2_IMPL -DV8_USE_EXTERNAL_STARTUP_DATA
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DV8_SHARED -DUSING_V8_SHARED
-DOPENSSL_SMALL -DBORINGSSL_SHARED_LIBRARY -DLEVELDB_PLATFORM_CHROMIUM=1
-DFEATURE_ENABLE_SSL -DFEATURE_ENABLE_VOICEMAIL -DEXPAT_RELATIVE_PATH
-DGTEST_RELATIVE_PATH -DNO_MAIN_THREAD_WRAPPING -DNO_SOUND_SYSTEM -DLINUX
-DWEBRTC_LINUX -DWEBRTC_POSIX -DCHROMEOS -I../.. -Igen -Igen/ash/resources
-Igen/ash/strings -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome
-Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome
-Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome -Igen/chrome
-Igen/chrome -Igen/chrome -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I../../skia/config
-I../../skia/ext -I../../third_party/skia/include/c
-I../../third_party/skia/include/config -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/images
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu
-Igen/ui/chromeos/resources -Igen/ui/chromeos/strings -Igen/ui/resources
-Igen/ui/resources -I../../third_party/protobuf/src -Igen/protoc_out
-I../../third_party/protobuf/src -I../../third_party/protobuf
-I../../third_party/khronos -I../../gpu -I../../third_party/libwebp
-I../../testing/gtest/include -Igen -I../../third_party/WebKit
-I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
-I../../third_party/npapi -I../../third_party/npapi/bindings
-I../../third_party/dom_distiller_js/dist/proto_gen -Igen/components/strings
-Igen/components/strings -Igen/components/strings -I../../v8/include
-I/usr/include/nss -I/usr/include/nspr -I../../third_party/boringssl/src/include
-Igen -I../../third_party/mesa/src/include -I../../third_party/opus/src/include
-I../../third_party/re2/src -Igen/extensions -Igen/extensions -Igen/extensions
-Igen -Igen/extensions/strings -I/usr/include/dbus-1.0
-I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -Igen
-I../../third_party/cacheinvalidation/overrides
-I../../third_party/cacheinvalidation/src -I../../third_party/leveldatabase
-I../../third_party/leveldatabase/src
-I../../third_party/leveldatabase/src/include
-I../../third_party/webrtc_overrides -I../../third_party/libjingle/overrides
-I../../third_party/libjingle/source -I../../testing/gtest/include
-I../../third_party -I../../third_party/libusb/src/libusb
-I../../third_party/zlib -Igen/ui/views/resources -fno-strict-aliasing
-funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
-fcolor-diagnostics -pthread -m64 -march=x86-64 -Wall -Wextra -Werror
-Wno-missing-field-initializers -Wno-unused-parameter -Wfor-loop-analysis
-Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -O0 -g2 -gsplit-dwarf -fvisibility=hidden -Xclang
-load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang
-add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs
-Xclang check-templates -Wheader-hygiene -Wstring-conversion
-Wexit-time-destructors -Wno-header-guard -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wno-undefined-bool-conversion
-Wno-tautological-undefined-compare -std=gnu++11
-Wno-reserved-user-defined-literal -fno-rtti -fno-exceptions -c
../../chrome/browser/chromeos/arc/arc_intent_helper_bridge_impl.cc -o
obj/chrome/browser/chromeos/chromeos/arc_intent_helper_bridge_impl.o
../../chrome/browser/chromeos/arc/arc_intent_helper_bridge_impl.cc:39:8: error:
no matching constructor for initialization of 'GURL'
  GURL gurl(url);
       ^    ~~~
../../url/gurl.h:56:3: note: candidate constructor not viable: no known
conversion from 'const mojo::String' to 'const GURL' for 1st argument
  GURL(const GURL& other);
  ^
../../url/gurl.h:59:12: note: candidate constructor not viable: no known
conversion from 'const mojo::String' to 'base::StringPiece' (aka
'BasicStringPiece<basic_string<char> >') for 1st argument
  explicit GURL(base::StringPiece url_string);
           ^
../../url/gurl.h:60:12: note: candidate constructor not viable: no known
conversion from 'const mojo::String' to 'base::StringPiece16' (aka
'BasicStringPiece<basic_string<unsigned short, base::string16_char_traits> >')
for 1st argument
  explicit GURL(base::StringPiece16 url_string);
           ^
../../url/gurl.h:52:3: note: candidate constructor not viable: requires 0
arguments, but 1 was provided
  GURL();
  ^
../../url/gurl.h:405:3: note: candidate constructor not viable: requires 2
arguments, but 1 was provided
  GURL(const std::string& url_string, RetainWhiteSpaceSelector);
  ^
../../url/gurl.h:75:3: note: candidate constructor not viable: requires 3
arguments, but 1 was provided
  GURL(std::string canonical_spec, const url::Parsed& parsed, bool is_valid);
  ^
../../url/gurl.h:65:3: note: candidate constructor not viable: requires 4
arguments, but 1 was provided
  GURL(const char* canonical_spec,
  ^

[brettw, 2016-01-20 06:20:57]

I don't have those files in my checkout. Whoever's sherriffing that downstream
bot should change
  GURL gurl(url);
to
  GURL gurl(url.get())