Closed
Bug 762049
Opened 12 years ago
Closed 12 years ago
Run <iframe mozbrowser> tests in- and out-of-process on all platforms except Windows
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(5 files)
1.87 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
48.86 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
50.01 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
8.49 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
1.54 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
At the moment, we run the <iframe mozbrowser> tests OOP on *nix and in-process on Windows. With bug 761939 fixed, we should be able to run the tests OOP on Windows. We should rejigger the test framework so we run all the tests both OOP and in-process on both Windows and *nix.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
These patches work, but they rely on bug 757182. Since the patches will rot as soon as anyone lands a test or test change to mozbrowser, I'm not going to mark for review until all the dependencies are met here. Although this is a lot of code churn, this should be a simple review; the changes are all mechanical.
Depends on: 757182
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 630787 [details] [diff] [review] Bug 762049 - Part 1: Rename test_browserFrame{1,2,3}.html. All dependencies landed (\o/). Mounir, would you like to do the honors?
Attachment #630787 -
Flags: review?(mounir)
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 630788 [details] [diff] [review] Bug 762049 - Part 2: Make remaining tests both in-process and OOP. The technique here is to create three files for every test: 1. test_browserElement_oop_Foo.html 2. test_browserElement_inproc_Foo.html 3. browserElement_Foo.js Files (1) and (2) are identical, and all of the test code is in (3). browserFrameHelpers (renamed to browserElementTestHelpers) sets the in/out-of process pref according to the filename. I didn't do this for the tests touched in part 1 because those tests are checking the security prefs, which should work the same in and out-of process. I didn't modify any of the test code, except for: * one test which relied on strict mode not being set; I fixed the test so we can run it with strict mode. * two tests which relied on an <iframe> being present in the html file. I want these html files to be mere shells, and it was easy enough to use appendElement to the same effect. I'm going to post a fourth patch adding PD licenses to all these files.
Attachment #630788 -
Flags: review?(mounir)
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 630789 [details] [diff] [review] Bug 762049 - Part 3: Rename browserFrameHelpers to browserElementTestHelpers. Slowly getting rid of "browser frame" in favor of "browser element", which I suspect will get a fast r+ from you. :)
Attachment #630789 -
Flags: review?(mounir)
Updated•12 years ago
|
Attachment #630787 -
Flags: review?(mounir) → review+
Comment 8•12 years ago
|
||
Comment on attachment 630789 [details] [diff] [review] Bug 762049 - Part 3: Rename browserFrameHelpers to browserElementTestHelpers. Review of attachment 630789 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thank you sed.
Attachment #630789 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #631038 -
Flags: review?(mounir)
Comment 10•12 years ago
|
||
Comment on attachment 630788 [details] [diff] [review] Bug 762049 - Part 2: Make remaining tests both in-process and OOP. Review of attachment 630788 [details] [diff] [review]: ----------------------------------------------------------------- r=me You could have given nicer <title> to your tests as long as you were here but that's not very important I believe ;) ::: dom/browser-element/mochitest/test_browserFramePromptConfirm.html @@ +82,5 @@ > </scr' + 'ipt></body></html>'; > } > > runTest(); > + nit: remove that empty line.
Attachment #630788 -
Flags: review?(mounir) → review+
Comment 11•12 years ago
|
||
Comment on attachment 631038 [details] [diff] [review] Part 4: Add CC0 license to all test JS files. Review of attachment 631038 [details] [diff] [review]: ----------------------------------------------------------------- r=me, assuming you didn't forget a file. BTW, why not html files?
Attachment #631038 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 12•12 years ago
|
||
> BTW, why not html files?
Mostly because most of our HTML mochitests don't have licenses. Is that a thing we're doing these days?
Comment 13•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #12) > > BTW, why not html files? > > Mostly because most of our HTML mochitests don't have licenses. Is that a > thing we're doing these days? AFAIK, we don't. But I doubt a lot of people take time to put licenses boilerplate in tests in general.
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=923a13185d93
Assignee | ||
Comment 15•12 years ago
|
||
Landed for FF16. https://hg.mozilla.org/integration/mozilla-inbound/rev/840a820e589c https://hg.mozilla.org/integration/mozilla-inbound/rev/b40369a421bd https://hg.mozilla.org/integration/mozilla-inbound/rev/e129ca091029 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd157a5da52f
Assignee | ||
Comment 16•12 years ago
|
||
And backed out because of Windows M2 bustage (I didn't notice on try because it manifests as a crash *after* all the browser element tests run). Windows OOP is apparently not all it's cracked up to be. But I still want this change, because it makes testing on *nix much better. I'll disable the OOP tests on Windows and file a follow-up bug for figuring out what's going on there. https://hg.mozilla.org/integration/mozilla-inbound/rev/2982a120fef5 https://hg.mozilla.org/integration/mozilla-inbound/rev/7219414f43ce https://hg.mozilla.org/integration/mozilla-inbound/rev/5d64c6f58785 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c8f23e0f49
Comment 17•12 years ago
|
||
It seems this also caused a gc crash during shutdown of a debug b2g desktop build. I just tried with the backout again and it doesn't crash any more.
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #17) > It seems this also caused a gc crash during shutdown of a debug b2g desktop > build. > I just tried with the backout again and it doesn't crash any more. Are you sure? This was a test-only change.
Comment 19•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #18) > (In reply to Gregor Wagner [:gwagner] from comment #17) > > It seems this also caused a gc crash during shutdown of a debug b2g desktop > > build. > > I just tried with the backout again and it doesn't crash any more. > > Are you sure? This was a test-only change. Ah wrong folder... It's still crashing :(
Assignee | ||
Comment 20•12 years ago
|
||
> Ah wrong folder... It's still crashing :(
You mean, it crashes without this change too? Could you please file a bug?
Assignee | ||
Updated•12 years ago
|
Summary: Run <iframe mozbrowser> tests in- and out-of-process on all platforms → Run <iframe mozbrowser> tests in- and out-of-process on all platforms except Windows
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #631537 -
Flags: review?(mounir)
Assignee | ||
Comment 22•12 years ago
|
||
New try push: https://tbpl.mozilla.org/?tree=Try&rev=20fac72a35ae
Assignee | ||
Comment 23•12 years ago
|
||
> And backed out because of Windows M2 bustage Here's a link to a busted log: https://tbpl.mozilla.org/php/getParsedLog.php?id=12465092&tree=Mozilla-Inbound
Comment 24•12 years ago
|
||
Comment on attachment 631537 [details] [diff] [review] Part 5: Don't use OOP on Windows. Review of attachment 631537 [details] [diff] [review]: ----------------------------------------------------------------- r=me even you keep your highly verbose and not clearer version ;) ::: dom/browser-element/mochitest/browserElementTestHelpers.js @@ +127,3 @@ > var oop; > +if (navigator.platform.indexOf('Win') != -1 || > + location.pathname.indexOf("_inproc_") != -1) { I would have done something like: // Enable or disable OOP depending on the test's filename. oop = location.pathname.indexOf("_inproc_") == -1; // Always disable OOP on Windows (bug 763081). oop = navigator.platform.indexOf('Win') != -1 ? false : oop;
Attachment #631537 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 25•12 years ago
|
||
> r=me even you keep your highly verbose and not clearer version ;)
You're right, mine is pretty ugly, isn't it?
Assignee | ||
Comment 26•12 years ago
|
||
Landed again for FF16: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f138abe4c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a827c12da43 https://hg.mozilla.org/integration/mozilla-inbound/rev/830251d84ccd https://hg.mozilla.org/integration/mozilla-inbound/rev/a53c96e72649 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3d46a285e6
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9f138abe4c3 https://hg.mozilla.org/mozilla-central/rev/3a827c12da43 https://hg.mozilla.org/mozilla-central/rev/830251d84ccd https://hg.mozilla.org/mozilla-central/rev/a53c96e72649 https://hg.mozilla.org/mozilla-central/rev/ee3d46a285e6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•