Closed Bug 198598 Opened 21 years ago Closed 21 years ago

standalone image/plugin: title/save/save as handling (inc. non-ASCII cases)

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: jshin1987, Assigned: jshin1987)

References

()

Details

(Keywords: intl)

Attachments

(1 file, 9 obsolete files)

This is a spin-off from bug 158006.

When an image with non-ASCII chars. in its name is opened in a standalone
browserpane (a tab or a window) with 'view image' or 'clicking
on the link to the image',  the 'document' charset of
the standalone image is set to ISO-8859-1. As a result, Mozilla
prefills filepicker with a mangled filename. How mangled? 
For instance, Mozilla interprets '0xB0 0xA1.jpg'(ignore
space and interpret 0xB0 and 0xA1 as octets corresponding
to hex numbers. '가.jpg' : set your encoding to UTF-8
and Korean string will be rendered correctly) in EUC-KR as if 
it's ISO-8859-1 and comes up with a suggested filename of 
'U+00B0 U+00A1  .jpg'(again ignore spaces: °¡.jpg )
) instead of 'U+AC00 .jpg' (0xB0 0xA1 in
EUC-KR is U+AC00). 

How to reproduce: 

go to the URL listed in the URL field. Right click on the third
blueball in the page and select 'view image'. A small blue ball
will come up in a window of its own. Now, right-click on it
and select 'save image as'.  Filepicker will show up
with 'ÆĶõ°ø.jpg'  instead of '파란공.jpg'
(set charset to UTF-8 to see all these characters.)

The fix can be pretty simple. Set charset of 
a _standalone_ non-textual document to the charset
of the referrer (from which non-textual 'document'
originates. 

BTW, if you have Win2k/XP, MacOS X or Unix-like OS
with UTF-8 locale, you'll see that the window title
bar has the correct Korean filename ('파란공.jpg').
So what I suggested above is hard to implement,
there should be another way to retrieve that
information...
OOops. I'm sorry
I forgot to set the encoding to UTF-8
when reporting this (Bugzilla should 
use UTF-8 by default !) 
mangled name (euc-kr interpreted as iso-8859-1)
is 'ÆĶõ°ø.jpg' and Korean name
is '파란공.jpg'.
Summary: mangled filename suggested when saving a standalone image with non-ASCII name → mangled filename suggested when saving a standalone image with non-ASCII name
Attached patch a tentative patch (obsolete) — Splinter Review
This solves the problem when an image is loaded
into its own browser pane by clicking on the
link to the image. In that case, originCharset
is set to the doc. charset of the referrer
so that by retrieving that value works fine.

However, when an image is loaded into its own browser 
pane with 'View Image', originCharset is set to
UTF-8 and setting the doccharset of the standalone
synthetic document to originCharset doesn't
help at all. 

It took me much longer than necessary to figure this
out because I didn't realize that change in content/html/document
gets only effective after recompiling in layout/...
Adding a few more people to CC.

The case that works with attachment 118129 [details] [diff] [review]
is clicking on links like below:

<a href="non-ascii_imagename.jpg">link name</a>
(in http://jshin.net/download.html,
 the link named '모질라.jpg'. view this with
charset set toUTF-8) 

The case that doesn't work with it is
right-clicking on links as below and selecting
'view image'. 

<img src="non-ascii_image.jpg">
(in the url above, a 'blueball' next to
'파란공') 

BTW, I put a call to 'UpdateDocumentCharacterSet'
in |CreateSynthetic...|, but it's just tentative
(as a proof of concept) so that there may be a 
better place(way) to call that. 

Anyway, to solve this problem for both cases,
I'll try another approach (as used in
my patch to bug 162765)




I tried using aChannel in |StartDocumentLoad| hoping that
uri obtained from aChannel may have charset of the referring
document as |originCharset|, but it's not the case. 
When I right-click on an image and select 'view image', 
|originCharset| of the image loaded in its own window
is set to UTF-8 while |mCharacterSet| of 'stand-alone
image document' is ISO-8859-1. I hoped to correct it
with |originCharset|, but it's UTF-8 so that it's of no use. 

