Bug 114654

Created attachment 198216 [details] Patch

Minor issues obviously, but Eclipse is annoying about them. * We're not initializing all of the members in GStreamerMediaPlayerPrivate and GStreamerMediaPlayerPrivateBase * isinf() isn't in std:: since we're not importing it with #import <cmath>. I'm trying to figure out how it actually is being imported (I assume gstreamer or glib, but I'm not sure).

Created attachment 198220 [details] Patch

Actually, I think the problem is that isinf() is only in std:: in C++11 (according to http://en.cppreference.com/w/cpp/header/cmath). That can be fixed in Eclipse by explicitly setting __GXX_EXPERIMENTAL_CXX0X__. I'm not sure there's any reason to force that though.

CCing Zan, who, I think, recently did some work related with isinf.

Patch looks excepted for the isinf part, I think. We are adding C++11 support so I guess it would make sense for you to bend Eclipse to that decision :)

(In reply to comment #2) > * isinf() isn't in std:: since we're not importing it with #import <cmath>. I'm trying to figure out how it actually is being imported (I assume gstreamer or glib, but I'm not sure). If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary. To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it?

(In reply to comment #7) > If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary. I could make it define these when --std=c++11 isn't set if that would be useful. > To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it? This is in the Eclipse IDE. Builds work fine (presumably --std=c++11 is set?), it's just annoying that Eclipse highlights every usage of std::isinf() as an error. It sounds like it's probably better to just document how to get Eclipse working with QtWebKit and WebKitGtk instead of changing this?

Do you want the patch with just initializing GStreamerMediaPlayerPrivate members?

Yes.

Oops didn't mean to do that.

Created attachment 198509 [details] Patch

Comment on attachment 198509 [details] Patch Cool, thanks Brendan!

The commit-queue encountered the following flaky tests while processing attachment 198509 [details]: media/track/track-mode.html bug 114361 (author: annacc@chromium.org) media/video-layer-crash.html bug 114744 (authors: annacc@chromium.org, eric.carlson@apple.com, jamesr@chromium.org, and vrk@chromium.org) transitions/color-transition-rounding.html bug 114182 (author: simon.fraser@apple.com) transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: peter@chromium.org) transitions/interrupt-zero-duration.html bug 114184 (authors: cmarrin@apple.com, rniwa@webkit.org, and simon.fraser@apple.com) transitions/multiple-background-transitions.html bug 114185 (author: simon.fraser@apple.com) transitions/cubic-bezier-overflow-color.html bug 114186 (author: peter@chromium.org) transitions/multiple-shadow-transitions.html bug 114187 (author: simon.fraser@apple.com) transitions/mismatched-shadow-transitions.html bug 114188 (author: simon.fraser@apple.com) transitions/color-transition-all.html bug 114189 (authors: ossy@webkit.org and simon.fraser@apple.com) transitions/negative-delay.html bug 114190 (author: simon.fraser@apple.com) transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: peter@chromium.org) transitions/min-max-width-height-transitions.html bug 114192 (author: simon.fraser@apple.com) transitions/cancel-transition.html bug 114193 (authors: ojan@chromium.org, rniwa@webkit.org, and simon.fraser@apple.com) transitions/border-radius-transition.html bug 114194 (author: simon.fraser@apple.com) transitions/flex-transitions.html bug 114195 (author: tony@chromium.org) transitions/mixed-type.html bug 114196 (author: mikelawther@chromium.org) transitions/multiple-mask-transitions.html bug 114197 (author: simon.fraser@apple.com) transitions/color-transition-premultiplied.html bug 114198 (author: simon.fraser@apple.com) transitions/mismatched-shadow-styles.html bug 114199 (author: simon.fraser@apple.com) transitions/mask-transitions.html bug 114200 (authors: ojan@chromium.org, oliver@apple.com, and simon.fraser@apple.com) transitions/cubic-bezier-overflow-length.html bug 114201 (author: peter@chromium.org) transitions/multiple-background-size-transitions.html bug 114202 (authors: mitz@webkit.org and simon.fraser@apple.com) transitions/clip-transition.html bug 114203 (authors: dglazkov@chromium.org and simon.fraser@apple.com) transitions/cubic-bezier-overflow-transform.html bug 114204 (author: peter@chromium.org) transitions/interrupted-accelerated-transition.html bug 56242 (authors: rniwa@webkit.org, simon.fraser@apple.com, and tonyg@chromium.org) transitions/background-transitions.html bug 114206 (author: simon.fraser@apple.com) http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: abarth@webkit.org and rniwa@webkit.org) fast/loader/javascript-url-in-object.html bug 114210 (authors: rniwa@webkit.org and sam@webkit.org) platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 198509 [details] Patch Rejecting attachment 198509 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 198509, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ects to file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp.rej patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp Hunk #1 FAILED at 108. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Philippe Normand']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/31627

