1084333 - Add some more methods and a property to BaseWindow class

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

Let's add this to the BaseWindow class:

> _proprietes (similar to this._dtds but containing only 1 file)
> getProperty(aName)
* will call utils.getProperty
This actually deserves a discussion.
Should we accept multiple files as we do with the _dtds (if we want this, we either need to update utils.getProperty or repeatedly call this in our current method, but surrounded with try catch and only fail if exception is caught for each of the files).

> getElement
(same method we use in all of our code)

> getElements
(will return [] for the base window)

With the info I get about the _proprietes, I will come with the fix right away so we can use this in all of our new ui windows libraries.

Comment 1

[Daniel Gherasim]

(In reply to daniel.gherasim from comment #0)
> > _proprietes (similar to this._dtds but containing only 1 file)

I would actually call it _propertiesFiles to actually reflect what it contains.
Pinging Henrik for some feedback about this new enhancements!

Comment 2

[Daniel Gherasim]

Missed some other enhancement we need: we should also allow null as aController for the BaseWindow, in case we first want to instantiate the Window class and then call the open method.

Let's also add this fix here.

Comment 3

[Daniel Gherasim]

Attachment: WIP.patch

Just a WIP.

Comment 4

[Daniel Gherasim]

Attachment: Part-1-Add-the-2-methods-and-change-the-assert.patch

Until we get feedback from Henrik related to the proprietesFile(s) prop and method, we can review and land the other changes as it's blocking us in creating new window libs.

Comment 5

[Andrei Eftimie]

Comment on attachment 8508504 [details] [diff] [review]
Part-1-Add-the-2-methods-and-change-the-assert.patch

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

lgtm

Comment 6

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

Comment on attachment 8508504 [details] [diff] [review]
Part-1-Add-the-2-methods-and-change-the-assert.patch

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

::: lib/ui/base-window.js
@@ +15,5 @@
>   *        Controller of the window
>   */
>  function BaseWindow(aController) {
> +  assert.notEqual(typeof aController, "undefined",
> +                  "A controller for the window has been specified");

Please note in the jsdoc that `null` is a special value, and maybe give an example when to use this value.

@@ +83,5 @@
> +  getElement: function BaseWindow_getElement(aSpec) {
> +    var elements = this.getElements(aSpec);
> +
> +    return (elements.length > 0) ? elements[0] : undefined;
> +  },

Hurray for getting this method removed from all the other sub classes. Can you btw. do that for the existing new ui window classes like browser window, so we can ensure it works correctly?

Comment 7

[Daniel Gherasim]

Attachment: Part-1-Add-the-2-methods-and-change-the-assert-v2.patch

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 8508504 [details] [diff] [review]
> @@ +83,5 @@
> > +  getElement: function BaseWindow_getElement(aSpec) {
> > +    var elements = this.getElements(aSpec);
> > +
> > +    return (elements.length > 0) ? elements[0] : undefined;
> > +  },
> 
> Hurray for getting this method removed from all the other sub classes. Can
> you btw. do that for the existing new ui window classes like browser window,
> so we can ensure it works correctly?

We currently don't use it in the BrowserWindow which is the only one inherited from BaseWindow that's landed.

But we are coming with PlacesOrganizer, UnknownContentTypeDialog and PageInfo libs where this will help.

Comment 8

[Daniel Gherasim]

Attachment: Part-1-Add-the-2-methods-and-change-the-assert-v2.patch

Submitted the wrong changes. Sorry1

Comment 9

[Daniel Gherasim]

Attachment: Part-2-Add-proprietes-handling.patch

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8508618 [details] [diff] [review]
Part-1-Add-the-2-methods-and-change-the-assert-v2.patch

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

Part 1 landed: 
http://hg.mozilla.org/qa/mozmill-tests/rev/419ded45f089 (default)

Comment 11

[Daniel Gherasim]

This needs backporting up to beta.

Comment 12

[Daniel Gherasim]

Got my answer on IRC.

Comment 13

[Daniel Gherasim]

Attachment: Part-2-Add-proprietes-handling-v2.patch

Proprietes handling added to the BaseWindow.

Comment 14

[Daniel Gherasim]

Attachment: Part2Testing.patch

This patch is just for testing.
Apply this to test Part-2.

Comment 15

[Daniel Gherasim]

Attachment: Part2Testing.patch

Comment 16

[Daniel Gherasim]

Comment on attachment 8508618 [details] [diff] [review]
Part-1-Add-the-2-methods-and-change-the-assert-v2.patch

Andreea can you take care of backporting this ?
Thanks!

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8508618 [details] [diff] [review]
Part-1-Add-the-2-methods-and-change-the-assert-v2.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/9dcb73f41d2b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/cf96c843574c (beta)

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8509474 [details] [diff] [review]
Part-2-Add-proprietes-handling-v2.patch

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

Looks good, I know you discussed on IRC about this.

Comment 19

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

Comment on attachment 8509474 [details] [diff] [review]
Part-2-Add-proprietes-handling-v2.patch

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

::: lib/ui/base-window.js
@@ +22,5 @@
>                    "A controller for the window has been specified");
>  
>    this._controller = aController;
>    this._dtds = [];
> +  this._propertiesURLs = [];

I would strip URLs, it would also apply to dtds otherwise.

@@ +56,5 @@
>  
>    /**
> +   * Get properties files
> +   *
> +   * @returns {string} External properties URLs

This is a string array similar to dtds. Please make the description similar.

@@ +124,5 @@
> +   *
> +   * @param {String} aName
> +   *        Property name
> +   */
> +  getProperty: function BaseWindow_getProperty(aName) {

Please update the method in utils.js for that. Do not introduce a new one. This would need updates for tests too.

We could have wrappers for BaseWindow in case of getEntity and getProperty, and I don't think we have to expose .dtds and .properties then.

Comment 20

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Comment on attachment 8509474 [details] [diff] [review]
> Part-2-Add-proprietes-handling-v2.patch
> 
> Review of attachment 8509474 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +124,5 @@
> > +   *
> > +   * @param {String} aName
> > +   *        Property name
> > +   */
> > +  getProperty: function BaseWindow_getProperty(aName) {
> 
> Please update the method in utils.js for that. Do not introduce a new one.
> This would need updates for tests too.
> 
> We could have wrappers for BaseWindow in case of getEntity and getProperty,
> and I don't think we have to expose .dtds and .properties then.

How about to enhance the method to _also allow_ array of URLs ? So we won't need to update all the tests which uses utils.getProperty ?

Anyway, I think filing a new bug for this will be great as this could be landed on all branches. (so see bug 1088007)

Comment 21

[Daniel Gherasim]

Attachment: Part-2-Add-proprietes-handling-v3.patch

We first need patch on bug 1088007 to be landed.
Then with this patch we can close this bug.

Comment 22

[Andrei Eftimie]

Comment on attachment 8512518 [details] [diff] [review]
Part-2-Add-proprietes-handling-v3.patch

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

lgtm

Comment 23

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

Comment on attachment 8512518 [details] [diff] [review]
Part-2-Add-proprietes-handling-v3.patch

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

When doing the addition for .getProperty() can we also add .getEntity() at the same time, and get rid of the .dtds property? I would be in favor of keeping the API for both l10n related methods the same.

::: lib/ui/base-window.js
@@ +110,5 @@
>      return [];
>    },
>  
>    /**
> +   * Get the value on an individial property of the page

nit: Get the value of a property of the current window

@@ +112,5 @@
>  
>    /**
> +   * Get the value on an individial property of the page
> +   *
> +   * @param {String} aName

nit: Lets call this aID. Properties are unique.

Comment 24

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v3.1.patch

(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 8512518 [details] [diff] [review]
> Part-2-Add-proprietes-handling-v3.patch
> 
> Review of attachment 8512518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When doing the addition for .getProperty() can we also add .getEntity() at
> the same time, and get rid of the .dtds property? I would be in favor of
> keeping the API for both l10n related methods the same.
> 

Henrik, could you please look over my attached patch and tell me if this was what you meant ?
I added a .getEntity() method and removed the dtds getter.

Comment 25

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

Teodor, if you want feedback on a patch please ask for feedback and not set the needinfo flag, which is for general information. Thanks.

Comment 26

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

Comment on attachment 8515863 [details] [diff] [review]
Part-2-Add-proprietes-handling-v3.1.patch

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

That's exactly what I meant. Good to see that. Do we have to update any test, which makes use of getEntity?

::: lib/ui/base-window.js
@@ +101,5 @@
>      return [];
>    },
>  
>    /**
> +   * Get the value of an entity of the current window

nit: I would use 'DTD entity' in the description here.

Comment 27

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #26)
> -----------------------------------------------------------------
> Do we have to update any
> test, which makes use of getEntity?
> 

Can you please specify what do you mean for a test that makes use of getEntity?
Is it the tests that use base-window as a requirement module at the moment ? If so I think only "lib/tests/testOpenMultipleWindows.js" needs to be updated and also the browser.js library uses base-window.

Or you mean all the tests that now use utils.getEntity() ? If so this will be a really big change as it is used in 30 tests and libraries.

Comment 28

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v3.1.patch

Fixed "Get the value of a *DTD* entity" comment.

Comment 29

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

(In reply to Teodor Druta from comment #27)
> Can you please specify what do you mean for a test that makes use of
> getEntity?

Only tests which make use of the new ui window classes.

Comment 30

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v4.patch

Updated test "lib/tests/testOpenMultipleWindows.js" and "firefox/lib/ui/browser.js" to use the getEntity method from BaseWindow class

Comment 31

[Andrei Eftimie]

Comment on attachment 8515997 [details] [diff] [review]
Part-2-Add-proprietes-handling-v4.patch

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

SO what Henrik was referring to for changes in regard to getEntity vs getProperty is the work done in bug 1088007.

Teodor, either file a new bug to get the getEntity changed in a similar way to getProperty in bug 1088007, or make the changes here.

::: lib/tests/testOpenMultipleWindows.js
@@ +19,5 @@
>  
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.baseWindow = new baseWindow.BaseWindow(aModule.controller);
> +  aModule.baseWindow._dtds = DTDS;

This is not how you'll want to approach this.

This test does not yet use the browser-window class. If you want to update this (which should probably be done in a different bug),
you'll need to instantiate browserWindow (not baseWindow) and all needed DTD's are already included in the browser-window class.

I think this test can remain unchanged for the purpose of the base window changes.

Comment 32

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v5.patch

Reverted changes back for lib/tests/testOpenMultipleWindows.js.
Modified getEntity() method in lib/utils.js to be able to have also a string as a parameter alongside an array of strings.
Modified lib/tests/testUtils.js:
  * Test getProperty() with a string alongside an array
  * Added a test for getEntity() method for checking if it works with an array of dtds urls.
  * Added a test for getEntity() method for checking if it works with a single string.

Comment 33

[Andrei Eftimie]

Comment on attachment 8516738 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.patch

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

Just one thing to change, looks ok otherwise.

::: lib/tests/testUtils.js
@@ +37,5 @@
>  }
>  
>  function testUtils() {
> +  // Test getProperty with urls array
> +  TEST_DATA.properties.properties.forEach(aProp => {

`properties.properties` reads a bit weird, but I don't have any significantly better idea. I'd welcome something else to avoid confusion, but in lack of better ideas, this will have to do :)

@@ +48,4 @@
>    }, undefined, "Invalid property has not raised an exception");
> +
> +  // Test getProperty with single string
> +  var prop =  utils.getProperty(TEST_DATA.properties.urls[0], TEST_DATA.properties.properties[0]);

nit: 2 spaces after assignment operator

::: lib/utils.js
@@ +302,5 @@
>   * @return The value of the requested entity
>   * @type string
>   */
>  function getEntity(aUrls, aEntityId) {
> +  aUrls = (typeof aUrls === "string") ? [aUrls] : aUrls;

Please don't reassign a value to an variable received as an argument.

> var urls = [...]

Comment 34

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

Comment on attachment 8516738 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.patch

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

::: lib/tests/testUtils.js
@@ +37,5 @@
>  }
>  
>  function testUtils() {
> +  // Test getProperty with urls array
> +  TEST_DATA.properties.properties.forEach(aProp => {

property_ids and entity_ids sounds reasonable to me.

Comment 35

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v5.1.patch

Fixed the surplus space.
Changed "properties.properties" and "dtd.entities" properties to entity_ids and property_ids.
Removed the reassignment of the aUrls parameter in getEntity() method.

Comment 36

[Andrei Eftimie]

Comment on attachment 8518088 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.1.patch

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

Ask a review from Henrik with the below mentioned item.

::: lib/tests/testUtils.js
@@ +13,5 @@
> +      "chrome://browser/locale/pageInfo.properties",
> +      "chrome://browser/locale/browser.properties",
> +      "chrome://global/locale/search/search.properties"
> +    ],
> +    properties_ids: [

I think Henrik meant to leave this as `ids`, so we reference them as:
> TEST_DATA.properties.ids
> TEST_DATA.dtds.ids

Comment 37

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v5.1.patch

Changed *.properties and *.entities properties to simple `ids` in testUtils.js.

Comment 38

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

Comment on attachment 8518186 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.1.patch

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

All nits. With those fixed you have my r=me.

::: firefox/lib/ui/browser.js
@@ +65,5 @@
>    var shiftKey = false;
>  
>    if (spec.private) {
>      menuItem = "#menu_newPrivateWindow";
> +    cmdKey = this.getEntity("privateBrowsingCmd.commandkey");

Lovely! :)

::: lib/tests/testUtils.js
@@ +37,4 @@
>  }
>  
>  function testUtils() {
> +  // Test getProperty with urls array

I don't see a need for this comment. All is already visible via the next line. Comments should not replicate what code is doing but explain it. If that is not necessary, just don't add a comment.

@@ +48,1 @@
>    }, undefined, "Invalid property has not raised an exception");

Please fix the comment this is not what we are testing here.

@@ +58,5 @@
> +  });
> +
> +  expect.throws(() => {
> +    var entity = utils.getEntity(TEST_DATA.dtds.urls, TEST_DATA.dtds.invalidEntity);
> +  }, undefined, "Invalid entity has not raised an exception");

Same here for the comment.

::: lib/utils.js
@@ +307,1 @@
>    // Add xhtml11.dtd to prevent missing entity errors with XHTML files

Please add a blank line above.

Comment 39

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v5.2.patch

Fixed code blocks comments added the missing blank line.

Comment 40

[Andrei Eftimie]

Comment on attachment 8520664 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.2.patch

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

I see some regressions:
> TEST-START | testAboutPrivateBrowsing.js | testCheckAboutPrivateBrowsing
> ERROR | Test Failure | {
>   "exception": {
>     "message": "urls is undefined", 
>     "lineNumber": 328, 
>     "name": "TypeError", 
>     "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///Users/andrei.eftimie/work/mozilla/bugs/1084333/mozmill-tests/lib/utils.js"
>   }
> }

> TEST-START | testOpenClosePBKeyboardShortcut.js | testOpenClosePBKeyboardShortcut
> ERROR | Test Failure | {
>   "exception": {
>     "message": "urls is undefined", 
>     "lineNumber": 328, 
>     "name": "TypeError", 
>     "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///Users/andrei.eftimie/work/mozilla/bugs/1084333/mozmill-tests/lib/utils.js"
>   }
> }

Please run all testruns and make sure to not regress anything.

Comment 41

[Teodor Druta]

Attachment: Part-2-Add-proprietes-handling-v5.3.patch

(In reply to Andrei Eftimie from comment #40)

Fixed.

All green now in testruns.
Testruns reports:
http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c42020c5
http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c42037bf
http://mozmill-crowd.blargon7.com/#/endurance/report/b67428b6e502c230ff85b6a4c4203ddf

Comment 42

[Andrei Eftimie]

Comment on attachment 8521363 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.3.patch

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

Looks great.

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/6cb54a77b634 (default)

Comment 43

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8521363 [details] [diff] [review]
Part-2-Add-proprietes-handling-v5.3.patch

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

::: lib/tests/testUtils.js
@@ +56,5 @@
> +  });
> +
> +  expect.throws(() => {
> +    var entity = utils.getEntity(TEST_DATA.dtds.urls, TEST_DATA.dtds.invalidEntity);
> +  }, undefined, "Non-existent property has not been found");

