WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
108466
[chromium] turn TestRunner library into a component build
https://bugs.webkit.org/show_bug.cgi?id=108466
Summary
[chromium] turn TestRunner library into a component build
jochen
Reported
2013-01-31 05:19:16 PST
that includes gyp changes and adding TESTRUNNER_EXPORT macros
Attachments
Patch
(96.03 KB, patch)
2013-02-05 14:00 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(92.79 KB, patch)
2013-02-05 14:13 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch for landing
(101.18 KB, patch)
2013-02-06 04:09 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(100.30 KB, patch)
2013-02-06 13:22 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(100.87 KB, patch)
2013-02-07 00:46 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(100.78 KB, patch)
2013-02-07 12:58 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Patch
(100.76 KB, patch)
2013-02-07 23:35 PST
,
jochen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
jochen
Comment 1
2013-02-05 14:00:37 PST
Created
attachment 186697
[details]
Patch
jochen
Comment 2
2013-02-05 14:13:18 PST
Created
attachment 186701
[details]
Patch
jochen
Comment 3
2013-02-05 14:14:13 PST
I verified that it builds in a chromium component build.
WebKit Review Bot
Comment 4
2013-02-05 14:16:53 PST
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
jochen
Comment 5
2013-02-05 14:42:43 PST
+dirk for component build insights
Adam Barth
Comment 6
2013-02-05 15:03:48 PST
Comment on
attachment 186701
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > + std::auto_ptr<TestInterfaces> m_interfaces;
fishd pointed me towards this web page explaining a gotcha with auto_ptr:
http://anki3d.org/stl-auto_ptr-a-bad-idea/
Adam Barth
Comment 7
2013-02-05 15:05:27 PST
Comment on
attachment 186701
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 >> + std::auto_ptr<TestInterfaces> m_interfaces; > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: >
http://anki3d.org/stl-auto_ptr-a-bad-idea/
Also
http://stackoverflow.com/questions/3697686/what-is-the-problem-with-auto-ptr
jochen
Comment 8
2013-02-05 15:08:12 PST
(In reply to
comment #6
)
> (From update of
attachment 186701
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > > + std::auto_ptr<TestInterfaces> m_interfaces; > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: >
http://anki3d.org/stl-auto_ptr-a-bad-idea/
I guess that problem can be avoided be adding a static create method to WebTestInterfaces, so nobdoy can derive from it.
jochen
Comment 9
2013-02-05 15:08:48 PST
(In reply to
comment #7
)
> (From update of
attachment 186701
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> > >> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > >> + std::auto_ptr<TestInterfaces> m_interfaces; > > > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > >
http://anki3d.org/stl-auto_ptr-a-bad-idea/
> > Also
http://stackoverflow.com/questions/3697686/what-is-the-problem-with-auto-ptr
Yes, but we don't have C++0x everywhere yet, do we?
jochen
Comment 10
2013-02-05 15:10:38 PST
(In reply to
comment #8
)
> (In reply to
comment #6
) > > (From update of
attachment 186701
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> > > > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > > > + std::auto_ptr<TestInterfaces> m_interfaces; > > > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > >
http://anki3d.org/stl-auto_ptr-a-bad-idea/
> > I guess that problem can be avoided be adding a static create method to WebTestInterfaces, so nobdoy can derive from it.
Actually, even when you derive from WebTestInterfaces, this shouldn't be a problem, as WebTestInterfaces has an explicit destructor.
Adam Barth
Comment 11
2013-02-05 15:30:09 PST
Comment on
attachment 186701
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:617 > -#if OS(LINUX) && USE(GTK) > +#if defined(__linux__) && USE(GTK)
Does USE(GTK) still work?
> Tools/DumpRenderTree/chromium/TestRunner/src/config.h:35 > +#define USE(feature) (defined WTF_USE_##feature && WTF_USE_##feature) > +#define ENABLE(feature) (defined ENABLE_##feature && ENABLE_##feature)
Ah, I see!
Adam Barth
Comment 12
2013-02-05 15:31:21 PST
Yeah, we just have to be careful to have an explicit destructor. fishd said that this was probably ok. He also mentioned that we're probably depending on base already via WebURL.
WebKit Review Bot
Comment 13
2013-02-05 17:08:14 PST
Comment on
attachment 186701
[details]
Patch
Attachment 186701
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/16379836
New failing tests: editing/spelling/spellcheck-async.html pointer-lock/pointerlockchange-pointerlockerror-events.html fast/speech/scripted/speechrecognition-basics.html editing/spelling/spelling-marker-description.html fast/speech/scripted/speechrecognition-errors.html pointer-lock/pointerlockchange-event-on-lock-lost.html fast/speech/scripted/navigate-away.html pointer-lock/pointerlockelement-null-when-pending.html
Dirk Pranke
Comment 14
2013-02-05 17:56:20 PST
Comment on
attachment 186701
[details]
Patch build/gyp changes look fine to me. why did you remove the OVERRIDE annotations from TestRunner.h ?
jochen
Comment 15
2013-02-06 04:09:43 PST
Created
attachment 186819
[details]
Patch for landing
jochen
Comment 16
2013-02-06 04:28:18 PST
(In reply to
comment #14
)
> (From update of
attachment 186701
[details]
) > build/gyp changes look fine to me. > > why did you remove the OVERRIDE annotations from TestRunner.h ?
I didn't work. But as Adam pointed out, the webkit API already has access to base/, so I now include <base/compiler_specific.h> and put the OVERRIDE macros back in
jochen
Comment 17
2013-02-06 04:29:16 PST
(In reply to
comment #11
)
> (From update of
attachment 186701
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=186701&action=review
> > > Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:617 > > -#if OS(LINUX) && USE(GTK) > > +#if defined(__linux__) && USE(GTK) > > Does USE(GTK) still work? > > > Tools/DumpRenderTree/chromium/TestRunner/src/config.h:35 > > +#define USE(feature) (defined WTF_USE_##feature && WTF_USE_##feature) > > +#define ENABLE(feature) (defined ENABLE_##feature && ENABLE_##feature) > > Ah, I see!
Related, I had to remove the ENABLE() #ifdefs: the feature defines are not visible via the webkit api. And since we're only using the WebKit API, we only use methods that are present regardless of the defines.
WebKit Review Bot
Comment 18
2013-02-06 05:13:01 PST
Comment on
attachment 186819
[details]
Patch for landing Clearing flags on attachment: 186819 Committed
r141991
: <
http://trac.webkit.org/changeset/141991
>
WebKit Review Bot
Comment 19
2013-02-06 05:13:06 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 20
2013-02-06 06:37:59 PST
Re-opened since this is blocked by
bug 109047
jochen
Comment 21
2013-02-06 13:22:58 PST
Created
attachment 186907
[details]
Patch
WebKit Review Bot
Comment 22
2013-02-06 14:15:54 PST
Comment on
attachment 186907
[details]
Patch Clearing flags on attachment: 186907 Committed
r142032
: <
http://trac.webkit.org/changeset/142032
>
WebKit Review Bot
Comment 23
2013-02-06 14:15:59 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 24
2013-02-06 14:35:27 PST
Re-opened since this is blocked by
bug 109095
jochen
Comment 25
2013-02-07 00:46:12 PST
Created
attachment 187008
[details]
Patch
jochen
Comment 26
2013-02-07 03:47:49 PST
Comment on
attachment 187008
[details]
Patch Clearing flags on attachment: 187008 Committed
r142090
: <
http://trac.webkit.org/changeset/142090
>
jochen
Comment 27
2013-02-07 03:47:56 PST
All reviewed patches have been landed. Closing bug.
Gavin Peters
Comment 28
2013-02-07 06:26:37 PST
Reverted
r142090
for reason: lots of selection expectations failures Committed
r142109
: <
http://trac.webkit.org/changeset/142109
>
jochen
Comment 29
2013-02-07 12:58:02 PST
Created
attachment 187155
[details]
Patch
jochen
Comment 30
2013-02-07 13:00:01 PST
Comment on
attachment 187155
[details]
Patch Clearing flags on attachment: 187155 Committed
r142165
: <
http://trac.webkit.org/changeset/142165
>
jochen
Comment 31
2013-02-07 13:00:09 PST
All reviewed patches have been landed. Closing bug.
Gavin Peters
Comment 32
2013-02-07 13:40:43 PST
Reverted
r142165
for reason: Broke linux_aura builds. Committed
r142173
: <
http://trac.webkit.org/changeset/142173
>
jochen
Comment 33
2013-02-07 23:35:50 PST
Created
attachment 187246
[details]
Patch
jochen
Comment 34
2013-02-07 23:39:53 PST
Comment on
attachment 187246
[details]
Patch Clearing flags on attachment: 187246 Committed
r142237
: <
http://trac.webkit.org/changeset/142237
>
jochen
Comment 35
2013-02-07 23:40:00 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug