adjust blink::equalIgnoringFragmentIdentifier to work better for data:

adjust blink::equalIgnoringFragmentIdentifier to work better for data:

According to at least one source I can find[1] fragment identifiers are not defined for data: urls. Therefore, theoretically, "equal ignoring fragment identifier" is equivalent to "equal" when comparing two data: urls.

Currently, the function returns true for the following pair of inputs:
  - data:text/html;charset=utf-8,<!-- http://www.example.com/demo#a -->foo
  - data:text/html;charset=utf-8,<!-- http://www.example.com/demo#a -->ba
But these are clearly not actually equal.

This is causing issues for Chrome Autofill because it relies on this function to determine if a navigation was to a new page, triggering a re-parsing of the forms on the page. In practice, this won't come up very often because it's not particularly likely that a user will navigate between two data: URLs that trigger this bug. The issue was discovered because of a failing unit test which happened to navigate between two data URLs that were incorrectly identified as identical.

blink::equalIgnoringFragmentIdentifier is used in many places throughout Blink, so this change has the potential to cause unexpected impact. However, from my survey of a few existing uses, it doesn't seem likely that any piece of code expects the above two URLs to be considered "equal".

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=243917#c6

BUG=417922, 323093

[Evan Stade, 2014-09-26 00:09:12]

estade@chromium.org changed reviewers:
+ abarth@chromium.org

[Evan Stade, 2014-09-26 00:10:47]

Here's my stab in the dark at fixing this bug (perhaps instead of changing this
function, m_parsed should be smarter about data urls?)

I can add tests if this lgty.

[Evan Stade, 2014-09-29 15:30:30]

estade@chromium.org changed reviewers:
+ jamesr@chromium.org

[Evan Stade, 2014-09-29 15:30:31]

+jamesr

[jamesr, 2014-09-29 22:29:13]

jamesr@chromium.org changed reviewers:
- jamesr@chromium.org

[jamesr, 2014-09-29 22:29:13]

KURL is more Adam's bag, I don't know enough about the details of URLs to give a
high quality review here.

[Evan Stade, 2014-10-13 18:32:51]

ping abarth

[abarth-chromium, 2014-10-13 20:17:22]

This CL lacks a description of the problem you're solving and lacks a test. 
It's not clear to me whether this change is the right solution because I don't
know what problem you're trying to solve.

[Evan Stade, 2014-10-13 20:42:04]

On 2014/10/13 20:17:22, abarth wrote:
> This CL lacks a description of the problem you're solving and lacks a test. 
> It's not clear to me whether this change is the right solution because I don't
> know what problem you're trying to solve.

There are more details in the bug report. See
https://code.google.com/p/chromium/issues/detail?id=417922

[Evan Stade, 2014-10-13 20:43:11]

On 2014/10/13 20:42:04, Evan Stade wrote:
> On 2014/10/13 20:17:22, abarth wrote:
> > This CL lacks a description of the problem you're solving and lacks a test. 
> > It's not clear to me whether this change is the right solution because I
don't
> > know what problem you're trying to solve.
> 
> There are more details in the bug report. See
> https://code.google.com/p/chromium/issues/detail?id=417922

Also see comment 2 on this CL: "I can add tests if this lgty."

[abarth-chromium, 2014-10-13 23:28:28]

abarth@chromium.org changed reviewers:
- abarth@chromium.org

[abarth-chromium, 2014-10-13 23:29:18]

I've removed myself as a reviewer.

[Evan Stade, 2014-10-14 00:14:07]

estade@chromium.org changed reviewers:
+ abarth@chromium.org

[Evan Stade, 2014-10-14 00:14:07]

+abarth again

Apologies if the above offended you. I gather the description at [1] is
insufficient to give you context for the purpose of this change. Could you help
me by letting me know how it's unclear and could be improved? If you don't have
time for this review, can you recommend another reviewer?

[1] https://code.google.com/p/chromium/issues/detail?id=417922

[abarth-chromium, 2014-10-14 00:59:00]

On 2014/10/14 at 00:14:07, estade wrote:
> Apologies if the above offended you. I gather the description at [1] is
insufficient to give you context for the purpose of this change. Could you help
me by letting me know how it's unclear and could be improved? If you don't have
time for this review, can you recommend another reviewer?