Anyway, it's the least important of three cases :

  - Click on an image link and load the linked image
    in a stand-alone browser pane : 
    'save as image' works fine with my patch

  - Right click on an image and select 'save as image' : ditto

  - Right click on an image and select 'view image' and
    then in the stand-alone image pane, right click on the
    image and select 'save as image' : does NOT work.

More important two cases work fine. As for the third case, 
I need to figure out  where |originCharset|
of channel/documentURL is set to UTF-8
(see bug 158006 comment #12) and do what's
necessary there. Or, if that's not desirable,
I have to find out the earliest place
where I can set |mDocumentCharacterSet|
to that of the referring document instead of ISO-8859-1
(which is the default value set in the ctor of |nsDocument|).

Anyway, the third case is least important and rarely used,
but the first two cases are frequently used and are often
a source of complaints from those who want to convert to Mozilla/Netscape
from MS IE. So, let's solve it first. 

The question is where to call a new function 
(|UpdateDocumentCharacterSet| or equivalent) added in my patch.
It can be called in StartDocumentLoad, OnStartRequest,
OnStopRequest, CreateSyntheticDocument or  somehwer else.

Darin and Christian, any opinion? This patch is similar
to the patch for bug 163998 and maybe we can save some lines of code
by putting my patch in |OnStopRequest| where |GetOriginCharset|
is called anyway for image title set-up.




Target Milestone: --- → mozilla1.4alpha
Attached patch a new patch (obsolete) — Splinter Review
A new patch. SetDocumentCharacterSet
invocation is now in OnStopRequest
to share originCharset obtained via
mDocumentURL with UpdateTitle.
Attachment #118129 - Attachment is obsolete: true
Comment on attachment 118132 [details] [diff] [review]
a new patch

So... why not just call UpdateTitle() as it happens now and just have
UpdateTitle() get the charset and set it if necessary?	Also, do we not care to
set the charset if the URI is not a URL?

Finally, should this code live in nsImageDocument, or is any of it relevant for
plug-ins?  Does it need to move up into nsMediaDocument?
Attached patch a new patch (obsolete) — Splinter Review
handle URI case.
Attachment #118132 - Attachment is obsolete: true
Per Boris' comment, I renamed UpdateTitle to UpdateTitleAndCharset and
put two tasks together to avoid repeating if-if twice in caller and callee.

> Finally, should this code live in nsImageDocument, 

  What's the role of nsMediaDocument? Is it responsible for (to-me) annoying
empty window that comes up when I click on the link to a file of a 
(non-textual, non-visual) mimetype for which I haven't yet specified helper appl. 
(or which is to be handled by Mozilla)?   For instance,

<a href="xxxx.yyy">Link</a>

where c-t for 'xxxx.yyy' is to be handled by Mozilla.   

> or is any of it relevant for plug-ins? 

  Could be, but have to check.   Let's say, there's a link to a PDF file
(or a flash animation)

<a href="non-asciiname.pdf">non-ascii link name</a>

  When it's clicked, does Mozilla do something similar to
what's done in nsImageDocument.cpp, (|CreateSynthetic...|:
rolling out <object> or <applet>....)? 
If that's the case, we have to take care of that case
as well. Another reason we have to set charset is
that plug-ins may behave differently on charset(I don't
know if there's any, but there might be...) 
The best way is to set charset to that of
the referring document as up in the hierarchy as possible.
|nsDocument| ctor inits it to ISO-8859-1... 



> Does it need to move up into  nsMediaDocument?

  If both nsImageDocument and nsMediaDocument need this,
doesn't it have to move up to nsHTMLDocument (or even higher
if plug-ins also need this? or is nsMediaDocument  in charge
of plug-ins as well?)? 
Somehow my comment added yesterday is not here(perhaps I forgot
to submit..) Anyway, I'm sorry
I didn't realize that nsImageDocument now inherits nsMediaDocument
(my tree was not up to date.) So does a new nsPluginDocument.
Then, yes, it makes sense to move UpdateTitleAndCharset
(or at least UpdateCharset part if we just want to use
the default title for plugin documents) up to
nsMediaDocument because both image and plugin document
need it. For instance, <a href="non-asciiname.pdf">non-ascii link name</a>
is clicked on and opened, 'File|Save Page as' brings in a filepicker
filled with a garbled suggested filename because DocumentCharset
is init'd to ISO-8859-1 in ctro of nsDocument.
 
