Closed Bug 569813 Opened 12 years ago Closed 12 years ago

Update AddonsAPI for Firefox 4.0

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [lib])

Attachments

(5 files, 4 obsolete files)

We have to update the AddonsAPI to match all the changes coming with Firefox 4. I can't start that work right now because we still expect changes. Until that point we should skip those tests in our Mozmill test-run.
Assignee: nobody → hskupin
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][AddonsRewrite]
We have to skip all add-ons tests for now.
Attachment #448982 - Flags: review?(anthony.s.hughes)
Attachment #448982 - Flags: review?(anthony.s.hughes) → review+
Temporary patch for skipping the tests landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/3a6432de4991
Component: Mozmill → Mozmill Tests
QA Contact: mozmill → mozmilltests
Whiteboard: [mozmill-test-failure][AddonsRewrite] → [shared module]
CC'ing Dave and Blair, who probably have interests on this bug.
Attachment #448982 - Attachment description: Temporarily skipping add-ons tests → Temporarily skipping add-ons tests [checked-in]
Attached patch WIP v1 (obsolete) — Splinter Review
Attached patch WIP v1.1 (obsolete) — Splinter Review
This new version contains a new shared module with the domHelper class. It will help us a lot in getting nodes from chrome or content documents in the future.
Attachment #478475 - Attachment is obsolete: true
Depends on: 599771
Depends on: 599702
Depends on: 599775
Attached patch WIP v1.2 (obsolete) — Splinter Review
Last WIP version before I will ask for review. With this patch mostly all elements we will need can be handled. Dependencies have been filed as separate bugs.

I will have to clean-up the shared module and group the class member functions.
Attachment #478602 - Attachment is obsolete: true
Summary: [shared module] Update AddonsAPI for Firefox 4.0 → Update AddonsAPI for Firefox 4.0
Blocks: 599865
Blocks: 599866
Blocks: 599867
Blocks: 599868
Attached patch Patch v1 (obsolete) — Splinter Review
First version of the patch. Everything we would need for now should be included. Remaining code we can fold in when needed by tests.
Attachment #478693 - Attachment is obsolete: true
Attachment #478783 - Flags: review?(gmealer)
Depends on: 600051
Depends on: 600052
Comment on attachment 478783 [details] [diff] [review]
Patch v1

OK, lots of comments, but some of them come down to the same issue repeated over a bunch of functions.  Perils of the 1750L patch, I guess. :)

Some of this patch uses

var foo = function()

...syntax for top-level functions, particularly in the helper class test. Think we generally want to move towards "function foo()" for new code, since it debugs better. I'll let it be your call, since technically these files were pre-existing but we're more or less replacing their contents.

Also, not sure I'm 100% on board with the use of "spec" objects quite this liberally.  I know we talked about it in theory, and this is an interesting experiment.  However, I would have only used spec objects for optional parameters, since those tend to be "flag" arguments.  I would have made non-optional parameters (for example, the Element in enable/disable) actual hard params since they're integral to the meaning of the function.  The spec documentation tends to be a little weaker across the board because expected types/values/defaults aren't comprehensively documented the way hard params are.

