Closed Bug 731532 Opened 12 years ago Closed 12 years ago

Fix issues in test pilot validation

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird12 fixed)

RESOLVED FIXED
Thunderbird 13.0
Tracking Status
thunderbird12 --- fixed

People

(Reporter: standard8, Assigned: Irving)

References

Details

Attachments

(2 files, 1 obsolete file)

Various issues were found in the AMO validation of the test pilot xpi - see attached files.

We need to fix these asap so we can start pushing out studies.

For the innerHTML stuff, see also https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion
Based on the discussion linked, I think we should push back on the evalInSandbox warning.

https://forums.mozilla.org/addons/viewtopic.php?f=14&t=4532&view=previous&sid=fb4b5f292960eda5d89a2198a8df5dd4
Status: NEW → ASSIGNED
(In reply to Irving Reid (:irving) from comment #1)
> Based on the discussion linked, I think we should push back on the
> evalInSandbox warning.
> 
> https://forums.mozilla.org/addons/viewtopic.
> php?f=14&t=4532&view=previous&sid=fb4b5f292960eda5d89a2198a8df5dd4

Agreed.
Attached patch Work in progress, needs testing (obsolete) — Splinter Review
Mostly replaced assignments to innerHTML with textContent, except one place where I use the HTML sanitizer service - this needs to be tested.
Attachment #601945 - Flags: feedback?(mbanner)
Comment on attachment 601945 [details] [diff] [review]
Work in progress, needs testing

This looks fine. I've been using it for the test we've got running at the moment and I've not noticed any issues.

We've not tested the survey generator, but we can always hook that up later.

The AMO validator output looks a lot better as well.

So I think this looks fine, but I'm going to see if Gregg can give us some feedback as well today before we land it.
Attachment #601945 - Flags: review+
Attachment #601945 - Flags: feedback?(mbanner)
Attachment #601945 - Flags: feedback?(glind)
Comment on attachment 601945 [details] [diff] [review]
Work in progress, needs testing

Looking back, I think I used appendElement() in a few places where I probably should have used appendChild() to add elements to the HTML content - I'm not sure whether these are interchangeable.
OK, now I've seen an error message in my stderr that needs to be tracked down:

JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert JavaScript argument arg 1 [nsIDOMEventTarget.addEventListener]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: chrome://testpilot/content/all-studies-window.js :: <TOP_LEVEL> :: line 190"  data: no]

Looking into it...
STR the addEventListener error: Start up TB with the patched test pilot, go to Tools/Test Pilot/All Your User Studies

Good thing the assertion message didn't give me any information about the JS object it was trying to convert, he says snarkily. An added dump tells me the inputs to addButton are:

"Submit", "submit-button-10002", "TestPilotXulWindow.onSubmitButton(10002);"

Converting the Javascript string argument into an anonymous function declaration makes this error go away, now testing further to make sure it actually works...
changed appendElement() to appendChild() and fixed the error in addEventListener. Still needs testing, particularly the survey_generator.js changes.
Attachment #601945 - Attachment is obsolete: true
Attachment #603030 - Flags: feedback?(mbanner)
Attachment #601945 - Flags: feedback?(glind)
Comment on attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

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

As discussed on irc, I've been running this for the last few days and not found any issues.

Although survey-generator.js may need a few more tweaks, we'll be testing any surveys before pushing them out, so I think we're all-right to go with it as-is.

Hence r=me.
Attachment #603030 - Flags: feedback?(mbanner) → review+
Checked in: 

http://hg.mozilla.org/comm-central/rev/66fd8a08a63b

I also did a version bump for test pilot so that we can push these to AMO:

http://hg.mozilla.org/comm-central/rev/6a007b23cc21
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Comment on attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

[Triage Comment]
Taking to beta as I want to get the version of test pilot we're distributing, synced up with the version on AMO.
Attachment #603030 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: