1137025 - Add a telemetry probe for whether showModalDialog is used

Status: RESOLVED FIXED

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

Description

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=324fdaa4a441

Comment 3

[(no longer active)]

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 4

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for esr24

This was an easy merge.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8570239 [details] [diff] [review]
Patch for esr24

This isn't needed.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8569559 [details] [diff] [review]
patch

[Approval Request Comment]
Please see comment 3 and comment 4.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: will wait for this to land on central before approving uplift.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20d0b5cb2071

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20d0b5cb2071

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/a24e0b849e64

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7843c91294

Comment 13

[Andrew McCreight [:mccr8]]

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.)

Comment 14

[(no longer active)]

That's terrible :(

Comment 15

[Bobby Holley (:bholley)]

Blake, how difficult would it be to implement showModalDialog for e10s?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

(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.

Comment 19

[Bobby Holley (:bholley)]

(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.

Comment 20

[(no longer active)]

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?

Comment 21

[Bobby Holley (:bholley)]

(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.

Comment 22

[Andrew Overholt [:overholt]]

(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.