1239586 - Add tests for document embedding in embed elements

Status: RESOLVED FIXED

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

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

As of bug 1237963, we can now embed documents using embed elements. Still need to write some tests for it.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Bonus points if they're web-platform-tests.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Mochitest for Embed element document support

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

:(

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Web Platform Test for document embedding

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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Web Platform Test for document embedding

Ok, actually attaching the correct files this time.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Web Platform Test for document embedding

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8710703 [details] [diff] [review]
Patch 1 (v4) - Web Platform Test for document embedding

r=me

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c203be84409

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c203be84409

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks! One nit (which I'll fix): you forgot a `var`.