Bug 92091

Created attachment 154016 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

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

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.

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.

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

Created attachment 154029 [details] Patch

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

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

Created attachment 154311 [details] Patch

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 > +2012-07-24 Kaustubh Atrawalkar <kaustubh@motorola.com> 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.

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

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.

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.

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.

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

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

Reopening to attach new patch.

Created attachment 154603 [details] Patch

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

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

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

Need to fix chromium patch for the same.

Created attachment 155225 [details] Cr Patch Trying once again!

Comment on attachment 155225 [details] Cr Patch Clearing flags on attachment: 155225 Committed r124011: <http://trac.webkit.org/changeset/124011>

All reviewed patches have been landed. Closing bug.