Bug 105156 – [EFL][WK2] Process launcher uses system() for wrapping the WebProcess when using WEB_PROCESS_CMD_PREFIX

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

Using system() will keep one extra UIProcess (in the example bellow, MiniBrowser) waiting and execute the shell. It waste memory that we sometimes cannot afford when debugging in a mobile environment.

WEB_PROCESS_CMD_PREFIX="xterm -title WebProcess -e gdb --args" bin/MiniBrowser

1000      7681 10.2  1.1 286572 92612 pts/0    SLl+ 12:12   0:01 bin/MiniBrowser
1000      7685  0.0  0.5 242876 47124 pts/0    S+   12:12   0:00 bin/MiniBrowser
1000      7686  0.0  0.0   2240   544 pts/0    S+   12:12   0:00 sh -c xterm -title WebProcess -e gdb --args /opt/tmpsantos/projects/webkit-efl-2/
1000      7687  0.1  0.0  11496  5776 pts/0    S+   12:12   0:00 xterm -title WebProcess -e gdb --args /opt/tmpsantos/projects/webkit-efl-2/WebKit
1000      7695 87.3  5.9 1345076 491764 pts/4  Rs+  12:12   0:09 gdb --args /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Debug/bin/WebProcess
1000      7698  0.4  0.3 168344 32672 pts/4    t    12:12   0:00 /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Debug/bin/WebProcess 27

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 200299 [details]
Patch

Proposed patch

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #1)
> Created an attachment (id=200299) [details]
> Patch
> 
> Proposed patch

Oops, messed up the changelog. Will fix it when adjusting the patch as per the reviewers requests. Sorry.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 200383 [details]
Patch

Fixed changelog mess (I had previously removed trailing whitespaces in messages not related to this patch), and fixed also a typo in the changelog/commit message.

[Build Bot]

Comment on attachment 200383 [details]
Patch

Attachment 200383 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/268349

New failing tests:
fast/frames/crash-remove-iframe-during-object-beforeload.html

[Build Bot]

Created attachment 200386 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Mikhail Pozdnyakov]

Comment on attachment 200383 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] ProcessLauncher: do not handle WEB_PROCESS_CMD_PREFIX with system()

Seems that the actual bug has another title.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40
> +    ProcessExecArgs(String cmd)

const String&

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:46
> +        cmd.split(' ', args);

so you've composed this string before in ProcessLauncher::launchProcess(), and now you're decomposing it again..

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
> +    char* const* args() { return const_cast<char* const*>(m_argv); }

why not const char* ?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71
> +    Vector<CString> m_splitArgs;

why is that needed?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:98
> +    String fullCmd = String::fromUTF8(executablePath.data()) + ' ' + String::number(sockets[0]) + ' ' + String::fromUTF8(pluginPath.data());

I'd consider using of StringBuilder

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:103
> +    ProcessExecArgs execArgs(fullCmd);

why do you need a class here? seems a function would be enough

[Sergio Correia (qrwteyrutiyoup)]

Thanks for the review, Mikhail.

