new URL('') should throw TypeError

new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this
change, because empty action url is by default resolved to
base url before this patch. Made appropriate code changes
as empty action url can be equal to base url only for
relative base urls.

BUG=463961
Committed: https://crrev.com/56d42205139458d71b64a9b2f773b4445460e3a6
Cr-Commit-Position: refs/heads/master@{#362589}

[ramya.v, 2015-11-04 19:20:35]

ramya.v@samsung.com changed reviewers:
+ rob.buis@samsung.com

[ramya.v, 2015-11-04 19:20:36]

PTAL!
Thanks.

[rwlbuis, 2015-11-04 20:07:35]

On 2015/11/04 19:20:36, ramya.v wrote:
> PTAL!
> Thanks.

Looks good, but are the autofill browsertest changes really needed?

[ramya.v, 2015-11-05 04:49:00]

On 2015/11/04 20:07:35, rwlbuis wrote:
> On 2015/11/04 19:20:36, ramya.v wrote:
> > PTAL!
> > Thanks.
> 
> Looks good, but are the autofill browsertest changes really needed?


Following tests were failing after this change. 
FormAutocompleteTest.NoLongerVisibleBothNoActions
PasswordGenerationAgentTest.DynamicFormTest
PasswordAutofillAgentTest.RememberLastNonEmptyUsernameAndPasswordOnSubmit_New
PasswordAutofillAgentTest.InitialAutocompleteForEmptyAction

If action url is coming empty password form wont' get filled or password
generated, so
modified tests where actual concern is not about testing null action url and
made appropriatecode
code changes where empty action url is by default taken as frame url when it is
specified empty.
(Only for relative urls it can be equal to base url now)

[rwlbuis, 2015-11-05 19:22:08]

On 2015/11/05 04:49:00, ramya.v wrote:
> On 2015/11/04 20:07:35, rwlbuis wrote:
> > On 2015/11/04 19:20:36, ramya.v wrote:
> > > PTAL!
> > > Thanks.
> > 
> > Looks good, but are the autofill browsertest changes really needed?
> 
> 
> Following tests were failing after this change. 
> FormAutocompleteTest.NoLongerVisibleBothNoActions
> PasswordGenerationAgentTest.DynamicFormTest
> PasswordAutofillAgentTest.RememberLastNonEmptyUsernameAndPasswordOnSubmit_New
> PasswordAutofillAgentTest.InitialAutocompleteForEmptyAction
> 
> If action url is coming empty password form wont' get filled or password
> generated, so
> modified tests where actual concern is not about testing null action url and
> made appropriatecode
> code changes where empty action url is by default taken as frame url when it
is
> specified empty.
> (Only for relative urls it can be equal to base url now)

Thanks for the explanation, I think you can add a summary to the Description.

The patch looks good to me, it can now go to other people (chromium OWNERS) for
review :)

[ramya.v, 2015-11-06 04:19:45]

Description was changed from

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

BUG=463961
==========

to

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this change, because empty
action url is by default resolved to base url before this patch. Made
appropriate code changes as empty action url can be equal to base url only for
relative base urls.

BUG=463961
==========

[ramya.v, 2015-11-06 04:19:46]

ramya.v@samsung.com changed reviewers:
+ vabr@chromium.org

[ramya.v, 2015-11-06 04:20:42]

On 2015/11/05 19:22:08, rwlbuis wrote:
> On 2015/11/05 04:49:00, ramya.v wrote:
> > On 2015/11/04 20:07:35, rwlbuis wrote:
> > > On 2015/11/04 19:20:36, ramya.v wrote:
> > > > PTAL!
> > > > Thanks.
> > > 
> > > Looks good, but are the autofill browsertest changes really needed?
> > 
> > 
> > Following tests were failing after this change. 
> > FormAutocompleteTest.NoLongerVisibleBothNoActions
> > PasswordGenerationAgentTest.DynamicFormTest
> >
PasswordAutofillAgentTest.RememberLastNonEmptyUsernameAndPasswordOnSubmit_New
> > PasswordAutofillAgentTest.InitialAutocompleteForEmptyAction
> > 
> > If action url is coming empty password form wont' get filled or password
> > generated, so
> > modified tests where actual concern is not about testing null action url and
> > made appropriatecode
> > code changes where empty action url is by default taken as frame url when it
> is
> > specified empty.
> > (Only for relative urls it can be equal to base url now)
> 
> Thanks for the explanation, I think you can add a summary to the Description.
> 
> The patch looks good to me, it can now go to other people (chromium OWNERS)
for
> review :)

