Closed Bug 678465 Opened 8 years ago Closed 8 years ago

'document-element-inserted' doesn't fire on ImageDocument

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: ochameau, Assigned: gkrizsanits)

References

Details

(Whiteboard: [jetpack:p2])

Attachments

(1 file, 7 obsolete files)

In Jetpack, we rely on this event to propose page-mod API, 
that allow developers to modify web contents.
Unfortunately, this event is not dispatched on images that have an ImageDocument instead of the regular HTMLDocument.

(FYI: 'content-document-global-created' works fine)

I was wondering if we can use a web standard event instead of document-element-inserted ? For example, listen to content-document-global-created and then watch for readystate change? Would it be equivalent ?
Blocks: 668258
ImageDocuments aren't created by the HTML parser, so moving this to a more generic component. Need to check media and plug-in docs while at it.

document-element-inserted has no equivalent readyState transition. Furthermore, readyStates for image, media and plug-in docs are mostly bogus; bug 666134.
Component: HTML: Parser → DOM
QA Contact: parser → general
CCing sicking based on a recollection that he has worked on these events.
I did, but I doubt I'll have time to fix this anytime soon. Would love someone else to pick it up.
Eddy, Gábor: perhaps one of you could take this on?
I can pick it up. sicking: could you give me some hints where to start with it?
Somewhere in the ImageDocument code there should be code that creates the elements that it contains. You'll have to modify that code to explicitly send the relevant notification.
Assignee: nobody → gkrizsanits
Attached patch first attempt to fix the bug (obsolete) — Splinter Review
Sicking: I added you as reviewer, if you don't have the time, could you suggest/CC someone else for review?
Attachment #557851 - Flags: review?(jonas)
Jonas: does this need sr, or is it ready for commission?
It's pretty simple, I don't think sr is needed
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
In my queue with a few other bits that are being sent to try first and then onto inbound.

To save time for future patches, could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/379147b5215f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/c62d1af37761

this was backed out in the above.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Probably this can be avoided by simple firing this event only for ImageDocument, instead of MultimediaDocument (where I just discovered that the event will be fired by xbl utils anyway), but before I create a new patch for this I would like to understand what is going on in Bug 689106. Alex, do you need this event only for ImageDocuments or do you know about any other cases when you don't receive this event?
Attached patch fix - 2nd version (obsolete) — Splinter Review
This seems to fix the issue we had with the first version. I still have to come up with a good test for this.
Attachment #557851 - Attachment is obsolete: true
Attachment #565901 - Flags: review?(bzbarsky)
I opened this bug only for images documents. We are not aware of any other case where we lack this event. Did you verified that we receive document-element-inserted in case of video?
Because I don't see any other usecase of this event, except from HTML parser:
http://mxr.mozilla.org/mozilla-central/search?string=document-element-inserted

