Last Comment Bug 735059 - Second and third execCommand() parameters should be optional
: Second and third execCommand() parameters should be optional
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla14
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 15:00 PDT by Aryeh Gregor (:ayg) (away until October 25)
Modified: 2012-03-15 08:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.09 KB, patch)
2012-03-12 15:13 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 2012-03-12 15:00:28 PDT
It's nice to be able to do execCommand("bold") instead of execCommand("bold", false, "").  All the other browsers allow it, and the spec says it should be allowed too.
Comment 1 Aryeh Gregor (:ayg) (away until October 25) 2012-03-12 15:13:07 PDT
Created attachment 605156 [details] [diff] [review]
Patch v1

This bug has actually annoyed me for months.
Comment 2 :Ehsan Akhgari 2012-03-12 15:18:54 PDT
Comment on attachment 605156 [details] [diff] [review]
Patch v1

Review of attachment 605156 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment below.

::: editor/libeditor/html/tests/test_bug735059.html
@@ +12,5 @@
> +/** Test for Bug 735059 **/
> +
> +// Value defaults to the empty string, which evaluates to true, so this
> +// disables CSS styling
> +document.execCommand("usecss");

If we ever regress this, this call will throw, and the test will finish successfully with a message saying "no actual checks performed" or something similar to that.  The correct way to test is to wrap the entire js in a try/catch and explicitly fail the test in the catch block.
Comment 3 Mozilla RelEng Bot 2012-03-12 19:15:09 PDT
Autoland Patchset:
	Patches: 605156
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=cb573ba079ca
Try run started, revision cb573ba079ca. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=cb573ba079ca
Comment 4 Aryeh Gregor (:ayg) (away until October 25) 2012-03-13 11:30:53 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> If we ever regress this, this call will throw, and the test will finish
> successfully with a message saying "no actual checks performed" or something
> similar to that.  The correct way to test is to wrap the entire js in a
> try/catch and explicitly fail the test in the catch block.

I've confirmed that an uncaught exception will actually cause a failure.  See bug 365929.  If I revert the IDL patch but leave the test, the test fails with message

"""
an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "Not enough arguments [nsIDOMHTMLDocument.execCommand]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: http://mochi.test:8888/tests/editor/libeditor/html/tests/test_bug735059.html?autorun=1&closeWhenDone=1&logFile=%2Fmnt%2Fextra%2Fcheckouts%2Fmozilla-central%2Fobjdir-ff-release%2Fmochitest-plain.log&fileLevel=INFO&consoleLevel=INFO&failureFile=/mnt/extra/checkouts/mozilla-central/objdir-ff-release/_tests/testing/mochitest/makefailures.json :: <TOP_LEVEL> :: line 16" data: no] at :0
"""

So no changes needed to the patch, per instructions on IRC.
Comment 5 Mozilla RelEng Bot 2012-03-13 11:37:50 PDT
Autoland Patchset:
	Patches: 605156
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=975dd82afb07
Try run started, revision 975dd82afb07. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=975dd82afb07
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-03-14 04:10:00 PDT
Indeed, yay for window.onerror
Comment 8 Marco Bonardo [::mak] 2012-03-15 08:16:37 PDT
https://hg.mozilla.org/mozilla-central/rev/5217bbf42aa7

Note You need to log in before you can comment on or make changes to this bug.