Spec compliant url.search/url.hash setters over empty values.

Spec compliant url.search/url.hash setters over empty values.

Setting the URLUtils properties 'search' and 'hash' to empty strings
should result in null query and hash URL components, respectively.

R=arv
BUG=410395
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181410

[sof, 2014-09-03 21:41:18]

sigbjornf@opera.com changed reviewers:
+ arv@chromium.org

[sof, 2014-09-03 21:41:19]

Please take a look.

[arv (Not doing code reviews), 2014-09-03 22:08:36]

Can we move this logic to KURL instead?

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:125: if (value[0] == '?')
Do we need to handle length 0 before this test?

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:126: kurl.setQuery(value.length() == 1 ?
String() : value.substring(1));
I thought KURL::setQuery did this?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[sof, 2014-09-04 05:18:39]

On 2014/09/03 22:08:36, arv wrote:
> Can we move this logic to KURL instead?
> 

Possibly. That has a wider risk of running to assumed-quirks elsewhere though.

[sof, 2014-09-04 06:52:24]

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
File Source/core/dom/DOMURLUtils.cpp (right):

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:125: if (value[0] == '?')
On 2014/09/03 22:08:36, arv wrote:
> Do we need to handle length 0 before this test?

No real need, [] returns 0 over out-of-bounds indices (cf. setHash().) Too
implicit?

https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUt...
Source/core/dom/DOMURLUtils.cpp:126: kurl.setQuery(value.length() == 1 ?
String() : value.substring(1));
On 2014/09/03 22:08:36, arv wrote:
> I thought KURL::setQuery did this?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That handling strips the leading '?', but doesn't clear the query for "?" (as it
should per http://url.spec.whatwg.org/#dom-url-search )

[sof, 2014-09-04 09:09:33]

On 2014/09/04 05:18:39, sof wrote:
> On 2014/09/03 22:08:36, arv wrote:
> > Can we move this logic to KURL instead?
> > 
> 
> Possibly. That has a wider risk of running to assumed-quirks elsewhere though.

Having "" clear the query portion runs into
https://bugs.webkit.org/show_bug.cgi?id=30103 (it might ring a distant bell.)

It is also not compatible to implement the per-spec handling of hash over
Location and have it clear the fragment on setting "". I've added
fast/dom/location-hash.html coverage for that.

I suggest we don't try to solve these thorny issues here.

[arv (Not doing code reviews), 2014-09-04 15:59:10]

LGTM

I agree that this CL is better kept focusing on the issue that this solves.

I still think we have a great opportunity to simplify this code now that we only
use GoogleURL.

[sof, 2014-09-04 18:35:32]

On 2014/09/04 15:59:10, arv wrote:
> LGTM
> 
> I agree that this CL is better kept focusing on the issue that this solves.
> 
> I still think we have a great opportunity to simplify this code now that we
only
> use GoogleURL.

Thanks, agreed on both counts. I left behind a pair of FIXMEs, mentioning
http://crbug.com/108690

[sof, 2014-09-04 18:35:37]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-04 18:36:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/537103002/60001

[commit-bot: I haz the power, 2014-09-04 19:36:26]

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

[commit-bot: I haz the power, 2014-09-04 19:36:28]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[sof, 2014-09-04 20:48:29]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-04 20:49:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/537103002/80001

[commit-bot: I haz the power, 2014-09-04 22:49:16]

Committed patchset #5 (id:80001) as 181410