@Vaclav can you please take a look!

Thanks

[ramya.v, 2015-11-06 04:21:17]

Description was changed from

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this change, because empty
action url is by default resolved to base url before this patch. Made
appropriate code changes as empty action url can be equal to base url only for
relative base urls.

BUG=463961
==========

to

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this 
change, because empty action url is by default resolved to 
base url before this patch. Made appropriate code changes 
as empty action url can be equal to base url only for 
relative base urls.

BUG=463961
==========

[vabr (Chromium), 2015-11-06 11:24:14]

Files with *password* in the name LGTM, with some comments and a question, which
I would still like to get resolved before landing the CL.

Thanks!
Vaclav

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:556:
fill_data_.action = GURL("");
Does GURL() and GURL("") result in the same object? If so, please use GURL().
Otherwise, please at least replace "" with std::string().

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1403: "<FORM
name='LoginTestForm' action='http://www.bidule.com'>"
What exactly went wrong with this test when the action was not specified?

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1124: const bool
action_is_empty = canonical_action == canonical_origin
nit: Could you please comment that omitting the action attribute would result in
|canonical_origin| for hierarchical schemes like http:, and in an empty URL for
non-hierarchical schemes like about: or data:? (Also: is that true? :))

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1125: ||
canonical_action.is_empty();
Could you swap the order of the checks, so that is_empty() goes before the
string equality? is_empty is much quicker, could spare some time in cases where
it is true, and won't add significant overhead in the rest.

(Also on lines 1141-1142.)

[ramya.v, 2015-11-06 13:45:14]

@Vaclav PTAL!
Thanks.

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:556:
fill_data_.action = GURL("");
On 2015/11/06 11:24:14, vabr (Chromium) wrote:
> Does GURL() and GURL("") result in the same object? If so, please use GURL().
> Otherwise, please at least replace "" with std::string().

Done.

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1403: "<FORM
name='LoginTestForm' action='http://www.bidule.com'>"
On 2015/11/06 11:24:14, vabr (Chromium) wrote:
> What exactly went wrong with this test when the action was not specified?

In PasswordAutofillAgent::WillSubmitForm
submitted_form is coming null because of the following logic in
CreatePasswordFormFromWebForm.
 password_form->action = form_util::GetCanonicalActionForForm(web_form);
  if (!password_form->action.is_valid())
    return scoped_ptr<PasswordForm>();

Hence IPC msg is not sent back and the test is failing.

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1124: const bool
action_is_empty = canonical_action == canonical_origin
On 2015/11/06 11:24:14, vabr (Chromium) wrote:
> nit: Could you please comment that omitting the action attribute would result
in
> |canonical_origin| for hierarchical schemes like http:, and in an empty URL
for
> non-hierarchical schemes like about: or data:? (Also: is that true? :))

Done.

https://codereview.chromium.org/1409293007/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1125: ||
canonical_action.is_empty();
On 2015/11/06 11:24:14, vabr (Chromium) wrote:
> Could you swap the order of the checks, so that is_empty() goes before the
> string equality? is_empty is much quicker, could spare some time in cases
where
> it is true, and won't add significant overhead in the rest.
> 
> (Also on lines 1141-1142.)

Done.

[vabr (Chromium), 2015-11-06 14:08:54]

