Bug 92091 - [DRT] LTC:: pageNumberForElementById() could be moved to Internals
Summary: [DRT] LTC:: pageNumberForElementById() could be moved to Internals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kaustubh Atrawalkar
URL:
Keywords:
Depends on:
Blocks: 87284
  Show dependency treegraph
 
Reported: 2012-07-24 03:19 PDT by Kaustubh Atrawalkar
Modified: 2012-07-30 03:34 PDT (History)
20 users (show)

See Also:


Attachments
Patch (41.52 KB, patch)
2012-07-24 04:24 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch (39.76 KB, patch)
2012-07-24 05:47 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch (37.54 KB, patch)
2012-07-25 03:20 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2012-07-26 03:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Cr Patch (3.72 KB, patch)
2012-07-30 00:01 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff
Cr Patch (3.72 KB, patch)
2012-07-30 00:32 PDT, Kaustubh Atrawalkar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaustubh Atrawalkar 2012-07-24 03:19:15 PDT
This one is a printing test specific API and can be moved to Internals.
Comment 1 Kaustubh Atrawalkar 2012-07-24 04:24:11 PDT
Created attachment 154016 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-24 04:26:27 PDT
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 3 WebKit Review Bot 2012-07-24 04:39:38 PDT
Comment on attachment 154016 [details]
Patch

Attachment 154016 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13326413
Comment 4 Hajime Morrita 2012-07-24 04:41:32 PDT
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 5 Hajime Morrita 2012-07-24 04:42:56 PDT
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.
Comment 6 Kaustubh Atrawalkar 2012-07-24 05:00:13 PDT
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).
Comment 7 Kaustubh Atrawalkar 2012-07-24 05:47:10 PDT
Created attachment 154029 [details]
Patch
Comment 8 Kaustubh Atrawalkar 2012-07-24 06:16:15 PDT
Chromium patch uploaded at - http://codereview.chromium.org/10800093/ Please review the same also. Thanks :)
Comment 9 Adam Barth 2012-07-24 10:38:47 PDT
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).
Comment 10 Kaustubh Atrawalkar 2012-07-25 03:20:06 PDT
Created attachment 154311 [details]
Patch
Comment 11 Adam Barth 2012-07-25 08:40:07 PDT
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.
Comment 12 Adam Barth 2012-07-25 08:42:04 PDT
Perhaps Timothy Hatcher knows whether Source/WebKit/mac/Misc/WebCoreStatistics.h	is a public header.
Comment 13 Adam Barth 2012-07-25 08:43:46 PDT
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 14 Timothy Hatcher 2012-07-25 08:47:41 PDT
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 15 Adam Barth 2012-07-25 09:03:28 PDT
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.
Comment 16 Kaustubh Atrawalkar 2012-07-26 00:06:31 PDT
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 :)
Comment 17 Kaustubh Atrawalkar 2012-07-26 00:08:18 PDT
Committed r123711: <http://trac.webkit.org/changeset/123711>
Comment 18 Gyuyoung Kim 2012-07-26 03:31:33 PDT
Reopening to attach new patch.
Comment 19 Gyuyoung Kim 2012-07-26 03:31:43 PDT
Created attachment 154603 [details]
Patch
Comment 20 Gyuyoung Kim 2012-07-26 03:36:18 PDT
Committed r123725: <http://trac.webkit.org/changeset/123725>
Comment 21 Kaustubh Atrawalkar 2012-07-30 00:01:16 PDT
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 22 WebKit Review Bot 2012-07-30 00:13:34 PDT
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).
Comment 23 Kaustubh Atrawalkar 2012-07-30 00:21:07 PDT
Need to fix chromium patch for the same.
Comment 24 Kaustubh Atrawalkar 2012-07-30 00:32:29 PDT
Created attachment 155225 [details]
Cr Patch

Trying once again!
Comment 25 WebKit Review Bot 2012-07-30 03:33:59 PDT
Comment on attachment 155225 [details]
Cr Patch

Clearing flags on attachment: 155225

Committed r124011: <http://trac.webkit.org/changeset/124011>
Comment 26 WebKit Review Bot 2012-07-30 03:34:08 PDT
All reviewed patches have been landed.  Closing bug.