Closed Bug 1267221 Opened 4 years ago Closed 4 years ago

WebNavigation onErrorOccurred details should contains an 'error' field

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: rpl, Assigned: freaktechnik)

References

Details

(Whiteboard: [webNavigation][good first bug] triaged)

Attachments

(1 file, 5 obsolete files)

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.
Whiteboard: [webNavigation]
Flags: blocking-webextensions?
Priority: -- → P2
Flags: blocking-webextensions?
Whiteboard: [webNavigation] → [webNavigation] triaged
Whiteboard: [webNavigation] triaged → [webNavigation][good first bug] triaged
Attached patch bug1267221-v1.patch (obsolete) — Splinter Review
Adds the error field for the data object emitted for an onErrorOccured and no other event. Will open a follow-up bug for tests.
Attachment #8762695 - Flags: review?(lgreco)
Attached patch bug1267221-v2.patch (obsolete) — Splinter Review
Fix typo in commit message and get error property from original data object.
Attachment #8762695 - Attachment is obsolete: true
Attachment #8762695 - Flags: review?(lgreco)
Attachment #8762702 - Flags: review?(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.
Attachment #8762702 - Flags: review?(lgreco) → feedback+
Attached patch bug1267221-test-v1.patch (obsolete) — Splinter Review
Attachment #8762715 - Flags: review?(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)
Attachment #8762715 - Flags: review?(lgreco)
Assignee: nobody → martin
Status: NEW → ASSIGNED
Attached patch bug1267221-test-v2.patch (obsolete) — Splinter Review
Attachment #8762715 - Attachment is obsolete: true
Attachment #8762851 - Flags: review?(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.
Attachment #8762851 - Flags: review?(lgreco) → feedback+
Comment on attachment 8762702 [details] [diff] [review]
bug1267221-v2.patch

Added r? assigned to Andrew for a final review.
Attachment #8762702 - Flags: review?(aswan)
Attached patch bug1267221-test-v3.patch (obsolete) — Splinter Review
Simplified the test as suggested by Luca.
Attachment #8762851 - Attachment is obsolete: true
Attachment #8762905 - Flags: review?(aswan)
Attachment #8762702 - Flags: review?(aswan) → review+
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).
Attachment #8762905 - Flags: review?(aswan) → review+
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).
Attachment #8762702 - Attachment is obsolete: true
Attachment #8762905 - Attachment is obsolete: true
Attachment #8763040 - Flags: review+
Iteration: --- → 50.1
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/ea9d62d5f09b
Add 'error' field to WebNavigation onErrorOccurred details. r=rpl,aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea9d62d5f09b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.