Thanks for addressing my comments!
Files with *password* in the name still LGTM.
Cheers,
Vaclav

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/1409293007/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1403: "<FORM
name='LoginTestForm' action='http://www.bidule.com'>"
On 2015/11/06 13:45:14, ramya.v wrote:
> On 2015/11/06 11:24:14, vabr (Chromium) wrote:
> > What exactly went wrong with this test when the action was not specified?
> 
> In PasswordAutofillAgent::WillSubmitForm
> submitted_form is coming null because of the following logic in
> CreatePasswordFormFromWebForm.
>  password_form->action = form_util::GetCanonicalActionForForm(web_form);
>   if (!password_form->action.is_valid())
>     return scoped_ptr<PasswordForm>();
> 
> Hence IPC msg is not sent back and the test is failing.

Acknowledged.

[ramya.v, 2015-11-06 14:39:51]

The CQ bit was checked by ramya.v@samsung.com

[commit-bot: I haz the power, 2015-11-06 14:40:17]

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

[commit-bot: I haz the power, 2015-11-06 14:48:40]

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

[commit-bot: I haz the power, 2015-11-06 14:48:41]

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...)

[ramya.v, 2015-11-07 01:18:02]

ramya.v@samsung.com changed reviewers:
+ abarth@chromium.org, estade@chromium.org

[ramya.v, 2015-11-07 01:18:03]

Added owners of 
form_autofill_util.cc
url_canon_relative.cc

@Evan & Abarth
Could you please take a look.

Thanks

[vabr (Chromium), 2015-11-10 09:49:37]

As of yesterday I can actually also review //components/autofill, so jumping in
place of Evan:

The change in form_autofill_util.cc LGTM.
Cheers,
Vaclav

On 2015/11/07 01:18:03, ramya.v wrote:
> Added owners of 
> form_autofill_util.cc
> url_canon_relative.cc
> 
> @Evan & Abarth
> Could you please take a look.
> 
> Thanks

[ramya.v, 2015-11-13 03:32:05]

On 2015/11/10 09:49:37, vabr (Chromium) wrote:
> As of yesterday I can actually also review //components/autofill, so jumping
in
> place of Evan:
> 
> The change in form_autofill_util.cc LGTM.
> Cheers,
> Vaclav
> 
> On 2015/11/07 01:18:03, ramya.v wrote:
> > Added owners of 
> > form_autofill_util.cc
> > url_canon_relative.cc
> > 
> > @Evan & Abarth
> > Could you please take a look.
> > 
> > Thanks

@abarth
Could you please review changes related to 
url_canon_relative.cc

Thanks

[rwlbuis, 2015-11-19 21:15:44]

On 2015/11/13 03:32:05, ramya.v wrote:
> On 2015/11/10 09:49:37, vabr (Chromium) wrote:
> > As of yesterday I can actually also review //components/autofill, so jumping
> in
> > place of Evan:
> > 
> > The change in form_autofill_util.cc LGTM.
> > Cheers,
> > Vaclav
> > 
> > On 2015/11/07 01:18:03, ramya.v wrote:
> > > Added owners of 
> > > form_autofill_util.cc
> > > url_canon_relative.cc
> > > 
> > > @Evan & Abarth
> > > Could you please take a look.
> > > 
> > > Thanks
> 
> @abarth
> Could you please review changes related to 
> url_canon_relative.cc
> 
> Thanks

@ramya I don't think abarth is actively reviewing chromium patches, maybe it is
better to find another reviewer.

[Evan Stade, 2015-11-19 22:01:09]

On 2015/11/19 21:15:44, rwlbuis wrote:
> On 2015/11/13 03:32:05, ramya.v wrote:
> > On 2015/11/10 09:49:37, vabr (Chromium) wrote:
> > > As of yesterday I can actually also review //components/autofill, so
jumping
> > in
> > > place of Evan:
> > > 
> > > The change in form_autofill_util.cc LGTM.
> > > Cheers,
> > > Vaclav
> > > 
> > > On 2015/11/07 01:18:03, ramya.v wrote:
> > > > Added owners of 
> > > > form_autofill_util.cc
> > > > url_canon_relative.cc
> > > > 
> > > > @Evan & Abarth
> > > > Could you please take a look.
> > > > 
> > > > Thanks
> > 
> > @abarth
> > Could you please review changes related to 
> > url_canon_relative.cc
> > 
> > Thanks
> 
> @ramya I don't think abarth is actively reviewing chromium patches, maybe it
is
> better to find another reviewer.

