Summary: | [EFL][DRT] LayoutTestController addUserScript implementation | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||
Component: | WebKit EFL | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 84792 | ||||||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2012-04-24 06:48:53 PDT
Created attachment 138670 [details]
LayoutTestController addUserScript implementation
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. 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. Created attachment 138764 [details]
LayoutTestController addUserScript implementation
(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. (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. 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()) Created attachment 138863 [details]
LayoutTestController addUserScript implementation
(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? Comment on attachment 138863 [details]
LayoutTestController addUserScript implementation
Looks fine.
(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. 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 Comment on attachment 138863 [details]
LayoutTestController addUserScript implementation
Need to rebase.
Created attachment 140877 [details]
to be landed.
Comment on attachment 140877 [details] to be landed. Attachment 140877 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12644657 Created attachment 140884 [details]
to be landed.
Comment on attachment 140884 [details] to be landed. Clearing flags on attachment: 140884 Committed r116506: <http://trac.webkit.org/changeset/116506> All reviewed patches have been landed. Closing bug. |