Closed Bug 237103 Opened 20 years ago Closed 20 years ago

nsIDOMNSHTMLDocument::QueryCommandIndeterm is not implemented

Categories

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

x86
Windows 2000
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: atremon, Unassigned)

Details

(Whiteboard: midas)

Attachments

(1 file, 2 obsolete files)

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 on attachment 143581 [details] [diff] [review]
QueryCommandIndeterm implementation in nsHTMLDocument

I tested it with "foreColor", "bold", "fontname" and "italic".
Attachment #143581 - Flags: review?(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.
Attachment #143581 - Flags: superreview+
Attachment #143581 - Flags: review?(jst)
Attachment #143581 - Flags: review?(brade)
Whiteboard: midas
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.
Attachment #143581 - Flags: review?(brade) → review+
Attachment #143581 - Attachment is obsolete: true
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/
Attachment #143697 - Flags: review+
Attachment #143964 - Flags: review?(brade)
Attachment #143964 - Flags: review?(brade) → review+
Brade, can you check the patch in (I do not have cvs write access) ?
Thanks!
Attachment #143581 - Flags: approval1.7b?
Comment on attachment 143697 [details] [diff] [review]
Changes corresponding to Johnny and Brade's comments

a=chofmann for 1.7
Attachment #143697 - Flags: approval1.7+
Attachment #143581 - Flags: approval1.7b? → approval1.7?
Checking in nsHTMLDocument.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v  <-- 
nsHTMLDocument.cpp
new revision: 3.546; previous revision: 3.545
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #143581 - Flags: approval1.7?
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: