Don't resolve non-local -webkit-clip-path references as local

Don't resolve non-local -webkit-clip-path references as local

Whether the reference was local or not was disregarded, meaning that
any URL with a valid fragment part could potential result in a valid
clip - regardless of whether it was local or not. I.e foo.svg#bar would
reference an element with the id 'bar' in the local document.

Check if the URL reference resolver flagged the reference as being local,
and only pass a non-null fragment if it was.

BUG=629826
Committed: https://crrev.com/d9357efcc40d785828b457ce7d6a020691593e99
Cr-Commit-Position: refs/heads/master@{#407300}

[fs, 2016-07-22 15:09:49]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 15:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-07-22 15:12:12]

Description was changed from

==========
Don't resolve non-local -webkit-clip-path references as local

Whether the reference was local or not was disregarded, meaning that
any URL with a valid fragment part could potential result in a valid
clip - regardless of whether it was local or not. I.e foo.svg#bar would
reference an element with the id 'bar' in the local document.

Check the resolved URL against the document URL, and clear the fragment
if it doesn't match.

BUG=629826
==========

to

==========
Don't resolve non-local -webkit-clip-path references as local

Whether the reference was local or not was disregarded, meaning that
any URL with a valid fragment part could potential result in a valid
clip - regardless of whether it was local or not. I.e foo.svg#bar would
reference an element with the id 'bar' in the local document.

Check if the URL reference resolver flagged the reference as being local,
and only pass a non-null fragment if it was.

BUG=629826
==========

[fs, 2016-07-22 16:09:41]

fs@opera.com changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[commit-bot: I haz the power, 2016-07-22 16:30:11]

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

[commit-bot: I haz the power, 2016-07-22 16:30:12]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-07-22 16:45:45]

I'm uncomfortable with a test that is expected to not find the right reference
in order to verify that we do not find the wrong reference. I recognize that my
discomfort is inevitable given we don't support external SVG references.

Maybe pretend we do support external references for the purposes of writing the
test? That is, create the external document and put some clip in it. Then note
in the expected result that it should change when we fix the external references
problem?

[fs, 2016-07-22 17:27:17]

On 2016/07/22 at 16:45:45, schenney wrote:
> I'm uncomfortable with a test that is expected to not find the right reference
in order to verify that we do not find the wrong reference. I recognize that my
discomfort is inevitable given we don't support external SVG references.
> 
> Maybe pretend we do support external references for the purposes of writing
the test? That is, create the external document and put some clip in it. Then
note in the expected result that it should change when we fix the external
references problem?

But then we'll have a test with an unclear pass condition - i.e an invalid
reference should never result in a valid reference. If the file referenced
actually existed we'd be testing that external references doesn't work (which
they don't in this case.) I guess we could reference a raster image instead, so
that the resource exist, but lacks the relevant fragment semantics? (I guess
that could turn valid eventually too though...)

[Stephen Chennney, 2016-07-22 17:43:28]

On 2016/07/22 17:27:17, fs wrote:
> On 2016/07/22 at 16:45:45, schenney wrote:
> > I'm uncomfortable with a test that is expected to not find the right
reference
> in order to verify that we do not find the wrong reference. I recognize that
my
> discomfort is inevitable given we don't support external SVG references.
> > 
> > Maybe pretend we do support external references for the purposes of writing
> the test? That is, create the external document and put some clip in it. Then
> note in the expected result that it should change when we fix the external
> references problem?
> 
> But then we'll have a test with an unclear pass condition - i.e an invalid
> reference should never result in a valid reference. If the file referenced
> actually existed we'd be testing that external references doesn't work (which
> they don't in this case.) I guess we could reference a raster image instead,
so
> that the resource exist, but lacks the relevant fragment semantics? (I guess
> that could turn valid eventually too though...)

I get your point that we are also testing that invalid references do not cause
problems. And I was always hesitant about the fact that the reference image
would be deliberately wrong according to spec. Considering all that, I'll get
over my angst about how broken this all is and just say LGTM.

[fs, 2016-07-22 17:47:18]

On 2016/07/22 at 17:43:28, schenney wrote:
> On 2016/07/22 17:27:17, fs wrote:
> > On 2016/07/22 at 16:45:45, schenney wrote:
> > > I'm uncomfortable with a test that is expected to not find the right
reference
> > in order to verify that we do not find the wrong reference. I recognize that
my
> > discomfort is inevitable given we don't support external SVG references.
> > > 
> > > Maybe pretend we do support external references for the purposes of
writing
> > the test? That is, create the external document and put some clip in it.
Then
> > note in the expected result that it should change when we fix the external
> > references problem?
> > 
> > But then we'll have a test with an unclear pass condition - i.e an invalid
> > reference should never result in a valid reference. If the file referenced
> > actually existed we'd be testing that external references doesn't work
(which
> > they don't in this case.) I guess we could reference a raster image instead,
so
> > that the resource exist, but lacks the relevant fragment semantics? (I guess
> > that could turn valid eventually too though...)
> 
> I get your point that we are also testing that invalid references do not cause
problems. And I was always hesitant about the fact that the reference image
would be deliberately wrong according to spec. Considering all that, I'll get
over my angst about how broken this all is and just say LGTM.

What do you mean by "reference image" above?

[Stephen Chennney, 2016-07-22 18:24:37]

> What do you mean by "reference image" above?

Sorry, I should have said "expected result".

That is, if we added an external SVG that we should find and use but don't, the
expected result checked in would be wrong according to spec (because we don't
implement the spec). Remarkably confusing thing to try to express clearly.

[fs, 2016-07-22 23:34:32]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-07-22 23:35:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 23:38:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-22 23:40:33]

Description was changed from

==========
Don't resolve non-local -webkit-clip-path references as local

Whether the reference was local or not was disregarded, meaning that
any URL with a valid fragment part could potential result in a valid
clip - regardless of whether it was local or not. I.e foo.svg#bar would
reference an element with the id 'bar' in the local document.

Check if the URL reference resolver flagged the reference as being local,
and only pass a non-null fragment if it was.

BUG=629826
==========

to

==========
Don't resolve non-local -webkit-clip-path references as local

Whether the reference was local or not was disregarded, meaning that
any URL with a valid fragment part could potential result in a valid
clip - regardless of whether it was local or not. I.e foo.svg#bar would
reference an element with the id 'bar' in the local document.

Check if the URL reference resolver flagged the reference as being local,
and only pass a non-null fragment if it was.

BUG=629826

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

[commit-bot: I haz the power, 2016-07-22 23:40:35]

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