784467 - switch computed style's url(invalid-url:) to [css3-values]'s url(about:invalid)

Status: RESOLVED FIXED

Description

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

In computed style, for an invalid URL, nsROCSSPrimitiveValue.cpp sometimes generates url(invalid-url:), because we had to generate something for invalid URLs.  http://dev.w3.org/csswg/css3-values/ describes url(about:invalid) for a similar purpose, so we should switch this to being about:invalid instead.

Comment 1

[Emmanuele Bassi (:ebassi)]

Created attachment 659681 [details] [diff] [review]
Trivial patch switching from url(invalid-url:) to url(about:invalid)

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 659681 [details] [diff] [review]
Trivial patch switching from url(invalid-url:) to url(about:invalid)

Perhaps add a comment saying something like:

  // http://dev.w3.org/csswg/css3-values/#attr-notation defines
  // about:invalid for a similar purpose, so let's also use it
  // here as the computed value for invalid URLs.

Though I wonder if it's an issue that this *does* round trip to something meaningful.  (Might we need to make about:invalid produce a particular empty document?)

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Spec says:

  The about:invalid URI references a non-existent document with a generic error condition.
  It can be used when a URI is necessary, but the default value shouldn't be resolveable
  as any type of document. 

So I think the answer to the last part of comment 2 is "no".

Comment 4

[Emmanuele Bassi (:ebassi)]

Created attachment 660097 [details] [diff] [review]
new patch, updated after review

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 660097 [details] [diff] [review]
new patch, updated after review

When I asked if it was an issue, I was more worried about "for the Web" than "according to the spec".

But I suppose let's go ahead and do this, even though it's a little weird.

Comment 6

[Emmanuele Bassi (:ebassi)]

setting the checkin-needed keyword as I don't have the level 2 commit bit yet

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I don't see a Try link here, so I've pushed it myself. I'll land this if it's green.
https://tbpl.mozilla.org/?tree=Try&rev=e29c3fd60ea9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c01e44583414

Should this have a test?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c01e44583414