Bug 88326 - [BlackBerry] [DRT] DRT should be able to run interactively and support multiple processes
Summary: [BlackBerry] [DRT] DRT should be able to run interactively and support multip...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-05 05:53 PDT by Xiaobo Wang
Modified: 2012-07-02 20:24 PDT (History)
4 users (show)

See Also:


Attachments
Git patch (13.80 KB, patch)
2012-06-06 05:20 PDT, Xiaobo Wang
rwlbuis: review-
Details | Formatted Diff | Diff
git patch updated for comments from Ming and Rob (14.16 KB, patch)
2012-06-15 00:39 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
updated (11.14 KB, patch)
2012-06-27 23:23 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
updated (11.22 KB, patch)
2012-06-28 02:44 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
updated (11.22 KB, patch)
2012-06-28 21:06 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff
updated (11.38 KB, patch)
2012-06-29 04:51 PDT, Xiaobo Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaobo Wang 2012-06-05 05:53:37 PDT
The current DumpRenderTree is designed to run in batch, and only one process can be run. So we can't take advantage of multiple process feature of NRWT. Need to update DumpRenderTree to have it accept input from NRWT, run it immediately and provide text/image/audio output to NRWT.
Comment 1 Xiaobo Wang 2012-06-06 05:20:37 PDT
Created attachment 145995 [details]
Git patch
Comment 2 Ming Xie 2012-06-13 08:25:46 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=145995&action=review

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:170
> +    m_doneFile = sdcardPath + "/done" + workerNumber;

Wouldn't use pid be better? We don't need to check this env var if we use getpid()

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:242
> +//        doneDrt();

code clean up?

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:328
> +void DumpRenderTree::ensurePPS()

Can we make this as a bool function, so we can handle differently if PPS isn't ready?

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:336
> +        currentPath += ("/" + nodes[i]);

Sorry, I'm a bit confused. what's the difference between the ppsPathString and currentPath here?

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:376
> +                m_refTests.append(testFile);

isn't refTests being removed from here?
Comment 3 Rob Buis 2012-06-13 11:55:53 PDT
Comment on attachment 145995 [details]
Git patch

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

Looks good, can maybe be cleaned up some more, also judging from Ming's comment.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:344
> +                S_IROTH | S_IWOTH);

I think alignment could be a bit better here, more can be put onto one line.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:354
> +    const int lenTestFile = 11;

Better to not hardcode this I think.
Comment 4 Xiaobo Wang 2012-06-15 00:39:38 PDT
Created attachment 147760 [details]
git patch updated for comments from Ming and Rob

1. Rename m_refTests to m_bufferedTests to have it more meaningful.
2. Clean up unused code.
3. Update function ensurePPS to return bool, and check it in runTests.
4. Replace hard coded string length with calculated value.
5. Code format improvement.

