659487 - Update addons.js module to incorporate remaining changes in Firefox 4

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

We have a couple of timing issues in the addons.js module. Not sure why they are existent now and not at the time when I have implemented this module. Given by Dave nothing has been changed.

Comment 1

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

It's not only the timing issue bug also my patch from bug 599771 which went in to Firefox 4.0 Beta 7. So those are a bit more updates.

Comment 2

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

Attachment: Patch v1

This patch updates mostly all the areas which are in use and needed by the Discovery Pane. I haven't touched the Search Pane which still needs updates but which are not that important right now.

Comment 3

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

Attachment: Patch v1 (nightly / aurora)

Patch for Firefox 6 with updated category names.

Comment 4

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

Comment on attachment 535087 [details] [diff] [review]
Patch v1 (nightly / aurora)

Wait that should be a patch for bug 659424. We should apply it step by step.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 535070 [details] [diff] [review]
Patch v1

>   waitForCategory : function AddonsManager_waitForCategory(aSpec) {
> ...
>+    try {
>+      this.controller.waitFor(function () {
>+        return self.changed &&
>+               this.selectedCategory.getNode() === category.getNode();
>+      }, "Category has been selected - got '" + this.selectedCategory.getNode().id +
>+         "', expected '" + category.getNode().id + "'" , timeout, undefined, this);

Sanity check here: do we expect the node to be changed as soon as the event listener has fired? 

If so, this should be a waitFor on the listener only, followed by an assertion on the node. The current code would not catch a delay between listener finish and node state change.

If not, then adding the event listener to the waitFor was arguably unnecessary, and made the code more complicated--you only have to wait for the final state change.

Other than that, looks fine.

I'm r+'ing, but it's really r=me re: above unless my logic's off-base. If it is, please follow up with a comment addressing it as you land.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Sorry, correcting myself. Since this is module code, wouldn't be an assertion on the node. It'd be an exception raise inside an if..then.

Comment 7

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

(In reply to comment #5)
> >   waitForCategory : function AddonsManager_waitForCategory(aSpec) {
> > ...
> >+    try {
> >+      this.controller.waitFor(function () {
> >+        return self.changed &&
> >+               this.selectedCategory.getNode() === category.getNode();
> >+      }, "Category has been selected - got '" + this.selectedCategory.getNode().id +
> >+         "', expected '" + category.getNode().id + "'" , timeout, undefined, this);
> 
> Sanity check here: do we expect the node to be changed as soon as the event
> listener has fired? 

When the ViewChanged event has fired the views will have populated lists and content. That's what we have to wait for.

> If so, this should be a waitFor on the listener only, followed by an
> assertion on the node. The current code would not catch a delay between
> listener finish and node state change.

The category changes immediately and will always be before the event is fired. Shall I still move this out?

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I'd only wait on the event fire myself, then, since it's the last thing that happens. That way the code's a little simpler and still just as functional.

If you want belt and suspenders, we can keep it the way it is. Downside is that if either that node or the event handler implementation changes the function will break, so it's a little more fragile.

With that in mind, I'm OK either way you go.

Comment 9

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

(In reply to comment #8)
> I'd only wait on the event fire myself, then, since it's the last thing that
> happens. That way the code's a little simpler and still just as functional.

We absolutely have to ensure that the right target category has been set. So to make it easier we can move out the category check and throw an error if the selected one is not the target category.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Yep, sounds like my original suggestion. Let's do that.

Comment 11

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

Attachment: Correct basics of the implementation [checked-in]

Patch with the proposed review changes. Thanks.

Comment 12

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6b5fb7dd4221 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/c229739c920e (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/898baa71c44b (beta)

Since we do not add add-ons tests for mozilla-2.0 I don't push it to this branch.

Comment 13

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

Well, we have to do a bit more here before we can really call it resolved.

Comment 14

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

Attachment: Fix of broken waitFor handling

This is a kinda important fix we have to do. The way how the waitFor methods are implemented right now is currently totally broken. We always have to register the event handler before the action takes place. Otherwise we will miss events.

This is the primarily work just to fix the addons module for now. At the end we have to come up with a proper event handling solution across all modules. Geo, that probably changes the ideas how we have to handle events in the API refactoring. I will file a new bug so we can move any discussion over there.

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 535703 [details] [diff] [review]
Fix of broken waitFor handling

>   getCategoryId : function AddonsManager_getCategoryId(aSpec) {
>...
>-    return category.getNode().id;
>+    return category.getNode().id.match(/category-(.*)/)[1];

Please add a comment above this explaining the intent of the regex (e.g. "stripping the category- prefix from the ID"). I'd like us to start doing this for all regexes, both for code review and for maintenance purposes.

BTW, not a code problem:

>+    // For the panes 'discover' and 'search' we have to increase the timeout
>+    // because the ViewChanged event is sent after the remote data has been loaded.
>+    if (categoryId === 'discover' || categoryId === 'search')
>+      timeout = TIMEOUT_REMOTE;

...I wanted to highlight this because I really like your comment. It clearly explains why we're doing that thing.

Comments like this would be useful in more of our code. Our reviews and maintenance are a little more difficult than I'd like in part because we don't always comment our intent. It also affects learning curve because it's difficult to associate existing code to any platform principles behind it.

Inline comments that explain the intent of the flow should be standard practice for even relatively straightforward code (especially back-end modules), and I'm likely going to start reviewing with that in mind in the future.

Rest looks good. r=me after you add the regex comment.

Comment 16

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

Attachment: Fix of broken waitFor handling v2 [checked-in]

Updated patch with comment added. Taking over r+. Thanks.

Comment 17

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

Comment on attachment 535743 [details] [diff] [review]
Fix of broken waitFor handling v2 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/100d6c02a27f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/4ec09b22e32b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/cba15d72a206 (beta)

The only remaining work on this bug now is the search pane elements.

Comment 18

[Alex Lakatos[:AlexLakatos]]

(In reply to comment #17)
> The only remaining work on this bug now is the search pane elements.
And can you please take a look and waitForDownloaded and refactor it? Thanks.

Comment 19

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

(In reply to comment #18)
> And can you please take a look and waitForDownloaded and refactor it? Thanks.

Will do. But it will have to wait until I'm back, except someone else steps in.

Comment 20

[Alex Lakatos[:AlexLakatos]]

We also need support for the notification panel. Mentioning it here so we don't forget about it.

Comment 21

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

This bug was for Firefox 4. so I propose to close it as fixed and lets get new issues filed as new bugs.

Alex, please file a new bug for the notification panel request. First please check if the API we have in tabs.js doesn't already cover that. Thanks.