I'll be happy to review this CL if you update the description of the CL to
explain (1) what problem you're solving and (2) why this change is the correct
one to solve that problem.  In addition, you'll need to include tests that
demonstrate that the problem described in (1) no longer occurs after your
change.  Ideally, your CL description would also include an analysis of how
other browsers behave in these situations.

I don't particularly want to review this change.  The intersection of data URLs
and fragment identifiers has a long, sad story that involves hard feelings
between many people in the world.  Our current behavior is a careful balance
between these competing ideologies and the practical reality of shipping a
browser that works.

It's unclear from digging around in the information you've linked whether your
CL (A) is attempting to improve theoretical purity of this issue according to
one of these ideological standpoints, (B) is attempting to improve how this code
functions in practice (e.g., makes a specific website work better than it did
previously or makes our behavior better match another browser), or (C) is
attempting to repair damage that we've caused previously in making changes in
pursuit of (A) or (B).

In summary, this CL provide no information upon which to judge whether the
codebase is improved or damaged by this change.

[Evan Stade, 2014-10-14 22:38:42]

updated description and added tests. ptal.

[abarth-chromium, 2014-10-15 01:49:01]

Was this a regression from https://codereview.chromium.org/23835019?

[Evan Stade, 2014-10-15 21:38:15]

On 2014/10/15 01:49:01, abarth wrote:
> Was this a regression from https://codereview.chromium.org/23835019

That seems quite likely. That patch landed Nov 22, 2013 and the Autofill test
was disabled on Nov 24, 2013.

[abarth-chromium, 2014-10-16 00:45:11]

It sounds like android_webview and the autofill code have different
philosophical positions as to whether data URLs have fragments.  For a long time
in Chrome, we took the position that they did not.  To fix android_webview
compat issues, we made them have fragments in that CL.

I'm not sure it makes sense to change this one specific function to pretend like
data URLs don't have fragments.  Instead, Chrome should have a coherent position
on that question.  That means the autofill code (maybe just tests?) needs to get
on the same page as android_webview.  Given the compat constraints from
android_webview, I think that means updating the autofill code (maybe just
tests?) to work in a world where data URLs have fragments.

not lgtm

[Evan Stade, 2014-10-16 00:53:53]

On 2014/10/16 00:45:11, abarth wrote:
> It sounds like android_webview and the autofill code have different
> philosophical positions as to whether data URLs have fragments.  For a long
time
> in Chrome, we took the position that they did not.  To fix android_webview
> compat issues, we made them have fragments in that CL.
> 
> I'm not sure it makes sense to change this one specific function to pretend
like
> data URLs don't have fragments.  Instead, Chrome should have a coherent
position
> on that question.  That means the autofill code (maybe just tests?) needs to
get
> on the same page as android_webview.  Given the compat constraints from
> android_webview, I think that means updating the autofill code (maybe just
> tests?) to work in a world where data URLs have fragments.
> 
> not lgtm

What is the definition of a fragment in a data URL? Currently it appears to be
"everything after the last hash mark". But it would surprise me if a hash mark
inside an HTML comment block was considered a valid delimiter for a fragment
identifier.

[abarth-chromium, 2014-10-16 01:00:35]

On 2014/10/16 at 00:53:53, estade wrote:
> What is the definition of a fragment in a data URL?

The folks in the "data URIs have fragments" camp claim that every URI,
regardless of scheme, is controlled by RFC3986 and that, therefore, the fragment
portion of a data URL is that part which matches part 9 of the regular
expression in

http://tools.ietf.org/html/rfc3986#appendix-B

> Currently it appears to be "everything after the last hash mark". But it would
surprise me if a hash mark inside an HTML comment block was considered a valid
delimiter for a fragment identifier.

I agree with you, which is why for many years Chrome believed that data URI
don't have fragments.  However, the practical argument that not recognizing
fragments breaks Android apps that use android_webview won out and now Chrome
does believe that data URIs have fragments.

As I wrote above, this topic has a long, sad story that involves hard feelings
between many people in the world.

[Evan Stade, 2014-10-16 01:04:33]

OK, thanks for the explanation.