Bug 84406

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

87284, 88851

Attachments:

Description	Flags
Patch	none
Patch	none
Updated patch	none
Updated patch	none
Updated patch	none
cr_linux_build_brk patch	none
Updated patch	morrita: review+

Hajime Morrita

Reported 2012-04-19 16:58:19 PDT

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

Attachments
Patch (81.36 KB, patch) 2012-04-23 16:49 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Patch (82.83 KB, patch) 2012-05-07 06:34 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Updated patch (81.70 KB, patch) 2012-06-08 06:11 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Updated patch (84.75 KB, patch) 2012-06-11 01:00 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Updated patch (84.76 KB, patch) 2012-06-11 04:21 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
cr_linux_build_brk patch (1.54 KB, text/plain) 2012-06-11 04:30 PDT, Kaustubh Atrawalkar	no flags	Details
Updated patch (81.64 KB, patch) 2012-06-11 21:09 PDT, Kaustubh Atrawalkar	morrita: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Kaustubh Atrawalkar

Comment 1 2012-04-23 14:18:52 PDT

Hi Morrita, I was working on this for sometime. Can I upload the patch for same?

Hajime Morrita

Comment 2 2012-04-23 15:49:37 PDT

(In reply to comment #1) > Hi Morrita, I was working on this for sometime. Can I upload the patch for same? Sure!

Kaustubh Atrawalkar

Comment 3 2012-04-23 16:49:26 PDT

Created attachment 138458 [details] Patch

WebKit Review Bot

Comment 4 2012-04-23 16:53:58 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.

Philippe Normand

Comment 5 2012-04-23 17:01:11 PDT

Comment on attachment 138458 [details] Patch Attachment 138458 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12526077

Build Bot

Comment 6 2012-04-23 17:13:43 PDT

Comment on attachment 138458 [details] Patch Attachment 138458 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12528078

Kent Tamura

Comment 7 2012-04-23 17:40:29 PDT

Comment on attachment 138458 [details] Patch Chromium WebKit API change looks ok.

Raphael Kubo da Costa (:rakuco)

Comment 8 2012-04-23 18:30:39 PDT

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.

WebKit Review Bot

Comment 9 2012-04-24 06:19:03 PDT

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

Kaustubh Atrawalkar

Comment 10 2012-05-07 06:34:05 PDT

Created attachment 140520 [details] Patch

Build Bot

Comment 11 2012-05-07 07:07:13 PDT

Comment on attachment 140520 [details] Patch Attachment 140520 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12648035

WebKit Review Bot

Comment 12 2012-05-07 07:07:47 PDT

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

Kaustubh Atrawalkar

Comment 13 2012-05-07 07:14:42 PDT

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.

Darin Adler

Comment 14 2012-05-07 10:33:36 PDT

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.

Kaustubh Atrawalkar

Comment 15 2012-05-08 02:16:53 PDT

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.

Darin Adler

Comment 16 2012-05-08 13:14:07 PDT

(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?

Kaustubh Atrawalkar

Comment 17 2012-06-08 06:11:27 PDT

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.

WebKit Review Bot

Comment 18 2012-06-08 06:33:46 PDT

Comment on attachment 146548 [details] Updated patch Attachment 146548 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12919502

Build Bot

Comment 19 2012-06-08 07:43:57 PDT

Comment on attachment 146548 [details] Updated patch Attachment 146548 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12910644

Gyuyoung Kim

Comment 20 2012-06-09 04:04:19 PDT

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

Kaustubh Atrawalkar

Comment 21 2012-06-11 01:00:02 PDT

Created attachment 146804 [details] Updated patch Fixed build break for chromium & win

Build Bot

Comment 22 2012-06-11 01:30:28 PDT

Comment on attachment 146804 [details] Updated patch Attachment 146804 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12944166

Hajime Morrita

Comment 23 2012-06-11 02:17:33 PDT

The code itself looks good. I'd r+ once bots become green and if you cannot catch Darin.

WebKit Review Bot

Comment 24 2012-06-11 03:29:14 PDT

Comment on attachment 146804 [details] Updated patch Attachment 146804 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12944190

Kaustubh Atrawalkar

Comment 25 2012-06-11 04:21:36 PDT

Created attachment 146828 [details] Updated patch Fixed build break for win

Kaustubh Atrawalkar

Comment 26 2012-06-11 04:30:52 PDT

Created attachment 146831 [details] cr_linux_build_brk patch Chromium - Webkit glue patch for cr-linux build break

WebKit Review Bot

Comment 27 2012-06-11 05:20:48 PDT

Comment on attachment 146828 [details] Updated patch Attachment 146828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12940306

Kaustubh Atrawalkar

Comment 28 2012-06-11 20:27:30 PDT

Corresponding chromium issue - https://chromiumcodereview.appspot.com/10546100/

Kaustubh Atrawalkar

Comment 29 2012-06-11 21:09:50 PDT

Created attachment 147005 [details] Updated patch Removed Chromium API for now. Will be removed once cr patch is checked in.

Hajime Morrita

Comment 30 2012-06-11 22:31:06 PDT

Comment on attachment 147005 [details] Updated patch r=me. Please wait until all bots become green.

Kaustubh Atrawalkar

Comment 31 2012-06-12 03:53:45 PDT

Committed r120054: <http://trac.webkit.org/changeset/120054>

Note You need to log in before you can comment on or make changes to this bug.