Last Comment Bug 731532 - Fix issues in test pilot validation
: Fix issues in test pilot validation
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :Irving Reid (No longer working on Firefox)
:
Mentors:
Depends on:
Blocks: 729127 736984 737892
  Show dependency treegraph
 
Reported: 2012-02-29 01:32 PST by Mark Banner (:standard8)
Modified: 2012-03-27 00:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Extracted Validation Log (3.81 KB, text/plain)
2012-02-29 01:32 PST, Mark Banner (:standard8)
no flags Details
Work in progress, needs testing (23.05 KB, patch)
2012-03-01 06:37 PST, :Irving Reid (No longer working on Firefox)
standard8: review+
Details | Diff | Splinter Review
Test pilot cleanup WIP, version 2 (26.01 KB, patch)
2012-03-05 13:04 PST, :Irving Reid (No longer working on Firefox)
standard8: review+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2012-02-29 01:32:27 PST
Created attachment 601553 [details]
Extracted Validation Log

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
Comment 1 :Irving Reid (No longer working on Firefox) 2012-02-29 11:52:58 PST
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
Comment 2 Mark Banner (:standard8) 2012-02-29 12:29:29 PST
(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.
Comment 3 :Irving Reid (No longer working on Firefox) 2012-03-01 06:37:38 PST
Created attachment 601945 [details] [diff] [review]
Work in progress, needs testing

Mostly replaced assignments to innerHTML with textContent, except one place where I use the HTML sanitizer service - this needs to be tested.
Comment 4 Mark Banner (:standard8) 2012-03-02 04:41:22 PST
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.
Comment 5 :Irving Reid (No longer working on Firefox) 2012-03-02 10:53:19 PST
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.
Comment 6 :Irving Reid (No longer working on Firefox) 2012-03-02 11:06:59 PST
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...
Comment 7 :Irving Reid (No longer working on Firefox) 2012-03-02 12:03:14 PST
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...
Comment 8 :Irving Reid (No longer working on Firefox) 2012-03-05 13:04:46 PST
Created attachment 603030 [details] [diff] [review]
Test pilot cleanup WIP, version 2

changed appendElement() to appendChild() and fixed the error in addEventListener. Still needs testing, particularly the survey_generator.js changes.
Comment 9 Mark Banner (:standard8) 2012-03-13 06:58:37 PDT
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.
Comment 10 Mark Banner (:standard8) 2012-03-13 08:06:46 PDT
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
Comment 11 Mark Banner (:standard8) 2012-03-20 07:39:38 PDT
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.
Comment 12 Mark Banner (:standard8) 2012-03-20 07:40:30 PDT
Checked in:

http://hg.mozilla.org/releases/comm-beta/rev/9502ae8490b0

Note You need to log in before you can comment on or make changes to this bug.