Closed Bug 462892 Opened 16 years ago Closed 13 years ago

need a mechanism for easily identifying synthetic documents (e.g. VideoDocument)

Categories

(Firefox :: Menus, defect)

Product:
Firefox
Component:
Menus
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 8

People

(Reporter: Dolske, Assigned: padenot)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [qa-][testday-20110930])

Attachments

(1 file, 2 obsolete files)

v1 -- Adressed roc concerns
5.00 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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?
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.
Blocks: 462714
Bug 416661 probably also applies to video, and would want this to test  the same way.
Summary: "View Video" context menu item shouldn't be shown on VideoDocuments → need a JS visible VideoDocument interface
Well, bug 416661 also needs an AudioDocument interface, so can an nsMediaDocument interface that covers it all (ie audio, video and image).
Blocks: 466909
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 ;)
No. There are other types of documents,and we shouldn't fix this with fragile hacks.
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...
They're not reliable enough to be used to determine presence of video controls.
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.
Blocks: 556563
Assignee: nobody → paul
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.
Attachment #548804 - Flags: review?(roc)
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?
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.
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.
(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.
I guess you'd check that the <video> is the only child of the <body> and the document is a toplevel frame.
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).
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.
The flag is set to PR_TRUE in |MediaDocument::Init()|, hence stand alone images, video, audio are synthetic, standalone svg is not.
Attachment #548804 - Attachment is obsolete: true
Attachment #551758 - Flags: review?(roc)
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.
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.
Attachment #551758 - Attachment is obsolete: true
Attachment #551774 - Flags: review?(roc)
Attachment #551758 - Flags: review?(roc)
Keywords: checkin-needed
Summary: need a JS visible VideoDocument interface → need a mechanism for easily identifying synthetic documents (e.g. VideoDocument)
http://hg.mozilla.org/mozilla-central/rev/1c473f70373b
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 8
This looks largely code-specific and not something QA can verify: qa-
Please set to qa+ if this is a mistake.
Whiteboard: [qa-][testday-20110930]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: