575485 - Refactor commonDialog code into a sharable JSM

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Just some more code cleanup, shouldn't be any change in functionality. Looks a bit bigger than it really is because I moved a couple functions around (so that earlyInit and onLoad are at the top of the file).

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 454743 [details] [diff] [review]
Patch v.1

>-#ifdef XP_MACOSX
>+    document.title = title;
>+    // OS X doesn't have a title on modal dialogs, this is hidden on other platforms.
>     document.getElementById("info.title").appendChild(document.createTextNode(title));
>-#else
>-    document.title = title;
>-#endif

>-#ifdef XP_MACOSX
>-          <!-- Dialog title is inside dialog for OS X -->
>+          <!-- Only shown on OS X, since it has no dialog title -->
>           <description id="info.title"/>
>-#endif

>+#info.title {
>+  display: none;
>+}

I don't see the point of this change. Looks like it would cause headache for cross-platform themes.

Also, your id selector is syntactically wrong.

Comment 2

[Justin Dolske [:Dolske]]

(In reply to comment #1)

> I don't see the point of this change.

I'd really prefer to keep the DOM the same between platforms, so that code doesn't need to treat it as a special case. The CSS can just hide it.

> Also, your id selector is syntactically wrong.

sigh. IDs with periods in them were such a terrible idea, whoever did that.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Part 1 (cleanup), v.2

Updated version of previous patch on this bug, with less moving around of code so diff is smaller.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Part 2 (propbag cleanup) v.1

This is a mostly mechanical change to have most of the code set properties on a JS object, instead of a nsIPropertyBag. Instead, the object is generically converted to a propertybag right before handing it to commonDialog.xul, and the results are converted back to a JS object.

This helps allow the tab-modal prompts to reuse this logic, without having to use a propertybag.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.1 (WIP)

This moves most of the code from commonDialog.js to CommonDialog.jsm, and converts that code to use the UI elements passed in instead of fetching them with getElementById() or whatever.

This allows the tab-modal prompt implementation to reuse this code, with so level of abstraction between this logic and the actual UI implementation.

Additionally, this will be superuseful for bug 573649. Fennec currently has its own prompt service implementation, and is a bit icky. With this, Fennec will be closer to being able to share the /toolkit prompt service, while still providing its own mobile UI.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Part 1 (cleanup), v.3

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Part 2 (propbag cleanup) v.2

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.2

I think this revision (Part 3 v.2) was the only patch that actually changed, but easier to just reattach all 3 than check. :)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 478932 [details] [diff] [review]
Part 3 (move to JSM), v.2

