Refactor window wrapper classes to get rid off the mixin classes

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: whimboo, Assigned: gmealer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [module-refactor])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
As decided the window wrapper classes have to be refactored so they don't require a mixin class anymore.

The work happening with this bug blocks my further work on bug 666245. Geo, it would be great if we can get patches as soon as possible. Adding dependency so bug 666245 doesn't get lost again.
(Reporter)

Comment 1

7 years ago
Once this patch has been landed I will also continue with the ContentWindow class on bug 639870.
Blocks: 639870
Created attachment 543325 [details] [diff] [review]
Windowing changes

This is the problem with doing these things in bugs. When we're building out the system, there's really no such thing as "a focused change," and trying to constrain to one will negatively affect progress. Pulling one string means rearranging the other strings to match.

Anyway, this diff spans a handful of files and functions. You'll need more context than just reading the code to understand why the changes were made and give me a decent check that I'm doing what I think I'm doing. It follows.

Testing included all unit tests and the UI tests. Everything passed. The bug we were talking about earlier turned out to be not making ChromeWindowWrapper a valid prototype object, which is also fixed.

Window model touchup, Summary of Changes
---------------------------------------------------------------

Classes vs. Mixins:

* getBrowserWindow() and openBrowserWindow() are now class factories. They return a default class, but that can be overridden to any child class by parameter.

* openBrowserWindow() returns ChromeWindowWrapper by default, per original behavior. getBrowserWindow() returns BrowserWindow by default, as original also did the mixin. 

* Ultimately, these should both default to ChromeWindowWrapper so they can be adopted by the A-Team, and we should have local passthrough functions that set the class parameter to return our specific classes. I'm unsure where to best put these and would like more input on that.

* ChromeWindowWrapper() now returns immediately if aDOMWindow is undefined. This behavior is necessary for it to be a valid prototype object for inheritance.

* ModalDialogObserver.prototype.observe() no longer creates a wrapper--client  code expected to create it in the callback if needed

* ChromeWindowWrapper.handleWindow no longer creates a wrapper--client code expected to create it in the callback if needed

Unclear terminology for window getters: 

Some is consistent w/ Firefox internals, but still unclear. In particular, "Last Opened" and "Most Recent" mean the same thing semantically, and any difference there is cultural only. Contributors need to not have to learn all the Firefox internal names to code in our system.

* getLastOpenedWindows -> getWindowsByAge
* getMostRecentWindows -> getWindowsByZOrder
* getLastOpenedWindow -> getNewestWindow
* getMostRecentWindow -> getTopmostWindow

Inconsistent filter handling: 

Some things took a filter callback w/ no value, especially the window list builders. This made them only appropriate for  _getWindow's flow rather than being useful directly even though they were ostensibly exported. First step was to enable all the "get" functions to take both filter and filter value.

* Added filter values to all functions taking filters.

_getWindow's structure made window retrieval complex:

_getWindow's taking the getWindowsBy* function as a callback made it unnecessarily complex, and caused it to have to handle the filter params. The change above suggested a different structure.

* Changed it to _getFirstWindow, now returns first value of a given list. get*Window functions now compute list with desired filter (see above), then pass it to _getFirstWindow for return value checking.

Too much reliance on checking null in window retrieval: 

We should favor exception handling unless we know the window might not be there so our tests call out unknown conditions automatically.

* _getFirstWindow takes a strict param (passed in through the wrappers) and favors throwing exceptions over returning null unless switched otherwise. This behavior also present in getNewestWindow and getTopmostWindow, as they use _getFirstWindow.

Automatic "create a browser if none exists" behavior isn't always wanted:

Though getBrowserWindow's open-if-not-there functionality is based on Mozmill's behavior, it needs a way to be switched off if you absolutely know you should already have a browser window open. Again, this is so tests fail loudly on unexpected conditions.

* Added aOpenIfNone param. Defaults to true to maintain old school behavior. If set to false, throws exceptions instead if no window is found.

Need a facility to easily set/reset custom modal dialog handlers:

Code was there, but reset function was private. Naming was inconsistent.

* _setDefaultModalDialogHandler -> setDefaultModalDialogHandler (public)
* initModalDialog -> setModalDialogHandler

handleWindow was doing too much:

it did the "get the last one that's not me" checking and did the callback. The first part is useful,  if you're not operating from callback (probably the default case).  

It's true that you could accomplish this by not specifying the callback (as documented) but then it's not actually handling the window anymore and the close param no longer made sense. This is a classic case of a function that's not cohesive because it does two different things, and should be split.

* Split handleWindow into findWindow (a la ModalDialogObserver's findWindow) and handleWindow, which requires the callback and uses findWindow.

UI test requires broken:

We didn't touch the UI test require paths when we changed init->head

* Paths fixed in UI tests.
Attachment #543325 - Flags: feedback?(hskupin)
Oh, and somehow forgot to mention, the BrowserMixin class is now a BrowserWindow child of ChromeWindowWrapper, which was the primary impetus for all this. :)

Tried BrowserWindowWrapper as a name for consistency but it was ridiculously long, and users shouldn't care that it's implemented as a wrapper. 

Kinda wonder if we shouldn't shorten the base class names too--I know ChromeWindow's taken, but ChromeWrapper and ContentWrapper would be fine for base class names where you do care a little more about implementation.

Comment 4

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15283699
(Reporter)

Comment 5