And now that you got more into this document creation code, do you have, by any chance, something to say about bug 668258 comment 3 question??
(In reply to Alexandre Poirot (:ochameau) from comment #17)
> I opened this bug only for images documents. We are not aware of any other
> case where we lack this event. Did you verified that we receive
> document-element-inserted in case of video?
> Because I don't see any other usecase of this event, except from HTML parser:
> http://mxr.mozilla.org/mozilla-central/search?string=document-element-
> inserted

Good that you asked! Yes, I checked it and I was tricked at first (see my prev. comment) since for videos there was indeed a document-element-inserted event fired (in some case) BUT not for the video document. It was for some XML document created in the background for video utils (XBL stuff). So you will have to check the document in that event for addons probably. 

Also I just realized while double checking this, is that I do fire the event twice now, since SetScriptGlobalObject can be called with null as the global object, in which case we probably don't want to fire the event. Will correct the patch soon and I will have to write test for it anyway...

> 
> And now that you got more into this document creation code, do you have, by
> any chance, something to say about bug 668258 comment 3 question??

I digged into this a little I'll try to answer it on the other thread.
I fixed the previous patch, a good test is still needed for it.
Attachment #565901 - Attachment is obsolete: true
Attachment #565901 - Flags: review?(bzbarsky)
Attachment #565961 - Flags: review?(bzbarsky)
Comment on attachment 565961 [details] [diff] [review]
some fixes for the previous version

Shouldn't MediaDocument::SetScriptGlobalObject call nsHTMLDocument::SetScriptGlobalObject?

Won't this misbehave (send a second notification) when the image document comes out of bfcache?  Presumably you want to make sure that the notification is only sent once for a given document.....
Attachment #565961 - Flags: review?(bzbarsky) → review-
Attached patch patch v4 (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #20)
> Won't this misbehave (send a second notification) when the image document
> comes out of bfcache?  Presumably you want to make sure that the
> notification is only sent once for a given document.....

I really couldn't find a way to ensure this without introducing a new bool member.
Attachment #565961 - Attachment is obsolete: true
Attachment #567070 - Flags: review?(bzbarsky)
Comment on attachment 567070 [details] [diff] [review]
patch v4

Yeah, bool member is fine here.

Maybe add a test for this?  Followup is fine.
Attachment #567070 - Flags: review?(bzbarsky) → review+
Depends on: 697775
Blocks: 697775
No longer depends on: 697775
Attached patch final version with test (obsolete) — Splinter Review
Here is an updated patch with test. Since this is the first time I attach a test, I thought it's might be better if some takes a look at it... Any feedback are welcome so I don't make the same mistake with the next tests I'm going to file soon. The code is the same as it was in the previous reviewed version.
Attachment #567070 - Attachment is obsolete: true
Attachment #572757 - Flags: review?(bzbarsky)
Comment on attachment 572757 [details] [diff] [review]
final version with test

This test may better belong in content/html/document/test.

Do you need a huge png image here?  If not, just us a small one!

You shouldn't be using enablePrivilege in new tests.  Does SpecialPowers not let you do what you want?

Using timeouts like this test does is a recipe for random orange; nothing guarantees that the media will load in 3 seconds.  Just make your document-element-inserted observer or observers drive the test progression instead.
Attachment #572757 - Flags: review?(bzbarsky) → review-
Attached patch fixed test (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #24)
> Comment on attachment 572757 [details] [diff] [review] [diff] [details] [review]
I realized I can use resource files from other directories too so I don't have to add any new image, and I can put the test in the directory where it really belongs...
 
> You shouldn't be using enablePrivilege in new tests.  Does SpecialPowers not
> let you do what you want?
I was not aware of SpecialPowers, thanks, I changed it.
Attachment #572757 - Attachment is obsolete: true
Attachment #573181 - Flags: review?(bzbarsky)
Whiteboard: [jetpack:p2]
Comment on attachment 573181 [details] [diff] [review]
fixed test

>+        media.setAttribute("src",tests[0]);
>+        tests = tests.slice(1);

  media.setAttribute("src", tests.shift());

But there's a bigger issue here: this test will pass as long as document-element-inserted is called on _some_ documents.  It should be checking that it's being called on the right documents.
Attachment #573181 - Flags: review?(bzbarsky) → review-
Attached patch fixed test (obsolete) — Splinter Review
In the test, the reason why I'm checking against media.contentDocument.location instead of doc.location, is that since I'm not in a chrome context I cannot access the properties of doc. But since I check that they are the same object, this should not be a problem.
Attachment #573181 - Attachment is obsolete: true
Attachment #581254 - Flags: review?(bzbarsky)
Comment on attachment 581254 [details] [diff] [review]
fixed test

>+++ b/content/html/document/test/test_document-element-inserted.html
>+        ok(media.contentDocument.location.toString().indexOf(loc) != -1);

This should have a test description as the scond argument, something like:

  "The loaded media should be " + loc

>+var observer = { observe: observe };

You probably don't need this.  You should be able to pass the |observe| function directly to add/removeObserver.

r=me with those nits.
Attachment #581254 - Flags: review?(bzbarsky) → review+
Attached patch final versionSplinter Review
Here is the final version. Ed, could you check this in for me?
Attachment #581254 - Attachment is obsolete: true
Attachment #581572 - Flags: review+
Attachment #581572 - Flags: checkin?(bmo)
This is waiting on bug 709193. Also, has this been past try? :-)
Keywords: checkin-needed
Whiteboard: [jetpack:p2] → [jetpack:p2] [waiting on bug 709193]
(In reply to Ed Morley [:edmorley] from comment #30)
> This is waiting on bug 709193. Also, has this been past try? :-)

This looks like a pass to me: https://tbpl.mozilla.org/?tree=Try&rev=4a33ea8a7a0a
The jetpack tests had some oranges but I checked with Alex and it's a none issue ( bug 705011 )
So I misinterpreted the output of the try server, ignore my previous comment.
Keywords: checkin-needed
Whiteboard: [jetpack:p2] [waiting on bug 709193] → [jetpack:p2]
Attachment #581572 - Flags: checkin?(bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/49481d05bd7c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla9 → mozilla11
Whoops, pre-emptive FIXED, need to get back into inbound mode, after our spell in m-c :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/49481d05bd7c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.