Bug 80215 – [Gtk] [Win32] WebKitGtk+ fails to build on Win32 after changeset 109584

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Platform

Created attachment 130008 [details]
proposed patch

Attempting to cross-compile WebKitGtk+ for windows results in error:

  CXXLD  libwebkitgtk-3.0.la
Creating library file: .libs/libwebkitgtk-3.0.dll.a
./.libs/libWebCore.a(libWebCore_la-ScrollAnimatorWin.o):ScrollAnimatorWin.cpp:(.text+0x6d0): multiple definition of `WebCore::ScrollAnimator::create(WebCore::ScrollableArea*)'
./.libs/libWebCore.a(libWebCore_la-ScrollAnimatorNone.o):ScrollAnimatorNone.cpp:(.text+0x1330): first defined here
collect2: error: ld returned 1 exit status
make[1]: *** [libwebkitgtk-3.0.la] Error 1
make[1]: Leaving directory `/home/pawel/src/WebKit'
make: *** [all] Error 2

[Philippe Normand]

Have you tried a clean build?
We had this issue on the bots too (Linux) and a clean build worked out.

[Philippe Normand]

Comment on attachment 130008 [details]
proposed patch

I don't see any X11 specific code in ScrollAnimatorNone, moving these files to TARGET_X11 looks only like a workaround for your cross-compiling issue.

Yes i tried clean build (as in "make clean"). Build stopped again in this same place.

With TARGET_WIN32 you have

if TARGET_WIN32
webcore_sources += \
<------>Source/WebCore/platform/ScrollAnimatorWin.cpp \
<------>Source/WebCore/platform/ScrollAnimatorWin.h \

Which in turn doubles the definitions from ScrollAnimatorNone.
So i think it should be like in attached patchlet or the other way around,
removing ScrollAnimatorWin. Moving just ScrollAnimatorNone into TARGET_X11
guard seemed logical to me and it feels a bit more cautious.

[Philippe Normand]

Indeed both conflict, I'm no expert about this code but maybe we should choose one and keep it whatever the platform is to keep scrolling experience as consistent as we can.

CCing Zan and Martin. What do you think?

Yea, sounds reasonable. The less duplication, the better. I'm sure they will come up with something nice.

[Zan Dobersek]

(In reply to comment #4)
> CCing Zan and Martin. What do you think?

Oddly enough, the ScrollAnimatorWin.cpp and the header do not include any Windows-specific headers either.

Anyway, like mentioned, I also think it would be best to use one class on every target, so I'd say removing ScrollAnimatorWin from the build and using ScrollAnimatorNone instead. ScrollAnimatorWin is also used by the WinCE port and it'd be best to avoid causing trouble if that class grows out of today's boundaries.

[Martin Robinson]

(In reply to comment #6)

> Anyway, like mentioned, I also think it would be best to use one class on every target, so I'd say removing ScrollAnimatorWin from the build and using ScrollAnimatorNone instead. ScrollAnimatorWin is also used by the WinCE port and it'd be best to avoid causing trouble if that class grows out of today's boundaries.

I think the important thing is to determine which animator best follows the scrolling behavior of GTK+ applications on Windows and use that one. If that turns out to be ScrollAnimatorWin.cpp, I think the risk of using it is very low.

[Zan Dobersek]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > Anyway, like mentioned, I also think it would be best to use one class on every target, so I'd say removing ScrollAnimatorWin from the build and using ScrollAnimatorNone instead. ScrollAnimatorWin is also used by the WinCE port and it'd be best to avoid causing trouble if that class grows out of today's boundaries.
> 
> I think the important thing is to determine which animator best follows the scrolling behavior of GTK+ applications on Windows and use that one. If that turns out to be ScrollAnimatorWin.cpp, I think the risk of using it is very low.

I'd like to hear bug reporter's feedback on the scrolling behavior with ScrollAnimatorWin class. If the behavior is acceptable/superior to ScrollAnimatorNone, the patch should be reuploaded with ChangeLog entries and reviewed.

Created attachment 131391 [details]
Patch with Changelog entry

I think behaviour with ScrollAnimatorNone is fine, updated patch to remove ScrollAnimatorWin

[WebKit Review Bot]

Comment on attachment 131391 [details]
Patch with Changelog entry

Rejecting attachment 131391 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
artin Rob..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/GNUmakefile.list.am
Hunk #1 FAILED at 4645.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/GNUmakefile.list.am.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Martin Rob..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

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

[Martin Robinson]

*** This bug has been marked as a duplicate of bug 80737 ***