237103 - nsIDOMNSHTMLDocument::QueryCommandIndeterm is not implemented

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Alexandre Trémon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7b) Gecko/20040303
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7b) Gecko/20040303

nsIDOMNSHTMLDocument::QueryCommandIndeterm is not implemented in nsHTMLDocument

Reproducible: Always
Steps to Reproduce:

Comment 1

[Alexandre Trémon]

Attachment: QueryCommandIndeterm implementation in nsHTMLDocument

Comment 2

[Alexandre Trémon]

Comment on attachment 143581 [details] [diff] [review]
QueryCommandIndeterm implementation in nsHTMLDocument

I tested it with "foreColor", "bold", "fontname" and "italic".

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 143581 [details] [diff] [review]
QueryCommandIndeterm implementation in nsHTMLDocument

+  PRBool dummy, dummy2;
+  if (!ConvertToMidasInternalCommand(commandID, commandID,
+				      cmdToDispatch, paramToCheck, dummy,
dummy2))

Any reason for two dummy variables? Couldn't the same one be passed as both
arguments?

+  nsresult rv;
+  nsCOMPtr<nsICommandParams> cmdParams = do_CreateInstance(
+					    NS_COMMAND_PARAMS_CONTRACTID, &rv);
+  if (!cmdParams)
+    return NS_ERROR_OUT_OF_MEMORY;

Since you bother passing rv to do_CreateInstance(), check it, and not the
return value, for success, i.e.:

+  nsresult rv;
+  nsCOMPtr<nsICommandParams> cmdParams = do_CreateInstance(
+					    NS_COMMAND_PARAMS_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);

If that succeeds, you know cmdParams is non-null.

+  rv = cmdParams->GetBooleanValue("state_mixed", _retval);
+  if (NS_FAILED(rv))
+    *_retval = PR_FALSE;

This check isn't really needed since you return rv to the caller, if the caller
looks at _retval w/o checking if the call succeeded or not, that's the callers
problem. And *_retval is set to false at the top of the method too.

sr=jst with those changes.

Comment 4

[Kathleen :Brade]

Comment on attachment 143581 [details] [diff] [review]
QueryCommandIndeterm implementation in nsHTMLDocument

I'd like to see an explicit comment that states something like:  Some commands
won't return a value for GetBooleanValue("state_mixed") so we expect to get a
failure rv.  It is fine to fail if the command doesn't have a "mixed" state
value (IMO).

r=brade if you fix the issues jst mentioned and add a comment something like
above.

Comment 5

[Alexandre Trémon]

Attachment: Changes corresponding to Johnny and Brade's comments

Comment 6

[Kathleen :Brade]

Comment on attachment 143697 [details] [diff] [review]
Changes corresponding to Johnny and Brade's comments

r=brade but please fix the typo in the comment:  s/to/too/

Comment 7

[Alexandre Trémon]

Attachment: if command does not have a state_mixed value, this call fails, so we fail too, which is what expected

Comment 8

[Alexandre Trémon]

Brade, can you check the patch in (I do not have cvs write access) ?
Thanks!

Comment 9

[chris hofmann]

Comment on attachment 143697 [details] [diff] [review]
Changes corresponding to Johnny and Brade's comments

a=chofmann for 1.7

Comment 10

[Kathleen :Brade]

Checking in nsHTMLDocument.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v  <-- 
nsHTMLDocument.cpp
new revision: 3.546; previous revision: 3.545