Closed Bug 738385 Opened 12 years ago Closed 12 years ago

queryCommand*() should not throw for commands that don't support them

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: ayg, Assigned: ayg)

Details

Attachments

(2 files, 4 obsolete files)

Spec:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandindeterm()
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandstate()
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#querycommandvalue()

For commands that don't have any special processing defined for indeterm/state/value, the spec says to return false/false/"" respectively.  E.g., document.queryCommandValue("bold"), or document.queryCommandIndeterm("stylewithcss").  This was discussed:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033235.html

Originally the spec said to throw, which matches Gecko.  But Ryosuke had compat fears.  I think throwing in these cases makes sense, but realistically I think the path of least resistance is what the spec says now.  Asking IE/WebKit/Opera to all change to throwing when they didn't throw before has much bigger compat risk than Gecko changing to not throw.
This sounds fine by me.  I doubt that there are too many web sites which rely on Gecko throwing in this case...  ;-)
Attached patch Patch v1 (obsolete) — Splinter Review
The code change here causes a lot of TEST-UNEXPECTED-PASS in browserscope, but no TEST-UNEXPECTED-FAIL (except the debug is()'s that dump the HTML).  The changes to currentStatus.js should serve as a good enough test of the functionality change.  The diff is bad, unfortunately, recording it as lots of additions followed by lots of deletions, because of how repetitive the data file is.

I'm not at all sure this is the best way to fix the code.
Attachment #608444 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 608444
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=37e27f74fefc
Try run started, revision 37e27f74fefc. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=37e27f74fefc
Comment on attachment 608444 [details] [diff] [review]
Patch v1

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

r=me, assuming that you auto-generated currentStatus.js after your change.  :-)

(And yes, that would serve as enough testing.)
Attachment #608444 - Flags: review?(ehsan) → review+
Try run for 37e27f74fefc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=37e27f74fefc
Results (out of 220 total builds):
    success: 173
    warnings: 31
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-37e27f74fefc
Whiteboard: [autoland-in-queue]
When fixing the test_bug408231.html failures, I noticed that formatblock and heading were still throwing exceptions.  This was because QueryCommandIndeterm() and QueryCommandState() were both calling the version of ConvertToMidasInternalCommandInner that doesn't ignore parameters, and that version had a code path for cmd_paragraphState that returned false if the parameter wasn't recognized.  In this case the parameter was empty, so it wound up throwing NS_ERROR_NOT_IMPLEMENTED.  We only really wanted this logic for execCommand, so I moved it to ExecCommand.  While I was at it, I changed QueryCommandIndeterm to use the two-parameter version of ConvertToMidasInternalCommand, because it didn't need the extra params.

(This patch causes test failures in browserscope, which are fixed in the next patch.)
Attachment #608444 - Attachment is obsolete: true
Attachment #608800 - Flags: review?(ehsan)
This is the same patch as the v1 I submitted before.  The only difference is in test changes: I updated test_bug408231.html to account for this bug's patches, and updated currentStatus.js to fix more TEST-UNEXPECTED-PASS that resulted from the new patch part 1 in this series.  I also added indeterm tests to test_bug408231.html, and made it test for specific exception types rather than just generically testing for an exception to be thrown.
Attachment #608802 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 608800, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=915764a4b8f1
Try run started, revision 915764a4b8f1. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=915764a4b8f1
Attachment #608802 - Flags: review?(ehsan) → review+
Attachment #608800 - Flags: review?(ehsan) → review+
Try run for 915764a4b8f1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=915764a4b8f1
Results (out of 219 total builds):
    exception: 1
    success: 171
    warnings: 47
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-915764a4b8f1
Whiteboard: [autoland-in-queue]
I don't think those crashtest failures are my fault, but let me double-check . . .
Whiteboard: [autoland-try:-u crashtest -b d]
Whiteboard: [autoland-try:-u crashtest -b d] → [autoland-in-queue]
Autoland Patchset:
	Patches: 608800, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b87aa6381e6e
Try run started, revision b87aa6381e6e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b87aa6381e6e
Ehsan, it looks like this causes an unexpected pass for 636074-1.html.  Is it okay if I just update crashtests.list, i.e., do

--- a/editor/libeditor/base/crashtests/crashtests.list
+++ b/editor/libeditor/base/crashtests/crashtests.list
@@ -2,11 +2,11 @@ load 336104.html
 load 382527-1.html
 load 402172-1.html
 load 407079-1.html
 load 407256-1.html
 load 430624-1.html
 load 459613.html
 load 475132-1.xhtml
 asserts-if(!Android,1) load 633709.xhtml # Bug 695364
-asserts-if(!Android,6) load 636074-1.html # Bug 439258, charged to the wrong test due to bug 635550
+load 636074-1.html
 load 713427-1.html
 load 713427-2.xhtml

?  Based on reading the relevant bugs (the ones I have access to . . .), I doubt that this patch actually fixes the problem, so maybe it's just masking it, in which case probably the test should somehow be updated so that it still fails.  But I don't know what in my patches could have possibly caused the assertions to suddenly stop, so I don't know how to update the test.
Try run for b87aa6381e6e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b87aa6381e6e
Results (out of 13 total builds):
    success: 6
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-b87aa6381e6e
Whiteboard: [autoland-in-queue]
Actually, this also seems to consistently cause

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/docshell/base/crashtests/432114-2.html | load failed: timed out waiting for reftest-wait to be removed

