Closed Bug 1239586 Opened 5 years ago Closed 5 years ago

Add tests for document embedding in embed elements

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 3 obsolete files)

As of bug 1237963, we can now embed documents using embed elements. Still need to write some tests for it.
Bonus points if they're web-platform-tests.
Sorry Ms2ger, meant to ni? you after I put the patch up. From checking out the other embed w-p-ts, I'm not sure how this should work as a w-p-t? Would the format of the mochi test (having a frame call to a parent function) work?
Flags: needinfo?(Ms2ger)
Yes, that would work fine.

In general, I think you should structure the test like so, to avoid slow timeout on failure.  In the parent, do:

  var childLoaded = false;
  onload = function() { /* check childLoaded here */ }

In the child do:

  parent.childLoaded = true;

In terms of how a wpt test would do this, something like:

  var t = async_test("description here");
  addEventListener("load", t.step_func_done(function() { assert_true(childLoaded); }));

or so.
Comment on attachment 8708104 [details] [diff] [review]
Patch 1 (v1) - Mochitest for Embed element document support

Clearing requests for this bug until I get the web platform test done
Flags: needinfo?(Ms2ger)
Attachment #8708104 - Flags: review?(bzbarsky)
Ok, I realize this needs to go into the web-platform github repo, but I figured I'd do a pre-review here before doing a pull request there, hence the fb?'s.

Questions:
- Do I have the test in the right category (directory)? Wasn't sure if it should go in semantics or rendering.
- I went with making an embed node on the fly, as attaching to the load event with a static element didn't seem to work. Is that ok?
Attachment #8708104 - Attachment is obsolete: true
Attachment #8710649 - Flags: feedback?(bzbarsky)
Attachment #8710649 - Flags: feedback?(Ms2ger)
Comment on attachment 8710649 [details] [diff] [review]
Patch 1 (v2) - Web Platform Test for document embedding

> I realize this needs to go into the web-platform github repo

Actually, you can check web platform tests in our tree directly, and jgraham will sync them upstream when he does the merge.  It's pretty sweet.  ;)

> Wasn't sure if it should go in semantics or rendering.

Semantics worksforme, I think.  No strong opinion.

> I went with making an embed node on the fly

You forgot to hg add or something; the file is missing from the diff...
Attachment #8710649 - Flags: feedback?(bzbarsky)
Ok, actually attaching the correct files this time.
Attachment #8710649 - Attachment is obsolete: true
Attachment #8710649 - Flags: feedback?(Ms2ger)
Attachment #8710663 - Flags: review?(bzbarsky)
Attachment #8710663 - Flags: review?(Ms2ger)
Comment on attachment 8710663 [details] [diff] [review]
Patch 1 (v3) - Web Platform Test for document embedding

>+     e.onload = t.step_func_done(function() {

What I meant in comment 5 is what I said:

  addEventListener("load", t.step_func_done(...));

As in, add the load listener to the test window itself, not the embed.  Then you can just use <embed> directly in the markup, I would think.
Attachment #8710663 - Flags: review?(bzbarsky)
Ok, yeah, original idea bz recommended works. Don't know what I did wrong the first time around to make it not work on my end, sorry about that.
Attachment #8710663 - Attachment is obsolete: true
Attachment #8710663 - Flags: review?(Ms2ger)
Attachment #8710703 - Flags: review?(bzbarsky)
Attachment #8710703 - Flags: review?(Ms2ger)
Comment on attachment 8710703 [details] [diff] [review]
Patch 1 (v4) - Web Platform Test for document embedding

r=me
Attachment #8710703 - Flags: review?(bzbarsky) → review+
Comment on attachment 8710703 [details] [diff] [review]
Patch 1 (v4) - Web Platform Test for document embedding

Just gonna go with bz's review on this so I can get this landed.
Attachment #8710703 - Flags: review?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/1c203be84409
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Target Milestone: mozilla46 → mozilla47
Thanks! One nit (which I'll fix): you forgot a `var`.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.