I moved UpdateTitleAndCharset to nsMediaDocument and 
made some changes as necessary in ns(Image|Media|Plugin)Document.
This works well with PDF and Acroread plugin and should work
with other plugins and mimetype pairs.
Attachment #118270 - Attachment is obsolete: true
One problem not yet taken care of is whether or not to make a separate string bundle
for non-image plugin document. In the current patch, I recycled 
communicator/locale/layout/ImageDocument.properties for both
nsPluginDocument. The result is that the titlebar
for non-image document (e.g. PDF) reads 

  documentname.pdf (application/pdf: Image) Mozilla (build .....)

It'd be fine if there were no 'Image'. 

I'm adding Tao to CC to seek his opinion because this also has
to do with L10N. I'm not sure whether the name of properties
file has to match the class name in which stringbundle is used.
As a bonus, with this patch, tabs with PDF (and other plugin doc) have also
more informative titles displayed (e.g. 'doc1.pdf (application/pdf.....').
Without 'Image', I'd be very much tempted to check this in after r/sr... 
Summary: mangled filename suggested when saving a standalone image with non-ASCII name → standalone image/plugin: title/save/save as handling (inc. non-ASCII cases)
>I'm not sure whether the name of properties
>file has to match the class name

No, it can be anything.
>>I'm not sure whether the name of properties file has to match the class name

>No, it can be anything.

 Thanks for the answer. I was not clear, but I was asking if there's any
convention/guideline  when it comes to name a properties file.
Anyway, which would you like more, making a new *separate* properties file
for plugin document or just recycling the existing properties file
for plugin docs (with 'Image' problem)? A third way might be 
to add a couple of new strings for plugin doc to the existing file
and use them for plugin docs. I'm inclined to the third one
with a possible name change in the file name. 
I added two strings for media documents and changed the
way stringbundle is accessed. I like this pretty much.
Attachment #118423 - Attachment is obsolete: true
Comment on attachment 118523 [details] [diff] [review]
a new patch with two new localizable strings added

this is the extension of bug 163998 so that I'm asking darin for sr while
asking r for Christian who's been monitoring this.

BTW, I took the third approach to 'stringbundle'.
Attachment #118523 - Flags: superreview?(darin)
Attachment #118523 - Flags: review?(cbiesinger)
Attached patch a new patch (obsolete) — Splinter Review
nsMedia/Image/Plugin Document.cpp have changed since my last patch.
I updated my patch to refleect the change.
Attachment #118523 - Attachment is obsolete: true
Attachment #118925 - Flags: superreview?(darin)
Attachment #118925 - Flags: review?(cbiesinger)
Attachment #118523 - Flags: superreview?(darin)
Attachment #118523 - Flags: review?(cbiesinger)
Comment on attachment 118925 [details] [diff] [review]
a new patch

+const PRUnichar* nsMediaDocument::sFormatNames[4] = 

firstly, please make it static, and secondly - why did you make this array
local in nsImageDocument, but global in nsMediaDocument? (also make it static
in nsImageDOcument please)