This is for entity.

Comment 44

[Andrei Eftimie]

(In reply to Andreea Matei [:AndreeaMatei] from comment #43)
> Comment on attachment 8521363 [details] [diff] [review]
> Part-2-Add-proprietes-handling-v5.3.patch
> 
> Review of attachment 8521363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/tests/testUtils.js
> @@ +56,5 @@
> > +  });
> > +
> > +  expect.throws(() => {
> > +    var entity = utils.getEntity(TEST_DATA.dtds.urls, TEST_DATA.dtds.invalidEntity);
> > +  }, undefined, "Non-existent property has not been found");
> 
> This is for entity.

Indeed we could replace `property` with `entity` here.
I've already landed the main patch, I propose to do this small change in a small followup patch.

Comment 45

[Andrei Eftimie]

Discussed on IRC to go with my proposal in comment 44.

Transplanted, all green:
https://hg.mozilla.org/qa/mozmill-tests/rev/237610e3006b (mozilla-aurora)

I'll wait for beta 1-2 days to see how this performs in 36 and 35
In the meantime Teodor please prepare a follow-up patch to change the expect message.

Comment 46

[Teodor Druta]

Attachment: fixtestutilsmsg.patch

Fixed the message in lib/tests/testUtils.js from property to entity.

Comment 47

[Andrei Eftimie]

Comment on attachment 8521444 [details] [diff] [review]
fixtestutilsmsg.patch

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

remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/f4928f35fda1 (default)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/34f040720bc7 (mozilla-aurora)

Comment 48

[Andrei Eftimie]

Transplanted:
[fix]      https://hg.mozilla.org/qa/mozmill-tests/rev/3eb83a90bee7 (mozilla-beta)
[followup] https://hg.mozilla.org/qa/mozmill-tests/rev/f6a75259abe4 (mozilla-beta)