isn't vabr@ an owner for form_autofill_util.cc

[rwlbuis, 2015-11-19 22:11:01]

On 2015/11/19 22:01:09, Evan Stade wrote:
> > @ramya I don't think abarth is actively reviewing chromium patches, maybe it
> is
> > better to find another reviewer.
> 
> isn't vabr@ an owner for form_autofill_util.cc

Ah, thanks, vabr did mention it now that you mention it.

@ramya sounds like it should work, but you may need to re-upload since likely
the patch will not apply anymore.

[Evan Stade, 2015-11-19 23:06:35]

On 2015/11/19 22:11:01, rwlbuis wrote:
> On 2015/11/19 22:01:09, Evan Stade wrote:
> > > @ramya I don't think abarth is actively reviewing chromium patches, maybe
it
> > is
> > > better to find another reviewer.
> > 
> > isn't vabr@ an owner for form_autofill_util.cc
> 
> Ah, thanks, vabr did mention it now that you mention it.
> 
> @ramya sounds like it should work, but you may need to re-upload since likely
> the patch will not apply anymore.

you probably still need abarth or someone else for url/url_canon_relative.cc

[ramya.v, 2015-11-20 03:37:36]

ramya.v@samsung.com changed reviewers:
+ brettw@chromium.org

[ramya.v, 2015-11-20 03:38:26]

On 2015/11/19 23:06:35, Evan Stade wrote:
> On 2015/11/19 22:11:01, rwlbuis wrote:
> > On 2015/11/19 22:01:09, Evan Stade wrote:
> > > > @ramya I don't think abarth is actively reviewing chromium patches,
maybe
> it
> > > is
> > > > better to find another reviewer.
> > > 
> > > isn't vabr@ an owner for form_autofill_util.cc
> > 
> > Ah, thanks, vabr did mention it now that you mention it.
> > 
> > @ramya sounds like it should work, but you may need to re-upload since
likely
> > the patch will not apply anymore.
> 
> you probably still need abarth or someone else for url/url_canon_relative.cc



@brettw Could you please review the changes related to url/url_canon_relative.cc

Thanks!

[vabr (Chromium), 2015-11-20 13:10:50]

On 2015/11/19 22:01:09, Evan Stade wrote:
> On 2015/11/19 21:15:44, rwlbuis wrote:
> > On 2015/11/13 03:32:05, ramya.v wrote:
> > > On 2015/11/10 09:49:37, vabr (Chromium) wrote:
> > > > As of yesterday I can actually also review //components/autofill, so
> jumping
> > > in
> > > > place of Evan:
> > > > 
> > > > The change in form_autofill_util.cc LGTM.
> > > > Cheers,
> > > > Vaclav
> > > > 
> > > > On 2015/11/07 01:18:03, ramya.v wrote:
> > > > > Added owners of 
> > > > > form_autofill_util.cc
> > > > > url_canon_relative.cc
> > > > > 
> > > > > @Evan & Abarth
> > > > > Could you please take a look.
> > > > > 
> > > > > Thanks
> > > 
> > > @abarth
> > > Could you please review changes related to 
> > > url_canon_relative.cc
> > > 
> > > Thanks
> > 
> > @ramya I don't think abarth is actively reviewing chromium patches, maybe it
> is
> > better to find another reviewer.
> 
> isn't vabr@ an owner for form_autofill_util.cc