This is the wrong patch (it's actually Part 1).

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.2

Bah, attached the wrong patch last time.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.3

Implements the delayed prompt stuff which previously was commented out as TODO; I had thought it was dead code but it looks like there are a couple of things still using it.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483056 [details] [diff] [review]
Part 3 (move to JSM), v.3

>diff --git a/toolkit/components/prompts/content/commonDialog.js b/toolkit/components/prompts/content/commonDialog.js

> function earlyInit() {

>+    // TODO style flush this to ensure buttons are present from binding?

Calling getButton below does conflict with the comment at the top of the method. But I think it's bogus now since we'll apply bindings when getting a reference to the element, IIRC. The "called before onload" thing seems to be ancient, is it still needed? Don't really see why this can't all be done onload.

>+    let ui = {

>+        button3            : dialog.getButton("extra2"),
>+        button2            : dialog.getButton("extra1"),
>+        button1            : dialog.getButton("cancel"),
>+        button0            : dialog.getButton("accept"),

The button2 -> extra1, button3 -> extra2 mapping between commonDialog and nsIPrompt is confusing :(

Button3 seems to actually never be used (no one sets button3Label). Can we just get rid of it?

>+        focusTarget        : window,

Hmm, the old code sets the listener on the window, but checks for event.target == document. I guess focus events are fired on both? Makes me nervous that this will break in one of the two cases (old prompts or tab-modal) - do we have test coverage for this functionality?

>diff --git a/toolkit/components/prompts/content/commonDialog.xul b/toolkit/components/prompts/content/commonDialog.xul

>-        <checkbox id="checkbox" oncommand="onCheckboxClick(this);"/>
>+        <checkbox id="checkbox"/>

Why not just call Dialog.onCheckbox() here as with the buttons, rather than adding the listener in earlyInit?

>diff --git a/toolkit/components/prompts/src/CommonDialog.jsm b/toolkit/components/prompts/src/CommonDialog.jsm

>+    onLoad : function(xulDialog) {

>+        // set the icon
>+        this.ui.infoIcon.className += " " + this.iconClass;

nit: switch this to use this.ui.infoIcon.classList.add(this.iconClass)?

>+        if (xulDialog)
>+            Services.obs.notifyObservers(xulDialog.ownerDocument.defaultView, "common-dialog-loaded", null);
>+        // TODO:
>+        // else
>+        //    (notify using what as the subject?)

Shouldn't do this both here and in commonDialogOnLoad(). Need to figure out what to do with this, as your comment suggests, but until then I suppose we should keep this code and remove the line in commonDialogOnLoad().

>+    onBlur : function (aEvent) {
>+    onFocus : function (aEvent) {
>+    startOnFocusDelay : function() {
>+    onFocusTimeout : function() {

This is implemented differently than it was before - the old code uses the pref value (2000ms) for the initial delay only, and a much shorter 250ms timeout for blur/focuses. I think we should probably keep that behavior.

This looks good otherwise, but will probably want to look this over once more once these are addressed.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to comment #12)

> Calling getButton below does conflict with the comment at the top of the
> method. But I think it's bogus now since we'll apply bindings when getting a
> reference to the element, IIRC. The "called before onload" thing seems to be
> ancient, is it still needed? Don't really see why this can't all be done
> onload.

I tried removing all this, so that everything just happens in an single onload function with no style flushing. Seems to work fine -- I don't see the dialog size bouncing around, nor test failures on my box from accessing binding bits before they've managed to apply.

I suppose there's a small risk this somehow might expose itself on slow test boxes, but it's an easy change to revery (ie, go back to earlyInit + onLoad) if needed.

> Button3 seems to actually never be used (no one sets button3Label). Can we 
> just get rid of it?

Hmmm, I think we could, but it's easy to keep and avoids any chance of someone complaining about this being an API change.

> >+        focusTarget        : window,
> 
> Hmm, the old code sets the listener on the window, but checks for event.target
> == document. I guess focus events are fired on both?

I'm not entirely sure why focus works the way it does, either. But from testing this seemed to work as expected...

> Makes me nervous that this
> will break in one of the two cases (old prompts or tab-modal) - do we have
> test coverage for this functionality?

No, but I think I can write a test.

I'm not planning on implementing this for tab-modal prompts (at least, not right away) since there won't be anything using it, and it's already an infrequently used feature.

> >-        <checkbox id="checkbox" oncommand="onCheckboxClick(this);"/>
> >+        <checkbox id="checkbox"/>
> 
> Why not just call Dialog.onCheckbox() here as with the buttons, rather than
> adding the listener in earlyInit?

Mainly just to avoid having the inline JS in both commonDialog.xul and tabprompts.xml... Keeps the core of both dialogs straight up XUL.
 

> >+        if (xulDialog)
> >+            Services.obs.notifyObservers(...)
> 
> Shouldn't do this both here and in commonDialogOnLoad().

Oops, didn't notice this was duplicated. Removed the commonDialogOnLoad one as you suggested.

> This is implemented differently than it was before - the old code uses the pref
> value (2000ms) for the initial delay only, and a much shorter 250ms timeout for
> blur/focuses. I think we should probably keep that behavior.

Oops, agreed. Misread what the original code was doing.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.4

Updated per above, added a test for the delay-prompt.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Mainly just to avoid having the inline JS in both commonDialog.xul and
> tabprompts.xml... Keeps the core of both dialogs straight up XUL.

My comment was really more "why are Dialog.onCheckbox() and Dialog.onButton*() handled differently?" They should either all be inline handlers, or all be added manually by the CommonDialog JSM... I prefer them just being inline, which gives commondialog.jsm users more flexibility in how the dialog is implemented (at little cost).

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 484950 [details] [diff] [review]
Part 3 (move to JSM), v.4

>diff --git a/toolkit/components/prompts/src/CommonDialog.jsm b/toolkit/components/prompts/src/CommonDialog.jsm

>+CommonDialog.prototype = {

>+    onLoad : function(xulDialog) {
>+        // This is called before onload fires, so we can't be certain that any elements
>+        // in the document have their bindings ready, so don't call any methods/properties
>+        // here on xul elements that come from xbl bindings.

Need to remove this since it's no longer true.

>+        if (this.args.enableDelay) {

The old code had another difference from this re-implementation. The buttons would not be reset until the initial countdown expired, regardless of focus/blurring. Your code makes it possible to avoid the full countdown by blurring/focusing immediately after opening (thus resetting the timer to the smaller default value). I think this probably doesn't matter; you can achieve nearly the same effect in attack scenarios by letting the countdown elapse on its own before focusing the prompted window, and the only user of this functionality is the fairly obscure enablePrivilege dialog (and the existing code seems to be broken...), but I thought it was worth noting.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Part 3 (move to JSM), v.5

Fixed nits.

Realized the other reason I moved onDialog out-of-line was I was thinking it was going to be called with |this| == <checkbox/>, but that doesn't seem to be the case (XUL? JSM? I was crazy?).

Comment 18

[Justin Dolske [:Dolske]]

Pushed:

Part 1: http://hg.mozilla.org/mozilla-central/rev/1769b8307bde
Part 2: http://hg.mozilla.org/mozilla-central/rev/a2ef5d0f5052
Part 3: http://hg.mozilla.org/mozilla-central/rev/fc2988ab64e5

Comment 19

[Mike Hommey [:glandium]]

Backed out because of persisting oranges in moth.

http://hg.mozilla.org/mozilla-central/rev/827f362fc804
http://hg.mozilla.org/mozilla-central/rev/1353a3498026
http://hg.mozilla.org/mozilla-central/rev/b09fa46d123c

Comment 20

[Marco Bonardo [:mak]]

persisting orange was mostly toolkit/mozapps/extensions/test/xpinstall/browser_auth.js
the dialog is shown (checked the screenshot) but the test times out

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Part 4 (test fixes), v.1

Hmm. I could have sworn I ran this through Try at some point, but I guess I didn't. Looked at the test mentioned in the last comment, as well as grepping through the tree for similar patterns, and found 2 tests that were looking at vars in the prompt's window to determine what kind of prompt it was. This changed a bit with the patches in this bug, so the tests failed.

This patch updates the code in those tests to work properly in the new world. Ran through Try, was green (with unrelated, known intermittent oranges). (Actually had one dumb mistake in harness.js that makes me suspect that's dead test code, but fixed it anyway and it passes locally).

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

a=beltzner, can land through b7 freeze

Comment 23

[Justin Dolske [:Dolske]]

Relanded with test fixes:

Part 1: http://hg.mozilla.org/mozilla-central/rev/b5a738c8e5d1
Part 2: http://hg.mozilla.org/mozilla-central/rev/91e39cc3c2e6
Part 3: http://hg.mozilla.org/mozilla-central/rev/3d5a71a11118
Part 4: http://hg.mozilla.org/mozilla-central/rev/6d271e1c2076

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 486391 [details] [diff] [review]
Part 4 (test fixes), v.1

>diff --git a/xpinstall/tests/harness.js b/xpinstall/tests/harness.js
>+      switch (window.args.promptType) {
>+        default:
>                 break;
>+        case "promptUserAndPass":
>                 break;

Does 'case' after 'default' actually work?
It looks like an explicit comment would be welcome, wouldn't it?

Comment 25

[Marco Bonardo [:mak]]

FWIW https://developer.mozilla.org/en/JavaScript/Reference/Statements/switch
"By convention, the default clause is the last clause, but it does not need to be so."

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)

Ah, thanks for the confirmation :-)
I'm still thinking breaking convention could use a comment, but I won't argue more.