nsIDOMNSHTMLDocument::QueryCommandIndeterm is not implemented

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
enhancement
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: Alexandre Trémon, Unassigned)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: midas)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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:
(Reporter)

Comment 1

15 years ago
Created attachment 143581 [details] [diff] [review]
QueryCommandIndeterm implementation in nsHTMLDocument
(Reporter)

Comment 2

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

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

Updated

15 years ago
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)

Updated

15 years ago
Whiteboard: midas

Comment 4

15 years ago
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+
(Reporter)

Comment 5

15 years ago
Created attachment 143697 [details] [diff] [review]
Changes corresponding to Johnny and Brade's comments
Attachment #143581 - Attachment is obsolete: true

Comment 6

15 years ago
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+
(Reporter)

Comment 7

15 years ago
Created attachment 143964 [details] [diff] [review]
if command does not have a state_mixed value, this call fails, so we fail too, which is what expected
Attachment #143697 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #143964 - Flags: review?(brade)

Updated

15 years ago
Attachment #143964 - Flags: review?(brade) → review+
(Reporter)

Comment 8

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

Updated

15 years ago
Attachment #143581 - Flags: approval1.7b?

Comment 9

15 years ago
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+

Updated

15 years ago
Attachment #143581 - Flags: approval1.7b? → approval1.7?

Comment 10

15 years ago
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
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Attachment #143581 - Flags: approval1.7?

Updated

10 years ago
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.