Intent to unship: xml:base attribute

[Xidorn Quan]

Tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=903372

Summary:
* It has been removed from the spec years ago.
* No other browser supports it.
* We pay performance penalty for it.
* It makes things trickier for stylo to handle URL values.

The tricky thing is that nsParserUtils::ParseFragment currently relies
on xml:base. That is going to be fixed in
https://bugzilla.mozilla.org/show_bug.cgi?id=1313278

Are there any objections?

- Xidorn

[Gijs Kruitbosch]

No objection, just a caveat that this looks like it will affect Reader
Mode, and there isn't great test coverage for this because Reader Mode
itself already does URL rewriting in addition to what parsefragment
does. bug 1280888 is relevant reading.

Furthermore, a question about how this relates to the HTML <base>
element - is that going to continue to be supported and/or does it imply
similar perf penalties / trickiness?

~ Gijs

[Gijs Kruitbosch]

On 16/02/2017 06:51, Xidorn Quan wrote:

[Xidorn Quan]

On Thu, Feb 16, 2017, at 10:25 PM, Gijs Kruitbosch wrote:
> On 16/02/2017 06:51, Xidorn Quan wrote:
> > Tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=903372
> >
> > Summary:
> > * It has been removed from the spec years ago.
> > * No other browser supports it.
> > * We pay performance penalty for it.
> > * It makes things trickier for stylo to handle URL values.
> >
> > The tricky thing is that nsParserUtils::ParseFragment currently relies
> > on xml:base. That is going to be fixed in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1313278
> >
> > Are there any objections?
>
> No objection, just a caveat that this looks like it will affect Reader
> Mode, and there isn't great test coverage for this because Reader Mode
> itself already does URL rewriting in addition to what parsefragment
> does. bug 1280888 is relevant reading.

Thanks for the pointer.

> Furthermore, a question about how this relates to the HTML <base>
> element - is that going to continue to be supported and/or does it imply
> similar perf penalties / trickiness?

This doesn't affect the <base> element. It would continue to be
supported. It doesn't have similar pref penalty given that it is a
document-wide URL.

The perf penalty of xml:base is basically that we have to dynamically
construct a URL from bottom to top along the tree whenever we need a
base URL of an element. And since this URL may be constructed
dynamically, the caller would have to hold a strong reference to it, and
thus an additional refcount manipulation. So a URL not dynamically
generated would be enough to avoid that penalty.

- Xidorn

[Boris Zbarsky]

On 2/16/17 7:12 AM, Xidorn Quan wrote:
> The perf penalty of xml:base is basically that we have to dynamically
> construct a URL from bottom to top along the tree whenever we need a
> base URL of an element. And since this URL may be constructed
> dynamically, the caller would have to hold a strong reference to it, and
> thus an additional refcount manipulation. So a URL not dynamically
> generated would be enough to avoid that penalty.

I assume this means that the Get/SetExplicitBaseURI API on nsINode is
still OK?

-Boris

[Ehsan Akhgari]

On 2017-02-16 1:51 AM, Xidorn Quan wrote:
> Tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=903372
>
> Summary:
> * It has been removed from the spec years ago.
> * No other browser supports it.
> * We pay performance penalty for it.
> * It makes things trickier for stylo to handle URL values.
>
> The tricky thing is that nsParserUtils::ParseFragment currently relies
> on xml:base. That is going to be fixed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1313278
>
> Are there any objections?

Do we have telemetry on its current usage? We've tried to remove Gecko
only features with the justification that since no other browser
supports it nobody probably uses it, and we have been proven wrong. It
seems like we need to have some information that shows this isn't used
in the wild before discussing whether we should remove it.

[Xidorn Quan]

Actually I'd like to remove it as well, since it still requires us to
climb the tree to get the base URI while its only usage can be done in a
much more efficient way. Actually I've filed a bug and been working on
removing it. [1]

Is there any reason you think we should keep the Get/SetExplicitBaseURI
API?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1340061

- Xidorn

[Xidorn Quan]

I don't think we have telemetry on that... We can probably add one, but
I'm not sure where is the best hook point of it. Is there telemetry for
any other attribute I can take as a reference?

A note is that ParseFragment's internal usages would need to be
carefully excluded from the telemetry.

- Xidorn

[Boris Zbarsky]

On 2/16/17 5:52 PM, Xidorn Quan wrote:
> Is there any reason you think we should keep the Get/SetExplicitBaseURI
> API?

As long as we can address its current users in some other way, no.

-Boris

[Ehsan Akhgari]

There are lots of telemetry probes, not quite sure what kind of example
you are looking for. Can you please be more specific?

In terms of what you want to measure, you should measure the places
where xml:base is currently used. I know of
<http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/dom/base/FragmentOrElement.cpp#383>
and
<http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/toolkit/components/feeds/FeedProcessor.js#1359>,
not 100% sure if that covers it all.

Usually the first step in measuring the usage is to define what you
actually want to measure. One immediate idea that comes to my mind is
what percentage of documents that we load use <xml:base> to define the
base URI. But there are many other ways to formulate the usage, so some
thought on that is necessary.

[Jet Villegas]

xml:base (bug 1349024) has been removed in Nightly 55 for 2 months
now, and we haven't sen any reports of ill effects. Let's have this
testing expand to Beta 55, and on to Release if all goes well. Bug
1350521 tracks this change riding the trains.

--Jet

On Thu, Feb 16, 2017 at 2:51 PM, Xidorn Quan <m...@upsuper.org> wrote:
> Tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=903372
>
> Summary:
> * It has been removed from the spec years ago.
> * No other browser supports it.
> * We pay performance penalty for it.
> * It makes things trickier for stylo to handle URL values.
>
> The tricky thing is that nsParserUtils::ParseFragment currently relies
> on xml:base. That is going to be fixed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1313278
>
> Are there any objections?
>

> - Xidorn
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Xidorn Quan]

On Wed, May 24, 2017, at 11:30 AM, Jet Villegas wrote:
> xml:base (bug 1349024) has been removed in Nightly 55 for 2 months
> now, and we haven't sen any reports of ill effects. Let's have this
> testing expand to Beta 55, and on to Release if all goes well. Bug
> 1350521 tracks this change riding the trains.

To be accurate, xml:base isn't removed (because the telemetry number
doesn't look quite good). Instead, we are making xml:base no longer
affect style attribute. And this is what has been done, and what we want
to expand the testing for.

- Xidorn