Closed Bug 841697 Opened 10 years ago Closed 10 years ago

Add a new context menu test

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 2 obsolete files)

We currently have no coverage on context menu options to catch regressions. We can start with images.
Attached patch wip (obsolete) — Splinter Review
Attached patch tests patch (obsolete) — Splinter Review
Attachment #714459 - Attachment is obsolete: true
Attachment #714535 - Flags: review?(mbrubeck)
Attached patch tests patchSplinter Review
Removed some debug statements and a couple nits I noticed.
Attachment #714535 - Attachment is obsolete: true
Attachment #714535 - Flags: review?(mbrubeck)
Attachment #714538 - Flags: review?(mbrubeck)
Comment on attachment 714538 [details] [diff] [review]
tests patch

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

Some minor issues, plus I want to see if we can kill some waitForMs calls.

::: browser/metro/base/tests/browser_context_menu_tests.js
@@ +28,5 @@
> +
> +    info(chromeRoot + "browser_context_menu_tests_01.html");
> +    yield addTab(chromeRoot + "browser_context_menu_tests_01.html");
> +
> +    yield waitForMs(500);

Do you know why this is needed?  I'd really really really like to avoid it.

@@ +30,5 @@
> +    yield addTab(chromeRoot + "browser_context_menu_tests_01.html");
> +
> +    yield waitForMs(500);
> +
> +    let tab = Browser.selectedTab.browser.contentWindow;

Can you name this variable something like "win" or "contentWindow"?  We usually use "tab" for instances of the Tab constructor in browser.js.  Also, you overwrite this binding near the end of the function.

@@ +64,5 @@
> +
> +    if (saveLocationPath.exists()) {
> +      info("had to remove old image!");
> +      saveLocationPath.remove(false);
> +      yield waitForMs(100);

Again, can we avoid using a timeout here?

@@ +110,5 @@
> +
> +    ////////////////////////////////////////////////////////////
> +    // Copy image location
> +
> +    let promise = waitForEvent(document, "popupshown");

You can remove the "let" here since "promise" is already defined.  (This is harmless, but I think it might dump a warning to the console.  There are several similar issues elsewhere in this function.  You don't need to knock yourself out trying to kill all of them, but maybe consider splitting up this function a bit.)

@@ +154,5 @@
> +    yield promise;
> +    ok(promise && !(promise instanceof Error), "promise error");
> +
> +    if (promise instanceof Error)
> +      return;

We should probably fail the test here with an ok(false) or something.
Attachment #714538 - Flags: review?(mbrubeck) → review-
Attached patch touchupsSplinter Review
Here are the touchups on top of the original. I's rather not break the test up for a couple reasons, I'm adding more tests for other content types which will be in new gTest tests, and since every test here uses the same tab keeping it within one test allows me to reuse that tab. (I can break it up if you prefer that anyway.)
Attachment #714951 - Flags: review?(mbrubeck)
Comment on attachment 714951 [details] [diff] [review]
touchups

r=mbrubeck for the combination of this plus attachment 714538 [details] [diff] [review].
Attachment #714951 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/17b80182ebd8
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.