Bug 42278 – Removes DontCheckEnums from some IDLs and fixes the corresponding enums.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marcus Bulach]

Removes DontCheckEnums from some IDLs and fixes the corresponding enums.

[Marcus Bulach]

Created attachment 61550 [details]
Patch

[Marcus Bulach]

Hi Darin,

This is the first follow up to https://bugs.webkit.org/show_bug.cgi?id=42250, it fixes a few IDL / enums and allow generating compile-time checks for them.
Would you mind taking a look please?

Thanks,
Marcus

[Darin Adler]

Comment on attachment 61550 [details]
Patch

>          enum orientType {
> -            VERTICAL   = 0,
> -            HORIZONTAL = 1,
> +            HORIZONTAL = 0,
> +            VERTICAL   = 1,
>              BOTH       = 2
>          };

This change seems risky. Was there any C++ code using these constants and thus getting things wrong? Can we construct a test that demonstrates we had them wrong?

The rest of the changes seem obviously good.

[Marcus Bulach]

Created attachment 61552 [details]
Patch

[Marcus Bulach]

(In reply to comment #3)
> (From update of attachment 61550 [details])
> >          enum orientType {
> > -            VERTICAL   = 0,
> > -            HORIZONTAL = 1,
> > +            HORIZONTAL = 0,
> > +            VERTICAL   = 1,
> >              BOTH       = 2
> >          };
> 
> This change seems risky. Was there any C++ code using these constants and thus getting things wrong? Can we construct a test that demonstrates we had them wrong?

yes! added the test, but I'm not sure if it's the best place. let me know if you'd like it to be on a test of its own.

> 
> The rest of the changes seem obviously good.
cool, thanks!

[WebKit Commit Bot]

Comment on attachment 61552 [details]
Patch

Rejecting patch 61552 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20688 test cases.
fast/events/overflow-events.html -> failed

Exiting early after 1 failures. 7260 tests run.
133.90s total testing time

7259 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://webkit-commit-queue.appspot.com/results/3577024

[Marcus Bulach]

Created attachment 61945 [details]
Patch

[Marcus Bulach]

(In reply to comment #3)
> (From update of attachment 61550 [details])
> >          enum orientType {
> > -            VERTICAL   = 0,
> > -            HORIZONTAL = 1,
> > +            HORIZONTAL = 0,
> > +            VERTICAL   = 1,
> >              BOTH       = 2
> >          };
> 
> This change seems risky. Was there any C++ code using these constants and thus getting things wrong? Can we construct a test that demonstrates we had them wrong?
> 
> The rest of the changes seem obviously good.

sorry to bother you again: there was an existing test (fast/events/overflow-events.html) that failed with this change.

the issue being that it tested against the numeric hardcoded values, and looks like it assumed the C++ side rather than the IDL definitions.

I changed the test to remove the hardcoded values and use the IDL-defined consts instead, would you mind taking another look? thanks!

[Darin Adler]

Comment on attachment 61945 [details]
Patch

Seems to me that having a test that actually tests the integer values was valuable. Too bad it was expecting the wrong values! Changing the test to use the named constants makes the test more readable, but changes what it tests.

[WebKit Commit Bot]

Comment on attachment 61945 [details]
Patch

Rejecting patch 61945 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Last 500 characters of output:
outTests/fast/events/script-tests/init-events.js
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/dom/Node.h
patching file WebCore/dom/Node.idl
patching file WebCore/dom/OverflowEvent.h
patching file WebCore/dom/OverflowEvent.idl
Hunk #1 FAILED at 23.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/dom/OverflowEvent.idl.rej
patching file WebCore/html/FileReader.cpp
patching file WebCore/html/FileReader.h
patching file WebCore/html/FileReader.idl

Full output: http://queues.webkit.org/results/3592218

[Marcus Bulach]

Created attachment 62817 [details]
Patch

[Marcus Bulach]

sorry about the delay. the latest patch merges with ToT and fixes the conflict issues reported above. another look please?

(In reply to comment #11)
> Created an attachment (id=62817) [details]
> Patch

[WebKit Commit Bot]

Comment on attachment 62817 [details]
Patch

Rejecting patch 62817 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
.cpp
	M	WebCore/html/FileReader.h
	M	WebCore/html/FileReader.idl
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/events/overflow-events.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://queues.webkit.org/results/3632121

[Adam Barth]

Comment on attachment 62817 [details]
Patch

Sorry, you seem to have hit an error in the commit queue.  Let's try again.

[Adam Barth]

Comment on attachment 62817 [details]
Patch

Oh, actually, you have tabs.

[Adam Barth]

Created attachment 64073 [details]
Patch for landing

[Jeremy Orlow]

Oh, thanks Adam!

[Marcus Bulach]

thanks Adam! this went off my radar.. :(

(In reply to comment #17)
> Oh, thanks Adam!

[WebKit Commit Bot]

Comment on attachment 64073 [details]
Patch for landing

Rejecting patch 64073 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
youtTests
Testing 20828 test cases.
media/video-size-intrinsic-scale.html -> timed out
Sampling process 79668 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 79668 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 17216 tests run.
862.47s total testing time

17215 test cases (99%) succeeded
1 test case (<1%) timed out
36 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3765063

[Jeremy Orlow]

Marcus, I guess just land manually?

[Marcus Bulach]

(In reply to comment #20)
> Marcus, I guess just land manually?

landing manually now.

[Marcus Bulach]

Committed r65226: <http://trac.webkit.org/changeset/65226>

[Eric Seidel (no email)]

The media timeouts are caused by some CoreVideo bug, which I thought was fixed.  (Or at least they were last time they happened.)

I haven't seen any timeouts since this bug, so maybe it was a WebCore change which was causing troubles.