428872 - |AddOverrideStyleSheet()| does not report missing file

Status: NEW

(SeaMonkey :: Composer, defect)

Description

[Serge Gautherie (:sgautherie)]

See bug 428852 comment 5 and 6.

While I was testing, I deleted the css files.

I got no warning/error in the Error Console about the missing files.
While this might be the desired behavior, I thought it odd.

|enableStyleSheet()| ignores missing files:
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/libeditor/html/nsHTMLEditor.cpp&rev=1.567&mark=3689#3681>
which seems reasonable.

But |AddOverrideStyleSheet()| has an unchecked/unused |rv| assignment,
and, at least, this should either be checked/used or removed:
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/libeditor/html/nsHTMLEditor.cpp&rev=1.567&mark=3609#3588>
To get a warning/error should help the developers/users...

The other function behavior should be verified too:
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/idl/nsIEditorStyleSheets.idl&rev=1.7#48>
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/libeditor/html/nsHTMLEditor.h&rev=1.239&mark=185-193#182>

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022702 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

I tried
{{
          var sgrv = editor.addOverrideStyleSheet(...);
          alert(sgrv);
}}
but I always get |undefined|, and Venkman shows |sgrv = void| :-(

Why don't I get the return value from the C++ function ?

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Why don't I get the return value from the C++ function ?

Could it be because the .idl says |void|, whereas the .h/.cpp says |NS_IMETHOD / NS_IMETHODIMP| ?
(See bug 124265.)

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsHTMLEditor.cpp> Adds |NS_ENSURE_SUCCESS(rv, rv)|

This seems to be the only "created but unchecked" return value in these functions.

(Untested, as I have no build environment.)

Comment 4

[[e-mail]]

Surely if the load fails then the sheet will be null anyway? How did you delete the stylesheet anyway? (I'm hoping you removed it from the .jar file too.)

FYI, returning an error rv in C++ translates to throwing an [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) (etc.)] in JS.

Comment 5

[Kathleen :Brade]

Comment on attachment 315576 [details] [diff] [review]
(Av1) <nsHTMLEditor.cpp> Adds |NS_ENSURE_SUCCESS(rv, rv)|

r- brade
I don't think we should throw an error if loading a stylesheet generates an error (and there is a stylesheet?). I don't really understand how we got into this situation so I can't approve this fix.  I'd like to see a test case for this to better understand what behavior is desired.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Surely if the load fails then the sheet will be null anyway? How did you delete

The only place which checks both is:
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/chrome/src/nsChromeRegistry.cpp&rev=1.368&mark=947,994-996#973>

> the stylesheet anyway? (I'm hoping you removed it from the .jar file too.)

For comment 0, I did (only) delete the file from the jar.
For comment 1, I changed (only) the file name in <editor.js>.
(For comment 1, the return value was "null", too, with a present/working sheet.)

> FYI, returning an error rv in C++ translates to throwing an [Exception...
> "Component returned failure code: 0x80004002 (NS_NOINTERFACE) (etc.)] in JS.

I would not want to get an exception:
I'm thinking of getting a warning, like "sheet not found".

But I don't know if/how that could be achieved.

(In reply to comment #5)
> (From update of attachment 315576 [details] [diff] [review])
> I don't think we should throw an error if loading a stylesheet generates an
> error (and there is a stylesheet?). I don't really understand how we got into

If we encounter an error while we do have a sheet,
that would be worth looking into, = being warned somehow.

> this situation so I can't approve this fix.  I'd like to see a test case for
> this to better understand what behavior is desired.

I don't have a testcase to run,
but steps would be like trying to load a sheet with a wrong name from <editor.js>.

I had a look at
<http://mxr.mozilla.org/seamonkey/search?string=LoadSheetSync&case=on&tree=seamonkey>
from which 6 (only) check the return value, and 4 of them propagate it.

I don't know more that that.