Stop pulling googleurl through DEPS.

Stop pulling googleurl through DEPS.

This is the final patch in this series of merging the external googleurl repo
into Chromium source code base.

BUG=229660
R=brettw@chromium.org,joth@chromium.org,blundell@chromium.org,thakis@chromium.org,thestig@chromium.org
TBR=brettw

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

[tfarina, 2013-07-25 16:39:00]

android -> Jonathan
ios -> Stuart or Blundell
licenses -> Pawel or Lei?
clang -> Nico
googleurl -> Brett

thanks

[mkosiba (inactive), 2013-07-25 16:40:09]

LGTM for android_webview

[Nico, 2013-07-25 17:35:58]

clang/ lgtm

https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeCla...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeCla...
tools/clang/plugins/ChromeClassTester.cpp:160: if (!check_url_directory_)
FindBarConstructs.cpp has this comment:

      } else if (args[i] == "check-url-directory") {
        // TODO(tfarina): Remove this once http://crbug.com/229660 is fixed.
        check_url_directory_ = true;

Since you say this is the last step, should this flag be removed too?

(As far as I can tell, we don't pass "-check-url-directory" to the plugin yet,
so I'm guessing the answer is "no" since the url/ directory isn't clean yet –
but then that bug shouldn't be closed until this is done too.)

[tfarina, 2013-07-25 19:30:03]

On 2013/07/25 17:35:58, Nico wrote:
> clang/ lgtm
> 
>
https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeCla...
> File tools/clang/plugins/ChromeClassTester.cpp (right):
> 
>
https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeCla...
> tools/clang/plugins/ChromeClassTester.cpp:160: if (!check_url_directory_)
> FindBarConstructs.cpp has this comment:
> 
>       } else if (args[i] == "check-url-directory") {
>         // TODO(tfarina): Remove this once http://crbug.com/229660 is fixed.
>         check_url_directory_ = true;
> 
> Since you say this is the last step, should this flag be removed too?
> 
Yes, but we need to clean url/ as you noticed.

> (As far as I can tell, we don't pass "-check-url-directory" to the plugin yet,
> so I'm guessing the answer is "no" since the url/ directory isn't clean yet –
> but then that bug shouldn't be closed until this is done too.)

Yes, that is a separate issue, that I will also address as part of
crbug.com/229660. But I think we could close 229660 when this lands, and file
another for tools/clang/plugins issue. Either is fine by be.

[Nico, 2013-07-25 19:43:04]

On Thu, Jul 25, 2013 at 12:30 PM, <tfarina@chromium.org> wrote:

> On 2013/07/25 17:35:58, Nico wrote:
>
>> clang/ lgtm
>>
>
>
> https://codereview.chromium.**org/20349002/diff/1/tools/**clang/plugins/**
>
ChromeClassTester.cpp<https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeClassTester.cpp>
>
>> File tools/clang/plugins/**ChromeClassTester.cpp (right):
>>
>
>
> https://codereview.chromium.**org/20349002/diff/1/tools/**clang/plugins/**
>
ChromeClassTester.cpp#**newcode160<https://codereview.chromium.org/20349002/diff/1/tools/clang/plugins/ChromeClassTester.cpp#newcode160>
>
>> tools/clang/plugins/**ChromeClassTester.cpp:160: if
>> (!check_url_directory_)
>> FindBarConstructs.cpp has this comment:
>>
>
>         } else if (args[i] == "check-url-directory") {
>>          // TODO(tfarina): Remove this once http://crbug.com/229660 is
>> fixed.
>>          check_url_directory_ = true;
>>
>
>  Since you say this is the last step, should this flag be removed too?
>>
>
> Yes, but we need to clean url/ as you noticed.
>
>
>  (As far as I can tell, we don't pass "-check-url-directory" to the plugin
>> yet,
>> so I'm guessing the answer is "no" since the url/ directory isn't clean
>> yet –
>> but then that bug shouldn't be closed until this is done too.)
>>
>
> Yes, that is a separate issue, that I will also address as part of
> crbug.com/229660. But I think we could close 229660 when this lands, and
> file
> another for tools/clang/plugins issue. Either is fine by be.
>

I'm happy with either too. If you use a new bug, consider updating the
comment in FindBadConstructs to refer to the new bug.


>
>
https://codereview.chromium.**org/20349002/<https://codereview.chromium.org/2...
>

[Lei Zhang, 2013-07-25 20:10:00]

tools/licenses lgtm

[Lei Zhang, 2013-07-25 20:51:09]

On 2013/07/25 20:10:00, Lei Zhang wrote:
> tools/licenses lgtm

and tools/licenses.py too

[Paweł Hajdan Jr., 2013-07-25 21:36:53]

Could you verify that after a clobber build googleurl still appears in
about:credits?

If it doesn't, please restore the special case in licenses.py.

[Lei Zhang, 2013-07-25 21:38:53]

On 2013/07/25 21:36:53, Paweł Hajdan Jr. wrote:
> Could you verify that after a clobber build googleurl still appears in
> about:credits?
> 
> If it doesn't, please restore the special case in licenses.py.

If googleurl has been merged into the Chromium code base, does it still need an
entry in about:credits?

[tfarina, 2013-07-25 21:39:05]

On Thu, Jul 25, 2013 at 6:36 PM,  <phajdan.jr@chromium.org> wrote:
> Could you verify that after a clobber build googleurl still appears in
> about:credits?
>
You mean rm -rf out/Debug and rebuilding?

> If it doesn't, please restore the special case in licenses.py.
>
Why, if we aren't using it anymore, why it needs to show in about:credits?

> https://codereview.chromium.org/20349002/



-- 
Thiago

[Paweł Hajdan Jr., 2013-07-25 22:03:36]

On 2013/07/25 21:39:05, tfarina wrote:
> On Thu, Jul 25, 2013 at 6:36 PM,  <mailto:phajdan.jr@chromium.org> wrote:
> > Could you verify that after a clobber build googleurl still appears in
> > about:credits?
> >
> You mean rm -rf out/Debug and rebuilding?

Yes, the chrome target.

> > If it doesn't, please restore the special case in licenses.py.
> >
> Why, if we aren't using it anymore, why it needs to show in about:credits?

It's moved to a new location, src/url, right?

[stuartmorgan, 2013-07-25 22:04:04]

ios/ LGTM

[tfarina, 2013-07-25 22:05:00]

On Thu, Jul 25, 2013 at 7:03 PM,  <phajdan.jr@chromium.org> wrote:
> On 2013/07/25 21:39:05, tfarina wrote:
>
>> On Thu, Jul 25, 2013 at 6:36 PM,  <mailto:phajdan.jr@chromium.org> wrote:
>> > Could you verify that after a clobber build googleurl still appears in
>> > about:credits?
>> >
>> You mean rm -rf out/Debug and rebuilding?
>
>
> Yes, the chrome target.
>
>
>> > If it doesn't, please restore the special case in licenses.py.
>> >
>> Why, if we aren't using it anymore, why it needs to show in about:credits?
>
>
> It's moved to a new location, src/url, right?
>
yes, but not pulled, the code is checked in (~2 months already).

> https://codereview.chromium.org/20349002/



-- 
Thiago

[Paweł Hajdan Jr., 2013-07-25 22:13:59]

On 2013/07/25 22:05:00, tfarina wrote:
> yes, but not pulled, the code is checked in (~2 months already).

It doesn't make a difference whether it's pulled or checked in directly. It must
have an entry in about:credits.

Could you just confirm that it does?

[tfarina, 2013-07-25 22:15:15]

On Thu, Jul 25, 2013 at 7:13 PM,  <phajdan.jr@chromium.org> wrote:
> It doesn't make a difference whether it's pulled or checked in directly. It
> must
> have an entry in about:credits.
>
> Could you just confirm that it does?
>
No, until I understand why it needs to be in about:credits, if it
isn't a separate third_party library. It's part of Chromium code base
now!

> https://codereview.chromium.org/20349002/



-- 
Thiago

[tfarina, 2013-08-04 05:00:59]

Pawel, I take your point. If I understood it correctly your concern is that the
code checked in //url/ was copied from google-url repository, hence we have to
give credit to it somehow.

Please, could you review licenses.py and the rest of this CL again?

Thank you!

[joth, 2013-08-04 06:14:13]

https://codereview.chromium.org/20349002/diff/23001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/20349002/diff/23001/tools/licenses.py#newcode108
tools/licenses.py:108: # wherever possible.
this comment suggests we should now just checkin a //url/README.chromium file ,
rather than add to the list below?

[tfarina, 2013-08-04 14:25:08]

https://codereview.chromium.org/20349002/diff/23001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/20349002/diff/23001/tools/licenses.py#newcode108
tools/licenses.py:108: # wherever possible.
On 2013/08/04 06:14:13, joth wrote:
> this comment suggests we should now just checkin a //url/README.chromium file
,
> rather than add to the list below? 
> 

Done.

[Paweł Hajdan Jr., 2013-08-05 23:13:08]

LGTM

[commit-bot: I haz the power, 2013-08-05 23:20:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/20349002/38001

[brettw, 2013-08-05 23:26:21]

Pawel: Your suggestion, or how it was implemented, is wrong.

This is not a "copy" of google-url, we should not be referencing the upstream
URL, etc. This is the new location for the repo.

There is a file in here based on a Mozilla file, and we need to make sure that
the license attribution is done properly for this file. But that's all.

[Paweł Hajdan Jr., 2013-08-06 01:08:34]

On 2013/08/05 23:26:21, brettw wrote:
> Pawel: Your suggestion, or how it was implemented, is wrong.

Sorry, I was confused by "external googleurl repo" in the commit message.

https://code.google.com/p/google-url/ doesn't mention anything about migration.

I'm fine with this. Thanks for explaining what's really going on here.

> This is not a "copy" of google-url, we should not be referencing the upstream
> URL, etc. This is the new location for the repo.

Alright, fully agreed now. I'd like to review the updated version before it's
committed though.

> There is a file in here based on a Mozilla file, and we need to make sure that
> the license attribution is done properly for this file. But that's all.

Is that file in a third_party directory under say src/url? It should be, see
http://www.chromium.org/developers/adding-3rd-party-libraries . If it already is
- great!

[tfarina, 2013-08-06 01:38:43]

On 2013/08/06 01:08:34, Paweł Hajdan Jr. wrote:
> On 2013/08/05 23:26:21, brettw wrote:
> > Pawel: Your suggestion, or how it was implemented, is wrong.
> 
> Sorry, I was confused by "external googleurl repo" in the commit message.
> 
> https://code.google.com/p/google-url/ doesn't mention anything about
migration.
> 
> I'm fine with this. Thanks for explaining what's really going on here.
> 
> > This is not a "copy" of google-url, we should not be referencing the
upstream
> > URL, etc. This is the new location for the repo.
> 
> Alright, fully agreed now. I'd like to review the updated version before it's
> committed though.
> 
Can you tell what you want to see? I'm not what the exact modifications I should
be doing. The one in licenses.py stay?
Do I get rid of the LICENSE.txt file? Do I keep README.chromium? Do I move
url_parse.cc into url/third_party/? If yes, then I'll have to make
url/url_parse.h a forwarding header to url/third_party/url_parse.h because Blink
includes it.

Please, clarify and move me on the right direction this time.

Thanks,

> > There is a file in here based on a Mozilla file, and we need to make sure
that
> > the license attribution is done properly for this file. But that's all.
> 
> Is that file in a third_party directory under say src/url? It should be, see
> http://www.chromium.org/developers/adding-3rd-party-libraries.
If you actually look at there, you will see it isn't in a third_party/ directory
just yet.

[brettw, 2013-08-06 04:02:46]

On 2013/08/06 01:08:34, Paweł Hajdan Jr. wrote:
> On 2013/08/05 23:26:21, brettw wrote:
> > Pawel: Your suggestion, or how it was implemented, is wrong.
> 
> Sorry, I was confused by "external googleurl repo" in the commit message.
> 
> https://code.google.com/p/google-url/ doesn't mention anything about
migration.

I was waiting for everything to land before freeze-drying the old repo.

> 
> > This is not a "copy" of google-url, we should not be referencing the
upstream
> > URL, etc. This is the new location for the repo.
> 
> Alright, fully agreed now. I'd like to review the updated version before it's
> committed though.
> 
> > There is a file in here based on a Mozilla file, and we need to make sure
that
> > the license attribution is done properly for this file. But that's all.
> 
> Is that file in a third_party directory under say src/url? It should be, see
> http://www.chromium.org/developers/adding-3rd-party-libraries . If it already
is

The file is url/url_parse.cc. It's arguably not copyright Mozilla but I was
always erring on the side of attribution for this kind of thing. In retrospect,
I probably should have done a bit more effort to find where we can draw the line
since this licensing mix has always caused a headache. I think it would be a
maintainability negative to move this to some third_party directory (since it's
really our code and goes with everything else), and it didn't cause a problem
before. But I don't really know how the licensing tools work.

[tfarina, 2013-08-06 22:12:44]

ping Pawel!

[Paweł Hajdan Jr., 2013-08-06 23:44:25]

Daniel, could you advise on whether src/url/url_parse.cc
(http://src.chromium.org/viewvc/chrome/trunk/src/url/url_parse.cc?revision=HEA...)
should be moved to url/third_party as
http://www.chromium.org/developers/adding-3rd-party-libraries seems to imply?

[Ryan Sleevi, 2013-08-07 00:50:10]

On 2013/08/06 23:44:25, Paweł Hajdan Jr. wrote:
> Daniel, could you advise on whether src/url/url_parse.cc
>
(http://src.chromium.org/viewvc/chrome/trunk/src/url/url_parse.cc?revision=HEA...)
> should be moved to url/third_party as
> http://www.chromium.org/developers/adding-3rd-party-libraries seems to imply?

Sub, because of:
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%...
(where most of the code landed in net/)

[tfarina, 2013-08-07 22:17:57]

Copying open-source-third-party-reviews@

[Daniel Berlin, 2013-08-07 22:19:51]

Yes, it should be in third_party :)


On Wed, Aug 7, 2013 at 3:17 PM,  <tfarina@chromium.org> wrote:
> Copying open-source-third-party-reviews@
>
> https://codereview.chromium.org/20349002/

[tfarina, 2013-08-07 22:22:36]

On Wed, Aug 7, 2013 at 7:19 PM, Daniel Berlin <dannyb@google.com> wrote:
> Yes, it should be in third_party :)
>
Daniel, do I need a README.chromium file in there? Do I need to copy
LICENSE.txt there too?

-- 
Thiago

[tfarina, 2013-08-07 23:53:53]

Pawel, please review recent changes of url/third_party/mozilla!

https://codereview.chromium.org/20349002/diff/66001/tools/licenses.py
File tools/licenses.py (right):

https://codereview.chromium.org/20349002/diff/66001/tools/licenses.py#newcode99
tools/licenses.py:99: os.path.join('url'),
Pawel, do we want to specialize this url/third_party/mozilla?

[Daniel Berlin, 2013-08-08 02:59:01]

Yes, and yes

On Wed, Aug 7, 2013 at 3:22 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Wed, Aug 7, 2013 at 7:19 PM, Daniel Berlin <dannyb@google.com> wrote:
>> Yes, it should be in third_party :)
>>
> Daniel, do I need a README.chromium file in there? Do I need to copy
> LICENSE.txt there too?
>
> --
> Thiago

[brettw, 2013-08-10 20:40:33]

lgtm

[commit-bot: I haz the power, 2013-08-11 18:43:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/20349002/66001

[commit-bot: I haz the power, 2013-08-11 18:47:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-11 19:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/20349002/86001

[commit-bot: I haz the power, 2013-08-11 19:02:31]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-11 19:11:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/20349002/86001

[commit-bot: I haz the power, 2013-08-11 20:25:27]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-11 22:06:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/20349002/102001

[commit-bot: I haz the power, 2013-08-12 05:01:53]

Change committed as 216922