In any case, I feel we should come to a final decision on what we want before we use spec quite this way in the future.  OTOH, no code change necessary this time around on that (except undoc'd default--I dinged those).  This can be the proof of concept; let's see how it works out in practice.

From addonsManager.waitForOpened():

>+    mozmill.utils.waitFor(function() {
>+      var tabs = self.getTabs();
>+
>+      // At least one tab should be open
>+      if (tabs.length == 0)
>+        return false;
>+
>+      // ... and we always use the first one
>+      tab = tabs[0];
>+
>+      return true;
>+    }, timeout, 100, "Add-ons Manager has not been opened");

Seems like this should use the isOpen property already defined, in case that calculation needs to change.  It should only be responsible for getting the current tab, since that's new. Maybe:

...waitFor(function() {
  var isOpen = this.isOpen;
  if (isOpen) {
    tab = self.getTabs()[0];
  }
  return isOpen;
}, timeout...

Also, think timeout is implemented optional but not documented as such.  Default should be given if documented as optional.  Alternatively, throw an error if it's not present (I prefer optional though).

>+  /**
>+   * Returns the addons from the currently selected view which match the
>+   * filter criteria
>+   *
>+   * @param {object} aSpec
>+   *        Information about the filter to apply
>+   *        Elements: attribute - DOM attribute of the wanted addon (optional)
>+   *                  value - Value of the DOM attribute (optional)
.
.
>+  getAddons : function addonsManager_addons(aSpec) {

Optional params should document their defaults.

>+  /**
>+   * Retrieve the given child element of the specified add-on
.
.
>+  getAddonElement : function addonsManager_getAddonElement(aSpec) {

Since "add-on element" is used all over the place to mean the Element object that is the add-on, should probably change this (and all references) to getAddonChildElement() so "AddonElement" isn't overloaded in meaning.

>+  /**
>+   * Returns the categories which match the filter criteria
>+   *
>+   * @param {object} aSpec
>+   *        Information about the filter to apply
>+   *        Elements: attribute - DOM attribute of the wanted category (optional)
>+   *                  value - Value of the DOM attribute (optional)
.
.
>+  getCategories : function addonsManager_categories(aSpec) {

Default behavior when params are omitted should be documented.

>+  getCategoryId : function addonsManager_getCategoryId(aSpec) {
.
.
.
>+    if (!category) {
>+      throw new Error(arguments.callee.name + ": Category not specified.");
>+    }
>+
>+    return category ? category.getNode().id : undefined;

Ternary shouldn't be necessary here.  If it were false, would've thrown above.  Just keep the true part.

>+  /**
>+   * Select the given category
>+   *
>+   * @param {object} aSpec
>+   *        Information for selecting a category
>+   *        Elements: category - Category element
>+   *                  skipWait - Do not wait (optional)
>+   */
>+  setCategory : function addonsManager_setCategory(aSpec) {

Optional params should document their defaults.  

Also, minor issue but we should avoid params that mean "don't do something" when true.  It creates a confusing double-negative when considering the boolean.  e.g., skip wait = don't wait, so skip wait = false means don't don't wait.  Clearer is doWait, default true.  Your call on whether to change that bit, though.

>+  /**
>+   * Select the category with the given id
>+   *
>+   * @param {object} aSpec
>+   *        Information for selecting a category
>+   *        Elements: id - Category id (search, discover, languages,
>+   *                       searchengines, extensions, themes, plugins,
>+   *                       availableUpdates, recentUpdates)
>+   *                  skipWait - Do not wait (optional)
>+   */
>+  setCategoryById : function addonsManager_setCategoryById(aSpec) {

Undoc'd default + negative parameter.

>+  /**
>+   * Waits until the specified category has been selected
>+   * 
>+   * @param {object} aSpec
>+   *        Object with parameters for customization
>+   *        Elements: category - Category element to wait for
>+   *                  timeout - Duration to wait for the target state (optional)
>+   */
>+  waitForCategory : function addonsManager_waitForCategory(aSpec) {

Undoc'd default.

(following bit's edited to only keep the +'s)

>   /**
>+   * Search for a specified add-on
>    *
>+   * @param {object} aSpec
>+   *        Information to execute the search
>+   *        Elements: skipWait - Do not wait (optional)
>+   *                  value - Search term
>    */
>+  search : function addonsManager_search(aSpec) {

Undoc'd default, negative param

>+  /**
>+   * Retrieve the currently selected search filter
>+   *
>+   * @returns Element which represents the currently selected search filter
>+   * @type {ElemBase}
>+   */
>+  get selectedSearchFilter() {
>+    var filter = this.getSearchFilter({attribute: "selected", value: "true"});
>+
>+    return (filter.length > 0) ? filter[0] : null;
>+  },

Not necessarily wrong, just sanity-checking.  I've seen most things we've worked with come back undefined if not found.  Is null consistent with other APIs?

>+
d>+  /**
>+   * Set the currently selected search filter status
>+   *
>+   * @param {string} aValue
>+   *        Filter for the search results (local, remote)
>+   */
>+  set selectedSearchFilter(aValue) {
>+    var filter = this.getSearchFilter({attribute: "value", value: aValue});
>+
>+    if (filter.length > 0) {
>+      this._controller.click(filter[0]);
>+      this.waitForSearchFilter({filter: filter[0]});
>     }
>   },

Do we want to actually throw an error if they supply a value other than "local" or "remote"?  Doesn't appear enforced by this function or getSearchFilter.

> 
>   /**
>+   * Returns the available search filters which match the filter criteria
>+   * @param {object} aSpec
>+   *        Information about the filter to apply
>+   *        Elements: attribute - DOM attribute of the wanted filter (optional)
>+   *                  value - Value of the DOM attribute (optional)
>+   *
>+   * @returns List of search filters
>+   * @type {array of ElemBase}
>    */
>+  getSearchFilter : function addonsManager_getSearchFilter(aSpec) {

Default behavior when params are omitted should be documented.

>   /**
>+   * Get the search filter element for the specified value
>    *
>+   * @param {string} aValue
>+   *        Search filter value (local, remote)
>+   *
>+   * @returns Search filter element
>+   * @type {ElemBase}
>+   */
>+  getSearchFilterByValue : function addonsManager_getSearchFilterByValue(aValue) {

Same issue, re: local/remote not enforced.

>+  getSearchFilterValue : function addonsManager_getSearchFilterValue(aSpec) {
>+    var spec = aSpec || { };
>+    var filter = spec.filter;
>+
>+    if (!filter) {
>+      throw new Error(arguments.callee.name + ": Search filter not specified.");
>+    }
>+
>+    return spec.filter ? spec.filter.getNode().value : null;

If spec.filter were false, you'd have thrown by now.  Ternary's not necessary. just: return filter.getNode().value;

>+  /**
>+   * Waits until the specified search filter has been selected
>+   * 
>+   * @param {object} aSpec
>+   *        Object with parameters for customization
>+   *        Elements: filter - Filter element to wait for
>+   *                  timeout - Duration to wait for the target state (optional)
>+   */
>+  waitForSearchFilter : function addonsManager_waitForSearchFilter(aSpec) {

Undoc'd default.


>+  /**
>+   * Waits until the active search has been finished
>+   * 
>+   * @param {object} aSpec
>+   *        Object with parameters for customization
>+   *        Elements: timeout - Duration to wait for the target state
>+   */
>+  waitForSearchFinished : function addonsManager_waitForSearchFinished(aSpec) {

Like waitForOpened(), suspect timeout is optional w/ an undoc'd default.

>+  /**
>+   * Returns the views which match the filter criteria
>+   *
>+   * @param {object} aSpec
>+   *        Information for getting the views matched by the criteria
>+   *        Elements: attribute - DOM attribute of the node
>+   *                  value - Value of the DOM attribute
>+   *
>+   * @returns Filtered list of views
>+   * @type {array of ElemBase}
>+   */
>+  getViews : function addonsManager_getViews(aSpec) {

Sanity-checking, are attribute and value required or optional?  Similar functions above had them optional.

If optional, please document default behavior if omitted.  If required, should probably throw if not supplied.


>+  ///////////////////////////////
>+  // UI Elements section
>+  ///////////////////////////////
>+
>+  /**
>+   * Retrieve an UI element based on the given specification
>+   *
>+   * @param {object} aSpec
>    *        Information of the UI element which should be retrieved
>    *        type: General type information
>    *        subtype: Specific element or property
>    *        value: Value of the element or property
>+   *        parent: Parent element for list nodes
>+   *
>+   * @returns Element which has been found
>    * @type {ElemBase}
>    */
>+  getElement : function addonsManager_getElement(aSpec) {
>+    var elements = this.getElements(aSpec);
> 
>+    return (elements.length > 0) ? elements[0] : null;
>+  },

same issue as above, re: null vs. undefined? Not necessarily a problem, just a sanity check.

>+
>+  /**
>+   * Retrieve list of UI elements based on the given specification
>+   *
>+   * @param {object} aSpec
>+   *        Information of the UI elements which should be retrieved
>+   *        type: General type information
>+   *        subtype: Specific element or property
>+   *        value: Value of the element or property
>+   *        parent: Parent element for list nodes
>+   *
>+   * @returns Elements which have been found
>+   * @type {array of ElemBase}
>+   */
>+  getElements : function addonsManager_getElements(aSpec) {

...normally I'd say optionals need to be defined, but that gets into why I really don't like getElement[s] as a pattern.  Optional for some specs, not for others.  It is what it is, but post-refactor I suspect these functions will be split up into individual element accessors and we won't be using a common entry point like this.

>+    var $ = new this._DOMUtilsAPI.domHelper(root);

Choose an alphanumeric variable name please.  $ -looks- like it has special meaning, even if it doesn't.  

Every different framework makes that mean something different--MS AJAX, it means a function alias (like you've used it), JQuery it means native JQuery object, ECMAScript says "only for machine-generated code."  Whatever the case, a casual reader will expect this to be somehow meaningful and rip hair out trying to figure out why it's used.  Until/unless we come to a standard as to when to use it, I don't think we should use it.

>diff --git a/shared-modules/testDOMUtilsAPI.js b/shared-modules/testDOMUtilsAPI.js

Heads up, new file now.

>+/**
>+ * DOM Helper class
>+ */
>+domHelper.prototype = {
>+  _nodes : [ ],

If I understand my JS prototyping correctly, this means all domHelper instances will share the same "static" list of nodes--i.e. you can set from one instance, and get from the other.  Are you sure that's what we want?  If not, this should be declared in the constructor.  If so, we should put in a quick comment explaining that's what we want and why since it's unusual.

>+  /**
>+   * Filter nodes given by the specified callback function
>+   *
>+   * @param {function} aCallback
>+   *        Function to test each element of the array.
>+   *        Parameters: element, index (optional) , array (optional)
>+   * @param {object} aThisObject
>+   *        Object to use as 'this' when executing callback.
>+   */
>+  filter : function domHelper_filter(aCallback, aThisObject) {
>+    if (aCallback)
>+      this._nodes = Array.filter(this._nodes, aCallback, aThisObject);
>+
>+    return this;
>+  },

Should document that it returns itself. Did you mean for it to just return itself with node list unchanged if no callback defined?  If not, should throw.

Also, and I'm sure this issue exists elsewhere, the object should arguably be using its own accessors to get/set the _nodes structure.  Downside is it's slower to set the structure, upside is that it's SPOT for that structure.

>+
>+  /**
>+   * Filter nodes by DOM property and its value
>+   *
>+   * @param {string} aProperty
>+   *        Property to filter for
>+   * @param {string} aValue
>+   *        Expected value of the DOM property (optional)
>+   */
>+  filterByDOMProperty : function domHelper_filterByDOMProperty(aProperty, aValue) {
>+    return this.filter(function(node) {
>+      if (aProperty && aValue)
>+        return node.getAttribute(aProperty) == aValue;
>+      else if (aProperty)
>+        return node.hasAttribute(aProperty);
>+      else
>+        return true;
>+    });
>+
>+    return this;
>+  },

Final return this unnecessary; already has an unconditional return above it.  Same doc issue as above re: passthrough behavior. Also, doesn't affect functionality, but since you said the first param to the callback should be "element" you might consider using "element" instead of "node" here for more consistency.

>+
>+  /**
>+   * Find nodes with the specified class name
>+   *
>+   * @param {string} aAttribute
>+   *        DOM attribute of the wanted node
>+   * @param {string} aValue
>+   *        Value of the DOM attribute
>+   */
>+  queryAnonymousNodes : function domHelper_queryAnonymousNodes(aAttribute, aValue) {
>+    var node = this._document.getAnonymousElementByAttribute(this._root,
>+                                                             aAttribute,
>+                                                             aValue);
>+    this.nodes = [node];
>+
>+    return this;
>+  },

Says it finds by class name, but doesn't take a class name (I'm sure it's one of the attributes/values, but that's not at all obvious, and plainly doesn't -just- take a class name).  Please make documentation and naming consistent with actual functionality.

>diff --git a/shared-modules/testTabbedBrowsingAPI.js b/shared-modules/testTabbedBrowsingAPI.js

New file again.


>+
>+/**
>  * Constructor
>  * 
>  * @param {MozMillController} controller
>  *        MozMill controller of the window to operate on
>  */
> function tabBrowser(controller)
> {
>   this._controller = controller;
>@@ -140,22 +182,24 @@ tabBrowser.prototype = {
> 
>     this._controller.open("about:blank");
>     this._controller.waitForPageLoad();
>   },
> 
>   /**
>    * Close an open tab
>    *
>+   * @param {object} aEvent
>    *        The event specifies how to close a tab (menu, middle click,
>    *        shortcut, or the tab close button). Only with middle click an
>    *        inactive tab can be closed.
>    */
>+  closeTab : function tabBrowser_closeTab(aEvent) {
>+    var event = aEvent || { type : "menu" };
>+

If aEvent is now optional, defaults need to be documented.

>   /**
>    * Open element (link) in a new tab
>    *
>+   * @param {object} aEvent
>    *        The event specifies how to open the element in a new tab
>    *        (contextMenu, middleClick)
>    */
>+  openInNewTab : function tabBrowser_openInNewTab(aEvent) {
>+    var event = aEvent || { type : "middleClick" };
>+

Ditto re: optional/defaults.

>   /**
>    * Open a new tab
>    *
>+   * @param {object} aEvent
>    *        The event specifies how to open a new tab (menu, shortcut,
>    *        new tab button, or double click on the tabstrip)
>    */
>+  openTab : function tabBrowser_openTab(aEvent) {
>+    var event = aEvent || { type : "menu" };
>+

Ditto re: optional/defaults.

Believe it or not, overall looks pretty good, and code functionality looks fine.  I think it's just five or six (mostly documentation/consistency) issues that happen to appear repeatedly.  Still, I'd like at least one more pass to fix some of them.  -PLEASE- highlight the ones you fix to me somehow (either diff of diffs, or just tell me which ones you're doing and not doing).
Attachment #478783 - Flags: review?(gmealer) → review-
Blocks: 600051, 600052
No longer depends on: 600051, 600052
No longer blocks: 600051
(In reply to comment #8)
> var foo = function()
> 
> we generally want to move towards "function foo()" for new code, since it
> debugs better. I'll let it be your call, since technically these files were

That was a left-over. Simply forgot to update this part.

> In any case, I feel we should come to a final decision on what we want before
> we use spec quite this way in the future.  OTOH, no code change necessary this
> time around on that (except undoc'd default--I dinged those).  This can be the
> proof of concept; let's see how it works out in practice.

Agree. Lets see the results of that style and let it flow into our shared module refactoring work next quarter. Thanks for bringing this up.

> From addonsManager.waitForOpened():
[..]
> Seems like this should use the isOpen property already defined, in case that
> calculation needs to change.  It should only be responsible for getting the
[..]
> Also, think timeout is implemented optional but not documented as such. 
> Default should be given if documented as optional.  Alternatively, throw an
> error if it's not present (I prefer optional though).

fixed

> >+  getAddonElement : function addonsManager_getAddonElement(aSpec) {
> 
> Since "add-on element" is used all over the place to mean the Element object
> that is the add-on, should probably change this (and all references) to
> getAddonChildElement() so "AddonElement" isn't overloaded in meaning.

I wanted to have the same naming style as getAddonButton. But I can see that it can be confusing. Especially for this function adding "Child" makes sense.

> >+  getCategoryId : function addonsManager_getCategoryId(aSpec) {
[..]
> >+    return category ? category.getNode().id : undefined;
> 
> Ternary shouldn't be necessary here.  If it were false, would've thrown above. 
> Just keep the true part.

fixed.

> Also, minor issue but we should avoid params that mean "don't do something"
> when true.  It creates a confusing double-negative when considering the
> boolean.  e.g., skip wait = don't wait, so skip wait = false means don't don't
> wait.  Clearer is doWait, default true.  Your call on whether to change that
> bit, though.

Accepted. I stumbled already about a couple of instances in other APIs. It always makes it hard to have double-negative conditions.

> >+  get selectedSearchFilter() {
[..]
> >+    return (filter.length > 0) ? filter[0] : null;
> >+  },
> 
> Not necessarily wrong, just sanity-checking.  I've seen most things we've
> worked with come back undefined if not found.  Is null consistent with other
> APIs?

We haven't made use of null vs. undefined in our API's yet. But I agree that undefined is the better solution. Changed to undefined. 

> Do we want to actually throw an error if they supply a value other than "local"
> or "remote"?  Doesn't appear enforced by this function or getSearchFilter.

getSearchFilter cannot enforce it because it allows you to filter by any attribute on those filter nodes. It's not only the value we can filter for. Given that "set selectedSearchFilter" should throw an error.

> >+  getSearchFilterByValue : function addonsManager_getSearchFilterByValue(aValue) {
> 
> Same issue, re: local/remote not enforced.

fixed.

> >+    var $ = new this._DOMUtilsAPI.domHelper(root);
> 
> Choose an alphanumeric variable name please.  $ -looks- like it has special
> meaning, even if it doesn't.  
>
> trying to figure out why it's used.  Until/unless we come to a standard as to
> when to use it, I don't think we should use it.

Renamed the class and the instance to nodeCollector, which is the original purpose of this class.

> >diff --git a/shared-modules/testDOMUtilsAPI.js b/shared-modules/testDOMUtilsAPI.js
>
> >+domHelper.prototype = {
> >+  _nodes : [ ],
> 
> If I understand my JS prototyping correctly, this means all domHelper instances
> will share the same "static" list of nodes--i.e. you can set from one instance,
> and get from the other.  Are you sure that's what we want?  If not, this should
> be declared in the constructor.  If so, we should put in a quick comment
> explaining that's what we want and why since it's unusual.

No, each instance can handle its own list of nodes. The way I have used it only assigns a default value to the member when the instance gets constructed. Successive write access to this member will always work on the instance member itself. But I can move it up into the constructor, which makes the code cleaner and puts all the initialization code at one place. 

> >+  filter : function domHelper_filter(aCallback, aThisObject) {
> >+    if (aCallback)
> >+      this._nodes = Array.filter(this._nodes, aCallback, aThisObject);
> >+
> >+    return this;
> >+  },
> 
> Should document that it returns itself. Did you mean for it to just return
> itself with node list unchanged if no callback defined?  If not, should throw.

Well, calling filter without a callback doesn't make sense. So yeah, we should throw.

> Also, and I'm sure this issue exists elsewhere, the object should arguably be
> using its own accessors to get/set the _nodes structure.  Downside is it's
> slower to set the structure, upside is that it's SPOT for that structure.

Right. Missed that. Good spot.

> >+  filterByDOMProperty : function domHelper_filterByDOMProperty(aProperty, 
[..]
> >+
> >+    return this;
> >+  },
> 
> Final return this unnecessary; already has an unconditional return above it. 

fixed

> Same doc issue as above re: passthrough behavior. Also, doesn't affect
> functionality, but since you said the first param to the callback should be
> "element" you might consider using "element" instead of "node" here for more
> consistency.

I have changed element to node because we are using element in the controllers scope. I don't want to mix names which will cause more confusion.

> >+  queryAnonymousNodes : function domHelper_queryAnonymousNodes(aAttribute, 
> 
> Says it finds by class name, but doesn't take a class name (I'm sure it's one
> of the attributes/values, but that's not at all obvious, and plainly doesn't
> -just- take a class name).  Please make documentation and naming consistent
> with actual functionality.

Taken. Missed that from a c&p action.

> Believe it or not, overall looks pretty good, and code functionality looks
> fine.  I think it's just five or six (mostly documentation/consistency) issues
> that happen to appear repeatedly.  Still, I'd like at least one more pass to
> fix some of them.  -PLEASE- highlight the ones you fix to me somehow (either
> diff of diffs, or just tell me which ones you're doing and not doing).

Thanks for checking that patch and your notes! Those are really helpful. The next revision of the patch follows in some minutes. You can use the interdiff feature of Bugzilla to check for differences between attached patches.
Attached patch Patch v2Splinter Review
Review comments addressed. Further I have found a glitch in handling the optional spec members. With the last patch even a false or 0 triggered a default value. This is fixed now.
Attachment #478783 - Attachment is obsolete: true
Attachment #479052 - Flags: review?(gmealer)
Blocks: 600291
Comment on attachment 479052 [details] [diff] [review]
Patch v2

Looks fine now, thanks for your changes.

Landed as http://hg.mozilla.org/qa/mozmill-tests/rev/4b3e6e75a9eb

Unfortunately, I didn't notice in hg out that there wasn't a commit message in your patch, so it landed without a descriptive one.  I'm unsure whether that can be edited after the fact or how.  Feel free to touch base with me on IRC and let me know how to fix that up.
Attachment #479052 - Flags: review?(gmealer) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 600359
(In reply to comment #11)
> your patch, so it landed without a descriptive one.  I'm unsure whether that
> can be edited after the fact or how.  Feel free to touch base with me on IRC
> and let me know how to fix that up.

Discussed on IRC and it's fine. At least the AddonsAPI is mentioned in the commit message which makes it understandable. And I hope that we do not have to backout the patch. :)
Backport patch of the made tabbrowser changes and the DOMUtilsAPI for older branches.
Attachment #479342 - Flags: review?(gmealer)
Depends on: 600502
Mistaken use of the 'am' object, should be 'this'
Attachment #479394 - Flags: review?(hskupin)
Comment on attachment 479394 [details] [diff] [review]
getSearchResults and handleUtilsButton fix

Makes totally sense. I have used that code in the helper class test first and missed to rename it.
Attachment #479394 - Flags: review?(hskupin) → review+
Comment on attachment 479342 [details] [diff] [review]
Backport (DOMUtils, Tabbrowser)

Looks good, and applies cleanly to both 1.9.1 and 1.9.2.  Per yesterday's conversation, please land the patch yourself. :)
Attachment #479342 - Flags: review?(gmealer) → review+
That should be the last patch attached to this bug. We should file new ones in the future. But this is just a silly one liner.
Attachment #479741 - Flags: review?(gmealer)
Comment on attachment 479741 [details] [diff] [review]
Don't add an empty node in the nodeCollector

LGTM, r+
Attachment #479741 - Flags: review?(gmealer) → review+
Depends on: 601158
Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Product: Testing → Mozilla QA
Version: Trunk → unspecified
Component: Mozmill Tests → Mozmill Shared Modules
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [shared module] → [lib]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.