Indeed I am an owner of autofill, and I approved that change long ago (#19).
But I'm not an owner for //url.

Cheers,
Vaclav

[ramya.v, 2015-11-25 09:20:02]

On 2015/11/20 13:10:50, vabr (Chromium) wrote:
> On 2015/11/19 22:01:09, Evan Stade wrote:
> > On 2015/11/19 21:15:44, rwlbuis wrote:
> > > On 2015/11/13 03:32:05, ramya.v wrote:
> > > > On 2015/11/10 09:49:37, vabr (Chromium) wrote:
> > > > > As of yesterday I can actually also review //components/autofill, so
> > jumping
> > > > in
> > > > > place of Evan:
> > > > > 
> > > > > The change in form_autofill_util.cc LGTM.
> > > > > Cheers,
> > > > > Vaclav
> > > > > 
> > > > > On 2015/11/07 01:18:03, ramya.v wrote:
> > > > > > Added owners of 
> > > > > > form_autofill_util.cc
> > > > > > url_canon_relative.cc
> > > > > > 
> > > > > > @Evan & Abarth
> > > > > > Could you please take a look.
> > > > > > 
> > > > > > Thanks
> > > > 
> > > > @abarth
> > > > Could you please review changes related to 
> > > > url_canon_relative.cc
> > > > 
> > > > Thanks
> > > 
> > > @ramya I don't think abarth is actively reviewing chromium patches, maybe
it
> > is
> > > better to find another reviewer.
> > 
> > isn't vabr@ an owner for form_autofill_util.cc
> 
> Indeed I am an owner of autofill, and I approved that change long ago (#19).
> But I'm not an owner for //url.
> 
> Cheers,
> Vaclav


Friendly Ping!
@brettw or @abarth
Could you please take a look at the changes related to the file
url_canon_relative.cc
Thanks!

[ramya.v, 2015-11-27 11:00:39]

ramya.v@samsung.com changed reviewers:
+ tsepez@chromium.org

[ramya.v, 2015-11-27 11:00:39]

@Tom

Looks like brettw and abarth are not available.
Could you please review changes related to
url_canon_relative.cc file.

Thanks!

[Tom Sepez, 2015-11-27 21:30:33]

lgtm

[ramya.v, 2015-11-28 10:08:27]

The CQ bit was checked by ramya.v@samsung.com

[commit-bot: I haz the power, 2015-11-28 10:08:30]

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

[commit-bot: I haz the power, 2015-11-28 10:15:41]

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

[commit-bot: I haz the power, 2015-11-28 10:15:41]

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...)

[ramya.v, 2015-11-30 03:44:12]

On 2015/11/28 10:15:41, commit-bot: I haz the power wrote:
> 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...)



@Tom

Still getting the following error. Can you please suggest someone.

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    url/url_canon_relative.cc

[vabr (Chromium), 2015-11-30 10:02:53]

On 2015/11/30 03:44:12, ramya.v wrote:
> On 2015/11/28 10:15:41, commit-bot: I haz the power wrote:
> > 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...)
> 
> 
> 
> @Tom
> 
> Still getting the following error. Can you please suggest someone.
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     url/url_canon_relative.cc

@ramya.v,
Looking at //url/OWNERS, only brettw@ and abarth@ are there, and the next OWNERS
up is //OWNERS, which should be used for creating new directories rather than
overriding more specific ones.

My suggestion would be to tak these steps, in the following order:
 (1) Send an off-CL e-mail to brettw@ pinging him for a review here.
 (2) If there is no response in two days, try the same for abarth@.
 (3) If there is no response for two more days, send a short e-mail to
chromium-dev@chromium.org saying that the current OWNERS of //url might be too
busy, asking if there were potential new OWNERS which the top-level OWNERS were
happy to add to //url/OWNERS, and asking in particular if anybody from //OWNERS
could approve url/url_canon_relative.cc in this CL in the meantime.

I'm really sorry to see that you get so much trouble getting this CL approved.
Please do not let yourself discouraged by this, it is usually much faster. :)

If you want me to help with anything from above, just let me know, I'd happy to.

Thanks!
Vaclav

[ramya.v, 2015-11-30 11:42:35]

