Bug 85836

Summary: [chromium] move event_listener_unittest to webkit_unit_tests
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, jamesr, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 85850    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Tony Chang 2012-05-07 15:26:57 PDT
[chromium] move event_listener_unittest to webkit_unit_tests
Comment 1 Tony Chang 2012-05-07 15:45:47 PDT
Created attachment 140609 [details]
Patch
Comment 2 Tony Chang 2012-05-07 15:46:27 PDT
First step in fixing https://code.google.com/p/chromium/issues/detail?id=126514 .
Comment 3 James Robinson 2012-05-07 15:48:49 PDT
Comment on attachment 140609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140609&action=review

Death to test_shell_tests (and test_shell)!

> Source/WebKit/chromium/tests/EventListenerTest.cpp:78
> +        std::string baseURL("http://www.test.com/");

nit: shouldn't we use example.com ? test.com is a real domain, example.com is an RFC 2606 (?) placeholder

> Source/WebKit/chromium/tests/data/listener/mutation_event_listener.html:1
> +<html>

<!DOCTYPE html> unless we need quirks mode for this test
Comment 4 Tony Chang 2012-05-07 15:52:49 PDT
Created attachment 140610 [details]
Patch for landing
Comment 5 Tony Chang 2012-05-07 15:53:10 PDT
(In reply to comment #3)
> (From update of attachment 140609 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140609&action=review
> 
> Death to test_shell_tests (and test_shell)!
> 
> > Source/WebKit/chromium/tests/EventListenerTest.cpp:78
> > +        std::string baseURL("http://www.test.com/");
> 
> nit: shouldn't we use example.com ? test.com is a real domain, example.com is an RFC 2606 (?) placeholder

Fixed.

> > Source/WebKit/chromium/tests/data/listener/mutation_event_listener.html:1
> > +<html>
> 
> <!DOCTYPE html> unless we need quirks mode for this test

Added.

Thanks for the fast review!
Comment 6 WebKit Review Bot 2012-05-07 17:05:05 PDT
Comment on attachment 140610 [details]
Patch for landing

Clearing flags on attachment: 140610

Committed r116375: <http://trac.webkit.org/changeset/116375>
Comment 7 WebKit Review Bot 2012-05-07 17:05:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Kent Tamura 2012-05-07 18:42:59 PDT
The patch caused build errors on Chromium-win and Chromium-mac.  I'm rolling it out soon.



1: .\tests\EventListenerTest.cpp(76) : error C2146: syntax error : missing ';' before identifier 'SetUp'

2: .\tests\EventListenerTest.cpp(76) : error C2182: 'override' : illegal use of type 'void'

3: .\tests\EventListenerTest.cpp(76) : error C2433: '`anonymous-namespace'::WebDOMEventListenerTest::override' : 'virtual' not permitted on data declarations

4: .\tests\EventListenerTest.cpp(77) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int

5: .\tests\EventListenerTest.cpp(85) : error C2146: syntax error : missing ';' before identifier 'TearDown'

6: .\tests\EventListenerTest.cpp(85) : error C2182: 'override' : illegal use of type 'void'

7: .\tests\EventListenerTest.cpp(85) : error C2433: '`anonymous-namespace'::WebDOMEventListenerTest::override' : 'virtual' not permitted on data declarations

8: .\tests\EventListenerTest.cpp(85) : error C2086: 'int `anonymous-namespace'::WebDOMEventListenerTest::override' : redefinition

9: .\tests\EventListenerTest.cpp(86) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int

10: .\tests\EventListenerTest.cpp(117) : error C2555: '`anonymous-namespace'::WebDOMEventListenerTest::SetUp': overriding virtual function return type differs and is not covariant from 'testing::Test::SetUp'

11: .\tests\EventListenerTest.cpp(117) : error C2555: '`anonymous-namespace'::WebDOMEventListenerTest::TearDown': overriding virtual function return type differs and is not covariant from 'testing::Test::TearDown'
Comment 9 James Robinson 2012-05-07 18:45:34 PDT
oh - OVERRIDE has to go after the function name, not before it.  It's an easy mechanical fix if you want to do that instead of reverting
Comment 10 WebKit Review Bot 2012-05-07 18:46:01 PDT
Re-opened since this is blocked by 85850
Comment 11 Kent Tamura 2012-05-07 18:53:29 PDT
Rolled out by http://trac.webkit.org/changeset/116383
Comment 12 Tony Chang 2012-05-08 10:05:19 PDT
Committed r116433: <http://trac.webkit.org/changeset/116433>
Comment 13 Tony Chang 2012-05-08 10:05:52 PDT
(In reply to comment #12)
> Committed r116433: <http://trac.webkit.org/changeset/116433>

Landed with the syntax fix.  I verified that it compiled using clang.