1254118 - Web protocol handlers need more testing coverage

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Florian Quèze [:florian]]

Web protocol handlers have a few different tests that each focus on a subset of the feature:
- browser/base/content/test/general/browser_registerProtocolHandler_notification.js (checks that the notification bar is correctly displayed)
- browser/components/feeds/test/test_registerHandler.html (checks the error handling of navigator.registerProtocolHandler)
- uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml (checks the nsIWebHandlerApp implementation)

But unfortunately none of these tests cover the feature as a whole. I think if such a test had existed, we would have caught bug 1253307 when bug 1116478 was being fixed.

On a slightly different but related topic: I started looking into this because I was trying to enable test_handlerApps.xhtml for e10s. It turns out the code exercised by this test, under normal circumstances, will only ever run in the chrome process, so running this with e10s doesn't make sense. Should this test be converted to a mochitest-chrome test, or be left as is and marked as 'ignore' in our e10s test list?

Comment 1

[Florian Quèze [:florian]]

Attachment: browser-chrome test

Here is a browser-chrome test that, I think would have caught bug 1253307, and covers the feature as a whole.
I included different code paths with todo() calls for the things that currently fail with e10s due to bug 1253307, so that waiting for a fix there doesn't stop us from increasing test coverage.

Felipe, could you please also answer the last question from the bug description?

Comment 2

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7dda133ebbc

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8727399 [details] [diff] [review]
browser-chrome test

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

One drive-by comment (since I'm working on bug 1253307).

::: uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js
@@ +22,5 @@
> +  }, 100);
> +  var moveOn = function() { clearInterval(interval); nextTest(); };
> +}
> +
> +function promiseWaitForCondition(aConditionFn) {

For this you can probably use BrowserTestUtils.waitForCondition.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8727399 [details] [diff] [review]
browser-chrome test

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

yeah, r+ with BTU.waitForCondition.

On your question in comment 0, you can just mark as Ignore with an appropriate comment in the spreadsheet. After the whole test effort is done we'll work towards appropriately annotating non-e10s-only tests in order to prepare for a future where we only run M-e10s (and create a new suite to keep running these non e10s tests)

Comment 5

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/d5c32bb6c8f41e013ef2e9f0f74094146d8113f9
Bug 1254118 - Web protocol handlers need more testing coverage, r=felipe.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5c32bb6c8f4