Bug 84718 – [EFL][DRT] LayoutTestController addUserScript implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Mikhail Pozdnyakov]

EFL's LayoutTestController needs addUserScript implementation in order to unskip 'userscripts' tests section.

[Mikhail Pozdnyakov]

Created attachment 138670 [details]
LayoutTestController addUserScript implementation

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138670 [details]
LayoutTestController addUserScript implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=138670&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:662
> +    DumpRenderTreeSupportEfl::addUserScript(browser->mainView(), source->ustring().utf8().data(), runAtStart, allFrames);

Please create the String object here instead of passing a char* around and thus avoid an unneeded conversion.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138670 [details]
LayoutTestController addUserScript implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=138670&action=review

> LayoutTests/platform/efl/Skipped:496
> +# To be investigated

I believe there already is a category for 'tests that need investigation', please don't duplicate it.

> LayoutTests/platform/efl/Skipped:500
> +# Unexpected flakiness

Ditto.

[Mikhail Pozdnyakov]

Created attachment 138764 [details]
LayoutTestController addUserScript implementation

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 138670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138670&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:662
> > +    DumpRenderTreeSupportEfl::addUserScript(browser->mainView(), source->ustring().utf8().data(), runAtStart, allFrames);
> 
> Please create the String object here instead of passing a char* around and thus avoid an unneeded conversion.

Thanks for review! Fixed.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 138670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138670&action=review
> 
> > LayoutTests/platform/efl/Skipped:496
> > +# To be investigated
> 
> I believe there already is a category for 'tests that need investigation', please don't duplicate it.
> 
> > LayoutTests/platform/efl/Skipped:500
> > +# Unexpected flakiness
> 
> Ditto.

Fixed.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138764 [details]
LayoutTestController addUserScript implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=138764&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:662
> +    DumpRenderTreeSupportEfl::addUserScript(browser->mainView(), WTF::String(source->characters(), source->length()), runAtStart, allFrames);

We generally create the String object by passing the StringImpl* returned by UString directly:
  String(source->ustring().impl())

[Mikhail Pozdnyakov]

Created attachment 138863 [details]
LayoutTestController addUserScript implementation

[Mikhail Pozdnyakov]

(In reply to comment #7)
> (From update of attachment 138764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138764&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:662
> > +    DumpRenderTreeSupportEfl::addUserScript(browser->mainView(), WTF::String(source->characters(), source->length()), runAtStart, allFrames);
> 
> We generally create the String object by passing the StringImpl* returned by UString directly:
>   String(source->ustring().impl())

Fixed.
Actually I see that WTF::String(source->characters(), source->length()) construction is used in LayoutTestControllerEFL:queueLoad(), do you think it also should be corrected same way?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138863 [details]
LayoutTestController addUserScript implementation

Looks fine.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #9)
> Actually I see that WTF::String(source->characters(), source->length()) construction is used in LayoutTestControllerEFL:queueLoad(), do you think it also should be corrected same way?

I'll tackle that in my bug 84634, thanks for the pointer.

[WebKit Review Bot]

Comment on attachment 138863 [details]
LayoutTestController addUserScript implementation

Rejecting attachment 138863 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
eeded at 71 with fuzz 2.
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp
Hunk #1 succeeded at 251 with fuzz 1 (offset 38 lines).
patching file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp
Hunk #1 succeeded at 694 with fuzz 1 (offset 37 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Antonio Go..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12648291

[Gyuyoung Kim]

Comment on attachment 138863 [details]
LayoutTestController addUserScript implementation

Need to rebase.

[Mikhail Pozdnyakov]

Created attachment 140877 [details]
to be landed.

[Gyuyoung Kim]

Comment on attachment 140877 [details]
to be landed.

Attachment 140877 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12644657

[Mikhail Pozdnyakov]

Created attachment 140884 [details]
to be landed.

[WebKit Review Bot]

Comment on attachment 140884 [details]
to be landed.

Clearing flags on attachment: 140884

Committed r116506: <http://trac.webkit.org/changeset/116506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.