Why can't I set review/commit-queue flags to '?'
Comment 5 Xiaobo Wang 2012-06-15 01:01:43 PDT
(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=145995&action=review
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:170
> > +    m_doneFile = sdcardPath + "/done" + workerNumber;
> 
> Wouldn't use pid be better? We don't need to check this env var if we use getpid()
>
Yes we can use pid technically. But I think work number is a better choice.
1. To be consistent with NRWT. NRWT doesn't know the pid of the remote process on the device, it only knows the worker number. Worker number is a logical concept, stands for a worker. Pid of the launcher is more like its implementation, the value is meaningless to NRWT. 
2. To be meaningful to testers, making it easier to debug.
 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:242
> > +//        doneDrt();
> 
> code clean up?
>
Updated.

> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:328
> > +void DumpRenderTree::ensurePPS()
> 
> Can we make this as a bool function, so we can handle differently if PPS isn't ready?
>
Updated.

> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:336
> > +        currentPath += ("/" + nodes[i]);
> 
> Sorry, I'm a bit confused. what's the difference between the ppsPathString and currentPath here?
> 
Here I'm trying to mkdir recursively from root to the final path. It's just like "mkdir -p <path>".

> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:376
> > +                m_refTests.append(testFile);
> 
> isn't refTests being removed from here?
Yes, good catch. I was too lazy to rename it to a more meaningful one. Changed to m_bufferedTests. This vector is used to accept input from PPS while a test is being run.
Comment 6 Xiaobo Wang 2012-06-15 01:03:16 PDT
(In reply to comment #3)
> (From update of attachment 145995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145995&action=review
> 
> Looks good, can maybe be cleaned up some more, also judging from Ming's comment.
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:344
> > +                S_IROTH | S_IWOTH);
> 
> I think alignment could be a bit better here, more can be put onto one line.
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:354
> > +    const int lenTestFile = 11;
> 
> Better to not hardcode this I think.

Good suggestions, updated in the new patch. Thanks Rob.
Comment 7 Xiaobo Wang 2012-06-27 23:23:54 PDT
Created attachment 149880 [details]
updated

Moving PPS code to platform
Comment 8 Ming Xie 2012-06-28 00:05:10 PDT
(In reply to comment #7)
> Created an attachment (id=149880) [details]
> updated
> 
> Moving PPS code to platform

This patch looks good to me. Maybe rename ensurePPS to isPpsReady() to make is more clear?

Rob, do you have other suggestions?
Comment 9 Xiaobo Wang 2012-06-28 02:44:19 PDT
Created attachment 149908 [details]
updated

Changing "#EOF" to "#DONE" for end of test.
Comment 10 Rob Buis 2012-06-28 18:50:30 PDT
Comment on attachment 149908 [details]
updated

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

Looks good, still some questions...

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:203
> +    unlink(getDrtPpsPath().c_str());

We used PPS instead of Pps everywhere I think.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:323
> +    if (!ensurePPS())

Do we log it if this happens?
Comment 11 Xiaobo Wang 2012-06-28 21:06:37 PDT
Created attachment 150079 [details]
updated

1. Rename getDrtPpsPath() to getPPSPath().
2. Add error logging and test done notification if ensurePPS() failed. There's no error logging in other places in DumpRenderTree.cpp, so I just write the log to stderr.
Comment 12 Xiaobo Wang 2012-06-29 04:51:03 PDT
Created attachment 150139 [details]
updated

Last patch is doesn't contains the update described!
Comment 13 Rob Buis 2012-06-29 04:54:18 PDT
Comment on attachment 150139 [details]
updated

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

Looks good.

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:324
> +        fprintf(stderr, "Failed to open PPS file '%s', error=%d\n", getPPSPath().c_str(), errno);

Is this our best log method?
Comment 14 Xiaobo Wang 2012-07-01 19:23:36 PDT
(In reply to comment #13)
> (From update of attachment 150139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150139&action=review
> 
> Looks good.
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:324
> > +        fprintf(stderr, "Failed to open PPS file '%s', error=%d\n", getPPSPath().c_str(), errno);
> 
> Is this our best log method?

I think so Rob. There's no app level log for torch-launcher. In main.cpp for torch-launcher, we also log errors in this way.
Comment 15 Rob Buis 2012-07-02 05:06:32 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 150139 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=150139&action=review
> > 
> > Looks good.
> > 
> > > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:324
> > > +        fprintf(stderr, "Failed to open PPS file '%s', error=%d\n", getPPSPath().c_str(), errno);
> > 
> > Is this our best log method?
> 
> I think so Rob. There's no app level log for torch-launcher. In main.cpp for torch-launcher, we also log errors in this way.

Ok, then this patch can go in :) I think Charles, George or Leo can cq+.
Comment 16 WebKit Review Bot 2012-07-02 20:23:59 PDT
Comment on attachment 150139 [details]
updated

Clearing flags on attachment: 150139

Committed r121723: <http://trac.webkit.org/changeset/121723>
Comment 17 WebKit Review Bot 2012-07-02 20:24:04 PDT
All reviewed patches have been landed.  Closing bug.