Closed Bug 1340926 Opened 3 years ago Closed 3 years ago

Add telemetry for xml:base attribute

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: site-compat)

Attachments

(4 files, 1 obsolete file)

We want to measure the impact of removing xml:base attribute before actually committing.
Assignee: nobody → xidorn+moz
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

It may need approval from a data collection peer. Francois, could you review this? It is simply adding a deprecated operation use counter.
Attachment #8839008 - Flags: feedback?(francois)
I intend to request uplifting all the patches up to 52beta so that we can get enough data for it as soon as possible.

Part 1 and part 3 shouldn't have much risk, but part 2 which slightly changes how use counter of deprecated operations works may be a little risky...
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

https://reviewboard.mozilla.org/r/113762/#review115442

datareview+
Attachment #8839008 - Flags: review+
Attachment #8839008 - Flags: feedback?(francois)
Comment on attachment 8839006 [details]
Bug 1340926 part 1 - Make nsDocument::IsAboutPage usable in const function.

https://reviewboard.mozilla.org/r/113758/#review115664
Attachment #8839006 - Flags: review?(ehsan) → review+
Comment on attachment 8839007 [details]
Bug 1340926 part 2 - Avoid record deprecated operation inside about: pages.

https://reviewboard.mozilla.org/r/113760/#review115666

OK, but which about: page are you explicitly worried about?  Is it possible that we fix the said page instead?  At least please clarify what this is really working around.
Attachment #8839007 - Flags: review?(ehsan) → review+
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

https://reviewboard.mozilla.org/r/113762/#review115670

Please reword the commit message to something like "Deprecate usage of xml:base" since that is what the patch is really doing.  The use counter telemetry is a side effect of that.
Attachment #8839008 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #11)
> OK, but which about: page are you explicitly worried about?  Is it possible
> that we fix the said page instead?  At least please clarify what this is
> really working around.

I'm especially worried about about:feeds and about:reader. Bug 1313278 would fix those pages, but there are risks that the patch there may break things, so I'd rather having it ride the train.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b08129753f55
part 1 - Make nsDocument::IsAboutPage usable in const function. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/8f893ce1a845
part 2 - Avoid record deprecated operation inside about: pages. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/be69c846233d
part 3 - Deprecate usage of xml:base. r=Ehsan,francois
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

Approval Request Comment
[Feature/Bug causing the regression]: not a regression
[User impact if declined]: no, but I want to have this uplifted to collect information for unblocking bug 903372
[Is this code covered by automated tests?]: should be covered by tests of telemetry
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: just all the tree patches here
[Is the change risky?]: part 1 and part 3 are not risky, part 2 could be a little risky.
[Why is the change risky/not risky?]: part 1 is a trivial change, and part 3 just adds a new deprecation operation to the list, so they are not risky. part 2 slightly changes how use counter for deprecation operations works, so a little risky.
[String changes made/needed]: a new string is added for message of the new deprecation.
Attachment #8839008 - Flags: approval-mozilla-beta?
Attachment #8839008 - Flags: approval-mozilla-aurora?
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/xml-base-attribute-has-been-deprecated/ (will change the version when the patch is uplifted)
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

too late for 52, especially with a string change
Attachment #8839008 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Julien Cristau [:jcristau] from comment #21)
> Comment on attachment 8839008 [details]
> Bug 1340926 part 3 - Deprecate usage of xml:base.
> 
> too late for 52, especially with a string change

The new string is just a warning shown in console in rare cases, so I don't think it is a big issue if it is not translated properly.
Hi :flod,
There is a string change, are you ok with it?
Flags: needinfo?(francesco.lodolo)
(In reply to Gerry Chang [:gchang] from comment #23)
> Hi :flod,
> There is a string change, are you ok with it?

Not really. If you need this on aurora, please create a patch without string changes (hard-code the message).

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #22)
> The new string is just a warning shown in console in rare cases, so I don't
> think it is a big issue if it is not translated properly.

It's not a matter of translated or not. Landing a string change will create unnecessary noise in tools, in particular invalidate all existing sign-offs on aurora a week from the end of the cycle. It might seem trivial from an external point of view, but it currently isn't (there's ongoing work to improve this).
Flags: needinfo?(francesco.lodolo)
I think it is fine to simply strip the change to dom/locales/en-US/chrome/dom/dom.properties.
Attached patch part 3 for uplift (obsolete) — Splinter Review
Approval Request Comment
[Feature/Bug causing the regression]: not a regression
[User impact if declined]: no, but I want to have this uplifted to collect information for unblocking bug 903372
[Is this code covered by automated tests?]: should be covered by tests of telemetry
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: just all the tree patches here
[Is the change risky?]: part 1 and part 3 are not risky, part 2 could be a little risky.
[Why is the change risky/not risky?]: part 1 is a trivial change, and part 3 just adds a new deprecation operation to the list, so they are not risky. part 2 slightly changes how use counter for deprecation operations works, so a little risky.
[String changes made/needed]: no, removed.
Attachment #8840756 - Flags: approval-mozilla-aurora?
Comment on attachment 8839008 [details]
Bug 1340926 part 3 - Deprecate usage of xml:base.

Collect the telemetry data of removing xml:base attribute. We can take it in aurora. Aurora53+. NI tomcat for the part 3 patch.
Flags: needinfo?(cbook)
Attachment #8839008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8840756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi xidorn, i tried to import part1,2 and the uplift part 3 and got

Hunk #1 FAILED at 45
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsDeprecatedOperationList.h.rej
abort: patch failed to apply

could you take a look (maybe needs a rebase ?
Flags: needinfo?(cbook) → needinfo?(xidorn+moz)
Attachment #8840756 - Attachment is obsolete: true
Flags: needinfo?(xidorn+moz)
Blocks: 1344102
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #31)
> I've added a note about this to the relevant release notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/53#HTMLXML

"The xml:base attribute has been removed"
This is not yet true, see bug 903372, see also bug 1349024.
(In reply to j.j. from comment #32)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #31)
> > I've added a note about this to the relevant release notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/53#HTMLXML
> 
> "The xml:base attribute has been removed"
> This is not yet true, see bug 903372, see also bug 1349024.

OK, thanks. I've removed the note again. Both the cited bugs have dev-doc-needed set on them, so I'll follow those instead, and add the information back to the release notes when they are fixed and time comes.

I've removed the dev-doc-needed keyword from this bug - we don't document xml:base anywhere on MDN, and a deprecation doesn't require a release note addition.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.