Closed Bug 463830 Opened 11 years ago Closed 11 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: cpearce)

References

(Blocks 1 open bug, )

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 4 obsolete files)

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.
Chris can probably fix this. The code is nsVideoDocument, and nsImageDocument has code for setting the title.
Assignee: nobody → chris
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #348129 - Flags: review?(roc)
You can listen for the metadataloaded event and update the title when that fires.
We're in a string freeze, so you'll need to add late-l10n here or something. Not sure how that works.
Summary: When loading an ogg audio/video file in a browser, window the title should give something useful → When loading an ogg audio/video file in a browser, the window title should give something useful
I think it would also be good to put the duration in the title.
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 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?
Attachment #348129 - Flags: ui-review?(beltzner)
(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.
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.
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".
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.
Keywords: uiwanted
"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.
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.
Attachment #348129 - Attachment is obsolete: true
Attachment #353148 - Flags: review?(roc)
Attachment #348129 - Flags: ui-review?(beltzner)
Attachment #348129 - Flags: review?(roc)
+  { 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.
>"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."
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?
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.
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.
Attachment #353148 - Attachment is obsolete: true
Attachment #353575 - Flags: review?(roc)
Attachment #353148 - Flags: review?(roc)
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"?
Attached patch Patch v3.1 (obsolete) — Splinter Review
(In reply to comment #19)
> +  { VIDEO_OGG, "ogg", "Ogg Audio", 0, 0 },
> 
> Didn't you want this to be "Ogg Video"?

Totally.
Attachment #353575 - Attachment is obsolete: true
Attachment #353619 - Flags: superreview?(roc)
Attachment #353619 - Flags: review?(roc)
Attachment #353575 - Flags: review?(roc)
We don't want to add to defaultMimeEntries. I guess someone can fix that when they check this in.
Comment on attachment 353619 [details] [diff] [review]
Patch v3.1

This is OK except for the defaultMimeEntries part.
Attachment #353619 - Flags: superreview?(roc)
Attachment #353619 - Flags: superreview+
Attachment #353619 - Flags: review?(roc)
Attachment #353619 - Flags: review+
Checked in as b873bd70913f but backed out due to test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229588582.1229592245.10136.gz#err5
Whiteboard: [needs new patch]
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)
>
(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...
OK, but we can't do that, like I said. So we have to dig a little deeper.
Blocks: 470641
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.
Attachment #353619 - Attachment is obsolete: true
Attachment #354753 - Flags: superreview?(roc)
Attachment #354753 - Flags: review?(roc)
Attachment #354753 - Flags: superreview?(roc)
Attachment #354753 - Flags: superreview+
Attachment #354753 - Flags: review?(roc)
Attachment #354753 - Flags: review+
Flags: blocking1.9.1?
Whiteboard: [needs new patch]
Roc, blocking 1.9.1?
Yes
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Keywords: checkin-needed
Whiteboard: [needs landing]
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)"
Keywords: checkin-needed
This patch missed string freeze for 1.9.1.
Whiteboard: [needs landing] → [needs landing][missed string freeze]
What does that mean exactly? We can't land it in 1.9.1 anymore?
Whiteboard: [needs landing][missed string freeze] → [missed string freeze]
(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)"
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
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.
Could also just use "<FOO> Media" at the expense of specificity while still being accurate in either case...
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.
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.
Attached patch cut-down fix v1Splinter Review
OK, this takes the really simple approach of just setting the filename as the title.
Attachment #357083 - Flags: superreview?(bzbarsky)
Attachment #357083 - Flags: review?(bzbarsky)
Whiteboard: [missed string freeze] → [missed string freeze][needs review]
Comment on attachment 357083 [details] [diff] [review]
cut-down fix v1

Since we plan to follow up with something nicer, looks good.
Attachment #357083 - Flags: superreview?(bzbarsky)
Attachment #357083 - Flags: superreview+
Attachment #357083 - Flags: review?(bzbarsky)
Attachment #357083 - Flags: review+
Whiteboard: [missed string freeze][needs review] → [missed string freeze][needs landing]
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [missed string freeze][needs landing] → [missed string freeze][needs 191 landing]
Bug 474096 filed to follow up with a better title.
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.
Keywords: late-l10n
Whiteboard: [missed string freeze][needs 191 landing] → [needs 191 landing]
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.
The test was disabled, so re-flagging for in-testsuite.
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.