463830 - When loading an ogg audio/video file in a browser, the window title should give something useful

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Martijn Wargers (dead)]

Currently, when I load an .ogg file into the browser, the title of the previous page remains (this bug looks a lot like bug 451256, except now it's only for .ogg document).
I think the titlebar should give the name of the video file and perhaps the size, just like happens for image documents.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris can probably fix this. The code is nsVideoDocument, and nsImageDocument has code for setting the title.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Adds UpdateTitle() to nsVideoDocument. An ogg video would have a title "$filename ($mine Video)" when loaded. e.g. foo.ogv would have title "foo.ogv (video/ogg Video)". I can't display the width/height, as that's only available after we've loaded meta data.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can listen for the metadataloaded event and update the title when that fires.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're in a string freeze, so you'll need to add late-l10n here or something. Not sure how that works.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would also be good to put the duration in the title.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The download manager has some way of producing localized duration messages, you might want to see if you can use that. If you can't, then forget about comment #5 for now.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 348129 [details] [diff] [review]
Patch v1

I think I'd like to get a UI review here before talking about the strings.

I'm not sure that users are actually helped by showing them the mimetype. Any idea on how the application preferences get the pretty names?

Re the time, I think that code is in DownloadUtils.jsm, so probably not available to c++.

Generally, strings with %S should have localization notes on what's in them. If you're using multiple variables, you should order them explicitly (%1$S etc), and use that name in the comment.

Looking at the code, I don't really see how 3 of the four strings are actually used, is there another entry point to UpdateTitleAndCharset that's not part of this patch?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> I'm not sure that users are actually helped by showing them the mimetype. Any
> idea on how the application preferences get the pretty names?

I think we'll actually get the pretty name for the media MIME type --- "Wave Audio", "Ogg Video", "Ogg Audio" or something like that.

> Re the time, I think that code is in DownloadUtils.jsm, so probably not
> available to c++.

Blagh.

> Generally, strings with %S should have localization notes on what's in them. If
> you're using multiple variables, you should order them explicitly (%1$S etc),
> and use that name in the comment.
> 
> Looking at the code, I don't really see how 3 of the four strings are actually
> used, is there another entry point to UpdateTitleAndCharset that's not part of
> this patch?

There are existing callers of that method.

Comment 9

[Axel Hecht [:Pike]]

The test says "320x240.ogv (video/ogg Video)".

I figured that there are existing callers to UpdateTitleAndCharset, but it seems that the patch adds all 4 strings, when only ever the one that's used for unspecified size is going to be instantiated. Thus I wonder if the other strings are added because we intend to use them at some point, or are just there and will never be displayed. If so, we shouldn't expose them to l10n.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

So what do we want to display in the video title? How about "$filename - $pretty_mime_type", e.g. "320x240.ogv - Ogg Video"? Is there a sanctioned way to get the pretty mime type?

I'm not convinced the average web user probably cares about video dimensions, so I don't think we should report that like nsImageDocument does.

I doubt the user would have a long-term interest in the duration. Once you've started playing it's no longer useful.

Time remaining is more relevant to video. We could display that in the title, and update it once per second using a timer, but we'd still need to localize the time. "Blagh".

Comment 11

[Justin Dolske [:Dolske]]

UX folks may have some input here.

I'd tend to think something simple would be fine. Say, "Video - filename.ogg". Duration, size, type are probably not very useful to most users.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

"filename.ogg (Video)" would match our titles for images 

>Duration, size, type are probably not very useful to most users.

agreed, also we should try to avoid displaying MIME types, since they aren't really human readable.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - video doc title is "$filename ($description)"

This makes the video doc title "$filename ($description)". e.g:

"320x240.ogv (Ogg Video)"
"audio.ogg (Ogg Audio)"
"more_audio.oga (Ogg Audio)"
"sound.wav (Wave Sound)"

I also added Ogg video/audio and WAV as default supported mime types in nsExternalHelperAppService. I had to do this so that the nsChannel::GetContentType() worked correctly.

I also renamed the seek test Ogg videos to have .ogv extension.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  { AUDIO_OGG, "ogg", "Ogg Audio", 0, 0 },

IMHO This should just be "Ogg Stream" or something and set the VIDEO_OGG type ... a lot (most?) Ogg videos are served as the "ogg" type. We really don't want to display "Audio" or (eventually) use an <audio> element for them.

Is it a problem that these MIME type names aren't localizable? Or are they localizable? I guess it's the same deal as for images, whatever that is.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>"Ogg Stream"

I'm concerned that when localized to human this becomes "incomprehensible incomprehensible." 

>We really don't want
>to display "Audio" or (eventually) use an <audio> element for them.

Often the path to understandable UI is technical inaccuracy.  I understand the desire to get Web sites to use the correct mime types and elements, but if an Ogg stream only contains audio, from the user's perspective it is just "Audio."

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I'm fine with just displaying "Audio" or "Video" in the title. The only reason to include some information about the format in the title is that we already do that for images ("JPEG Image", "GIF Image", etc). So, we'd just have

"320x240.ogv (Video)"
"audio.ogg (Video)"
"more_audio.oga (Audio)"
"sound.wav (Audio)"

Note that many .oggs contain only audio, but we'd display "(Video)" since we're not sure until we get quite deep into decoding the stream whether there's a video track. Maybe we could fix that in a followup bug, although it's tricky since whether there's an audio track can, in principle, change during playback.

Anyway, is this what you want?

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

We can also include the format if we want to launch "Ogg" into mainstream vernacular (like MP3, or the image foramts), so:

kittens.ogv (Ogg Video)

Primarily I just want to avoid the word "stream" since it is more likely to semantically map to flowing water, instead of flowing data, and in general is more arcane than "Video" or "Audio."

>Note that many .oggs contain only audio, but we'd display "(Video)" since we're
>not sure until we get quite deep into decoding the stream whether there's a
>video track. Maybe we could fix that in a followup bug

Yeah, that sounds fine.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - v2 with *.ogg as "Video Ogg"

This is pretty much v2 unbitrotten, but maps *.ogg files to "Ogg Video" rather than "Ogg Audio". As Alex says, it's technically incorrect and doesn't match specs, but it reflects how the .ogg extension is used and perceived.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we should add to defaultMimeEntries. We want that to remain overridable by the system. Boris specifically requested this when I landed the nsVideoDocument work originally.

+  { VIDEO_OGG, "ogg", "Ogg Audio", 0, 0 },

Didn't you want this to be "Ogg Video"?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3.1

(In reply to comment #19)
> +  { VIDEO_OGG, "ogg", "Ogg Audio", 0, 0 },
> 
> Didn't you want this to be "Ogg Video"?

Totally.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't want to add to defaultMimeEntries. I guess someone can fix that when they check this in.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 353619 [details] [diff] [review]
Patch v3.1

This is OK except for the defaultMimeEntries part.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in as b873bd70913f but backed out due to test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229588582.1229592245.10136.gz#err5

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 353619 [details] [diff] [review]
Patch v3.1

Sorry for commenting late. Can we break the "prevailing local style" and put the LOCALIZATION NOTES to the actual entries? That'd be better. I adjusted the whole thing to be less ambiguous, too.

I wouldn't mind moving the existing comments to their entities, too, in particular the mediatitle ones.

>diff --git a/dom/locales/en-US/chrome/layout/MediaDocument.properties b/dom/locales/en-US/chrome/layout/MediaDocument.properties
>--- a/dom/locales/en-US/chrome/layout/MediaDocument.properties
>+++ b/dom/locales/en-US/chrome/layout/MediaDocument.properties
>@@ -41,12 +41,23 @@
> #LOCALIZATION NOTE (ImageTitleWithoutDimensions): first %S is filename, second %S is type
> #LOCALIZATION NOTE (ImageTitleWithDimensions): first %S is type, second %S is width and third %S is height
> #LOCALIZATION NOTE (ImageTitleWithNeitherDimensionsNorFile): first %S is type
> #LOCALIZATION NOTE (MediaTitleWithFile): first %S is filename, second %S is type
> #LOCALIZATION NOTE (MediaTitleWithNoInfo): first %S is type
>+
> ImageTitleWithDimensionsAndFile=%S (%S Image, %Sx%S pixels)
> ImageTitleWithoutDimensions=%S (%S Image)
> ImageTitleWithDimensions=(%S Image, %Sx%S pixels)
> ImageTitleWithNeitherDimensionsNorFile=(%S Image)
>+
>+#LOCALIZATION NOTE (VideoFileNameWithTypeAndDimensions): %1$S is filename, %2$S is type, %3$S is width, %4$S is height.
>+VideoFileNameWithTypeAndDimensions=%1$S (%2$S, %3$Sx%4$S pixels)
>+#LOCALIZATION NOTE (VideoFileNameWithType): %1$S is filename, %2$S is type
>+VideoFileNameWithType=%1$S (%1$S)
>+#LOCALIZATION NOTE (VideoFileNameWithDimensions): %1$S is filename, $2$S is width, %3$S is height
>+VideoFileNameWithDimensions=%1$S (%2$Sx%3$S pixels)
>+#LOCALIZATION NOTE (VideoFileNameWithNeitherDimensionsNorFile): %S is filename.
>+VideoFileNameWithNeitherDimensionsNorFile=%S
>+
> MediaTitleWithFile=%S (%S Object)
> MediaTitleWithNoInfo=(%S Object)
>

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #22)
> (From update of attachment 353619 [details] [diff] [review])
> This is OK except for the defaultMimeEntries part.

Without defaultMimeEntries, nsExternalHelperAppService::GetFromTypeAndExtension() returns the description as "()". That's why I put it there. Hence the test failure...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but we can't do that, like I said. So we have to dig a little deeper.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4 - with application/ogg in extraMimeEntries

As v3.1, but without additions to defaultMimeEntries, and with application/ogg added to extraMimeEntries. Tests now pass without needing defaultMimeEntries changes. Also moved the localization notes to beside the things they're explaining.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Roc, blocking 1.9.1?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes

Comment 30

[cajbir (:cajbir)]

Applied to mozilla-cetrna, built and tested on Linux before pusing and got a mochitest failure in test_audioDocumentTitle.html:

not ok - Audio doc title incorrect got "r11025_s16_c1.wav (Waveform Audio)", expected "r11025_s16_c1.wav (Wave Sound)"

Comment 31

[Axel Hecht [:Pike]]

This patch missed string freeze for 1.9.1.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What does that mean exactly? We can't land it in 1.9.1 anymore?

Comment 33

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #17)

A bit late to the party, but wanted to summarize what's been concluded here and then agree with it.  Alex is right that "ogg stream" is incomprehensible, and that "video" and "audio" are absolutely needed.  Like was said, the most consistent and understandable combination seems to be giving what type of format the video/audio is in and then whether it is audio video.  This is equivalent to what we do for images, which is (#IMAGETYPE image).

"320x240.ogv (Ogg Video)"
"audio.ogg (Ogg Video)"
"more_audio.oga (Ogg Audio)"
"sound.wav (Wave Audio)"

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably the right thing to do would be to map the MIME types to just "Ogg" and "Wave", and initially just display
  foo.ogg (Ogg)
and then when metadataloaded fires, check the videoWidth/videoHeight. If both are zero we assume there's no video track and treat it as "Audio", otherwise "Video", changing the title to get
  foo.ogg (Ogg Audio)
or
  foo.ogg (Ogg Video)

However, I'm not sure what we can do here now that we've missed the string freeze :-(. Axel, please tell

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW "metadataloaded" fires almost immediately in most cases so you wouldn't be stuck with just "Ogg" for long.

Alternatively we could make the initial title just "foo.ogg" and add "(Ogg Audio)"/"(Ogg Video)" once we know.

Comment 36

[Jeff Walden [:Waldo]]

Could also just use "<FOO> Media" at the expense of specificity while still being accurate in either case...

Comment 37

[Axel Hecht [:Pike]]

To me, this isn't blocking fx3.1. I doubt that we'll see a lot of video streams out there not embedded in a <video> tag, and for those cases that do, this is not making them unusable either. The awesomebar lists the video files under their filename, too, from my tests, even if the page title is confusing when surfing there.

There's not problem in taking this fix just on central either, AFAICT.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The fact that the title of the previous page stays the same in the new page is a problem that we should fix for 3.1, though.

I'll cut down this patch into a patch that just replaces the title with the file name (so no localization needed). Then I'll spin off the rest of the problem into a separate bg.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: cut-down fix v1

OK, this takes the really simple approach of just setting the filename as the title.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 357083 [details] [diff] [review]
cut-down fix v1

Since we plan to follow up with something nicer, looks good.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7e0d34e958e6 and then http://hg.mozilla.org/mozilla-central/rev/361a930d2c4b to fix bustage due to missing test files.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 474096 filed to follow up with a better title.

Comment 43

[Axel Hecht [:Pike]]

Thanks, good workaround for 1.9.1. Removing all the l10n highlights from this bug, as triage of this one doesn't have any impact on l10n anymore.

Comment 44

[Daniel Holbert [:dholbert]]

Pushed both changesets from comment 41 (patch & missing tests) to 1.9.1 as:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7cdf97d8e0f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/580c4289d4e6

Comment 45

[Marcia Knous [:marcia]]

verified fixed on the 1.9.1 branch using : Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090203 Shiretoko/3.1b3pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090203 Shiretoko/3.1b3pre. Adding keyword.

Comment 46

[Boris Zbarsky [:bzbarsky]]

The test was disabled, so re-flagging for in-testsuite.