Add a telemetry probe for whether showModalDialog is used

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed, firefox-esr3138+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

For bug 981796, we should probably figure out how often showModalDialog is used on the web.

Given that Chrome already ripped this out, there shouldn't be too many uses out there.
Posted patch patchSplinter Review
Attachment #8569559 - Flags: review?(jst)
Attachment #8569559 - Flags: review?(jst) → review+
It would probably be a good idea to backport this patch to ESR as well, and get some data from that channel.  When Blink removed this API, a lot of the objections came from enterprise intranet apps that used this.
Comment on attachment 8569559 [details] [diff] [review]
patch

Approval Request Comment
[Risks and why]: No risk. It would be good to get as much telemetry data on this on the ESR branches as possible.
Attachment #8569559 - Flags: approval-mozilla-aurora?
Posted patch Patch for esr24 (obsolete) — Splinter Review
This was an easy merge.
Attachment #8570239 - Flags: review+
Comment on attachment 8570239 [details] [diff] [review]
Patch for esr24

This isn't needed.
Attachment #8570239 - Attachment is obsolete: true
Comment on attachment 8569559 [details] [diff] [review]
patch

[Approval Request Comment]
Please see comment 3 and comment 4.
Attachment #8569559 - Flags: approval-mozilla-esr31?
Triage drive-by: will wait for this to land on central before approving uplift.
https://hg.mozilla.org/mozilla-central/rev/20d0b5cb2071
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8569559 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8569559 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The telemetry for DOM_WINDOW_SHOWMODALDIALOG_USED for beta 38 right now shows usage in 1.6% of sessions. (I think that's what it means.)
That's terrible :(
Blake, how difficult would it be to implement showModalDialog for e10s?
Probably not terrible, but I'd really like to avoid it if at all possible.

Andrew, what's the policy about stuff like this? This API has been slated for removal for a while now. Clearly it's still used on more than a few sites in the wild, but it isn't terribly popular.
Flags: needinfo?(overholt)
Is it possible that some addons or even chrome code uses showModalDialog and we're counting that too?

1.6% is very high number, we can't really think of removing API used that often.
(In reply to Olli Pettay [:smaug] from comment #17)
> Is it possible that some addons or even chrome code uses showModalDialog and
> we're counting that too?

At a glance, I don't see any in mozilla-central outside of testing code.  I do 145 uses of it in addons MXR, in around maybe 60 addons.  I don't know how popular those addons are.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #16)
> Probably not terrible, but I'd really like to avoid it if at all possible.
> 
> Andrew, what's the policy about stuff like this? This API has been slated
> for removal for a while now. Clearly it's still used on more than a few
> sites in the wild, but it isn't terribly popular.

Yeah 1.6% is way too much. Modulo the e10s piece, showModalDialog is much less problematic for us than it was for Chrome (I can explain why offline), so I don't think we need to hurry it.

Put another way, it was never a beautiful web feature. We implemented it (and paid a cost) for a compat win, and I don't think we should take a compat hit like that by removing it too early.

There may be more to this story though, and it would be nice to have more data to get a clearer picture of what's going on.
Can we get a different telemetry probe based on whether the function is accessed from chrome or content to see if this coming from some popular add-ons, or from actual websites?
(In reply to Bobby Holley (:bholley) from comment #19)
> showModalDialog is much
> less problematic for us than it was for Chrome (I can explain why offline),

Oh, I guess this is public now:

https://code.google.com/p/chromium/issues/detail?id=350535

Basically, showModalDialog was the only place that Blink spun the event loop, whereas Gecko spins it in a lot of places. They also handled it much worse than Gecko do, so it was a lot more of a win for them to get rid of it.
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #16)
> Andrew, what's the policy about stuff like this? This API has been slated
> for removal for a while now. Clearly it's still used on more than a few
> sites in the wild, but it isn't terribly popular.

We don't have an explicit policy for cases like this, sorry.
Flags: needinfo?(overholt)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.