[webkit-reviews] review denied: [Bug 92091] [DRT] LTC:: pageNumberForElementById() could be moved to Internals : [Attachment 154029] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 92091: [DRT] LTC:: pageNumberForElementById() could be moved to Internals
https://bugs.webkit.org/show_bug.cgi?id=92091

Attachment 154029: Patch
https://bugs.webkit.org/attachment.cgi?id=154029&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154029&action=review


This is very close, but I'd like to iterate on this patch once more.

> Source/WebCore/testing/Internals.h:56
> +#define MAX_PAGE_WIDTH  800
> +#define MAX_PAGE_HEIGTH 600

Generally, we don't use #define for constants.	Instead, we use the "const"
keyword.

> Source/WebCore/testing/Internals.h:200
> +    int pageNumber(Element* element) { return pageNumber(element,
MAX_PAGE_WIDTH, MAX_PAGE_HEIGTH); }
> +    int pageNumber(Element* element, float pageWidth) { return
pageNumber(element, MAX_PAGE_WIDTH, MAX_PAGE_HEIGTH); }
> +    int pageNumber(Element*, float, float);

The compiler can do this work for you automatically:

int pageNumber(Element*, float pageWidth = 800, float pageHeight = 600);

> Source/WebKit/win/Interfaces/IWebFramePrivate.idl:-101
> -    HRESULT pageNumberForElementById([in] BSTR id, [in] float
pageWidthInPixels, [in] float pageHeightInPixels, [out, retval] int*
pageNumber);

I'm not sure if we need to keep binary compatibility for IWebFramePrivate.idl. 
To be on the same side, I would leave this in the IDL and just have an empty
implementation with a comment.

> Source/autotools/symbols.filter:56
>
+_ZN7WebCore12PrintContext20pageNumberForElementEPNS_7ElementERKNS_9FloatSizeE;


I think we need something similar to this for the apple-mac (or maybe the
apple-win build).