Summary: | [DRT] LTC:: counterValueForElementById() could be moved to Internals | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||||
Component: | Tools / Tests | Assignee: | Hajime Morrita <morrita> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, cmarcelo, darin, dglazkov, fishd, gustavo, gyuyoung.kim, jamesr, kaustubh.ra, pnormand, rakuco, tkent, tkent+wkapi, vivekgalatage, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 87284, 88851 | ||||||||||||||||||
Attachments: |
|
Description
Hajime Morrita
2012-04-19 16:58:19 PDT
Hi Morrita, I was working on this for sometime. Can I upload the patch for same? (In reply to comment #1) > Hi Morrita, I was working on this for sometime. Can I upload the patch for same? Sure! Created attachment 138458 [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 138458 [details] Patch Attachment 138458 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12526077 Comment on attachment 138458 [details] Patch Attachment 138458 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12528078 Comment on attachment 138458 [details]
Patch
Chromium WebKit API change looks ok.
Comment on attachment 138458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138458&action=review > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:-88 > -String DumpRenderTreeSupportEfl::counterValueByElementId(const Evas_Object* ewkFrame, const char* elementId) Please remove the method declaration from the header as well. Comment on attachment 138458 [details] Patch Attachment 138458 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12530221 Created attachment 140520 [details]
Patch
Comment on attachment 140520 [details] Patch Attachment 140520 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12648035 Comment on attachment 140520 [details] Patch Attachment 140520 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12634990 Does anyone know where i need to add symbols for win & cr-linux port? I did added in symbols.filter for gtk port and it fixed the gtk build break issue. Comment on attachment 140520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140520&action=review Thanks for taking this on! > Source/WebCore/testing/Internals.cpp:1011 > +String Internals::counterValueForElementById(String elementId) This is not what we should do. Instead we should make an internals function named counterValue that takes an element. The only reason the old version worked with an ID was that it was easier to do it that in DumpRenderTree at the time it was done. Thanks Darin for the review. So, as per your suggestion, we should be using API counterValueForElement in the layout test cases as well? correct me if wrong. (In reply to comment #15) > Thanks Darin for the review. So, as per your suggestion, we should be using API counterValueForElement in the layout test cases as well? correct me if wrong. Here’s how I suggest it work in JavaScript: var counterValue = internals.counterValue(element); No need for “ForElement” in the function name, nor to involve IDs at all. For existing tests where it’s done by ID we can clean up the test or just do a mechanical change like this: var counterValue = internals.counterValue(document.getElementById(elementId)); Does that make sense? Created attachment 146548 [details]
Updated patch
After long time finally corrected the patch as per review comments. Uploading for review. Darin, can you review? Thanks.
Comment on attachment 146548 [details] Updated patch Attachment 146548 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12919502 Comment on attachment 146548 [details] Updated patch Attachment 146548 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12910644 (In reply to comment #19) > (From update of attachment 146548 [details]) > Attachment 146548 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/12910644 To avoid build break on win port, you have to add a symbol to Source/WebKit2/win/WebKit2.def. Created attachment 146804 [details]
Updated patch
Fixed build break for chromium & win
Comment on attachment 146804 [details] Updated patch Attachment 146804 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12944166 The code itself looks good. I'd r+ once bots become green and if you cannot catch Darin. Comment on attachment 146804 [details] Updated patch Attachment 146804 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12944190 Created attachment 146828 [details]
Updated patch
Fixed build break for win
Created attachment 146831 [details]
cr_linux_build_brk patch
Chromium - Webkit glue patch for cr-linux build break
Comment on attachment 146828 [details] Updated patch Attachment 146828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12940306 Corresponding chromium issue - https://chromiumcodereview.appspot.com/10546100/ Created attachment 147005 [details]
Updated patch
Removed Chromium API for now. Will be removed once cr patch is checked in.
Comment on attachment 147005 [details]
Updated patch
r=me. Please wait until all bots become green.
Committed r120054: <http://trac.webkit.org/changeset/120054> |