[webkit-reviews] review denied: [Bug 12327] Windows build bustage + vcproj/sln craziness : [Attachment 13322] Split patch for r19793, part 3: Compile fixes

[bugzilla-request-daemon at macosforge.org]

Adam Roben <aroben at apple.com> has denied Adam Roben <aroben at apple.com>'s
request for review:
Bug 12327: Windows build bustage + vcproj/sln craziness
http://bugs.webkit.org/show_bug.cgi?id=12327

Attachment 13322: Split patch for r19793, part 3: Compile fixes
http://bugs.webkit.org/attachment.cgi?id=13322&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
-#include <wtf/Platform.h>
+#include "wtf/Platform.h"

   This is the standard Mac "framework-style" #include, so this shouldn't
change.

-void CSSPrimitiveValue::setFloatValue(unsigned short unitType, double
floatValue, ExceptionCode& ec)
+void CSSPrimitiveValue::setFloatValue(unsigned short ut, double floatValue,
ExceptionCode& ec)
 {
+    UnitTypes unitType = (UnitTypes)ut;

   Why not just change the type of the parameter to UnitTypes? Also, if we do
end up casting, static_cast would be better.

+    virtual void copyNonAttributeProperties(const Element * /*source*/) {}

   The * should be next to the type.

-    class ResourceRequest;
+    struct ResourceRequest;

   We like to keep forward declarations of classes and structs separate, so
just put this declaration below all the class declarations, with an empty line
in between.

 private:
-    DeprecatedStringData(const DeprecatedStringData &);

   Why was this removed? It's used to make sure that DeprecatedStringData can't
be copied (although really DeprecatedStringData should just inherit from
Noncopyable).

+#if PLATFORM(MAC)
     String fileButtonNoFileSelectedLabel();
+#endif

   So far we've only #ifdef'd out strings that pertain to platform-specific
features. "No file selected" is not a platform-specific feature, so I don't
think we should add this #if.

+    unsigned short outlineSize() const { return (unsigned short)max(0,
outlineWidth() + outlineOffset()); }

   static_cast would be better here.

+#include "FrameLoader.h"  // Need to give the compiler the right size of a
+			   // FramePolicyFunction so the stack layout doesn't
+			   // get hosed through functions which take one

   I think a comment at the end of the list of #includes would be better than
having it inline like this.

   r- for now until the above are addressed. We'll also need to make sure that
these changes don't break other platforms.