+	 const PRUnichar *formatStrings[4]  = {fileStr.get(), 
+	   PromiseFlatString(aTypeStr).get(), widthStr.get(), heightStr.get()};
+	 mStringBundle->FormatStringFromName(aFormatNames[3], formatStrings, 4, 

I believe that the temporary from PromiseFlatString will be destroyed by the
time you call FormatStringFromName, so the second array element points to
invalid data...

same in the else part

+    if (valUni) {
please use:
 if (!valUni.IsEmpty())
Attachment #118925 - Flags: review?(cbiesinger) → review-
Thank you for review.

> +const PRUnichar* nsMediaDocument::sFormatNames[4] = 

> firstly, please make it static, 

   I guess I don't have to(and VC++ wouldn't allow it) because it's the
definition of a static member array declared in nsMediaDocument class
declaration as

 static const PRUnichar*       sFormatNames[4];

> and secondly - why did you make this array
> local in nsImageDocument, but global in nsMediaDocument? 

  Because I want to use sFormatNames as the default value for the 2nd 
argument of nsMediaDocument::UpdateTitleAndCharset. 

+  nsresult UpdateTitleAndCharset(const nsAString&  aTypeStr, 
+                                 const PRUnichar** aFormatNames = sFormatNames,
+                                 nscoord           aWidth = 0, 
+                                 nscoord           aHeight = 0 );
+
+  nsCOMPtr<nsIStringBundle>     mStringBundle;
+  static const PRUnichar*       sFormatNames[4];  

In nsPluginDocument, it's 
invoked with only the first argument (aTypeStr) while in nsImageDocument,
the second argument(sFormatNames) is overriden by  the locally defined value
(|formatNames|). 

> (also make it static in nsImageDOcument please)
> I believe that the temporary from PromiseFlatString will be destroyed by the
> time you call FormatStringFromName, 

> please use:
>  if (!valUni.IsEmpty())

Thanks, these are taken care of. 
Can you review this, instead? Thanks
Attachment #118925 - Attachment is obsolete: true
Comment on attachment 119150 [details] [diff] [review]
a new patch with Christian's concern addressed

>definition of a static member array

sigh. I didn't realize that, I'm sorry.

r=me
Attachment #119150 - Flags: review+
Attachment #118925 - Flags: superreview?(darin)
Comment on attachment 119150 [details] [diff] [review]
a new patch with Christian's concern addressed

Thank you, Christian.
Darin, can I get sr?
Attachment #119150 - Flags: superreview?(darin)
Comment on attachment 119150 [details] [diff] [review]
a new patch with Christian's concern addressed

darin seems busy. asking
bz for sr because he's kept his eyes on this.
Attachment #119150 - Flags: superreview?(darin) → superreview?(bzbarsky)
It looks like I'm chasing a moving target. nsImage/MediaDocument.cpp 
have changed since my last patch. Christopher, can I get your r
carried over to this patch? Boris, can I get sr?
Attachment #119150 - Attachment is obsolete: true
Comment on attachment 120146 [details] [diff] [review]
same patch modified to match recent changes in the tree

>Index: layout/html/forms/src/ImageDocument.properties
>+#LOCALIZATION NOTE (MediaTitleWithFile): first %s is filename, second %S is type

Make both %S uppercase, please.

Also, we should really find a better place for this file to live, eventually...
like content/html/document

Maybe "Object" instead of "Media"?  Not sure what would be best here, but
"(application/x-shockwave-flash Media)" looks odd to me.

>Index: content/html/document/src/nsImageDocument.cpp
>-  nsresult UpdateTitle();
>+  nsresult UpdateTitleAndCharset();
> 
>   nsCOMPtr<nsIStringBundle>     mStringBundle;
>   nsCOMPtr<nsIDOMElement>       mImageElement;

Don't we want to remove mStringBundle here?

>+  if (!mStringBundle) 
>+    return NS_OK;

So we don't need to update the charset if there is no stringbundle?  Don't we
need the charset for save as anyway?

>+  static const PRUnichar* formatNames[4] = 
>+  {
>+    NS_LITERAL_STRING("ImageTitleWithNeitherDimensionsNorFile").get(),
>+    NS_LITERAL_STRING("ImageTitleWithoutDimensions").get(),
>+    NS_LITERAL_STRING("ImageTitleWithDimesions").get(),
>+    NS_LITERAL_STRING("ImageTitleWithDimensionsAndFile").get()
>+  };

This will crash on Linux (unless you build with GCC 3.2 and have --fshort-wchar
enabled) and other platforms where NS_LITERAL_STRING("foo") doesn't just expand
to L"foo".  Please come up with an alternate way to do it (eg make this a const
char const * and do runtime conversion as needed in nsMediaDocument.

>Index: content/html/document/src/nsMediaDocument.cpp
>+const PRUnichar* nsMediaDocument::sFormatNames[4] = 
>+{
>+  NS_LITERAL_STRING("MediaTitleWithoutFile").get(),
>+  NS_LITERAL_STRING("MediaTitleWithFile").get(),
>+  NS_LITERAL_STRING("").get(),
>+  NS_LITERAL_STRING("").get(),
>+};

Same here.

>+nsresult 
>+nsMediaDocument::UpdateTitleAndCharset(const nsAString& aTypeStr, 

Why?  Make it an nsACString; then you can drop a lot of conversions in the
callers....


>+          if (NS_FAILED(rv))
>+            fileStr.Assign(NS_ConvertUTF8toUCS2(fileName));
>+        }
>+      } else {
>+        fileStr.Assign(NS_ConvertUTF8toUCS2(fileName));
>+      }

You could combine those into a single |if (fileStr.IsEmpty())| check, no?

>+    nsXPIDLString valUni;

This needs a better name.

>+    // if this is an imageDoc and we got a valid size (sometimes we do not).

Just say "if we got a valid size".  The fact that nsImageDocument even exists
should be a matter of sublime indifference to nsMediaDocument

>+        const PRUnichar *formatStrings[4]  = {fileStr.get(), 
>+          PromiseFlatString(aTypeStr).get(), widthStr.get(), heightStr.get()};

What if PromiseFlatString has to construct an object?  This will dereference
garbage memory.. You want to hold on to the flatString for the duration of the
FormatStringFromName call (this will continue to apply when you switch to
nsACString; then you will want: |NS_ConvertUTF8toUCS2 unicodeType(aTypeStr);
const PRUnichar * ....|

This applies some places below too.

>+        mStringBundle->FormatStringFromName(aFormatNames[3], formatStrings, 4, 

Could we have a nice enum (scoped to nsMediaDocument only) for the indices into
the name array instead of using the raw numbers?

>+    if (valUni) {
>+      // set it on the document
>+      SetTitle(valUni);

We want to set the title unconditionally, even if everything fails -- otherwise
we will be showing the title from the previous page, no?  In any case, test
|!valUni.IsEmpty()| instead of using the conversion operator....

>Index: content/html/document/src/nsMediaDocument.h
>+#include "nsCoord.h"

Why?  More below.

>+  // nsIHTMLDocument
>+  nsresult Init();

Shouldn't that be NS_IMETHOD then?

>-  virtual nsresult CreateSyntheticDocument();
>+  nsresult CreateSyntheticDocument();

This needs to stay virtual, since subclasses may need to override (and
nsImageDocument does!)

>+  nsresult UpdateTitleAndCharset(const nsAString&  aTypeStr, 
>+                                 const PRUnichar** aFormatNames = sFormatNames,
>+                                 nscoord           aWidth = 0, 
>+                                 nscoord           aHeight = 0 );

This needs some documentation.	Like the expected length of aFormatNames.  And
what the width and height are of.

Make aTypeStr an nsACString.

Don't use nscoord.  Use PRInt32 or something.  Those lengths are lenghts in
pixels, not nscoords.....

My apologies for taking so long; please fix the above and I promise to be more
prompt.  ;)
Attachment #120146 - Flags: superreview-
.
Assignee: law → jshin
Attachment #119150 - Flags: superreview?(bzbarsky) → superreview-
Attached patch a new patch per Boris' comment (obsolete) — Splinter Review
Thanks a lot for your thorough review. A few points had been addressed in
my second last patch per Christian's review comment and on my own, but somehow 

my last patch got mix-ups from an earlier patch. However, there were several
other points I hadn't thought about before all of which I took care of in
this patch. Can you take a look again? TIA.
BTW, I moved ImageDocument.properites to content/html/document/src
and renamed it as MediaDocument.properties.
Attachment #120146 - Attachment is obsolete: true
Comment on attachment 120181 [details] [diff] [review]
a new patch per Boris' comment

Christian is away. Asking
Simon and Boris for r/sr
Attachment #120181 - Flags: superreview?(bzbarsky)
Attachment #120181 - Flags: review?(smontagu)
Comment on attachment 120181 [details] [diff] [review]
a new patch per Boris' comment

>Index: content/html/document/src/MediaDocument.properties

Don't you need to add this file to the build system? 

The file itself looks fine.

>Index: content/html/document/src/nsMediaDocument.h
>+  // nsIHTMLDocument
>+  NS_IMETHOD Init();

So I just went and checked, and this is in fact not an nsIHTMLDocument method,
so the answer to my question is "No, it should not be NS_IMETHOD" and that
misleading comment should be removed.

>+  // |aFormatNames[]| need

needs.

> to have four elements, format name with filename
>+  // and dimension, with dimension but w/o filename, with filename but w/o

"elements: a format name with filename and dimension, a format name with
dimension but w/o filename, a format name with filename but w/o dimension, and
a format name with neither of them".

Don't you want to say what order the names should come in?  The order in which
you list them here does not match the order they actually need to be in in the
array....

>+  // dimension, and with neither of them. See |nsImageDocument| for example.

Just put an example in the comment; no reason to send people off to some rather
.cpp that may even go away while this .h stays to look for some code in that
cpp.

Shouldn't UpdateTitleAndCharset be virtual?

>Index: content/html/document/src/nsMediaDocument.cpp
> NS_IMETHODIMP
>+nsMediaDocument::Init()

so this would become just an nsresult return.

>+    if (!fileName.IsEmpty() && NS_FAILED(rv))
>+      fileStr.Assign(NS_ConvertUTF8toUCS2(fileName));

This seems wrong.  What if NS_SUCCEEDED(rv) but originCharset was empty (a URI
which never got an origin charset set)?  I'd just do

if (fileStr.IsEmpty()) {
  fileStr.Assign(NS_ConvertUTF8toUCS2(fileName));
}

here.

>+  nsAutoString typeStr = NS_ConvertASCIItoUCS2(aTypeStr);

NS_ConvertASCIItoUCS2 typeStr(aTypeStr);

then you only copy once, not twice.

>+        nsDependentCString fmtName(aFormatNames[eWithDimAndFile]);
>+        mStringBundle->FormatStringFromName(NS_ConvertASCIItoUCS2(fmtName).get(),
>+                                            formatStrings, 4, getter_Copies(title));

NS_ConvertASCIItoUCS2 fmtName(aFormatNames[eWithDimAndFile]);

then just use fmtName.get()

Same in the other branches.

>+  else { // string bundle not available. use hard-coded string.

Um.  Why not set the title to an empty string in this case? I think that would
be preferable to doing non-localized stuff....

The rest looks fine.
Attachment #120181 - Flags: superreview?(bzbarsky)
Attachment #120181 - Flags: superreview-
Attachment #120181 - Flags: review?(smontagu)
Thank you for review. I'll fix what you pointed out. 

> Shouldn't UpdateTitleAndCharset be virtual?

Did you mean |UpdateTitleAndCharset(.....)| in nsMediaDocument? 
In that case, UpdateTitleAndCharset(void) in nsImageDocument has 
to be renamed to avoid hiding  UpdateTitleAndCharset(.....) in 
nsMediaDocument, doesn't it? Perhaps, what can  be done is to add virtual
|UpdateTitleAndCharset(void)| to nsMediaDocument the default implementation
of which is empty. Optionally, we can rename |UpdateTitleAndCharset(.....)|
(that does most of chores) in nsMediaDocument. BTW, the return value
of UpdateTitleAndCharset is not checked anywhere so that I'm tempted
to change its return type void. What would you say? 

>>+  else { // string bundle not available. use hard-coded string.

>Um.  Why not set the title to an empty string in this case? I think that would
>be preferable to doing non-localized stuff....

I'm not sure although I can go either way. It seems to me that either leaving 
the title of the previous page (that is likely to carry some relevant info.)
alone(as in older patches) or doing non-localized stuff (as in the last patch,
well, except for 'pixels' and 'Image', it's just made of
MIME type and filename.) is more friendly than setting the title to an empty
string. 
Target Milestone: mozilla1.4alpha → mozilla1.4beta
> Did you mean |UpdateTitleAndCharset(.....)| in nsMediaDocument? 

Yes.  I just realized that those actually take totally different args...  I'm
not sure what I think about that, to be truthful...  It seems very confusing to
me to have two functions with identical names and different arguments in classes
that inherit from each other like that....

If no one checks the retval, by all means make it void.

Setting the title to an empty string will just make the titlebar say "Mozilla".
 It's a _lot_ more friendly than leaving the title of the previous page, since
the title, if any, should match the content.  I fail to see how the title of the
previous page can claim to do so.

In fact, I see nothing unfriendly about setting the title to an empty string...
How about this in nsMediaDocument.h? 

  // When implemented in a subclass, this should invoke
  // |nsMediaDocument::UpdateTitleAndCharset| with parameters filled out
  // as necessary and as appropriate for the subclass.
  virtual void UpdateTitleAndCharset() = 0;
  //.... comment on void UpdateTitleAndCharset(.......)
  void UpdateTitleAndCharset (......); 

This is still confusing and increases the size of vtbl, but is arguably a bit
cleaner
than before.

> I fail to see how the title of the previous page can claim to do so.
  
  It depends.  For instance, when the previous page is about topic A
with the title indicative of that and the link in the page
to a pdf file on topic A (or a related topic) is clicked on and
the pdf file is loaded,  the prev. page title has some relevance... 
On the other hand, it could be confusing in a sense..

> In fact, I see nothing unfriendly about setting the title to an empty string...

  It's less friendly than 'xyz.jpg (JPEG : Image w x h pixels)' or 
'abc.pdf (application/pdf)', isn't it? :-)  Well, I guess this is not
that important so that I'd not insist. 
> This is still confusing

Yep.  It's no less confusing than the existing patch...

Perhaps we should change the imagedocument function to "OnImageSizeAvailable" or
something?  As in, name it based on when it's called, not based on what it does...

addressing Boris' concerns except for making Update... virtual,
which we decided not to do over IRC.
Attachment #120181 - Attachment is obsolete: true
Attachment #120296 - Flags: superreview?(bzbarsky)
Comment on attachment 120296 [details] [diff] [review]
yet another patch 

>Index: content/html/document/src/jar.mn
>+        locale/en-US/communicator/content/MediaDocument.properties

Keep this in communicator/layout/ like it was before ("content" in a jar and
"content" the root dir have nothing to do with each other).

Don't forget to also change that in nsImageDocument.cpp

>Index: content/html/document/src/nsMediaDocument.h
>+  nsresult  Init();

This needs to be virtual, since subclasses need to be able to override it, no? 
Why not just look at the superclass (nsHTMLDocument) when declaring such things
and declare them the same way?

This patch is missing the CVS removal of the current ImageDocument.properties

No need for a new patch for these nits; just make sure to fix them when you
check in.
Attachment #120296 - Flags: superreview?(bzbarsky) → superreview+
It seems that this was checked in on Apr 12 17:40, can this be marked fixed?
Ooops. I'm sorry I forgot to change the status after checking in the patch.
Thank you all for your help. 
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Verified in the 2003-04-22-08 macho and win32 trunk builds.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: