1267221 - WebNavigation onErrorOccurred details should contains an 'error' field

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Based on the WebNavigation docs (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/onErrorOccurred), the onErrorOccurred event details should contains an |error| string field.

Currently an error message in defined in WebNavigation.jsm, but it is not copied into the event details reported to the WebExtensions add-on context that should receive it.

Comment 1

[Martin Giger [:freaktechnik]]

Attachment: bug1267221-v1.patch

Adds the error field for the data object emitted for an onErrorOccured and no other event. Will open a follow-up bug for tests.

Comment 2

[Martin Giger [:freaktechnik]]

Attachment: bug1267221-v2.patch

Fix typo in commit message and get error property from original data object.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8762702 [details] [diff] [review]
bug1267221-v2.patch

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

Thanks Martin, the change is good.
 
we just need to create a new test scenario for it in the 'toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html' file before we can move it to his final review.

Comment 4

[Martin Giger [:freaktechnik]]

Attachment: bug1267221-test-v1.patch

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8762715 [details] [diff] [review]
bug1267221-test-v1.patch

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

Thanks Martin, 
the new test looks good, there are just 3 small eslint errors to fix.

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
@@ +82,4 @@
>  const FRAME_MANUAL = BASE + "/file_webNavigation_manualSubframe.html";
>  const FRAME_MANUAL_PAGE1 = BASE + "/file_webNavigation_manualSubframe_page1.html";
>  const FRAME_MANUAL_PAGE2 = BASE + "/file_webNavigation_manualSubframe_page2.html";
> +const INVALID_PAGE = 'https://invalid.localhost/';

This string needs double quotes (or eslint will raise errors)

@@ +542,5 @@
> +
> +  let win = window.open();
> +
> +  received = [];
> +  yield loadAndWait(win, "onErrorOccurred", INVALID_PAGE, () => win.location = INVALID_PAGE);

This line raise another eslint error (because the arrow function seems to return the assignment), as we don't need the return value, let's put curly braces around it:

  () => { ... }

@@ +551,5 @@
> +  ok(found, "Got the onErrorOccurred event");
> +
> +  if (found) {
> +      ok(found.details.error.match(/Error code [0-9]+/),
> +          "Got the expected error string in the onErrorOccurred event");

eslint indentation error (it should be 4 spaces instead of 6)

Comment 6

[Martin Giger [:freaktechnik]]

Attachment: bug1267221-test-v2.patch

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8762851 [details] [diff] [review]
bug1267221-test-v2.patch

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

Thanks Martin,
eslint is very happy now :-)

Do you mind to apply to this patch another couple of very small tweaks?

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
@@ +497,5 @@
>    info("webnavigation extension unloaded");
>  });
> +
> +add_task(function* webnav_error_event() {
> +  function backgroundScriptTransitions() {

we should call this `backgroundScriptErrorEvent` (or something like that)

and small tweak possible, given that we're interested into only one webNavigation event (the onErrorOccurred), is simplify the code from line 502 to line 516.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try of the above two patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d168eeb0c27

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8762702 [details] [diff] [review]
bug1267221-v2.patch

Added r? assigned to Andrew for a final review.

Comment 10

[Martin Giger [:freaktechnik]]

Attachment: bug1267221-test-v3.patch

Simplified the test as suggested by Luca.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8762905 [details] [diff] [review]
bug1267221-test-v3.patch

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

Looks good, thanks!  Can you squash the two commits into a single one before landing?  (make sure to mark the old patches obsolete then you can go ahead and set r+ on the combined patch).

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 1-Bug_1267221___Add__error__field_to_WebNavigation_onErrorOccurred_details__r_rpl_aswan.patch

I'm attaching the two patches from Martin (as reviewed from me and Andrew), squashed in a single patch.

Re-applying the r+ (there are no changes from the previously reviewed patches).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ea9d62d5f09b
Add 'error' field to WebNavigation onErrorOccurred details. r=rpl,aswan

Comment 14

[Carsten Book [:Tomcat]]

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