1081047 - Add new ui module for the unknownContentType dialog

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Daniel Gherasim]

Add a new UI module for the unknownContentType dialog.
We need this for handling downloads (bug 908649).

This will be inherited from the BaseWindow class implemented in bug 1006996 (so will currently live only on aurora & nightly).

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

Patch is up!
Keep in mind that it needs the patch from bug 1081006 first.

Comment 2

[Andrei Eftimie]

Comment on attachment 8503086 [details] [diff] [review]
v1.patch

Review of attachment 8503086 [details] [diff] [review]:
-----------------------------------------------------------------

The deps have landed.

Unfortunately some conflicts have appeared:
> patching file firefox/lib/ui/browser.js
> Hunk #1 FAILED at 2
> 1 out of 2 hunks FAILED -- saving rejects to file firefox/lib/ui/browser.js.rej

Other than that looks pretty good. I'll test it once it can be applied.

::: firefox/lib/tests/testUnknownContentTypeDialog.js
@@ +33,5 @@
> +function testUnknownContentTypeDialog() {
> +  var uCTDialog = browserWindow.openUnknownContentTypeDialog(TEST_DATA.url);
> +
> +  TEST_DATA.elements.forEach(aElement => {
> +    if (aElement.type && aElement.type === "button") {

You don't need to check for the existence of type. In case it isn't defined, the check will be `undefined === "button" // false`

Comment 3

[Daniel Gherasim]

Attachment: v1.1.patch

Updated!

Comment 4

[Andrei Eftimie]

Comment on attachment 8505457 [details] [diff] [review]
v1.1.patch

Review of attachment 8505457 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple changes.

::: firefox/lib/tests/testUnknownContentTypeDialog.js
@@ +11,5 @@
> +const TEST_DATA = {
> +  url: "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/Firefox%203.6.dmg",
> +  elements: [
> +    {name: "cancel", value: "button", type: "button"},
> +    {name: "accept", value: "button", type: "button"},

Since both "value" and "type" are identical for the testing purposes, we can remove the type and check for value directly.


I'd also change this into a key/value store on an object:
> elements: {
>   cancel: "button",
>   accept: "button",
>   [..]
> }

::: firefox/lib/ui/browser.js
@@ +112,5 @@
> + *
> + * @param {string} URL of the file which has to be downloaded
> + */
> +BrowserWindow.prototype.openUnknownContentTypeDialog = function BW_openUnknownContentTypeDialog(aUrl) {
> +  assert.ok(aUrl, "Url has been defined");

URL

I think we might also want to have the variable name `aURL`

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +62,5 @@
> + *
> + * @returns {ElemBase[]} Elements which have been found
> + */
> +UnknownContentTypeDialog.prototype.getElements = function UCTD_getElements(aSpec={}) {
> +  var elems = null;

The default value should be [], not null (to not break getElement)

Comment 5

[Daniel Gherasim]

Attachment: v2.patch

Thanks Andrei.
This needs the patch from bug 1084333 applied first (1st part patch).

Comment 6

[Daniel Gherasim]

Attachment: v2.1.patch

All we need here landed an default (and will be backported soonish).
Updated the patch, all is fine and ready now!

Comment 7

[Daniel Gherasim]

So this is not blocked anymore.

Comment 8

[Andrei Eftimie]

Comment on attachment 8509438 [details] [diff] [review]
v2.1.patch

Review of attachment 8509438 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

I only have 1 comment as mentioned inline.
I'd like Henrik's opinion there.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +17,5 @@
> + * @param {MozMillController} [aController=null]
> + *        MozMillController of the SaveFile Window
> + */
> +function UnknownContentTypeDialog(aController=null) {
> +  baseWindow.BaseWindow.call(this, aController);

This leaves the way open for the following scenario, is that something feasible and something we would want as a flow in a test?
I assume we'd always want the open method to be used...

1. open an UnkownContentType dialog, retrieve it's controller
2. create a new instance and pass-in the controller retrieved at 1

If it's not I would propose to take the option to provide a controller at instantiation time out.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8509438 [details] [diff] [review]
v2.1.patch

Review of attachment 8509438 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testUnknownContentTypeDialog.js
@@ +30,5 @@
> + * Bug 1081047
> + * Add new ui module for the unknownContentType dialog
> + */
> +function testUnknownContentTypeDialog() {
> +  var uCTDialog = browserWindow.openUnknownContentTypeDialog(TEST_DATA.url);

nit: var dialog

@@ +39,5 @@
> +                                     subtype: element});
> +    }
> +    else {
> +      var el = uCTDialog.getElement({type: element});
> +    }

I do not see why we need this separation. Maybe let me continue to check the class itself.

::: firefox/lib/ui/browser.js
@@ +114,5 @@
> + * @param {string} aURL
> + *         URL of the file which has to be downloaded
> + */
> +BrowserWindow.prototype.openUnknownContentTypeDialog = function BW_openUCTD(aURL) {
> +  assert.ok(aURL, "URL has been defined");

Please make that parameter consistent to the other open methods, which we will have for inline preferences and places. We should not start to vary. So the callback should come from the test itself.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +10,5 @@
> +var windows = require("../../../lib/windows");
> +
> +/**
> + * Save file of unknown type window class
> + * Implemented in bug 1081047

Please remove that line.

@@ +17,5 @@
> + * @param {MozMillController} [aController=null]
> + *        MozMillController of the SaveFile Window
> + */
> +function UnknownContentTypeDialog(aController=null) {
> +  baseWindow.BaseWindow.call(this, aController);

Right. That's not how it will work. There should not be a default null parameter. In case of top-level windows we always need a controller. It should be similar to the other upcoming window classes. So we cannot have an internal open method, but have to rely on the waitForWindowState() call. With the returned controller we can instantiate this dialog class. With that method we can also instantiate such a class if the appropriate dialog is already open.

@@ +39,5 @@
> + *        Parent of the to find element
> + *
> + * @returns {ElemBase[]} Elements which have been found
> + */
> +UnknownContentTypeDialog.prototype.getElements = function UCTD_getElements(aSpec={}) {

I don't see getElement defined on that class.

@@ +85,5 @@
> +
> +/**
> + * Helper for opening the Unknown Content Type dialog
> + *
> + * @param {function} aCalback

aCallback

@@ +86,5 @@
> +/**
> + * Helper for opening the Unknown Content Type dialog
> + *
> + * @param {function} aCalback
> + *        Callback function that opens the unknownContentType dialog

nit: remove function

Comment 10

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8509438 [details] [diff] [review]
> v2.1.patch
> 
> Review of attachment 8509438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +39,5 @@
> > +                                     subtype: element});
> > +    }
> > +    else {
> > +      var el = uCTDialog.getElement({type: element});
> > +    }
> 
> I do not see why we need this separation. Maybe let me continue to check the
> class itself.
> 

This is needed so we don't duplicate the nodeCollector code lines for each button.
So we can search for element type: button, subtype:anonid_of_the_button.

> ::: firefox/lib/ui/browser.js
> @@ +114,5 @@
> > + * @param {string} aURL
> > + *         URL of the file which has to be downloaded
> > + */
> > +BrowserWindow.prototype.openUnknownContentTypeDialog = function BW_openUCTD(aURL) {
> > +  assert.ok(aURL, "URL has been defined");
> 
> Please make that parameter consistent to the other open methods, which we
> will have for inline preferences and places. We should not start to vary. So
> the callback should come from the test itself.
> 
> @@ +17,5 @@
> > + * @param {MozMillController} [aController=null]
> > + *        MozMillController of the SaveFile Window
> > + */
> > +function UnknownContentTypeDialog(aController=null) {
> > +  baseWindow.BaseWindow.call(this, aController);
> 
> Right. That's not how it will work. There should not be a default null
> parameter. In case of top-level windows we always need a controller. It
> should be similar to the other upcoming window classes. So we cannot have an
> internal open method, but have to rely on the waitForWindowState() call.
> With the returned controller we can instantiate this dialog class. With that
> method we can also instantiate such a class if the appropriate dialog is
> already open.
> 

So if I understand you well, this is the structure you want ? :

BrowserWindow.prototype.openUnknownContentTypeDialog = function BW_openUCTD(aCallback) {
  return unknownContentTypeDialog.open(aCallback);
};

and we remove the .open method from the class itself and only keep the global one:

UnknownContentTypeDialog.prototype.open = function UCTD_open(aCallback) {
  assert.equal(typeof aCallback, "function",
               "Callback has been defined");

  var controller = windows.waitForWindowState(() => {
    aCallback();
  }, {id: "unknownContentType", state: "open"});
  return new UnknownContentTypeDialog(controller);
}

And if in a test the windows is already open and we have it's controller, we don't make use of the open method but just instantiate tha class right ?

> @@ +39,5 @@
> > + *        Parent of the to find element
> > + *
> > + * @returns {ElemBase[]} Elements which have been found
> > + */
> > +UnknownContentTypeDialog.prototype.getElements = function UCTD_getElements(aSpec={}) {
> 
> I don't see getElement defined on that class.
> 

We have it now in the base class, we don't need for redefine it here.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to daniel.gherasim from comment #10)
> > > +    else {
> > > +      var el = uCTDialog.getElement({type: element});
> > > +    }
> > 
> > I do not see why we need this separation. Maybe let me continue to check the
> > class itself.
> 
> This is needed so we don't duplicate the nodeCollector code lines for each
> button.
> So we can search for element type: button, subtype:anonid_of_the_button.

Yeah, please drop that. I wanted to remove this comment before I hit send, but missed it. So please forget about it.

> > > +function UnknownContentTypeDialog(aController=null) {
> > > +  baseWindow.BaseWindow.call(this, aController);
> > 
> > Right. That's not how it will work. There should not be a default null
> > parameter. In case of top-level windows we always need a controller. It
> > should be similar to the other upcoming window classes. So we cannot have an
> > internal open method, but have to rely on the waitForWindowState() call.
> > With the returned controller we can instantiate this dialog class. With that
> > method we can also instantiate such a class if the appropriate dialog is
> > already open.
> 
> So if I understand you well, this is the structure you want ? :
> 
> BrowserWindow.prototype.openUnknownContentTypeDialog = function
> BW_openUCTD(aCallback) {
>   return unknownContentTypeDialog.open(aCallback);
> };
> 
> and we remove the .open method from the class itself and only keep the
> global one:
> [..]
> And if in a test the windows is already open and we have it's controller, we
> don't make use of the open method but just instantiate tha class right ?

Exactly. But if you have another idea to make it happen that way please let us know. 

> > I don't see getElement defined on that class.
> 
> We have it now in the base class, we don't need for redefine it here.

As discussed on IRC yesterday, I missed that check-in. So indeed it is not necessary here.

Comment 12

[Teodor Druta]

Attachment: v2.2.patch

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8513346 [details] [diff] [review]
v2.2.patch

As usual please only request review from Andreea and Andrei first. Thanks.

Comment 14

[Andrei Eftimie]

Comment on attachment 8513346 [details] [diff] [review]
v2.2.patch

Review of attachment 8513346 [details] [diff] [review]:
-----------------------------------------------------------------

Please try to include a short commment / description of what's updated in the patch. Helps with the review process.


I don't have a strong opinion here. This looks ok to me.
Please fix the mentioned nits and ask a review from Henrik.

::: firefox/lib/tests/testUnknownContentTypeDialog.js
@@ +37,5 @@
> +
> +  for (var element in TEST_DATA.elements) {
> +    if (TEST_DATA.elements[element] === "button") {
> +      var el = dialog.getElement({type: TEST_DATA.elements[element],
> +                                     subtype: element});

check the indentation here please.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +80,5 @@
> + * @returns {UnknownContentTypeDialog} New instance of UnknownContentTypeDialog
> + */
> +function open(aCallback) {
> +  assert.equal(typeof aCallback, "function",
> +               "Callback has been specified");

You could leave this in a single line.

Comment 15

[Teodor Druta]

Attachment: v2.2.patch

Updated the patch following the review guidelines.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8513496 [details] [diff] [review]
v2.2.patch

Review of attachment 8513496 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tests/testUnknownContentTypeDialog.js
@@ +45,5 @@
> +    }
> +    expect.equal(el.getNode().localName, TEST_DATA.elements[element],
> +                 "Elemens has been found - " + element);
> +  }
> +  dialog.close();

nit: Why has the empty line above been removed?

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +19,5 @@
> +function UnknownContentTypeDialog(aController) {
> +  assert.notEqual(typeof aController, "undefined",
> +                  "A controller for the unknown content type dialog has been specified");
> +  this._controller = aController;
> +  baseWindow.BaseWindow.call(this, this._controller);

Please see bug 996530 how Daniel is doing that for the Places window.

@@ +25,5 @@
> +
> +UnknownContentTypeDialog.prototype = Object.create(baseWindow.BaseWindow.prototype);
> +UnknownContentTypeDialog.prototype.constructor = UnknownContentTypeDialog;
> +
> +

Why this extra new empty line?

@@ +83,5 @@
> +  assert.equal(typeof aCallback, "function", "Callback has been specified");
> +  var controller = windows.waitForWindowState(() => {
> +    aCallback();
> +  }, {id: "unknownContentType", state: "open"});
> +  return new UnknownContentTypeDialog(controller);

Please really check your usage and non-usage of empty lines to separate code blocks better. Here two are missing.

Comment 17

[Teodor Druta]

Attachment: v2.3.patch

Fixed empty lines.
Updated the constructor similar to the one from Bug 996530 Places Window.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8516011 [details] [diff] [review]
v2.3.patch

Review of attachment 8516011 [details] [diff] [review]:
-----------------------------------------------------------------

With the correct list of dtd and properties urls you will get my r+

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +19,5 @@
> +function UnknownContentTypeDialog(aController) {
> +  baseWindow.BaseWindow.call(this, aController);
> +
> +  this._dtds = ["chrome://browser/locale/browser.dtd",
> +                "chrome://mozapps/locale/downloads/downloads.dtd"];

I do not see that we need any of those dtds here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#10

Also you may want to add the following for the properties:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul#36

Comment 19

[Teodor Druta]

Attachment: v2.4.patch

Updated with the correct dtds and properties.

Comment 20

[Andrei Eftimie]

Comment on attachment 8517989 [details] [diff] [review]
v2.4.patch

Review of attachment 8517989 [details] [diff] [review]:
-----------------------------------------------------------------

Great. I see everything was addressed. All looks good.

https://hg.mozilla.org/qa/mozmill-tests/rev/5cf3a5b96c20 (default)

Comment 21

[Andrei Eftimie]

Looks good on Aurora, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/cbae44b7132a (mozilla-aurora)

It should be safe for Beta as well since it doesn't change existing code. Will make sure all is green.

Comment 22

[Andrei Eftimie]

All green on Beta, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/8f99f48f3d4f (mozilla-beta)