Last Comment Bug 462892 - need a mechanism for easily identifying synthetic documents (e.g. VideoDocument)
: need a mechanism for easily identifying synthetic documents (e.g. VideoDocument)
Status: RESOLVED FIXED
[qa-][testday-20110930]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Paul Adenot (:padenot)
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
Depends on:
Blocks: 462294 462714 466909 556563
  Show dependency treegraph
 
Reported: 2008-11-03 11:30 PST by Justin Dolske [:Dolske]
Modified: 2011-10-17 12:10 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0 - Added VideoDocument empty interface. (8.39 KB, patch)
2011-07-27 09:05 PDT, Paul Adenot (:padenot)
roc: review+
Details | Diff | Splinter Review
Patch v0 - Add mozIsSyntheticDocument attribute to nsIDOMDocument, and set to true for stand alone image / media files. (4.17 KB, patch)
2011-08-09 07:37 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
v1 -- Adressed roc concerns (5.00 KB, patch)
2011-08-09 08:35 PDT, Paul Adenot (:padenot)
roc: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2008-11-03 11:30:13 PST
Bug 448603 added support for directly loading OGG videos, bug 462294 added a context menu item to make use of this from videos already embedded in a page. When you're already viewing media directly, the "View Video" context menu shouldn't be shown.

Images already do this, there code in nsContextMenu.js to check |img.ownerDocument instanceof ImageDocument|. I'm not sure if 448603 added support for "instanceof VideoDocument"... It doesn't seem to work, but nor is mxr very enlightening as to where  "ImageDocument" is coming from so I'm not sure why that works. [Magic in the nsDOMClassInfo.cpp defines?]

Roc: is there a better way to detect when we're in a VideoDocument? Or does that need fixed?
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-11-03 13:36:11 PST
There is no VideoDocument interface, but I suppose we could add an empty interface so you could test for it. Another way would be to check that there's no src attribute or <source> children, but that's an ugly hack. Adding an empty interface sounds better.
Comment 2 Justin Dolske [:Dolske] 2008-11-25 14:31:10 PST
Bug 416661 probably also applies to video, and would want this to test  the same way.
Comment 3 Nochum Sossonko [:Natch] 2008-11-26 15:09:20 PST
Well, bug 416661 also needs an AudioDocument interface, so can an nsMediaDocument interface that covers it all (ie audio, video and image).
Comment 4 Nochum Sossonko [:Natch] 2008-11-27 00:31:31 PST
Can this all be achieved by checking for !nsIHTMLDocument? How many documents are there that can be problematic, i.e. not showing a "Show Video" label in the context menu on image documents isn't a problem, as there aren't any videos in an image document ;)
Comment 5 Justin Dolske [:Dolske] 2008-11-27 08:56:36 PST
No. There are other types of documents,and we shouldn't fix this with fragile hacks.
Comment 6 Nochum Sossonko [:Natch] 2009-01-21 10:55:10 PST
Well, seems like mime type checks are currently being used in browser.js, see bug 454534 comment 8. I think that can work here, and can be considered pretty reliable...
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-22 14:46:58 PST
They're not reliable enough to be used to determine presence of video controls.
Comment 8 Nochum Sossonko [:Natch] 2009-01-22 16:59:04 PST
It's about whether to gray-out (disable) the "View Video" on the context menu, nothing more. If it's not reliable though, i guess not.
Comment 9 Paul Adenot (:padenot) 2011-07-27 09:05:45 PDT
Created attachment 548804 [details] [diff] [review]
Patch v0 - Added VideoDocument empty interface.

Here is a patch proposal, adding an empty interface for a standalone video, in order to test for it using |instanceof| in js.

