Bug 12562 - Fix test cases that are now spewing console errors that they are not testing for
Summary: Fix test cases that are now spewing console errors that they are not testing for
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-02-03 15:26 PST by Sam Weinig
Modified: 2007-02-04 19:04 PST (History)
2 users (show)

See Also:


Attachments
patch (75.37 KB, patch)
2007-02-03 15:36 PST, Sam Weinig
aroben: review-
Details | Formatted Diff | Diff
updated patch (113.11 KB, patch)
2007-02-04 07:58 PST, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
updated patch (78.04 KB, patch)
2007-02-04 18:18 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-02-03 15:26:45 PST
After the recent change to the DRT to dump messages to the console, it became apparent that some tests have errors in them that they are not testing for.  Forthcoming patch fixes a bunch of these.
Comment 1 Sam Weinig 2007-02-03 15:36:22 PST
Created attachment 12903 [details]
patch
Comment 2 Adam Roben (:aroben) 2007-02-03 15:48:23 PST
Comment on attachment 12903 [details]
patch

   This is an awesome patch. Some of the expected results seem a bit questionable, though.

-SUCCESS - Didn't crash
+SUCCESS

-    <div>SUCCESS - Didn't crash</div>
+    <div id="result">SUCCESS - Didn't crash</div>

   The two above changes don't seem to match up. Why is " - Didn't crash" not appearing in the expected results?

+caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of child 0 {HTML} of document

   If the caret is inside the <input>, won't it have a focus ring (which is what this test says shouldn't happen)?

+try { [2, 5, 9].forEach(printEltAndException); }
+catch (e) { print(e); }

   Can you add newlines here as if it were an if-else block?

+try { [2, 5, 8, 1, 4].some(isBigEnoughAndException); }
+catch (e) { print(e); }
+
+try { [12, 5, 8, 1, 44].some(isBigEnoughAndException); }
+catch (e) { print(e); }

   Ditto.

-caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+selection end:   position 0 of child 5 {DIV} of child 7 {DIV} of child 1 {BODY} of child 0 {HTML} of document

   Why are we ending up with a selection now?
Comment 3 Sam Weinig 2007-02-03 16:27:31 PST
(In reply to comment #2)
> (From update of attachment 12903 [details] [edit])
>    This is an awesome patch. Some of the expected results seem a bit
> questionable, though.
> 
> -SUCCESS - Didn't crash
> +SUCCESS
> 
> -    <div>SUCCESS - Didn't crash</div>
> +    <div id="result">SUCCESS - Didn't crash</div>
> 
>    The two above changes don't seem to match up. Why is " - Didn't crash" not
> appearing in the expected results?

It now displays only 'SUCCESS' because the way the patch was set up was to  call the js which called         document.getElementById('result').innerHTML = 'SUCCESS'; 
Before, the id was not set on the div, so this call could not be completed.  I could get rid of this all together and the output would not change.

> +caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of
> child 0 {HTML} of document
> 
>    If the caret is inside the <input>, won't it have a focus ring (which is
> what this test says shouldn't happen)?

I'm not exactly sure what is happening here (I need to investigate it further) but this is not clicking inside the <input>.  If it was, with a eventSender.mouseMoveTo(100, 45) like its sister test input-text-click-inside.html, the png would show focus ring.  As I said I'm not exactly sure what the caret: ... syntax means in this case.

> +try { [2, 5, 9].forEach(printEltAndException); }
> +catch (e) { print(e); }
> 
>    Can you add newlines here as if it were an if-else block?
> 
> +try { [2, 5, 8, 1, 4].some(isBigEnoughAndException); }
> +catch (e) { print(e); }
> +
> +try { [12, 5, 8, 1, 44].some(isBigEnoughAndException); }
> +catch (e) { print(e); }
> 
>    Ditto.

I did it in this style to be consistent with the rest of the test.  I will change it.

> -caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of
> child 1 {BODY} of child 0 {HTML} of document
> +selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7
> {DIV} of child 1 {BODY} of child 0 {HTML} of document
> +selection end:   position 0 of child 5 {DIV} of child 7 {DIV} of child 1
> {BODY} of child 0 {HTML} of document
> 
>    Why are we ending up with a selection now?
> 

We are selecting the third table element.  At first I thought that was correct, but now I can't see why.  Will look into it further as well.
Comment 4 Sam Weinig 2007-02-04 07:54:15 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 12903 [details] [edit] [edit])

> > +caret: position 1 of child 0 {INPUT} of child 0 {DIV} of child 0 {BODY} of
> > child 0 {HTML} of document
> > 
> >    If the caret is inside the <input>, won't it have a focus ring (which is
> > what this test says shouldn't happen)?
> 
> I'm not exactly sure what is happening here (I need to investigate it further)
> but this is not clicking inside the <input>.  If it was, with a
> eventSender.mouseMoveTo(100, 45) like its sister test
> input-text-click-inside.html, the png would show focus ring.  As I said I'm not
> exactly sure what the caret: ... syntax means in this case.

I talked to mitz on IRC about this and he says this is the correct behavior.  This indicates that the caret is not inside the input, which the png also shows.

> > -caret: position 9 of child 0 {#text} of child 7 {DIV} of child 7 {DIV} of
> > child 1 {BODY} of child 0 {HTML} of document
> > +selection start: position 9 of child 0 {#text} of child 3 {DIV} of child 7
> > {DIV} of child 1 {BODY} of child 0 {HTML} of document
> > +selection end:   position 0 of child 5 {DIV} of child 7 {DIV} of child 1
> > {BODY} of child 0 {HTML} of document
> > 
> >    Why are we ending up with a selection now?
> > 
> 
> We are selecting the third table element.  At first I thought that was correct,
> but now I can't see why.  Will look into it further as well.
> 

I am changing this so that we no longer have a selection by increasing the leapForward time to 1000ms.

New patch forthcoming.

Comment 5 Sam Weinig 2007-02-04 07:58:38 PST
Created attachment 12918 [details]
updated patch
Comment 6 Darin Adler 2007-02-04 18:04:06 PST
Comment on attachment 12918 [details]
updated patch

The console error part of this patch looks great. r=me on that.

But the changes inside ksvg2 seem unrelated and probably shouldn't be in this patch.
Comment 7 Sam Weinig 2007-02-04 18:18:54 PST
Created attachment 12926 [details]
updated patch

Oops!  I forgot those where in my tree.  Removed.
Comment 8 Darin Adler 2007-02-04 18:28:17 PST
Comment on attachment 12926 [details]
updated patch

r=me
Comment 9 Sam Weinig 2007-02-04 19:04:04 PST
Landed in r19397.