Bug 84406 – [DRT] LTC:: counterValueForElementById() could be moved to Internals

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

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

[Kaustubh Atrawalkar]

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

[Hajime Morrita]

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

[Kaustubh Atrawalkar]

Created attachment 138458 [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.

[Philippe Normand]

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 on attachment 138458 [details]
Patch

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

[Kent Tamura]

Comment on attachment 138458 [details]
Patch

Chromium WebKit API change looks ok.

[Raphael Kubo da Costa (:rakuco)]

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 on attachment 138458 [details]
Patch

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

[Kaustubh Atrawalkar]

Created attachment 140520 [details]
Patch

[Build Bot]

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 on attachment 140520 [details]
Patch

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

[Kaustubh Atrawalkar]

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 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]

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]

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

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 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 on attachment 146548 [details]
Updated patch

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

[Gyuyoung Kim]

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

Created attachment 146804 [details]
Updated patch

Fixed build break for chromium & win

[Build Bot]

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]

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

[WebKit Review Bot]

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]

Created attachment 146828 [details]
Updated patch

Fixed build break for win

[Kaustubh Atrawalkar]

Created attachment 146831 [details]
cr_linux_build_brk patch

Chromium - Webkit glue patch for cr-linux build break

[WebKit Review Bot]

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]

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

[Kaustubh Atrawalkar]

Created attachment 147005 [details]
Updated patch

Removed Chromium API for now. Will be removed once cr patch is checked in.

[Hajime Morrita]

Comment on attachment 147005 [details]
Updated patch

r=me. Please wait until all bots become green.

[Kaustubh Atrawalkar]

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