Closed Bug 859241 Opened 11 years ago Closed 11 years ago

Update addons.js and private-browsing.js to enhance the way .close() works

Categories

(Mozilla QA Graveyard :: Mozmill Tests, enhancement)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox21 fixed, firefox22 fixed, firefox23 fixed, firefox24 fixed, firefox-esr17 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox21 --- fixed
firefox22 --- fixed
firefox23 --- fixed
firefox24 --- fixed
firefox-esr17 --- fixed

People

(Reporter: mario.garbi, Assigned: mario.garbi)

References

Details

(Whiteboard: [lib])

Attachments

(2 files, 4 obsolete files)

patch v1.4
5.24 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
patch v1.4 ESR17
1.56 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review 
Add a check to the close() method in addons.js so we verify if the Addons Manager is opened before we try to close it.
In bug 786306 comment 66 I mentioned that the current behaviour is by design. Why are we changing it without further discussion? Also, looking at the patch I'm not sure I can see what value this is adding...
I'm sorry about not talking about this before posting it. It will fail with except.fail if we don't have an addons manager opened, and the try block catches any other failures.

We could remove this if it's not desired or isn't useful, I was just trying to come up with a fix as suggested in bug 786306 comment 64.
(In reply to mario garbi from comment #3)
> I'm sorry about not talking about this before posting it. It will fail with
> except.fail if we don't have an addons manager opened, and the try block
> catches any other failures.

That's currently the expected behaviour. We even have a test for that scenario: http://hg.mozilla.org/qa/mozmill-tests/file/3000a00d0858/lib/tests/testAddonsManager.js#l91

> We could remove this if it's not desired or isn't useful, I was just trying
> to come up with a fix as suggested in bug 786306 comment 64.

I don't see this as something that needs fixing. I think it's reasonable to guard the close with an if statement in the teardown, as that will still allow us to fail within the tests if we try to close the addons manager when it is not open.
Summary: Update addons.js to enhance the way addonsManager.close() works → Update addons.js and private-browsing.js to enhance the way .close() works
Comment on attachment 734580 [details] [diff] [review]
Patch v1.0

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

No longer needs review as we have established another way to do this.
Attachment #734580 - Flags: review?(andreea.matei) → review-
Comment on attachment 736800 [details] [diff] [review]
patch v1.1

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

You can add reports for Nightly only until that one has landed, no need for the rest atm.

::: lib/addons.js
@@ +202,5 @@
>    /**
>     * Close the Addons Manager
> +   * @param {boolean} aAssertSuccess
> +   *        Boolean value that decides how we handle Addons Manager already
> +   *        being closed

Please find a better description here, one that mentions what is 'true' used for (mostly we'll be using it in teardowns()).

@@ +219,5 @@
>          aomTabs[0].controller.waitForPageLoad();
>        }
>      }
>      catch (ex) {
> +      if(!aAssertSuccess){

Spaces needed after if and between ')' and '{'

::: lib/ui/private-browsing.js
@@ +134,2 @@
>  
> +      //Try to close the window in case it is not already closed

This message can be moved now outside the try block and needs a space after '//'.
Attachment #736800 - Flags: review?(andreea.matei) → review-
Comment on attachment 736800 [details] [diff] [review]
patch v1.1

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

::: lib/ui/private-browsing.js
@@ +142,5 @@
> +      }, "A private browsing window has been closed");
> +    }
> +    catch (ex) {
> +      if (!aAssertSuccess) {
> +        throw new Error(arguments.callee.name + ": Private Browsing Window has been closed.");

Please don't make use of `arguments.callee.name` anymore given that we will not be able to use it anymore once we have turned on strict mode. Just take the message as parameter. That's enough.
Comment on attachment 738930 [details] [diff] [review]
Patch v1.2

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

Only a small thing left that was not addressed and also the commit message is outdated.

::: lib/addons.js
@@ +219,5 @@
>          aomTabs[0].controller.waitForPageLoad();
>        }
>      }
>      catch (ex) {
> +      if(!aAssertSuccess) {

Still missing a space after if and the condition
Attachment #738930 - Flags: review?(andreea.matei) → review-
Attached patch Patch v1.3 (obsolete) — — Splinter Review 
Modified the commit message and indentation issue. Thank you.
Attachment #738930 - Attachment is obsolete: true
Attachment #739048 - Flags: review?(andreea.matei)
Comment on attachment 739048 [details] [diff] [review]
Patch v1.3

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

::: lib/addons.js
@@ +201,5 @@
>  
>    /**
>     * Close the Addons Manager
> +   * @param {boolean} aAssertSuccess
> +   *        Boolean value that sets the function to ignore failures if true

This sounds backwards to me.. 'assert success' should mean that we throw if unsuccessful, not that we ignore.
Attachment #739048 - Flags: review?(andreea.matei) → review-
From the discutions on IRC I've understood that we want the close() method to have the boolean value default to false, if we only throw errors when the value is set to true then we end up ignoring closing issues and I was under the impression we do not want that.

I could either change the name of the parameter into something more appropriate or change the way we handle close() so that we ignore failures with default false. The later might involve refactoring if we want to keep throwing errors when close fails.
(In reply to mario garbi from comment #14)
> From the discutions on IRC I've understood that we want the close() method
> to have the boolean value default to false, if we only throw errors when the
> value is set to true then we end up ignoring closing issues and I was under
> the impression we do not want that.
> 
> I could either change the name of the parameter into something more
> appropriate or change the way we handle close() so that we ignore failures
> with default false. The later might involve refactoring if we want to keep
> throwing errors when close fails.

You are correct in that the default should be false, and the default behaviour should be as it currently is (assert success). We will only want to set it to true (don't assert success) when we're in teardown. In this case the argument would need to be aIgnoreFailures or similar. Either way we need the name of the argument to reflect the behaviour, which is not the case in your current patch.
Attached patch patch v1.4 — — Splinter Review 
Changed the parameter name as requested, to better reflect the behavior.
Attachment #739048 - Attachment is obsolete: true
Attachment #739530 - Flags: review?(andreea.matei)
Comment on attachment 739530 [details] [diff] [review]
patch v1.4

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/68b574aaf5f2 (default)
Attachment #739530 - Flags: review?(andreea.matei) → review+
The reports above used the same patch as Default.
For ESR17 and release I will come with a different patch since there are some issues.
Attached patch patch v1.4 ESR17 — — Splinter Review 
This patch works with ESR17 only, for Release we have some issues with Release 20 build.The code implemented in Bug 834632 (for 21) seems to have landed on Release Branch, for which we still have build 20.0.1 as latest. Basically the patch for bug 834632 adds a pluginState check of a label that doesn't exists in 20.0.1 and this causes the tests to fail with "pluginState[i] is undefined". As release 21 is around the corner we could simply wait for it to land and release branch will work ok again.

Release reports:
WinXP:
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec8f3bb
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec93c01

Linux 12.04:
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec91587
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec9a3d2

Mac 10.6.8:
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec91be1
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145ec9a6ec
Attachment #748771 - Flags: review?
Attachment #748771 - Flags: review? → review?(andreea.matei)
Comment on attachment 748771 [details] [diff] [review]
patch v1.4 ESR17

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

Aurora was merged, here are the rest:
http://hg.mozilla.org/qa/mozmill-tests/rev/631893229b1d (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/cc020e6123fa (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/9d5d7a907c7e (esr17)

Thanks!
Attachment #748771 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox20: affected → ---
status-firefox21: affected → fixed
status-firefox22: affected → fixed
status-firefox24: --- → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: