1541136 - TEST-UNEXPECTED-FAIL | [snip]mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest and TEST-UNEXPECTED-FAIL | [snip]/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Fresh bustage, just in :-(

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1554226816/build/tests/mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1554226816/build/tests/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS

Log says:
17:50:58 INFO - SUMMARY-UNEXPECTED-FAIL | testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest
17:50:58 INFO - EXCEPTION: menuitem is null
17:50:58 INFO - at: test-calendar-utils.js line 627
17:50:58 INFO - menulistSelect test-calendar-utils.js:627 5
17:50:58 INFO - handleNewCalendarWizard test-calendar-utils.js:577 5
17:50:58 INFO - testSmokeTest/< testBasicFunctionality.js:85 9

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ea09774456976ada64ed753a00724ef6bc&tochange=92d1c344e7c53af9b8ad8a4192d93478f7

Looks like
Bug 1498569, Replace wizard.xml attributes with event listeners, r=Gijs

changed <wizardpage>

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/46eb5a67c089
temporarily disable testBasicFunctionality.js and testLocalICS.js. rs=bustage-fix

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1541136-calendar-wizardpage-1.diff

You're right, the <wizardpage> change broke things. This patch only fixes the instances in calendar, there are more. I will look at them shortly.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1541136-mail-wizardpage-1.diff

The non-calendar code that needed the same changes, but clearly doesn't break any tests without the changes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/70c4a25e3eaf
Backed out changeset 46eb5a67c089 to re-enable tests
https://hg.mozilla.org/comm-central/rev/cb6bda3df23a
Replace on* attributes of <wizardpage> with addEventListener, calendar only; rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/ae892d0e67b2
Replace on* attributes of <wizardpage> with addEventListener, non-calendar; rs=bustage-fix DONTBUILD

Comment 5

[:aceman]

Comment on attachment 9055408 [details] [diff] [review]
1541136-mail-wizardpage-1.diff

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

Thanks, but you haven't tested this much. Any of the wizard dialogs is broken.

Please also remove wizard.xml from security.uris_using_eval_with_system_principal as m-c has done, if we do not use any other evals in it.

::: mail/components/im/content/imAccountWizard.js
@@ +9,5 @@
>  var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
>  
>  var PREF_EXTENSIONS_GETMOREPROTOCOLSURL = "extensions.getMoreProtocolsURL";
>  
> +document.getElementById("accountprotocol").addEventListener("pageadvanced", accountWizard.selectProtocol);

TypeError: document.getElementById(...) is null imAccountWizard.js:13:10

TypeError: accountWizard is undefined imAccountWizard.xul:1:1

::: mailnews/base/prefs/content/AccountWizard.js
@@ +73,5 @@
> +    document.documentElement.canAdvance = true;
> +  });
> +  accounttypePage.addEventListener("pageadvanced", acctTypePageUnload);
> +  let identityPage = document.getElementById("identitypage");
> +  identityPage.addeEventListener("pageshow", identityPageInit);

typo: addeEventListener

::: mailnews/extensions/newsblog/content/feedAccountWizard.js
@@ +6,5 @@
>  var {FeedUtils} = ChromeUtils.import("resource:///modules/FeedUtils.jsm");
>  
> +document.getElementById("accountsetuppage").addEventListener(
> +  "pageshow", FeedAccountWizard.accountSetupPageValidate
> +);

TypeError: document.getElementById(...) is null in feedAccountWizard.js:8:10

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1541136-mail-wizardpage-2.diff

Let's try that again.

Comment 7

[Ben Campbell]

Thanks Geoff! New patch seems to work fine for me on the Feeds and Newsgroups wizard at least.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1541136-follow-up.patch

This is the difference between version 1 and version 2 of the non-calendar part.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/007c7ec19799
Replace on* attributes of <wizardpage> with addEventListener, non-calendar, follow-up. rs=bustage-fix,jorgk DONTBUILD

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6ae5ec3f014b
Follow-up: remove one remaining onwizardfinish attribute; rs=me DONTBUILD

Comment 13

[:aceman]

Comment on attachment 9055725 [details] [diff] [review]
1541136-follow-up.patch

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

Thanks.

::: mailnews/extensions/newsblog/content/feedAccountWizard.js
@@ +9,5 @@
>  var FeedAccountWizard = {
>    accountName: "",
>  
> +  onLoad() {
> +    document.documentElement.addEventListener("wizardfinish", this.onFinish.bind(this));

What about the 'wizardcancel' event? Where did that go?

Comment 14

[Jorg K (CEST = GMT+2)]

You were meant to review the other patch, version 2, not the difference between v1 and v2 which I landed :-(

The feed wizard cancel was a no-op returning true to allow the panel to close, which is no longer needed, at least that's my understanding.

Comment 15

[:aceman]

Comment on attachment 9055649 [details] [diff] [review]
1541136-mail-wizardpage-2.diff

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

OK, this looks better now.

But creating some accounts still shows errors, I'll file that separately as I am not sure if that is caused by these changes.

Comment 16

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #14)

The feed wizard cancel was a no-op returning true to allow the panel to close, which is no longer needed, at least that's my understanding.

Exactly this.