Closed
Bug 731532
Opened 13 years ago
Closed 13 years ago
Fix issues in test pilot validation
Categories
(Thunderbird :: General, defect)
Thunderbird
General
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)
3.81 KB,
text/plain
|
Details | |
26.01 KB,
patch
|
standard8
:
review+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•13 years ago
|
||
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
Reporter | ||
Comment 2•13 years ago
|
||
(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.
Assignee | ||
Comment 3•13 years ago
|
||
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)
Reporter | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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...
Assignee | ||
Comment 7•13 years ago
|
||
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...
Assignee | ||
Comment 8•13 years ago
|
||
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)
Reporter | ||
Comment 9•13 years ago
|
||
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+
Reporter | ||
Comment 10•13 years ago
|
||
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: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Reporter | ||
Comment 11•13 years ago
|
||
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+
Reporter | ||
Comment 12•13 years ago
|
||
status-thunderbird13:
--- → fixed
Reporter | ||
Updated•13 years ago
|
status-thunderbird12:
--- → fixed
status-thunderbird13:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•