841697 - Add a new context menu test

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

We currently have no coverage on context menu options to catch regressions. We can start with images.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

Comment 2

[Jim Mathies [:jimm]]

Attachment: tests patch

Comment 3

[Jim Mathies [:jimm]]

Attachment: tests patch

Removed some debug statements and a couple nits I noticed.

Comment 4

[Matt Brubeck (: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.

Comment 5

[Jim Mathies [:jimm]]

Attachment: touchups

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.)

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 714951 [details] [diff] [review]
touchups

r=mbrubeck for the combination of this plus attachment 714538 [details] [diff] [review].

Comment 7

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b80182ebd8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17b80182ebd8