(In reply to comment #6)
> (From update of attachment 200383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200383&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] ProcessLauncher: do not handle WEB_PROCESS_CMD_PREFIX with system()
> 
> Seems that the actual bug has another title.
> 

Yeah, I thought I could change it and ask someone to rename the actual bug, but I will just make the title in the patch to be the same as the bug, instead.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40
> > +    ProcessExecArgs(String cmd)
> 
> const String&
> 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:46
> > +        cmd.split(' ', args);
> 
> so you've composed this string before in ProcessLauncher::launchProcess(), and now you're decomposing it again..
> 

Indeed. I will pass the possible prefix as a separate parameter and break only it. 

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
> > +    char* const* args() { return const_cast<char* const*>(m_argv); }
> 
> why not const char* ?
> 

I was matching the prototype of execvp():  int execvp(const char *file, char *const argv[]); 

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71
> > +    Vector<CString> m_splitArgs;
> 
> why is that needed?
> 

Because I was making the items in argv point to them, rather than copying them with something like strdup() - which would require me to free() them in the destructor -, and hence they should exist while argv itself exist.  

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:98
> > +    String fullCmd = String::fromUTF8(executablePath.data()) + ' ' + String::number(sockets[0]) + ' ' + String::fromUTF8(pluginPath.data());
> 
> I'd consider using of StringBuilder
>

I am going to take a look at StringBuilder, but probably if I send them as separate arguments, I will be able to avoid composing it here to decompose it in the class.
 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:103
> > +    ProcessExecArgs execArgs(fullCmd);
> 
> why do you need a class here? seems a function would be enough
>

It seemed simpler to handle the deallocation logic in a destructor, rather than having to do it manually.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 200414 [details]
Patch

Addressing Mikhail's comments: 1) fixed bug title; 2) not composing/decomposing command string; now we decompose the prefix only, and have the other parameters passed as different arguments; 3) not keeping another Vector<String>; now we copy the strings to m_argv with strdup and release them with free in the destructor.

[Mikhail Pozdnyakov]

Comment on attachment 200414 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38
> +class ProcessExecArgs {

Still think it should not be class, we never need having an object instance.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53
> +        for (i = 0, numArgs = prefixArgs.size(); i < numArgs; i++)

I'd put numArgs = prefixArgs.size() out of 'for()'

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:101
> +    String processCmdPrefix;

Seems this var is not needed, m_launchOptions.processCmdPrefix can be passed directly.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 201073 [details]
Patch

Not using a class anymore; we now have a function to create the array and another to deallocate it

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #9)
> (From update of attachment 200414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200414&action=review
> 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38
> > +class ProcessExecArgs {
> 
> Still think it should not be class, we never need having an object instance.
> 

Okay, I removed the class in the new patch.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53
> > +        for (i = 0, numArgs = prefixArgs.size(); i < numArgs; i++)
> 
> I'd put numArgs = prefixArgs.size() out of 'for()'
> 

Done.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:101
> > +    String processCmdPrefix;
> 
> Seems this var is not needed, m_launchOptions.processCmdPrefix can be passed directly.

m_launchOptions.processCmdPrefix only exists under #ifndef NDEBUG, so I think we need this variable, just like we need pluginPath, for instance. Anyway, please take a look at the updated version and let me know if you still think this variable is not needed.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 201345 [details]
Patch

Addressing feedback from Mikhail given on IRC: using a struct as a scope guard to handle the deallocation of the arguments array allocated to be used by execvp().

[Chris Dumez]

Comment on attachment 201345 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53
> +    for (i = 0; i < numArgs; i++) {

Nit: prefixed increment for style

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:54
> +        args[i] = new char[strlen(splitArgs[i].utf8().data()) + 1];

We should not call strlen here as the returned CString stores the size.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55
> +        strcpy(args[i], splitArgs[i].utf8().data());

You are calling utf8() twice, you should probably store the CString in a local variable

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71
> +            for (size_t i = 0; m_args[i]; i++)

++i

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109
> +    ArgsScopeGuard guard(args);

Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it.

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 201345 [details]
Patch

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

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53
>> +    for (i = 0; i < numArgs; i++) {
> 
> Nit: prefixed increment for style

Ah, okay, didn't know it was the preferred style. Thanks.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55
>> +        strcpy(args[i], splitArgs[i].utf8().data());
> 
> You are calling utf8() twice, you should probably store the CString in a local variable

Good point, it makes sense to store the CString and use it here and for the length. I will do that.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109
>> +    ArgsScopeGuard guard(args);
> 
> Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it.

Good question. I will look into OwnArrayPtr and see if I can find a way to use it here. Thanks for your review and suggestions.

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (From update of attachment 201345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201345&action=review
> 
> >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53
> >> +    for (i = 0; i < numArgs; i++) {
> > 
> > Nit: prefixed increment for style
> 
> Ah, okay, didn't know it was the preferred style. Thanks.
> 
> >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55
> >> +        strcpy(args[i], splitArgs[i].utf8().data());
> > 
> > You are calling utf8() twice, you should probably store the CString in a local variable
> 
> Good point, it makes sense to store the CString and use it here and for the length. I will do that.
> 
> >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109
> >> +    ArgsScopeGuard guard(args);
> > 
> > Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it.
> 
> Good question. I will look into OwnArrayPtr and see if I can find a way to use it here. Thanks for your review and suggestions.
I guess in order to use WTF OwnPtr, you just need to define deleteOwnedPtr(char* const* args), then createArgsArray could return PassOwnPtr.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 201462 [details]
Patch

1) using prefixed increment style; 2) storing the CString to avoid calling utf8() twice; 3) using OwnArrayPtr + deleteOwnedArrayPtr to take care of the deallocating the memory when the args variable goes out of scope.

[Chris Dumez]

Comment on attachment 201462 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38
> +template <> void deleteOwnedArrayPtr<char* const>(char* const* args)

Someone should confirm but shouldn't this be in wtf/ instead?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40
> +    if (args) {

We usually do the opposite in WebKit: Return early.

if (!arg)
    return;

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63
> +    char** args = new char*[splitArgs.size() + 1]; // extra room for null

We should probably adopt here instead of the return statement. Comment should start with a capital letter and end with a '.'.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:64
> +    size_t i, numArgs = splitArgs.size();

Having several statements on the same line is uncommon in WebKit.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:65
> +    for (i = 0; i < numArgs; ++i) {

i can be declared here: size_t i = 0

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68
> +        strcpy(args[i], param.data());

strncpy?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:104
> +    OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath);

Looks much better indeed.

[Mikhail Pozdnyakov]

Comment on attachment 201462 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
> +        args[i] = new char[param.length() + 1];

why '+1' ?

[Chris Dumez]

Comment on attachment 201462 [details]
Patch

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

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
>> +        args[i] = new char[param.length() + 1];
> 
> why '+1' ?

For the ending '\0' character I assume.
"The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')."

strlen() is called inside CString.

[Mikhail Pozdnyakov]

Comment on attachment 201462 [details]
Patch

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

>>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
>>> +        args[i] = new char[param.length() + 1];
>> 
>> why '+1' ?
> 
> For the ending '\0' character I assume.
> "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')."
> 
> strlen() is called inside CString.

than we should probably set last byte to null.

[Chris Dumez]

Comment on attachment 201462 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
>>>> +        args[i] = new char[param.length() + 1];
>>> 
>>> why '+1' ?
>> 
>> For the ending '\0' character I assume.
>> "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')."
>> 
>> strlen() is called inside CString.
> 
> than we should probably set last byte to null.

Agreed, would be safer indeed.

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 201462 [details]
Patch

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

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40
>> +    if (args) {
> 
> We usually do the opposite in WebKit: Return early.
> 
> if (!arg)
>     return;

Noted.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63
>> +    char** args = new char*[splitArgs.size() + 1]; // extra room for null
> 
> We should probably adopt here instead of the return statement. Comment should start with a capital letter and end with a '.'.

Okay, I will update it to adopt here.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:65
>> +    for (i = 0; i < numArgs; ++i) {
> 
> i can be declared here: size_t i = 0

Will do.

>>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
>>> +        args[i] = new char[param.length() + 1];
>> 
>> why '+1' ?
> 
> For the ending '\0' character I assume.
> "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')."
> 
> strlen() is called inside CString.

Yes, it's for the ending '\0'. 
From CString.h: // The data is implicitly allocated 1 character longer than length(), as it is zero-terminated.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68
>> +        strcpy(args[i], param.data());
> 
> strncpy?

Agreed.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 201709 [details]
Patch

Updated as per Christophe's comments

[Mikhail Pozdnyakov]

Comment on attachment 201709 [details]
Patch

lgtm

[Chris Dumez]

Comment on attachment 201709 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:37
> +// FIXME: Should be moved to wtf?

Benjamin, what do you think about this one?
I think this is generic and potentially reusable.

[Benjamin Poulain]

Comment on attachment 201709 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:49
> +namespace WTF {
> +template <> void deleteOwnedArrayPtr<char* const>(char* const* args)
> +{
> +    if (!args)
> +        return;
> +
> +    for (size_t i = 0; args[i]; ++i)
> +        delete[] args[i];
> +    delete[] args;
> +}
> +} // namespace WTF
> +

This should not be moved to WTF, this should not exist at all.

You can change behaviors of WebKit ADTs with traits and template arguments. But certainly _NOT_ by doing template specialization of internal structure.

By doing this, you are breaking the semantic of OwnArrayPtr. Anyone who modify this file later (and any reviewer checking patch) would assume the common well defined behavior. This would introduce subtle bugs and make reviews impossible.

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52
> +static PassOwnArrayPtr<char* const> createArgsArray(const String& prefix, const String& executablePath, const String& socket, const String& pluginPath)

Why not just return a Vector<OwnArrayPtr<char* const>> here?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:106
> +    OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath);

It looks like this should be in the if (!pid) branch.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #26)
> (From update of attachment 201709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201709&action=review
> 
> 
> This should not be moved to WTF, this should not exist at all.
> 
> You can change behaviors of WebKit ADTs with traits and template arguments. But certainly _NOT_ by doing template specialization of internal structure.
> 
> By doing this, you are breaking the semantic of OwnArrayPtr. Anyone who modify this file later (and any reviewer checking patch) would assume the common well defined behavior. This would introduce subtle bugs and make reviews impossible.


It seemed to make sense to do this specialization for char*, but I see your point here. Indeed, things like this could introduce near-impossible to debug problems.

> 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52
> > +static PassOwnArrayPtr<char* const> createArgsArray(const String& prefix, const String& executablePath, const String& socket, const String& pluginPath)
> 
> Why not just return a Vector<OwnArrayPtr<char* const>> here?
> 

After your suggestion I did some experiments with it and I am soon going to post a patch for review.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:106
> > +    OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath);
> 
> It looks like this should be in the if (!pid) branch.

Ideally, yes, since it's only used there. However, there is a comment below warning not to perform memory allocation in the middle of the fork()/exec(), due to potential deadlock risks, and hence the allocation part is done in the parent process.


Thanks for the your time, Benjamin!

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 202775 [details]
Patch

Addressing Benjamin's feedback. Now using a Vector<OwnArrayPtr<char>> to store the arguments to be passed to execvp().

[EFL EWS Bot]

Comment on attachment 202775 [details]
Patch

Attachment 202775 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/684012

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #29)
> (From update of attachment 202775 [details])
> Attachment 202775 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/684012

I am not able to reproduce this linking problem with either a debug or release build. Does anyone have a clue on what caused it?

[Chris Dumez]

I think Gyuyoung is fighting with the EFL bots, so it may not be related to your patch :/

[Benjamin Poulain]

Comment on attachment 202775 [details]
Patch

I like that new approach.

This is EFL code, so it is better if Christophe r+.

[Chris Dumez]

Comment on attachment 202775 [details]
Patch

LGTM.r=me.

[Chris Dumez]

Could you please reupload for landing to make sure the ews likes it? I believe Gyuyoung fixed the bots.

[Gyuyoung Kim]

(In reply to comment #34)
> Could you please reupload for landing to make sure the ews likes it? I believe Gyuyoung fixed the bots.

Yes, EFL buildbot and ews are fixed now. Please check this patch on ews again.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 203443 [details]
Patch

Reuploading to make sure EWS is fine with the patch.

[EFL EWS Bot]

Comment on attachment 203443 [details]
Patch

Attachment 203443 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/662405

[Gyuyoung Kim]

(In reply to comment #37)
> (From update of attachment 203443 [details])
> Attachment 203443 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/662405

There is below build error. I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit. There is no same usage in WebKit. Why do you need to use this use ?

make[1]: *** [Tools/MiniBrowser/efl/CMakeFiles/MiniBrowser.dir/all] Error 2
../../lib/libewebkit2.so.0.1.0: undefined reference to `WTF::OwnArrayPtr<char>::OwnArrayPtr(WTF::OwnArrayPtr<char> const&)'

[Darin Adler]

(In reply to comment #38)
> I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit.

We can use Vector<OwnArrayPtr> if we add an OwnArrayPtr section to the wtf/VectorTraits.h source file.

[Gyuyoung Kim]

(In reply to comment #39)
> (In reply to comment #38)
> > I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit.
> 
> We can use Vector<OwnArrayPtr> if we add an OwnArrayPtr section to the wtf/VectorTraits.h source file.

Thank you for pointing it out. I file a bug for this(Bug 117131). Could you please review it ?

[Gyuyoung Kim]

I think this patch can pass efl-wk2 ews now. Please re-upload one more time.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 203558 [details]
Patch

Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131.

[Chris Dumez]

(In reply to comment #42)
> Created an attachment (id=203558) [details]
> Patch
> 
> Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131.

r=me again. FYI, in the future you can simply update the "Reviewed By" lines in the Changelogs and simply set cq? flag since the patch already got r+.

[WebKit Commit Bot]

Comment on attachment 203558 [details]
Patch

Clearing flags on attachment: 203558

Committed r151098: <http://trac.webkit.org/changeset/151098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #43)
> (In reply to comment #42)
> > Created an attachment (id=203558) [details] [details]
> > Patch
> > 
> > Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131.
> 
> r=me again. FYI, in the future you can simply update the "Reviewed By" lines in the Changelogs and simply set cq? flag since the patch already got r+.

Ah, thanks for this info. I was wondering whether there was some way to "keep" the r+. Thanks for your review again, and thanks to everyone else involved :)