255279 - show timer for enablePrivilege dialog delay

Status: RESOLVED WONTFIX

(Core :: Security, defect)

Description

[Ryan Polk (Quark)]

Ben Goodger: "I'd really prefer to see the countdown shown in some way, so that
the user doesn't immediately wonder what's happened..."

XPInstall already has this, so it shouldn't be too hard to implement this in
commonDialog.js for the Suite and for Firefox.

Comment 1

[Ryan Polk (Quark)]

Attachment: testcase

stolen from another bug to easily test the patch.

Comment 2

[Ryan Polk (Quark)]

Attachment: xpfe aviary patch

I only have an aviary tree, and I have a working toolkit patch but it requires
another patch to land.	So this is just a copy of the toolkit patch for xpfe,
since that has the requisite patch landed already.  Might as well start the
critique now.

I'll post toolkit's patch once the other one is landed.

Comment 3

[Ryan Polk (Quark)]

Comment on attachment 155879 [details] [diff] [review]
xpfe aviary patch

I'm not happy about the globals, but don't know of another way to do it.

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 155879 [details] [diff] [review]
xpfe aviary patch

I'm not fond of the counting down thing so I'll punt the review to Ben who
wanted it.

One thing though: if the delay is not an even number of seconds (e.g. I was
originally thinking of 2500ms as the default value) then it gets wonky. The
setInterval is always 1000 regardless of how much delay is left so 2005 would
round up to a full three second delay. That's not so bad, but the button text
would say (2.005), (1.005) and (0.005) with that last hanging around for a full
second just in case people missed it :-)

Comment 5

[Jesse Ruderman]

Comment on attachment 155879 [details] [diff] [review]
xpfe aviary patch

> -    var delayInterval = 2000;
> +    gDelayInterval = 2000;

You missed an instance when you renamed this variable.

> +    string = gCommonDialogParam.GetString(11) + " (" + (gDelayInterval/1000) + ')';

I don't know if hard-coding the parens and the order screws up localization, or
if that code is OK for all languages.

> -    setTimeout(commonDialogReenableButtons, delayInterval);
> +    gCommonDialogInterval = setInterval("commonDialogReenableButtons()", 1000);

Any reason to pass a string instead of a function?  I prefer passing functions,
because it's simpler and makes code easier to audit for security holes.

> I'm not happy about the globals, but don't know of another way to do it.

Use setTimeout instead of setInterval, eliminating one of the global variables,
and give commonDialogReenableButtons an argument of how many milliseconds are
left, eliminating the other.

(Another solution: If you moved commonDialogReenableButtons inside
showControls, variables in commonDialogReenableButtons would still be in scope
and you wouldn't have to use globals.  But that would be a little ugly.)

>  function commonDialogReenableButtons()

You should rename this function since with your patch, it sometimes changes the
labels instead of reenabling the buttons.  Or better yet, make re-enabling the
buttons and updating the labels separate functions.

> +    string = gCommonDialogParam.GetString(11) + " (" + (gDelayInterval/1000) + ')';
> +    setLabelForNode(document.documentElement.getButton("extra2"), string);
> +    string = gCommonDialogParam.GetString(10) + " (" + (gDelayInterval/1000) + ')';
> +    setLabelForNode(document.documentElement.getButton("extra1"), string);
> +    string = gCommonDialogParam.GetString(8) + " (" + (gDelayInterval/1000) + ')';
> +    setLabelForNode(document.documentElement.getButton("accept"), string);

Instead of duplicating this block from commonDialogReenableButtons in
showControls, have showControls call commonDialogReenableButtons immediately.

Comment 6

[Ryan Polk (Quark)]

> I don't know if hard-coding the parens and the order screws up localization, or
> if that code is OK for all languages.

The way I see it used elsewhere is by .getFormattedString().  But that uses a
stringbundle, and for some reason commonDialog doesn't get its .properties
localizations using a stringbundle.  Is there a reason for this?

Looking a little further, XPInstall has two seperate localizations, one for a
disabled label and one that's enabled.  So I'm lost on how to better handle that
string part, and it looks like it could have localization impact if done correctly.

Other comments addressed locally, for now.

Comment 7

[Ryan Polk (Quark)]

Attachment: v2, toolkit

One new function, don't change anything else.  Timer display is up to half a
second off, depending on user pref, but the button itself enables at the right
time.

Still can't find another method besides stringbundle to be l10n safe.

Comment 8

[Daniel Veditz [:dveditz]]

This dialog is gone