I've tested this patch by fixing bug 556563, and it seems to work quite well.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-01 01:09:01 PDT
An alternative, simpler patch might be to add a mozSyntheticDocument attribute to the root element of our synthetic documents, and make our UI code look for that attribute instead of doing instanceof. What do you think, Gavin, Justin?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-01 01:13:11 PDT
Another approach would be to not add any new API at all, and instead check document.contentType, which is "video/ogg" for videos for example.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-02 13:36:48 PDT
I think a contentType check is too hard to get right (particularly since we support different kinds of videos, and could potentially add more). A mozSyntheticDocument property would work, and might be nicer than adding all the gunk in that patch.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-02 17:35:25 PDT
(In reply to comment #12)
> I think a contentType check is too hard to get right (particularly since we
> support different kinds of videos, and could potentially add more).

True.

> A
> mozSyntheticDocument property would work, and might be nicer than adding all
> the gunk in that patch.

OK.

Another possible approach would be to simply check if the <video> is the only child of the <body>. Sure, an actual Web page could be doing that, but in that case you're viewing the video standalone already so it probably doesn't make sense to offer a View Video command.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-02 17:36:01 PDT
I guess you'd check that the <video> is the only child of the <body> and the document is a toplevel frame.
Comment 15 Paul Adenot (:padenot) 2011-08-08 06:19:21 PDT
What do we want to do here ?

I initially implemented this feature that way since it was suggested by roc in comment 1 and is was consistent with the way stand-alone images context menu is handled in |nsContextMenu.js|, but I agree it was quite a pain to implement (but I've learned quite a lot of things, though).
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-08 15:34:01 PDT
Let's go with the approach of explicitly marking a document as synthetic. Since just an attribute could be spoofed by content, let's add an mIsSynthetic boolean flag to nsIDocument (with a getter and setter method), and add a readonly mozSyntheticDocument DOM attribute to nsIDOMDocument to return the value of the flag. Then it's easy for any JS to reliably tell whether the document is synthetic.
Comment 17 Paul Adenot (:padenot) 2011-08-09 07:37:23 PDT
Created attachment 551758 [details] [diff] [review]
Patch v0 - Add mozIsSyntheticDocument attribute to nsIDOMDocument, and set to true for stand alone image / media files.

The flag is set to PR_TRUE in |MediaDocument::Init()|, hence stand alone images, video, audio are synthetic, standalone svg is not.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-09 07:55:16 PDT
Comment on attachment 551758 [details] [diff] [review]
Patch v0 - Add mozIsSyntheticDocument attribute to nsIDOMDocument, and set to true for stand alone image / media files.

Review of attachment 551758 [details] [diff] [review]:
-----------------------------------------------------------------

Otherwise looks great

::: content/base/public/nsIDocument.h
@@ +1693,5 @@
>    PRPackedBool mIsBeingUsedAsImage;
>  
> +  // True is this document is synthetic : stand alone image, video, audio
> +  // file, etc.
> +  PRPackedBool mIsSyntheticDocument;

I think you need to initialize this to false somewhere.

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +319,5 @@
>    /**
> +   * True if this document is synthetic : stand alone image, video, audio file,
> +   * etc.
> +   */
> +  readonly attribute boolean        mozIsSyntheticDocument;

rev nsIDOMDocument IID.

Also, I think this should be mozSyntheticDocument. I don't think we have a tradition of using "is" in boolean DOM attribute names.
Comment 19 Paul Adenot (:padenot) 2011-08-09 08:35:19 PDT
Created attachment 551774 [details] [diff] [review]
v1 -- Adressed roc concerns

Initializing the member to PR_FALSE seems unnecessary : http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?mark=1535-1536#1535

I've addressed your other comment, though.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-30 14:48:37 PDT
This looks largely code-specific and not something QA can verify: qa-
Please set to qa+ if this is a mistake.
Comment 23 Eric Shepherd [:sheppy] 2011-10-17 12:10:11 PDT
Documented written:

https://developer.mozilla.org/en/DOM/document.mozSyntheticDocument

And listed on Firefox 8 for developers as well as:

https://developer.mozilla.org/en/DOM/document

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