That one probably *is* my fault, most likely due to the refactoring.  I'll look more closely at it.
(In reply to Aryeh Gregor from comment #12)
> Ehsan, it looks like this causes an unexpected pass for 636074-1.html.  Is
> it okay if I just update crashtests.list, i.e., do
> 
> --- a/editor/libeditor/base/crashtests/crashtests.list
> +++ b/editor/libeditor/base/crashtests/crashtests.list
> @@ -2,11 +2,11 @@ load 336104.html
>  load 382527-1.html
>  load 402172-1.html
>  load 407079-1.html
>  load 407256-1.html
>  load 430624-1.html
>  load 459613.html
>  load 475132-1.xhtml
>  asserts-if(!Android,1) load 633709.xhtml # Bug 695364
> -asserts-if(!Android,6) load 636074-1.html # Bug 439258, charged to the
> wrong test due to bug 635550
> +load 636074-1.html
>  load 713427-1.html
>  load 713427-2.xhtml
> 
> ?  Based on reading the relevant bugs (the ones I have access to . . .), I
> doubt that this patch actually fixes the problem, so maybe it's just masking
> it, in which case probably the test should somehow be updated so that it
> still fails.  But I don't know what in my patches could have possibly caused
> the assertions to suddenly stop, so I don't know how to update the test.

This assertion occurs in other tests as well, but I'm curious to know *why* your patch hides this.  I'm pretty sure that something in that test is now throwing...

(In reply to Aryeh Gregor from comment #14)
> Actually, this also seems to consistently cause
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///c:/talos-slave/test/build/reftest/tests/docshell/base/crashtests/
> 432114-2.html | load failed: timed out waiting for reftest-wait to be removed
> 
> That one probably *is* my fault, most likely due to the refactoring.  I'll
> look more closely at it.

Hmm, this one looks like something throwing as well.  I think there's probably a bug in your patch.
It turns out that the problem that the moved check missed a condition that the original check had -- it was nested in if (gMidasCommandTable[i].useNewParam), but the moved code was being run even if that was false.  Now instead of ConvertToMidasInternalCommandInner returning false if it gets an invalid parameter for cmd_paragraphState, it truncates the output parameter.  ExecCommand then separately checks if the output parameter is empty and returns NS_OK if so.  This fixes the crashtest failures I was getting, and also means that QueryCommandState won't throw for formatBlock etc.
Attachment #608800 - Attachment is obsolete: true
Attachment #611807 - Flags: review?(ehsan)
OS: Windows 2000 → All
Whiteboard: [autoland-try:611807,608802:-u all]
Whiteboard: [autoland-try:611807,608802:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 611807, 608802
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e56453b68661
Try run started, revision e56453b68661. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e56453b68661
Attachment #611807 - Flags: review?(ehsan) → review+
Try run for e56453b68661 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e56453b68661
Results (out of 226 total builds):
    success: 168
    warnings: 57
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-e56453b68661
Whiteboard: [autoland-in-queue]
Oops, dumb logic error in that patch.  I should have run tests in editor/ and content/html/document/ before submitting.  Now I have and it passes.  Interdiff:

diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
--- a/content/html/document/src/nsHTMLDocument.cpp
+++ b/content/html/document/src/nsHTMLDocument.cpp
@@ -2944,17 +2944,17 @@ ConvertToMidasInternalCommandInner(const
             for (j = 0; j < ArrayLength(gBlocks); ++j) {
               if (convertedParam.Equals(gBlocks[j],
                                         nsCaseInsensitiveCStringComparator())) {
                 outParam.Assign(gBlocks[j]);
                 break;
               }
             }
 
-            if (j != ArrayLength(gBlocks)) {
+            if (j == ArrayLength(gBlocks)) {
               outParam.Truncate();
             }
           }
           else {
             CopyUTF16toUTF8(inParam, outParam);
           }
         }
       }

Sorry for the review spam!
Attachment #611807 - Attachment is obsolete: true
Attachment #612134 - Flags: review?(ehsan)
Whiteboard: [autoland-try:612134,608802:-u all]
Whiteboard: [autoland-try:612134,608802:-u all] → [autoland-in-queue]
Comment on attachment 612134 [details] [diff] [review]
Patch part 1, v3 -- Refactor some editing logic in nsHTMLDocument.cpp

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

Oops, should have caught that!
Attachment #612134 - Flags: review?(ehsan) → review+
Autoland Patchset:
	Patches: 612134, 608802
	Branch: mozilla-central => try
Patch 608802 could not be applied to mozilla-central.
patching file content/html/content/test/test_bug408231.html
Hunk #1 FAILED at 52
Hunk #2 FAILED at 124
2 out of 3 hunks FAILED -- saving rejects to file content/html/content/test/test_bug408231.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Whiteboard: [autoland-in-queue]
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 612134, 612813
	Branch: mozilla-central => try
Patch 612813 could not be applied to mozilla-central.
patching file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js
Hunk #2 FAILED at 9373
Hunk #9 FAILED at 23824
2 out of 9 hunks FAILED -- saving rejects to file editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Whiteboard: [autoland-in-queue]
Try run passed; failures are due only to bug 279330.  See <https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3> and <https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c> for evidence to that effect.  Will land with bug 279330.
https://hg.mozilla.org/mozilla-central/rev/307272b019d4
https://hg.mozilla.org/mozilla-central/rev/bcc6fb80cd73
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.