Bug 88326 – [BlackBerry] [DRT] DRT should be able to run interactively and support multiple processes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xiaobo Wang]

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.

[Xiaobo Wang]

Created attachment 145995 [details]
Git patch

[Ming Xie]

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?

[Rob Buis]

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.

[Xiaobo Wang]

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

[Xiaobo Wang]

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

[Xiaobo Wang]

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

[Xiaobo Wang]

Created attachment 149880 [details]
updated

Moving PPS code to platform

[Ming Xie]

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

[Xiaobo Wang]

Created attachment 149908 [details]
updated

Changing "#EOF" to "#DONE" for end of test.

[Rob Buis]

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?

[Xiaobo Wang]

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.

[Xiaobo Wang]

Created attachment 150139 [details]
updated

Last patch is doesn't contains the update described!

[Rob Buis]

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?

[Xiaobo Wang]

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

[Rob Buis]

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

[WebKit Review Bot]

Comment on attachment 150139 [details]
updated

Clearing flags on attachment: 150139

Committed r121723: <http://trac.webkit.org/changeset/121723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.