On 2015/11/30 10:02:53, vabr (Chromium) wrote:
> On 2015/11/30 03:44:12, ramya.v wrote:
> > On 2015/11/28 10:15:41, commit-bot: I haz the power wrote:
> > > 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...)
> > 
> > 
> > 
> > @Tom
> > 
> > Still getting the following error. Can you please suggest someone.
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     url/url_canon_relative.cc
> 
> @ramya.v,
> Looking at //url/OWNERS, only brettw@ and abarth@ are there, and the next
OWNERS
> up is //OWNERS, which should be used for creating new directories rather than
> overriding more specific ones.
> 
> My suggestion would be to tak these steps, in the following order:
>  (1) Send an off-CL e-mail to brettw@ pinging him for a review here.
>  (2) If there is no response in two days, try the same for abarth@.
>  (3) If there is no response for two more days, send a short e-mail to
> mailto:chromium-dev@chromium.org saying that the current OWNERS of //url might
be too
> busy, asking if there were potential new OWNERS which the top-level OWNERS
were
> happy to add to //url/OWNERS, and asking in particular if anybody from
//OWNERS
> could approve url/url_canon_relative.cc in this CL in the meantime.
> 
> I'm really sorry to see that you get so much trouble getting this CL approved.
> Please do not let yourself discouraged by this, it is usually much faster. :)
> 
> If you want me to help with anything from above, just let me know, I'd happy
to.
> 
> Thanks!
> Vaclav

Thanks Vaclav for your suggestion :) Will follow the same.

[brettw, 2015-11-30 17:46:18]

https://codereview.chromium.org/1409293007/diff/80001/url/url_canon_relative.cc
File url/url_canon_relative.cc (right):

https://codereview.chromium.org/1409293007/diff/80001/url/url_canon_relative....
url/url_canon_relative.cc:78: if (!is_base_hierarchical) {
We should get a test for this case in url_canon_unittest.cc. See the line:
  TEST(URLCanonTest, ResolveRelativeURL) {

[ramya.v, 2015-12-01 07:37:03]

@brett

Updated as per review comments. PTAL!

Thanks.

https://codereview.chromium.org/1409293007/diff/80001/url/url_canon_relative.cc
File url/url_canon_relative.cc (right):

https://codereview.chromium.org/1409293007/diff/80001/url/url_canon_relative....
url/url_canon_relative.cc:78: if (!is_base_hierarchical) {
On 2015/11/30 17:46:18, brettw wrote:
> We should get a test for this case in url_canon_unittest.cc. See the line:
>   TEST(URLCanonTest, ResolveRelativeURL) {

Done.

[brettw, 2015-12-01 17:44:02]

src/url LGTM

[ramya.v, 2015-12-02 00:16:09]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2015-12-02 00:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1409293007/#ps120001
(title: "Updated testcase")

[commit-bot: I haz the power, 2015-12-02 00:17:23]

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

[commit-bot: I haz the power, 2015-12-02 02:24:53]

Description was changed from

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this 
change, because empty action url is by default resolved to 
base url before this patch. Made appropriate code changes 
as empty action url can be equal to base url only for 
relative base urls.

BUG=463961
==========

to

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this 
change, because empty action url is by default resolved to 
base url before this patch. Made appropriate code changes 
as empty action url can be equal to base url only for 
relative base urls.

BUG=463961
==========

[commit-bot: I haz the power, 2015-12-02 02:24:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-02 02:25:39]

Description was changed from

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this 
change, because empty action url is by default resolved to 
base url before this patch. Made appropriate code changes 
as empty action url can be equal to base url only for 
relative base urls.

BUG=463961
==========

to

==========
new URL('') should throw TypeError

As per the spec https://url.spec.whatwg.org/#no-scheme-state
If base url is null or not relative and starting character of
url is not # return failure. So empty url should also throw
exception in this case.

Few tests related to autofill were failing after this
change, because empty action url is by default resolved to
base url before this patch. Made appropriate code changes
as empty action url can be equal to base url only for
relative base urls.

BUG=463961

Committed: https://crrev.com/56d42205139458d71b64a9b2f773b4445460e3a6
Cr-Commit-Position: refs/heads/master@{#362589}
==========

[commit-bot: I haz the power, 2015-12-02 02:25:40]

Patchset 7 (id:??) landed as
https://crrev.com/56d42205139458d71b64a9b2f773b4445460e3a6
Cr-Commit-Position: refs/heads/master@{#362589}