[webkit-reviews] review denied: [Bug 51116] Building webkit with Visual Studio 2010 fails due to ambiguous 'operator =' methods in RefPtr. : [Attachment 77383] Fixes nullptr_t related build issues on vs2010

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jake <jake at jakeonthenet.com>'s request
for review:
Bug 51116: Building webkit with Visual Studio 2010 fails due to ambiguous
'operator =' methods in RefPtr.
https://bugs.webkit.org/show_bug.cgi?id=51116

Attachment 77383: Fixes nullptr_t related build issues on vs2010
https://bugs.webkit.org/attachment.cgi?id=77383&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77383&action=review

Thanks for the patch. Patches need to include change logs. Please submit a new
version with a change log.

> JavaScriptCore/wtf/NullPtr.h:36
> +// Don't declare this when building with visual studio 2010+
> +
> +#if !defined(_MSC_VER) || _MSC_VER < 1600

This should go a few lines down:

    #if !__has_feature(cxx_nullptr) && (!defined(_MSC_VER) || _MSC_VER < 1600)

No need to add an extra level of #if.

> JavaScriptCore/wtf/RefPtr.h:77
> +		template<typename U> RefPtr& operator=(const RefPtr<U>&);

Can't land a patch with tabs in it.

> WebCore/dom/ViewportArguments.h:77
> +	   , userScalable((bool) ValueAuto)

This fix is incorrect. Bug 50982 has a reviewed patch already in the commit
queue to fix this.

> WebCore/page/Geolocation.h:159
> +    class PositionCacheWrapper {
> +    public:
> +	   PositionCacheWrapper()
> +	   {
> +	   }
> +	   ~PositionCacheWrapper()
> +	   {
> +	   }
> +	   void setCachedPosition(Geoposition* cachedPosition) {}
> +	   Geoposition* cachedPosition() {return 0;}
> +    };

Adding an entire new copy of the class is not the optimal way to fix this. I
believe platforms that are not implementing Geolocation should not be compiling
this file at all.

This seems unrelated to the compiler and should not be lumped in with the
compiler-specific fixes.