Bug 92091 – [DRT] LTC:: pageNumberForElementById() could be moved to Internals

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kaustubh Atrawalkar]

This one is a printing test specific API and can be moved to Internals.

[Kaustubh Atrawalkar]

Created attachment 154016 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 154016 [details]
Patch

Attachment 154016 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13326413

[Hajime Morrita]

Comment on attachment 154016 [details]
Patch

The code looks good. I'm not confident whether this API isn't used outside DRT, especially Mac port.
Do you know the revision where the API was added? If it indicates that the API is only for test, it is good news.
Or you could ask some Apple folks if it is used in production or not.

[Hajime Morrita]

Comment on attachment 154016 [details]
Patch

r- due to cr-linux redness. You keep the API for now and romove it after chromium side change.

[Kaustubh Atrawalkar]

The API was added in https://trac.webkit.org/changeset/54205. 
After that only remaining port specific LTC implementations were added (https://trac.webkit.org/search?changeset=on&wiki=on&q=pageNumberForElementById).

[Kaustubh Atrawalkar]

Created attachment 154029 [details]
Patch

[Kaustubh Atrawalkar]

Chromium patch uploaded at - http://codereview.chromium.org/10800093/ Please review the same also. Thanks :)

[Adam Barth]

Comment on attachment 154029 [details]
Patch

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).

[Kaustubh Atrawalkar]

Created attachment 154311 [details]
Patch

[Adam Barth]

Comment on attachment 154311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154311&action=review

One question below.

> Source/WebKit/gtk/ChangeLog:14
> [phone]  Kaustubh Atrawalkar  <[e-mail]>

This ChangeLog got corrupted.

> Source/WebKit/mac/Misc/WebCoreStatistics.h:-89
> -- (int)pageNumberForElement:(DOMElement*)element:(float)pageWidthInPixels:(float)pageHeightInPixels;

I don't know whether this is a public API.

[Adam Barth]

Perhaps Timothy Hatcher knows whether Source/WebKit/mac/Misc/WebCoreStatistics.h	is a public header.

[Adam Barth]

In #webkit, we determined that it was SPI.

I would be on the safe side and leave it.  If someone from Apple says it's ok to remove, we can remove it.

[Timothy Hatcher]

Comment on attachment 154311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154311&action=review

>> Source/WebKit/mac/Misc/WebCoreStatistics.h:-89
>> -- (int)pageNumberForElement:(DOMElement*)element:(float)pageWidthInPixels:(float)pageHeightInPixels;
> 
> I don't know whether this is a public API.

Yes, please add this back. There might be internal Apple clients.

[Adam Barth]

Comment on attachment 154311 [details]
Patch

I think a more precise state for this patch would be r+/cq-.  Please revert the changes to Source/WebKit/mac before landing.

[Kaustubh Atrawalkar]

Thanks Adam. I have reverted the mac changes. Added comment in win/WebFrame.cpp for removing the function later. Landing the patch soon. Thanks for the review :)

[Kaustubh Atrawalkar]

Committed r123711: <http://trac.webkit.org/changeset/123711>

[Gyuyoung Kim]

Reopening to attach new patch.

[Gyuyoung Kim]

Created attachment 154603 [details]
Patch

[Gyuyoung Kim]

Committed r123725: <http://trac.webkit.org/changeset/123725>

[Kaustubh Atrawalkar]

Created attachment 155219 [details]
Cr Patch

Patch for removing cr specific code after merging cr glue patch - http://src.chromium.org/viewvc/chrome?view=rev&revision=148610

[WebKit Review Bot]

Comment on attachment 155219 [details]
Cr Patch

Cleared review? from attachment 155219 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Kaustubh Atrawalkar]

Need to fix chromium patch for the same.

[Kaustubh Atrawalkar]

Created attachment 155225 [details]
Cr Patch

Trying once again!

[WebKit Review Bot]

Comment on attachment 155225 [details]
Cr Patch

Clearing flags on attachment: 155225

Committed r124011: <http://trac.webkit.org/changeset/124011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.