Bug 80215 - [Gtk] [Win32] WebKitGtk+ fails to build on Win32 after changeset 109584
Summary: [Gtk] [Win32] WebKitGtk+ fails to build on Win32 after changeset 109584
Status: RESOLVED DUPLICATE of bug 80737
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-03 15:00 PST by tuxator
Modified: 2012-03-12 15:57 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (970 bytes, patch)
2012-03-03 15:00 PST, tuxator
pnormand: review-
tuxator: commit-queue?
Details | Formatted Diff | Diff
Patch with Changelog entry (1.12 KB, patch)
2012-03-12 13:15 PDT, tuxator
mrobinson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tuxator 2012-03-03 15:00:58 PST
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
Comment 1 Philippe Normand 2012-03-04 07:40:08 PST
Have you tried a clean build?
We had this issue on the bots too (Linux) and a clean build worked out.
Comment 2 Philippe Normand 2012-03-04 07:44:32 PST
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.
Comment 3 tuxator 2012-03-04 12:22:55 PST
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.
Comment 4 Philippe Normand 2012-03-04 12:56:42 PST
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?
Comment 5 tuxator 2012-03-04 13:07:20 PST
Yea, sounds reasonable. The less duplication, the better. I'm sure they will come up with something nice.
Comment 6 Zan Dobersek 2012-03-04 13:46:31 PST
(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.
Comment 7 Martin Robinson 2012-03-04 19:02:01 PST
(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.
Comment 8 Zan Dobersek 2012-03-07 11:49:09 PST
(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.
Comment 9 tuxator 2012-03-12 13:15:44 PDT
Created attachment 131391 [details]
Patch with Changelog entry

I think behaviour with ScrollAnimatorNone is fine, updated patch to remove ScrollAnimatorWin
Comment 10 WebKit Review Bot 2012-03-12 15:19:26 PDT
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
Comment 11 Martin Robinson 2012-03-12 15:57:50 PDT

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