7 years ago
(In reply to comment #3)
> Kinda wonder if we shouldn't shorten the base class names too--I know
> ChromeWindow's taken, but ChromeWrapper and ContentWrapper would be fine for
> base class names where you do care a little more about implementation.

I wouldn't. Both should have the Wrapper suffix in their name. You will mostly never use them in any way.
(Reporter)

Comment 6

7 years ago
Comment on attachment 543325 [details] [diff] [review]
Windowing changes

> var windows = require('../ui/windows');

As agreed on the class should be moved to 'lib/api'. We an do all the re-arranging if the first patch has been landed. Otherwise it's hard to check the differences. Same would apply to browser.js which has to be moved to 'lib/ui'.

>+ * Gets the topmost browser window. If there are none at that time, optionally
>+ * opens one. Otherwise will raise an exception if none are found.
>  *
>  * @memberOf driver
>+ * @param {Class} [aWindowClass=browser.BrowserWindow] Class to construct and return
>+ * @param {Boolean] [aOpenIfNone=true] Open a new browser window if none are found.
>  * @returns {windows.Window}
>  */
>-function getBrowserWindow() {

When we raise an exception we also have to note that in the jsdoc lines. Instead of {Class} we should use the ChromeWindowWrapper base class as type.

>+function getBrowserWindow(aWindowClass, aOpenIfNone) {
>+  if (aOpenIfNone === undefined) {
>+    aOpenIfNone = true;
>+  }

I'm not a huge fan of overriding function parameters because that will lead to confusion.

>+  aWindowClass = aWindowClass || require('../mixins/browser').BrowserWindow;

I thought we don't use mixins anymore. So the require patch should point to 'lib/ui/browser' as agreed on.

>+  // If implicit open is off, turn on strict checking, and vice versa.
>+  let win = getTopmostWindow(windows.filterWindowByType, "navigator:browser", !aOpenIfNone);
>   let browser = null;

Keep in mind that the driver object will be part of Mozmill. We talked about that already. So using BrowserWindow here would require another refactoring of the driver module. IMO it should still fallback to ChromeWindowWrapper and we pass in BrowserWindow. "navigator:browser" can remain in there because it's a special browser function.

>  * @memberOf driver
>+ * @param {Class} [aWindowClass=window.ChromeWindowWrapper] Class to construct and return
>  * @returns {windows.Window}

Same as mentioned above.

>+function openBrowserWindow(aWindowClass) {
>+  aWindowClass = aWindowClass || windows.ChromeWindowWrapper;

That's what I expect and not windows.BrowserWindow as used above.

>+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
>+ *                                 Otherwise, returns null.
[..]
>-function _getWindow(aWindowCallback, aFilterCallback, aValue) {

Which needs a @throws param to be added.

>+function _getFirstWindow(aWindowList, aStrict) {

We should rename this function to _getFirstWindowFromList if we still need it. Meanwhile I don't think it's particular helpful.

>+  if (aWindowList === [] && aStrict)
>+    throw new errors.UnknownEntityError("No windows were found with the specified filter");

Don't throw an UnknownEntityError. That's not the type of error we want here. Entity is for l10n purposes. Also it would be great to have the specified filter in the error message. Shouldn't we compare with length === 0?

>+function getWindowsByAge(aFilterCallback, aFilterValue) {
>+  // Reverse the list, since naturally comes back old->new
>+  return _getWindows(services.wm.getEnumerator(""),
>+                     aFilterCallback, aFilterValue).reverse();

Lets do that in two lines of code. It's hard to read.

>+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
>+ *                                 Otherwise, returns null.

@throws missing. (That will also apply to any remaining function. I will not note it again)

>+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
>+ *                                 Otherwise, returns null.

The second line is already covered by @returns.

>+ * @class Wrapper for Firefox main DOM window

We only write tests for Firefox, so IMO we can call it 'Wrapper for a browser window'.

>+BrowserWindow.prototype.__defineGetter__("content",
>+                                         function BrowserWindow_getContent() {
>+  return this._controller.tabs;
>+});

We should better call that property 'tabs'. I already had trouble with it because it doesn't really match to content.

>+ * Waits for the page in the current tab to finish loading.
[..]
>+BrowserWindow.prototype.openURL

Well, this comment if off. Please adjust to the underlying action.

>+ * Waits for the page in the current tab to finish loading.
[..]
>+function BrowserWindow_waitForPageLoad(aTimeout) {
>+    // Support for background tabs has to be included
>+  this._controller.waitForPageLoad(aTimeout);

Please add a TODO comment that we have to also add support for background tabs or implement it right away. It is the first parameter in controller method. I just don't want to forget about it.

>+function ChromeWindowWrapper_findWindow(aFilterCallback, aFilterValue,
>+                                        aWait) {
>+  if (aWait === undefined) {
>+    aWait = true;

We should specify the timeout here instead of a boolean. 

>+  let myWindow = this._window;

Use 'var self = this' to check the _window property in findFilter.

>+  function findFilter(aWindow, aValue) {
>+    return (aWindow !== myWindow) && aFilterCallback(aWindow, aValue);
>+  }

Meanwhile I would even consider to use (aWindow.opener === self._window && ...), so that we really wait for the new window opened by the current one.

>+  if (aWait) {
>+    // wait up to 5 sec
>+    driver.waitFor(function () {
>+      win = driver.getNewestWindow(findFilter, aFilterValue, false);
>+      return win !== null;
>+    }, "Window has been found");
>+
>+    return win;
>+  }
>+  else {
>+    // return immediate result
>+    return driver.getNewestWindow(findFilter, aFilterValue, false);
>+  }

Instead of if/else move the aWait check into the waitFor call. That makes it easier.

> ChromeWindowWrapper.prototype.handleWindow =
> function ChromeWindowWrapper_handleWindow(aFilterCallback, aFilterValue,
>-                                    aCallback, aClose) {

I would completely refactor this method and only pass in a DOMWindow, the callback, and the closing flag.

>-ChromeWindowWrapper.prototype._setDefaultModalDialogHandler =
>+ChromeWindowWrapper.prototype.setDefaultModalDialogHandler =

Please leave it as it is. It's a private method on this class. Sorry for not having added it to the jsdoc comment.

In general it's a great improvement to what we have right now. Lets get this finalized as soon as possible so we can unblock other work which is dependent on this bug. Thanks!
Attachment #543325 - Flags: feedback?(hskupin) → feedback+
Mostly sounds good. Wanted to get some clarification on some points:

(In reply to comment #6)
> Comment on attachment 543325 [details] [diff] [review] [review]
> Windowing changes
> 
> > var windows = require('../ui/windows');
> 
> As agreed on the class should be moved to 'lib/api'. We an do all the
> re-arranging if the first patch has been landed. Otherwise it's hard to
> check the differences. Same would apply to browser.js which has to be moved
> to 'lib/ui'.

Agreed. We also agreed to do the file moves after this checkin because otherwise there'd be too much -everything +everything.

Still want me to make this change this time?


> 
> >+ * Gets the topmost browser window. If there are none at that time, optionally
> >+ * opens one. Otherwise will raise an exception if none are found.
> >  *
> >  * @memberOf driver
> >+ * @param {Class} [aWindowClass=browser.BrowserWindow] Class to construct and return
> >+ * @param {Boolean] [aOpenIfNone=true] Open a new browser window if none are found.
> >  * @returns {windows.Window}
> >  */
> >-function getBrowserWindow() {
> 
> When we raise an exception we also have to note that in the jsdoc lines.
> Instead of {Class} we should use the ChromeWindowWrapper base class as type.

Do we have to note exceptions? This isn't Java, where they need to be declared, and doing that requires analyzing every level in the code stack to see if anything beneath can throw.

I recommend we do not note that, except in special cases where the exception is very significant (TimeoutError, maybe). It's something we could only do inconsistently if at all. Exceptions shouldn't come up in the normal way of things anyway.

Re: Class, we discussed this in IRC. If I say ChromeWindowWrapper, that typically means an object instance of type ChromeWindowWrapper. What we need is the class/constructor of it or its child. I'm open to better ways to express that, but don't really want to go with what you suggested above.

> 
> >+function getBrowserWindow(aWindowClass, aOpenIfNone) {
> >+  if (aOpenIfNone === undefined) {
> >+    aOpenIfNone = true;
> >+  }
> 
> I'm not a huge fan of overriding function parameters because that will lead
> to confusion.

Not sure what you mean here. That's how you set a default parameter value when it's boolean.

Unless you mean the parameter itself? I personally kind of think it's an antipattern to have automatic behavior in a test API that can't be disabled. The theory is you should know exact state at all times, and the code should loudly fail if the state isn't as expected.

> 
> >+  aWindowClass = aWindowClass || require('../mixins/browser').BrowserWindow;
> 
> I thought we don't use mixins anymore. So the require patch should point to
> 'lib/ui/browser' as agreed on.

Yes, see above. You told me not to move the file until after the review so you could actually review changes rather than the file moving.

My intention was to quickly reorganize all this stuff after checking in, figured we could finish that + review in an hour or so.

> 
> >+  // If implicit open is off, turn on strict checking, and vice versa.
> >+  let win = getTopmostWindow(windows.filterWindowByType, "navigator:browser", !aOpenIfNone);
> >   let browser = null;
> 
> Keep in mind that the driver object will be part of Mozmill. We talked about
> that already. So using BrowserWindow here would require another refactoring
> of the driver module. IMO it should still fallback to ChromeWindowWrapper
> and we pass in BrowserWindow. "navigator:browser" can remain in there
> because it's a special browser function.

I mentioned this in my post comment.

I left in the BrowserWindow dependency because the function I'm replacing here originally had a mixin dependency that you put in. So they were both dependent on our code.

I know we can't sell it to A-Team that way, so suggested refactoring to the basic version you describe + a wrapper that lives in "our" modules that supplies BrowserWindow as the default instead. Then the tests use our wrapper instead of the Mozmill default.

There are other and perhaps cleaner ways to handle this (notably, setting a globally-accessible options singleton somewhere with your desired default window class) but they require us and the A-Team to agree on protocol.

That said, I do plan on creating that singleton to handle other options like how long to implicitly wait on control access, so if you want to go that route I'm game.

If we don't do one of those things, though, anyone who uses get/openBrowserWindow will have to import the browser module and supply the class, which gets a little tedious.

> 
> >  * @memberOf driver
> >+ * @param {Class} [aWindowClass=window.ChromeWindowWrapper] Class to construct and return
> >  * @returns {windows.Window}
> 
> Same as mentioned above.

Sure, noted. I'll go with whatever we decide above.

> 
> >+function openBrowserWindow(aWindowClass) {
> >+  aWindowClass = aWindowClass || windows.ChromeWindowWrapper;
> 
> That's what I expect and not windows.BrowserWindow as used above.

Yep. Your original was generic so I kept this one generic too. Ultimately you want functions available that both return BrowserWindows by default, though, however we manage to do it.

> 
> >+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
> >+ *                                 Otherwise, returns null.
> [..]
> >-function _getWindow(aWindowCallback, aFilterCallback, aValue) {
> 
> Which needs a @throws param to be added.

See comment above. Documenting all errors gets super-painful if you're not based on code that 100% does the same, and I'm not sure we want to get into that.

> 
> >+function _getFirstWindow(aWindowList, aStrict) {
> 
> We should rename this function to _getFirstWindowFromList if we still need
> it. Meanwhile I don't think it's particular helpful.

Its purpose is to be the one source of logic for the aStrict parameter.

I'll change the name if you want, but I wouldn't normally choose to reflect a data structure in a function name. Reason is that you may change the structure later, and if so you have to change the functions that use it too. Plus it mixes up implementation specifics with interface semantics.

> 
> >+  if (aWindowList === [] && aStrict)
> >+    throw new errors.UnknownEntityError("No windows were found with the specified filter");
> 
> Don't throw an UnknownEntityError. That's not the type of error we want
> here. Entity is for l10n purposes. Also it would be great to have the
> specified filter in the error message. Shouldn't we compare with length ===
> 0?

length === 0 and [] === [] are equivalent, I think. I'll make the change though, if you find it clearer.

I'll change the error. I'm not sure about the filter name one; is the name property set on every function? If not, that'll be a royal pain to do.

> 
> >+function getWindowsByAge(aFilterCallback, aFilterValue) {
> >+  // Reverse the list, since naturally comes back old->new
> >+  return _getWindows(services.wm.getEnumerator(""),
> >+                     aFilterCallback, aFilterValue).reverse();
> 
> Lets do that in two lines of code. It's hard to read.

OK, will do.

> 
> >+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
> >+ *                                 Otherwise, returns null.
> 
> @throws missing. (That will also apply to any remaining function. I will not
> note it again)

Yep, let's decide above, then I'll apply globally if necessary.

> 
> >+ * @param {Boolean} [aStrict=true] If true, throws error if no window found.
> >+ *                                 Otherwise, returns null.
> 
> The second line is already covered by @returns.

I'll look at the returns line. I suspect I repeated it because it's specifically relevant to what that parameter does (changes the return protocol on no window found). If so, I'd probably like to keep it. 

> 
> >+ * @class Wrapper for Firefox main DOM window
> 
> We only write tests for Firefox, so IMO we can call it 'Wrapper for a
> browser window'.

...so far.

Won't be once we're covering Fennic though.

I'd prefer to keep it specific unless you have a particularly good reason for not doing so (generally true). But I'd probably change it to Desktop Firefox to differentiate from Mobile.

> 
> >+BrowserWindow.prototype.__defineGetter__("content",
> >+                                         function BrowserWindow_getContent() {
> >+  return this._controller.tabs;
> >+});
> 
> We should better call that property 'tabs'. I already had trouble with it
> because it doesn't really match to content.

I was trying to distinguish between "tabs" the UI control (i.e. "buttons" that switch tabs) and "tabs" the content inside them.

How does it not match to content? It's a list of the content documents inside the tabs.

I'm kind of hesitant to go back to "tabs"; the only advantage I see there is alignment with Mozmill naming, but the tests won't be referencing the Mozmill version of that property at all. contentTabs maybe? 

I'll let you make the final call here (including "tabs," if you really want it).

> 
> >+ * Waits for the page in the current tab to finish loading.
> [..]
> >+BrowserWindow.prototype.openURL
> 
> Well, this comment if off. Please adjust to the underlying action.

Sure, looks like maybe a chunk of comment for waitForPageLoad got left up there.

> 
> >+ * Waits for the page in the current tab to finish loading.
> [..]
> >+function BrowserWindow_waitForPageLoad(aTimeout) {
> >+    // Support for background tabs has to be included
> >+  this._controller.waitForPageLoad(aTimeout);
> 
> Please add a TODO comment that we have to also add support for background
> tabs or implement it right away. It is the first parameter in controller
> method. I just don't want to forget about it.

The full waitForPageLoad protocol (all params) is exposed in the test porting branch that we need to merge Real Soon. I can still TODO it if you want, but rest assured we won't forget and it's already done. :)

> 
> >+function ChromeWindowWrapper_findWindow(aFilterCallback, aFilterValue,
> >+                                        aWait) {
> >+  if (aWait === undefined) {
> >+    aWait = true;
> 
> We should specify the timeout here instead of a boolean. 

OK. The tactic we took in openURL (at least the version being merged, can't remember in main branch) is that timeout=0 is don't wait. Still OK with that?

> 
> >+  let myWindow = this._window;
> 
> Use 'var self = this' to check the _window property in findFilter.
> 
> >+  function findFilter(aWindow, aValue) {
> >+    return (aWindow !== myWindow) && aFilterCallback(aWindow, aValue);
> >+  }

Why? Is there a clarity problem there? 

Mine's perfectly valid code: checks a DOM window vs. a DOM window, and I don't need the this/self for anything else. Unless there's a specific clarity issue or some functional thing I missed, I don't want to make this change.

> 
> Meanwhile I would even consider to use (aWindow.opener === self._window &&
> ...), so that we really wait for the new window opened by the current one.

I don't get this one, can you explain further? I might not be familiar with .opener?

> 
> >+  if (aWait) {
> >+    // wait up to 5 sec
> >+    driver.waitFor(function () {
> >+      win = driver.getNewestWindow(findFilter, aFilterValue, false);
> >+      return win !== null;
> >+    }, "Window has been found");
> >+
> >+    return win;
> >+  }
> >+  else {
> >+    // return immediate result
> >+    return driver.getNewestWindow(findFilter, aFilterValue, false);
> >+  }
> 
> Instead of if/else move the aWait check into the waitFor call. That makes it
> easier.

I don't want it going into waitFor if you said not to wait. I'm pretty certain the code will be less obvious that way because you said not to wait, and there it is waiting (even if you immediately return true).

If you want to show me what you have in mind, I can reconsider, but right now the code does exactly what it says it does, which is the goal.

> 
> > ChromeWindowWrapper.prototype.handleWindow =
> > function ChromeWindowWrapper_handleWindow(aFilterCallback, aFilterValue,
> >-                                    aCallback, aClose) {
> 
> I would completely refactor this method and only pass in a DOMWindow, the
> callback, and the closing flag.

OK. So the flow then is they find their own stuff first, get the DOMWindow back from that, then call it?

fooWindow.handleWindow(fooWindow.findWindow(someFilter, someCriteria), someCallback)

vs.

fooWindow.handleWindow(someFilter, someCriteria, someCallback)

Dunno. Even considering that you'd probably break the one above apart into two lines, I think I like the last one better. Did you have some different flow in mind?

Note that you'd have to use findWindow here to get the "that isn't me" part of your finding algorithm (and I agree it's needed).

Again, I'll let this be your call. I don't care much because I don't intend to use handleWindow much, but I think it works a little better in the current way.

> 
> >-ChromeWindowWrapper.prototype._setDefaultModalDialogHandler =
> >+ChromeWindowWrapper.prototype.setDefaultModalDialogHandler =
> 
> Please leave it as it is. It's a private method on this class. Sorry for not
> having added it to the jsdoc comment.

I knew it was private, even acknowledged I was promoting it to public (and why) in the comment I posted with the patch :)

We need a function to reset to default. In pseudocode:

set new modal handler
try
  do some stuff to launch the modal
finally
  reset to default modal handler

So it can either be that function or another function that does nothing but call that function. Why does this one need to remain private?

> 
> In general it's a great improvement to what we have right now. Lets get this
> finalized as soon as possible so we can unblock other work which is
> dependent on this bug. Thanks!

Yep, sorry for the delays while I was pulled aside on other stuff.

For all of my commenting back, for the most part I'm willing to go with your finals comments on anything other than stuff I said "really don't want to". Even those, if I can see why you want the change I can probably live with.

Just please take a second look with my comments in mind and I'll get a final patch implemented in the morning against your followup.
(Reporter)

Comment 8

7 years ago
(In reply to comment #7)
> > As agreed on the class should be moved to 'lib/api'. We an do all the
> > re-arranging if the first patch has been landed. Otherwise it's hard to
> > check the differences. Same would apply to browser.js which has to be moved
> > to 'lib/ui'.
> 
> Agreed. We also agreed to do the file moves after this checkin because
> otherwise there'd be too much -everything +everything.
> 
> Still want me to make this change this time?

No, as mentioned below for the other modules we should do that afterward. Simply missed to remove that comment last time.

> > When we raise an exception we also have to note that in the jsdoc lines.
> 
> Do we have to note exceptions? This isn't Java, where they need to be
> declared, and doing that requires analyzing every level in the code stack to
> see if anything beneath can throw.
> 
> I recommend we do not note that, except in special cases where the exception
> is very significant (TimeoutError, maybe). It's something we could only do
> inconsistently if at all. Exceptions shouldn't come up in the normal way of
> things anyway.

http://code.google.com/p/jsdoc-toolkit/wiki/TagThrows

It would at least help us on the API level to not have to check the code of methods for possible exceptions, but a look into the docs would explain it. 

> > Instead of {Class} we should use the ChromeWindowWrapper base class as type.
> 
> Re: Class, we discussed this in IRC. If I say ChromeWindowWrapper, that
> typically means an object instance of type ChromeWindowWrapper. What we need
> is the class/constructor of it or its child. I'm open to better ways to
> express that, but don't really want to go with what you suggested above.

Makes sense. So the description should be clear enough then. That would be totally ok.

> > >+function getBrowserWindow(aWindowClass, aOpenIfNone) {
> > >+  if (aOpenIfNone === undefined) {
> > >+    aOpenIfNone = true;
> > >+  }
> > 
> > I'm not a huge fan of overriding function parameters because that will lead
> > to confusion.
> 
> Not sure what you mean here. That's how you set a default parameter value
> when it's boolean.

It's more that you re-assign a new value to aOpenIfNone instead of using a new variable for the cleaned-up value. It's a read-only parameter, so I wouldn't expect that it can get assigned a new value. Also what I haven't seen the last time, you missed the typeof operator for this check.

> I left in the BrowserWindow dependency because the function I'm replacing
> here originally had a mixin dependency that you put in. So they were both
> dependent on our code.

So leave it but please file a follow-up bug so we can get this worked out.

> > >+function openBrowserWindow(aWindowClass) {
> > >+  aWindowClass = aWindowClass || windows.ChromeWindowWrapper;
> > 
> > That's what I expect and not windows.BrowserWindow as used above.
> 
> Yep. Your original was generic so I kept this one generic too. Ultimately
> you want functions available that both return BrowserWindows by default,
> though, however we manage to do it.

As you said we can have our own wrappers. So it should be way easier and we do not force the import of the browser module, but keep the driver module clean. We could also augment new methods into the driver class.

> > >+function _getFirstWindow(aWindowList, aStrict) {
[..] 
> I'll change the name if you want, but I wouldn't normally choose to reflect
> a data structure in a function name. Reason is that you may change the
> structure later, and if so you have to change the functions that use it too.
> Plus it mixes up implementation specifics with interface semantics.

Good call. But even in case of a change to the structure the interface will change because the first parameter will not be a list anymore. Anyway, that's all about naming. Do what you think is the best. It's a non-blocker.

> > >+  if (aWindowList === [] && aStrict)
> > >+    throw new errors.UnknownEntityError("No windows were found with the specified filter");
> > 
> > Don't throw an UnknownEntityError. That's not the type of error we want
> > here. Entity is for l10n purposes. Also it would be great to have the
> > specified filter in the error message. Shouldn't we compare with length ===
> > 0?
> 
> length === 0 and [] === [] are equivalent, I think. I'll make the change
> though, if you find it clearer.

We should be consistent with other code around those lines where we already have used the length property. Just make it consistent. Either way would be fine.

> I'll change the error. I'm not sure about the filter name one; is the name
> property set on every function? If not, that'll be a royal pain to do.

Not sure what you mean here. Are you talking about filterWindowByMethod?

> I'm kind of hesitant to go back to "tabs"; the only advantage I see there is
> alignment with Mozmill naming, but the tests won't be referencing the
> Mozmill version of that property at all. contentTabs maybe? 

For ui elements the tabs will be available under browser.ui.tabBar.tabs. So we would have a distinction between ui and back-end. IMHO I would assume that for Mozmill the interface will not change and tabs will remain. So our chrome wrapper should apply to it.

> The full waitForPageLoad protocol (all params) is exposed in the test
> porting branch that we need to merge Real Soon. I can still TODO it if you
> want, but rest assured we won't forget and it's already done. :)

So please make sure we can get the test branch merged ASAP. Changes to the API should go in immediately and independent from tests. We should find the best strategy here, similar to what we already have started to discuss in our chat yesterday.

> > >+function ChromeWindowWrapper_findWindow(aFilterCallback, aFilterValue,
> > >+                                        aWait) {
> > >+  if (aWait === undefined) {
> > >+    aWait = true;
> > 
> > We should specify the timeout here instead of a boolean. 
> 
> OK. The tactic we took in openURL (at least the version being merged, can't
> remember in main branch) is that timeout=0 is don't wait. Still OK with that?

Sure. As long as a test can overwrite the default timeout.

> > >+  let myWindow = this._window;
> > 
> > Use 'var self = this' to check the _window property in findFilter.
> > 
> > >+  function findFilter(aWindow, aValue) {
> > >+    return (aWindow !== myWindow) && aFilterCallback(aWindow, aValue);
> > >+  }
> 
> Why? Is there a clarity problem there? 
> 
> Mine's perfectly valid code: checks a DOM window vs. a DOM window, and I
> don't need the this/self for anything else. Unless there's a specific
> clarity issue or some functional thing I missed, I don't want to make this
> change.

'this' inside findFilter is the function itself and not the object contained in. So far cases like those you usually define a new reference to the current instance of the object, you can use in a callback (closure) to work with.

> > Meanwhile I would even consider to use (aWindow.opener === self._window &&
> > ...), so that we really wait for the new window opened by the current one.
> 
> I don't get this one, can you explain further? I might not be familiar with
> .opener?

.opener is the reference to the window, which has triggered the new window. With a check like that we can ensure that when multiple windows are opening a new window at the same time, we retrieve the right one. But .opener can also be null if no parent window is wanted. We can do it later, but in this case please file a bug.

> > >+  if (aWait) {
> > >+    // wait up to 5 sec
> > >+    driver.waitFor(function () {
> > >+      win = driver.getNewestWindow(findFilter, aFilterValue, false);
> > >+      return win !== null;
> > >+    }, "Window has been found");
> > >+
> > >+    return win;
> > >+  }
> > >+  else {
> > >+    // return immediate result
> > >+    return driver.getNewestWindow(findFilter, aFilterValue, false);
> > >+  }
> > 
> > Instead of if/else move the aWait check into the waitFor call. That makes it
> > easier.
> 
> I don't want it going into waitFor if you said not to wait. I'm pretty
> certain the code will be less obvious that way because you said not to wait,
> and there it is waiting (even if you immediately return true).
> 
> If you want to show me what you have in mind, I can reconsider, but right
> now the code does exactly what it says it does, which is the goal.

If timeout would be 0 we could set a delay of 0ms and waitFor would return immediately too. That way we also raise the appropriate exception that the window has not been found, which is not the case for your second code path.

> fooWindow.handleWindow(fooWindow.findWindow(someFilter, someCriteria),
> someCallback)
> 
> vs.
> 
> fooWindow.handleWindow(someFilter, someCriteria, someCallback)
> 
> Dunno. Even considering that you'd probably break the one above apart into
> two lines, I think I like the last one better. Did you have some different
> flow in mind?
> 
> Note that you'd have to use findWindow here to get the "that isn't me" part
> of your finding algorithm (and I agree it's needed).
> 
> Again, I'll let this be your call. I don't care much because I don't intend
> to use handleWindow much, but I think it works a little better in the
> current way.

Thought more about and yes the interface is easier to use. Lets keep it as it is.

> > >-ChromeWindowWrapper.prototype._setDefaultModalDialogHandler =
> > >+ChromeWindowWrapper.prototype.setDefaultModalDialogHandler =
> > 
> > Please leave it as it is. It's a private method on this class. Sorry for not
> > having added it to the jsdoc comment.
> 
> I knew it was private, even acknowledged I was promoting it to public (and
> why) in the comment I posted with the patch :)
> 
> We need a function to reset to default. In pseudocode:
> 
> set new modal handler
> try
>   do some stuff to launch the modal
> finally
>   reset to default modal handler
> 
> So it can either be that function or another function that does nothing but
> call that function. Why does this one need to remain private?

Why would you set a custom modal handler in a test which you wouldn't use and reset to the default handler again? Even in cases when you want to check for a possible appearing modal dialog, you will have to call waitForDialog and wrap it into try/catch to do further checks if it doesn't appear. This step automatically resets the handler to the default one. Can you please give me a real usage scenario?
(In reply to comment #8)

> > I recommend we do not note that, except in special cases where the exception
> > is very significant (TimeoutError, maybe). It's something we could only do
> > inconsistently if at all. Exceptions shouldn't come up in the normal way of
> > things anyway.
> 
> http://code.google.com/p/jsdoc-toolkit/wiki/TagThrows
> 
> It would at least help us on the API level to not have to check the code of
> methods for possible exceptions, but a look into the docs would explain it. 

Yeah, I know the tag exists and we're technically able to do it. The only languages where I know it's typical to document exceptions are the ones that force you to declare which exceptions can be thrown (Java, primarily).

It's not that I'm pushing back against this particular change; it's that if we want to do it in a way that's useful, it has to be consistent. That means if:

A calls B calls C calls D calls E, and E throws...

A, B, C, D, and E all have to have the @throws tag to be consistent. 

The problem comes in when C->E are either platform or Mozmill library/object calls. To do this right, we'd have to research all those base functions and know exactly what they throw. 

This is no problem in a language like Java because they were forced to declare so you just roll up everything you directly call and redeclare the @throws. In our case, this is a ton of work because that stuff isn't documented.

I kind of afraid the best we could do is inconsistent, and inconsistent is worse than none because not only do you do the work, people have to second-guess you and look at code anyway.

I hear what you're saying re: making it useful, but tests generally shouldn't have exception handling in them anyway. So this would primarily be for us, but we'll probably have to look at code anyhow. I'm just concerned this isn't worth the cost.

We have to go over the docs and redo tags and such then. Can we at least revisit this at that time?

> > > I'm not a huge fan of overriding function parameters because that will lead
> > > to confusion.
> > 
> > Not sure what you mean here. That's how you set a default parameter value
> > when it's boolean.
> 
> It's more that you re-assign a new value to aOpenIfNone instead of using a
> new variable for the cleaned-up value. It's a read-only parameter, so I
> wouldn't expect that it can get assigned a new value. Also what I haven't
> seen the last time, you missed the typeof operator for this check.

This is a pretty standard pattern. It's equivalent to python's def foo(bar="default"), which internally does:

bar = (bar == None)? "default" : bar

It's just that Javascript doesn't have this as a syntactic sugar feature, so you spin your own. Typically it's done as:

bar = bar | "default"

It's actually kind of unusual, in my experience, to assign off to other variables when you're assigning defaults. It's the only time where I support writing to an input variable. 

It's true that Crockford does in his "elements of Javascript style" but every other reference I've found reassigns to the param. My assumption is that Crockford's style is (like in many other cases) a little outdated.

I did a little more research and I see your point on typeof foo === 'undefined'. It's done that way in page site code because the "undefined" constant can be reassigned by other libraries. I doubt it's necessary in our case but I'll make the change.

> > I'll change the error. I'm not sure about the filter name one; is the name
> > property set on every function? If not, that'll be a royal pain to do.
> 
> Not sure what you mean here. Are you talking about filterWindowByMethod?

I was referring to your idea to make the exception message call out the filter used. I'm not sure that's straightforward to do.

> For ui elements the tabs will be available under browser.ui.tabBar.tabs. So
> we would have a distinction between ui and back-end. IMHO I would assume
> that for Mozmill the interface will not change and tabs will remain. So our
> chrome wrapper should apply to it.

OK, I'll make the change. In my case, it bugs me because when you have the same name in two places, it should refer to the same thing; it's a learning tool. This will be an exception to that rule.

> So please make sure we can get the test branch merged ASAP. Changes to the
> API should go in immediately and independent from tests. We should find the
> best strategy here, similar to what we already have started to discuss in
> our chat yesterday.

No doubt I've sat on that merge too long, but you're not going to get API changes before tests. The flow while we're incubating is to shape the API as needed while the tests are being written.

It's one thing to regiment this after the first version is released but while we're in development you might need to put up with my more organic flow on this a bit.

That said, stuff like merge branches shouldn't be outstanding more than a week or so, so I'll work with you to tighten this up.

> > > >+  let myWindow = this._window;
> > > 
> > > Use 'var self = this' to check the _window property in findFilter.
> > > 
> > > >+  function findFilter(aWindow, aValue) {
> > > >+    return (aWindow !== myWindow) && aFilterCallback(aWindow, aValue);
> > > >+  }
> > 
> > Why? Is there a clarity problem there? 
> > 
> > Mine's perfectly valid code: checks a DOM window vs. a DOM window, and I
> > don't need the this/self for anything else. Unless there's a specific
> > clarity issue or some functional thing I missed, I don't want to make this
> > change.
> 
> 'this' inside findFilter is the function itself and not the object contained
> in. So far cases like those you usually define a new reference to the
> current instance of the object, you can use in a callback (closure) to work
> with.

Check again; I rewrote the filter so the object isn't referenced inside it. I did that more or less exactly because the this/self thing bugs me a little as a hack, and I want to avoid it if we can.

Technically, with waitFor, we should be using the thisObject parameter in that situation anyway.

Anyway, when you only need the window property once, you shouldn't be calling it repeatedly; best to save it off and reuse it. It's a property, so you have no idea if it's expensive to get or cheap.

> .opener is the reference to the window, which has triggered the new window.
> With a check like that we can ensure that when multiple windows are opening
> a new window at the same time, we retrieve the right one. But .opener can
> also be null if no parent window is wanted. We can do it later, but in this
> case please file a bug.

OK. I'll try to understand what you mean here. You might need to file the bug on this one, though--not really fair to me to have an idea and then tell me to figure it out and file. I kind of expect you to file bugs explaining your own ideas. :)

> 
> > > >+  if (aWait) {
> > > >+    // wait up to 5 sec
> > > >+    driver.waitFor(function () {
> > > >+      win = driver.getNewestWindow(findFilter, aFilterValue, false);
> > > >+      return win !== null;
> > > >+    }, "Window has been found");
> > > >+
> > > >+    return win;
> > > >+  }
> > > >+  else {
> > > >+    // return immediate result
> > > >+    return driver.getNewestWindow(findFilter, aFilterValue, false);
> > > >+  }
> > > 
> > > Instead of if/else move the aWait check into the waitFor call. That makes it
> > > easier.
> > 
> > I don't want it going into waitFor if you said not to wait. I'm pretty
> > certain the code will be less obvious that way because you said not to wait,
> > and there it is waiting (even if you immediately return true).
> > 
> > If you want to show me what you have in mind, I can reconsider, but right
> > now the code does exactly what it says it does, which is the goal.
> 
> If timeout would be 0 we could set a delay of 0ms and waitFor would return
> immediately too. That way we also raise the appropriate exception that the
> window has not been found, which is not the case for your second code path.

findWindow doesn't raise an exception in the "don't wait" case, since that's used to figure out if the window exists. That's precisely why I wrote it that way.

> > We need a function to reset to default. In pseudocode:
> > 
> > set new modal handler
> > try
> >   do some stuff to launch the modal
> > finally
> >   reset to default modal handler
> > 
> > So it can either be that function or another function that does nothing but
> > call that function. Why does this one need to remain private?
> 
> Why would you set a custom modal handler in a test which you wouldn't use
> and reset to the default handler again? Even in cases when you want to check
> for a possible appearing modal dialog, you will have to call waitForDialog
> and wrap it into try/catch to do further checks if it doesn't appear. This
> step automatically resets the handler to the default one. Can you please
> give me a real usage scenario?


I don't understand. I'm saying you would need to reset the default handler immediately after handling the dialog, which you seem to be saying too. Are you saying some other piece of code resets default handler? And before the test is finished (i.e. teardown is too late)?

As for a real world example, since I need to reanalyze your unit test code, let me follow up with that and then talk to you over IRC. I'll roll results back up into the bug.

Rest of the comments sounded fine, and I'll make the changes.
(Reporter)

Comment 10

7 years ago
(In reply to comment #9)
> It's not that I'm pushing back against this particular change; it's that if
> we want to do it in a way that's useful, it has to be consistent. That means
> if:
> 
> A calls B calls C calls D calls E, and E throws...
> 
> A, B, C, D, and E all have to have the @throws tag to be consistent. 

Depends on if something is getting caught in a subcall and alternatively handled. In cases like those the exception will not be populated up to A.

> I hear what you're saying re: making it useful, but tests generally
> shouldn't have exception handling in them anyway. So this would primarily be
> for us, but we'll probably have to look at code anyhow. I'm just concerned
> this isn't worth the cost.

Agreed for now. Lets see how it works out for us and how often we have to check underlying code for possible exception code paths. If that's too often we can add documentation from the base up if needed.

> It's actually kind of unusual, in my experience, to assign off to other
> variables when you're assigning defaults. It's the only time where I support
> writing to an input variable. 
> 
> It's true that Crockford does in his "elements of Javascript style" but
> every other reference I've found reassigns to the param. My assumption is
> that Crockford's style is (like in many other cases) a little outdated.

Ok, lets do it as it is right now. But those assignments should really only be allowed at the top of the function body and no-where else.

> > > I'll change the error. I'm not sure about the filter name one; is the name
> > > property set on every function? If not, that'll be a royal pain to do.
> > 
> > Not sure what you mean here. Are you talking about filterWindowByMethod?
> 
> I was referring to your idea to make the exception message call out the
> filter used. I'm not sure that's straightforward to do.

Oh, ok. You should be able to get the function name:
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function#Properties_2

Oh wait. Checking the code shows me that the filter callback is not there. With the code I have had before we would have had the change.

> > For ui elements the tabs will be available under browser.ui.tabBar.tabs. So
> > we would have a distinction between ui and back-end. IMHO I would assume
> > that for Mozmill the interface will not change and tabs will remain. So our
> > chrome wrapper should apply to it.
> 
> OK, I'll make the change. In my case, it bugs me because when you have the
> same name in two places, it should refer to the same thing; it's a learning
> tool. This will be an exception to that rule.

Or talk to Clint if they are up for changing it to contentTabs. Your proposal was fine but we would need their feedback.

> That said, stuff like merge branches shouldn't be outstanding more than a
> week or so, so I'll work with you to tighten this up.

Sounds good.

> > 'this' inside findFilter is the function itself and not the object contained
> > in. So far cases like those you usually define a new reference to the
> > current instance of the object, you can use in a callback (closure) to work
> > with.
> 
> Check again; I rewrote the filter so the object isn't referenced inside it.
> I did that more or less exactly because the this/self thing bugs me a little
> as a hack, and I want to avoid it if we can.

Right, but myWindow doesn't give me more information, especially when we are in the callback. It's just another variant with a different name. self instead is used way more often, not only in our framework. For me it's clearer to what it refers to.

> Technically, with waitFor, we should be using the thisObject parameter in
> that situation anyway.

We have only a single use case for it. So not sure if we really would need this parameter. So far I can only see its usage in this method.

> Anyway, when you only need the window property once, you shouldn't be
> calling it repeatedly; best to save it off and reuse it. It's a property, so
> you have no idea if it's expensive to get or cheap.

So what when the value of the property changes, you will not be able to detect that because you have temporarily saved the initial state and can't compare it anymore with the current value. It's not that important here but e.g. in the addons.js module.

> OK. I'll try to understand what you mean here. You might need to file the
> bug on this one, though--not really fair to me to have an idea and then tell
> me to figure it out and file. I kind of expect you to file bugs explaining
> your own ideas. :)

Ok, will do. Should this be marked blocking the refactoring round of Milestone 2?

> > > > >+  if (aWait) {
> > > > >+    // wait up to 5 sec
> > > > >+    driver.waitFor(function () {
> > > > >+      win = driver.getNewestWindow(findFilter, aFilterValue, false);
> > > > >+      return win !== null;
> > > > >+    }, "Window has been found");
> > > > >+
> > > > >+    return win;
> > > > >+  }
> > > > >+  else {
> > > > >+    // return immediate result
> > > > >+    return driver.getNewestWindow(findFilter, aFilterValue, false);
> > > > >+  }
[..]
> > If timeout would be 0 we could set a delay of 0ms and waitFor would return
> > immediately too. That way we also raise the appropriate exception that the
> > window has not been found, which is not the case for your second code path.
> 
> findWindow doesn't raise an exception in the "don't wait" case, since that's
> used to figure out if the window exists. That's precisely why I wrote it
> that way.

I'm still not sure why we need that. In the normal case you always expect that a window has been found - and we should raise an exception. If you want to do a negative test, you could use expect.throws to catch it.

Also you are hiding the exception handling behind a numerical parameter, which is not obvious to outside code.

> I don't understand. I'm saying you would need to reset the default handler
> immediately after handling the dialog, which you seem to be saying too. Are
> you saying some other piece of code resets default handler? And before the
> test is finished (i.e. teardown is too late)?

Well, you have to call .waitForDialog() right after you have initiated the action. It will reset the modal dialog handler to the default one automatically. There is no such code to reset the handler necessary in the test.
(In reply to comment #10)

> Depends on if something is getting caught in a subcall and alternatively
> handled. In cases like those the exception will not be populated up to A.

True that; that's part of the analysis too.

> Ok, lets do it as it is right now. But those assignments should really only
> be allowed at the top of the function body and no-where else.

Definitely agreed here. If I did that wrong, I'll change it.

> Oh, ok. You should be able to get the function name:
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/
> Function#Properties_2
> 
> Oh wait. Checking the code shows me that the filter callback is not there.
> With the code I have had before we would have had the change.

Well, let me look at it. I can probably do something sane with this that'll let us report the name, if I can count on it being there. When I refactored, I didn't know we'd want to do that, of course.

> Or talk to Clint if they are up for changing it to contentTabs. Your
> proposal was fine but we would need their feedback.

Sure. In the meantime, I'll make your requested change.

> Right, but myWindow doesn't give me more information, especially when we are
> in the callback. It's just another variant with a different name. self
> instead is used way more often, not only in our framework. For me it's
> clearer to what it refers to.

OK. Clearer is key. I'll change it.

> 
> > Anyway, when you only need the window property once, you shouldn't be
> > calling it repeatedly; best to save it off and reuse it. It's a property, so
> > you have no idea if it's expensive to get or cheap.
> 
> So what when the value of the property changes, you will not be able to
> detect that because you have temporarily saved the initial state and can't
> compare it anymore with the current value. It's not that important here but
> e.g. in the addons.js module.

If the DOMWindow value of the calling window changes, I suspect it'd break a heck of a lot more than that! I just meant in this situation, where that almost has to be a static value. Like I said, though, no worries--I'll change it.

> Ok, will do. Should this be marked blocking the refactoring round of
> Milestone 2?

We should commune and figure out a better way to do these tracking bugs, milestones, etc. We're kind of off the rails on what I set up before, and especially with the Pivotal integration we may want to tweak things a bit.

But for now, sure.

> I'm still not sure why we need that. In the normal case you always expect
> that a window has been found - and we should raise an exception. If you want
> to do a negative test, you could use expect.throws to catch it.
> 
> Also you are hiding the exception handling behind a numerical parameter,
> which is not obvious to outside code.

Well, that's a good point. It was behind a boolean param until we discussed making the timeout value the param value, but you're right--it's kind of a kludge either way.

OK, I'm convinced. It'll throw an exception if not found, and I'll simplify that routine.

> Well, you have to call .waitForDialog() right after you have initiated the
> action. It will reset the modal dialog handler to the default one
> automatically. There is no such code to reset the handler necessary in the
> test.

OK, that was the key I was missing. I'll put it back to private.

We're settled on everything. Next post will be the patch. Thanks for hanging with!
(Reporter)

Updated

7 years ago
Blocks: 671110
Created attachment 545795 [details] [diff] [review]
Windowing changes, modified based on feedback

Here's the fixes made (and not):

* Move windows class to lib/api		Bug 671452
* Move browser class to lib/ui		Bug 671452
* @throws				deferred
* typeof = 'undefined'			DONE
* Genericized vs. local browser funcs	Bug 671458
* _getFirstWindowFromList		DONE
* Change UnknownEntityError		DONE
* length == 0				DONE
* call out filter name			DONE
* split getWindowsByAge			DONE
* review comment vs. @returns		DONE
* revert content to tabs		WONTFIX
* openURL comment fix			DONE
* findWindow wait->number		DONE
* findFilter aWindow->wrapper		DONE
* Remove "null if no wait" logic	DONE
* _setDefaultModalDialogHandler	        DONE

All tests ran and passed.

Regarding reverting content -> tabs, that wontfix needs more explanation especially since I'm flip-flopping on you after saying, however grudgingly, I'd do it.

I'm uncomfortable renaming content until we've discussed it more. It's also not a property introduced in this patch (though it's true I touched it in changing the browser window from a mixin) and thus wasn't really up for discussion as part of this checkin. That's the basics of why I'm wontfixing it for now.

More specifically:

Regarding your arguments for it, after more thought I'm not convinced. It seems like they boil down to being a little confusing for you since we're changing semantics from what you're already used to, and that it doesn't align with Mozmill naming.

I'm sympathetic to the first argument, but you're not really the target audience here; you're too expert, basically, so you're no longer unbiased or learning. Our target audience is people brand new to using our API, and even Mozmill in general.

I know there's going to be a little bit of pain (across the board) coming off vanilla Mozmill to a rethought API, but you'd get used to it quickly enough. If we pick a bad name, every single person who encounters it will be worse off than you would be if we picked a good-but-different name.

As far as the second argument goes, that's a non-issue as far as I'm concerned. We are, for all intents and purposes, treating Mozmill as a low-level library and covering its interface with higher-level semantics and flows as appropriate. 

Staying close to Mozmill is not a design goal; I've only aligned where necessary to sell stuff back to A-Team, but the BrowserWindow will remain our class and thus the design is ours and ours alone. My choice to simply expose their tabs object under a different name was born of expediency, and may not even by the final way we do this.

And at the end of it all, I just really hate 'tabs' as a name, and don't think I have to convince Mozmill to change their name to pick something I feel is more appropriate for an API designed by us. 

But at the same time, perhaps 'content' has a specific Firefox association somewhere I don't get and am crossing up with. So, if you like, we can discuss what you'd find more clear. 

Here's my reasoning:

That property is an object that primarily serves up content documents for currently displayed pages; tabs are really a selector, not what it's actually returning. So, tabs.activeTab() == content document from the active tab, tabs.getTab(2) == content document from tab 2, etc.

Good design says you want to name accessors after what they return, not what they take as selectors, which is why the Mozmill naming is wrong IMO.

So I'd be OK with 'content' (as I picked), 'documents', 'tabContent', 'tabDocuments', 'contentDocuments'; or perhaps even 'pages', 'tabPages', 'pageContent', 'pageDocuments'; stuff along those lines.

But whatever they are, they aren't tabs. Tabs are the UI frames those things live in, not the documents shown within them.
Attachment #543325 - Attachment is obsolete: true
Attachment #545795 - Flags: feedback?(hskupin)
(Reporter)

Comment 13

7 years ago
Comment on attachment 545795 [details] [diff] [review]
Windowing changes, modified based on feedback

>+function _getFirstWindowFromList(aWindowList, aFilterCallback, aFilterValue, aStrict) {

I'm not that happy that we now have to pass the filter callback and value into this function simply to set the correct message. It would be great to have a check in _getWindows instead. Can we make this function throw if strict is set? There we would have all parameters already available for a detailed error message. It's getting called anyway right before _getFirstWindowFromList.

>+ * @param {Boolean} [aWait=5000]
>+ *        Max length of time in ms to wait for the window to appear
>+ *
>+ * @returns {DOMWindow} The window
>+ */
>+ChromeWindowWrapper.prototype.findWindow =
>+function ChromeWindowWrapper_findWindow(aFilterCallback, aFilterValue,
>+                                        aWait) {

The parameter has to be called aTimeout so we match all other instances.

Otherwise it looks great.

Re tab vs. content, lets leave it as it is. If we think it has to be changed we can do this later. For now also based on your comments you are probably right. Once we have built-up the first ui map - in this case the browser - we will see how users will react.
Attachment #545795 - Flags: feedback?(hskupin) → feedback+
(Reporter)

Comment 14

7 years ago
(In reply to comment #13)
> >+ * @param {Boolean} [aWait=5000]
> >+ *        Max length of time in ms to wait for the window to appear
> 
> The parameter has to be called aTimeout so we match all other instances.

Oh, and it's a Number not Boolean.
OK, I'll take a few minutes to review the _getWindows stuff and see if it makes sense to put it there. I didn't initially because the list-getting functions never error-checked, they just returned empty lists. But maybe it'll work better. 

Good catches re: timeout and Number. I'll make those changes.

I'm going to go ahead and merge after doing those touchups. I'll post the commit link on here so you can check after the fact if you'd like.
Merged: https://github.com/geoelectric/mozmill-api-refactor/commit/498ece5a642bf76505e34b50a90a3079a1bacff7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

7 years ago
Looks way better. Thanks for making those changes and the landing.
You need to log in before you can comment on or make changes to this bug.