Bug 182580 – Gigacage DumpRenderTree and WebKitTestRunner

• Component: bmalloc
• Status: NEW

[JF Bastien]

As Saam suggested in #182457, this will give us better test coverage.

[Radar WebKit Bug Importer]

<rdar://problem/37325535>

[JF Bastien]

Created attachment 333310 [details]
patch

[EWS Watchlist]

Attachment 333310 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/ProcessCheck.mm:38:  Extra space between ^ and block definition.  [whitespace/brackets] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 333310 [details]
patch

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

> Source/bmalloc/bmalloc/ProcessCheck.mm:53
> +            || [processName hasPrefix:@"WebKitTestRunner"];

I don't think this is exactly what we should do. I think the goal should be for the testing environment to be a lot like the environment it's enabled in (e.g, when running the browser). Therefore, this should probably only be enabled in the
"WebKitTestRunner WebContent" process.

[JF Bastien]

(In reply to Saam Barati from comment #4)
> Comment on attachment 333310 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333310&action=review
> 
> > Source/bmalloc/bmalloc/ProcessCheck.mm:53
> > +            || [processName hasPrefix:@"WebKitTestRunner"];
> 
> I don't think this is exactly what we should do. I think the goal should be
> for the testing environment to be a lot like the environment it's enabled in
> (e.g, when running the browser). Therefore, this should probably only be
> enabled in the
> "WebKitTestRunner WebContent" process.

Where does this name come from? Grepping around find a script that deals with "WebKitTestRunner Web Content" (note the space), but I can't find how we create it.

[Saam Barati]

(In reply to JF Bastien from comment #5)
> (In reply to Saam Barati from comment #4)
> > Comment on attachment 333310 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333310&action=review
> > 
> > > Source/bmalloc/bmalloc/ProcessCheck.mm:53
> > > +            || [processName hasPrefix:@"WebKitTestRunner"];
> > 
> > I don't think this is exactly what we should do. I think the goal should be
> > for the testing environment to be a lot like the environment it's enabled in
> > (e.g, when running the browser). Therefore, this should probably only be
> > enabled in the
> > "WebKitTestRunner WebContent" process.
> 
> Where does this name come from? Grepping around find a script that deals
> with "WebKitTestRunner Web Content" (note the space), but I can't find how
> we create it.

No idea where it comes from.

[Alexey Proskuryakov]

That’s not the process name, it is only the name that LaunchServices uses for display purposes. The process name remains com.apple.webkit.WebContent.

I think that special casing the name is the wrong approach anyway. This should be an explicit opt-in that the client can elect to enable.

[JF Bastien]

(In reply to Alexey Proskuryakov from comment #7)
> That’s not the process name, it is only the name that LaunchServices uses
> for display purposes. The process name remains com.apple.webkit.WebContent.
> 
> I think that special casing the name is the wrong approach anyway.

For this specific change (DUmpRenderTree and WebKitTestRunner), or in general? A follow-up will make this an entitlement instead, but for now this general approach was decided to be the right approach.

> This should be an explicit opt-in that the client can elect to enable.

[Alexey Proskuryakov]

I don't know what the arguments were originally, but the fact that you now ran into difficulties demonstrates that it may have not been the right decision.