Last Comment Bug 678465 - 'document-element-inserted' doesn't fire on ImageDocument
: 'document-element-inserted' doesn't fire on ImageDocument
Status: RESOLVED FIXED
[jetpack:p2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Gabor Krizsanits [:krizsa :gabor]
:
Mentors:
Depends on: 689106
Blocks: 668258 697775
  Show dependency treegraph
 
Reported: 2011-08-12 02:51 PDT by Alexandre Poirot [:ochameau] PTO, back on 1st
Modified: 2012-02-01 13:58 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first attempt to fix the bug (3.23 KB, patch)
2011-09-02 09:08 PDT, Gabor Krizsanits [:krizsa :gabor]
jonas: review+
Details | Diff | Splinter Review
fix - 2nd version (2.81 KB, patch)
2011-10-10 04:46 PDT, Gabor Krizsanits [:krizsa :gabor]
no flags Details | Diff | Splinter Review
some fixes for the previous version (3.38 KB, patch)
2011-10-10 09:46 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review-
Details | Diff | Splinter Review
patch v4 (6.22 KB, patch)
2011-10-14 06:50 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review
final version with test (75.88 KB, patch)
2011-11-08 01:47 PST, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review-
Details | Diff | Splinter Review
fixed test (5.46 KB, patch)
2011-11-09 06:36 PST, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review-
Details | Diff | Splinter Review
fixed test (8.60 KB, patch)
2011-12-13 06:36 PST, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: review+
Details | Diff | Splinter Review
final version (8.61 KB, patch)
2011-12-14 03:06 PST, Gabor Krizsanits [:krizsa :gabor]
gkrizsanits: review+
Details | Diff | Splinter Review

Description Alexandre Poirot [:ochameau] PTO, back on 1st 2011-08-12 02:51:31 PDT
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 ?
Comment 1 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-08-31 06:01:28 PDT
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.
Comment 2 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-08-31 06:02:56 PDT
CCing sicking based on a recollection that he has worked on these events.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-31 09:26:04 PDT
I did, but I doubt I'll have time to fix this anytime soon. Would love someone else to pick it up.
Comment 4 Myk Melez [:myk] [@mykmelez] 2011-08-31 09:37:45 PDT
Eddy, Gábor: perhaps one of you could take this on?
Comment 5 Gabor Krizsanits [:krizsa :gabor] 2011-08-31 09:40:19 PDT
I can pick it up. sicking: could you give me some hints where to start with it?
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-31 09:55:36 PDT
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.
Comment 7 Gabor Krizsanits [:krizsa :gabor] 2011-09-02 09:08:19 PDT
Created attachment 557851 [details] [diff] [review]
first attempt to fix the bug

Sicking: I added you as reviewer, if you don't have the time, could you suggest/CC someone else for review?
Comment 8 Myk Melez [:myk] [@mykmelez] 2011-09-20 12:01:25 PDT
Jonas: does this need sr, or is it ready for commission?
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-20 13:53:07 PDT
It's pretty simple, I don't think sr is needed
Comment 10 Ed Morley [:emorley] 2011-09-22 04:44:09 PDT
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 :-)
Comment 12 Ed Morley [:emorley] 2011-09-22 17:32:43 PDT
https://hg.mozilla.org/mozilla-central/rev/379147b5215f
Comment 13 Rob Campbell [:rc] (:robcee) 2011-10-04 05:37:35 PDT
https://hg.mozilla.org/mozilla-central/rev/c62d1af37761

this was backed out in the above.
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2011-10-04 09:09:35 PDT
This is the correct link for the backout diff:
http://hg.mozilla.org/mozilla-central/rev/00060b8e57f7
Comment 15 Gabor Krizsanits [:krizsa :gabor] 2011-10-05 05:13:04 PDT
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?
Comment 16 Gabor Krizsanits [:krizsa :gabor] 2011-10-10 04:46:03 PDT
Created attachment 565901 [details] [diff] [review]
fix - 2nd version

This seems to fix the issue we had with the first version. I still have to come up with a good test for this.
Comment 17 Alexandre Poirot [:ochameau] PTO, back on 1st 2011-10-10 05:40:20 PDT
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??
Comment 18 Gabor Krizsanits [:krizsa :gabor] 2011-10-10 06:54:53 PDT
(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.
Comment 19 Gabor Krizsanits [:krizsa :gabor] 2011-10-10 09:46:43 PDT
Created attachment 565961 [details] [diff] [review]
some fixes for the previous version

I fixed the previous patch, a good test is still needed for it.
Comment 20 Boris Zbarsky [:bz] 2011-10-11 21:47:30 PDT
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.....
Comment 21 Gabor Krizsanits [:krizsa :gabor] 2011-10-14 06:50:34 PDT
Created attachment 567070 [details] [diff] [review]
patch v4

(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.
Comment 22 Boris Zbarsky [:bz] 2011-10-14 11:34:35 PDT
Comment on attachment 567070 [details] [diff] [review]
patch v4

Yeah, bool member is fine here.

Maybe add a test for this?  Followup is fine.
Comment 23 Gabor Krizsanits [:krizsa :gabor] 2011-11-08 01:47:51 PST
Created attachment 572757 [details] [diff] [review]
final version with test

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.
Comment 24 Boris Zbarsky [:bz] 2011-11-08 19:44:27 PST
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.
Comment 25 Gabor Krizsanits [:krizsa :gabor] 2011-11-09 06:36:22 PST
Created attachment 573181 [details] [diff] [review]
fixed test

(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.
Comment 26 Boris Zbarsky [:bz] 2011-12-12 10:36:38 PST
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.
Comment 27 Gabor Krizsanits [:krizsa :gabor] 2011-12-13 06:36:10 PST
Created attachment 581254 [details] [diff] [review]
fixed test

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.
Comment 28 Boris Zbarsky [:bz] 2011-12-13 10:55:09 PST
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.
Comment 29 Gabor Krizsanits [:krizsa :gabor] 2011-12-14 03:06:09 PST
Created attachment 581572 [details] [diff] [review]
final version

Here is the final version. Ed, could you check this in for me?
Comment 30 Ed Morley [:emorley] 2011-12-14 03:56:47 PST
This is waiting on bug 709193. Also, has this been past try? :-)
Comment 31 Gabor Krizsanits [:krizsa :gabor] 2011-12-14 08:01:05 PST
(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 )
Comment 32 Gabor Krizsanits [:krizsa :gabor] 2011-12-14 08:32:50 PST
So I misinterpreted the output of the try server, ignore my previous comment.
Comment 33 Ed Morley [:emorley] 2011-12-15 03:03:21 PST
https://tbpl.mozilla.org/?tree=Try&rev=52aa1b06d4c2
Comment 35 Ed Morley [:emorley] 2011-12-15 07:13:28 PST
Whoops, pre-emptive FIXED, need to get back into inbound mode, after our spell in m-c :-)
Comment 36 Ed Morley [:emorley] 2011-12-16 06:17:27 PST
https://hg.mozilla.org/mozilla-central/rev/49481d05bd7c

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