I'm confused. The commit-queue message says the patch didn't apply, but the git repos say otherwise (https://github.com/WebKit/webkit/commit/c74d372a14a8fc3184b3a61398e13d1f8cb3fa18). Do I need to do anything with this?

Landed in http://trac.webkit.org/changeset/148611

Reviving the discussion here in hope of resolving this problem. (In reply to comment #8) > (In reply to comment #7) > > If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary. > > I could make it define these when --std=c++11 isn't set if that would be useful. > Let's keep this as a backup plan. It makes sense though I'm not at the moment 100% sure it's the right approach. Also, FWIW, the MediaPlayerPrivateGStreamer source code is at the moment not compiled with std=c++11. > > To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it? > > This is in the Eclipse IDE. Builds work fine (presumably --std=c++11 is set?), it's just annoying that Eclipse highlights every usage of std::isinf() as an error. > Again, -std=c++11 is not used when compiling WebCore. If I'm understanding right, the build _does_ work but it's the Eclipse static code analysis that's marking the std::isinf as an error? Could you try including various headers in the MediaPlayerPrivateGStreamer.cpp file and reporting the outcome regarding the std::isinf being marked as an error? The three headers I'd like you to try are - <math.h> (what's normally included in WebCore nowadays when stdlib's math functions are required), - <cmath> (what I hope someday will replace the inclusions from the previous line), - <wtf/MathExtras.h> (the helper header that also sets up missing math functions for various compiler configurations, also includes <cmath>). > It sounds like it's probably better to just document how to get Eclipse working with QtWebKit and WebKitGtk instead of changing this? Hrm, I'd prioritize getting to the source of the problem, though I'm admittedly not affected by it.

(In reply to comment #18) > If I'm understanding right, the build _does_ work but it's the Eclipse static code analysis that's marking the std::isinf as an error? Yes, the build works. I even went to the trouble to make it build inside of Eclipse to see if it would pick up the settings automatically, but it didn't help (as suggested here: https://code.google.com/p/chromium/wiki/LinuxEclipseDev). I actually get errors (reported in Eclipse's editor but not the build) on std::isinf (but not ::isinf) and roundf One guess could be related to my relatively old Ubuntu version (12.04), but I assume if it was a GCC problem, it wouldn't compile. > - <math.h> (what's normally included in WebCore nowadays when stdlib's math functions are required), > - <cmath> (what I hope someday will replace the inclusions from the previous line), > - <wtf/MathExtras.h> (the helper header that also sets up missing math functions for various compiler configurations, also includes <cmath>). I get the same results for all three: * roundf not resolved * std::isinf(float): Invalid arguments ' Candidates are: __gnu_cxx::__enable_if<&0[std::__is_arithmetic<#0>::__value],int>::__type isinf(#0) '

It works on my home computer (Fedora 18). Since this only happens on a really old distro (Ubuntu 12.04), I doubt this is really worth spending any time on.