Closed Bug 1340926 Opened 9 years ago Closed 9 years ago

Add telemetry for xml:base attribute

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: site-compat)

Attachments

(4 files, 1 obsolete file)

Bug 1340926 part 1 - Make nsDocument::IsAboutPage usable in const function.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1340926 part 2 - Avoid record deprecated operation inside about: pages.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1340926 part 3 - Deprecate usage of xml:base.
59 bytes, text/x-review-board-request
ehsan.akhgari
: review+
gchang
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta-
Details
part 3 for uplift
2.94 KB, patch
Details | Diff | Splinter Review
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)
Keywords: dev-doc-needed, site-compat
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
I've added a note about this to the relevant release notes: https://developer.mozilla.org/en-US/Firefox/Releases/53#HTMLXML
Keywords: dev-doc-needed → dev-doc-complete
(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.
Keywords: dev-doc-complete → dev-doc-needed
(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.

Attachment

General

Creator:
Created:
Updated:
Size: