Bug 69366 – Web Inspector: CodeGenerator should not use pointers for out params of RefPtr type.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

CodeGenerator should not use pointers for out params of RefPtr type.

[Eric Seidel (no email)]

It should return PassRefPtr's instead?  PassRefPtr is strictly superior to RefPtr& when possible.

[Ryosuke Niwa]

(In reply to comment #1)
> It should return PassRefPtr's instead?  PassRefPtr is strictly superior to RefPtr& when possible.

This is only for *out* arguments, meaning that callees are supposed to override the values of those variables. We currently use RefPtr<..>* result :(

[Ilya Tikhonovsky]

In some cases we have two or more out params. As example setBreakpointByUrl, setBreakpoint, setScriptSource etc. That is the reason do not pass these data via return statement.

[Ryosuke Niwa]

(In reply to comment #2)
> (In reply to comment #1)
> > It should return PassRefPtr's instead?  PassRefPtr is strictly superior to RefPtr& when possible.
> 
> This is only for *out* arguments, meaning that callees are supposed to override the values of those variables. We currently use RefPtr<..>* result :(

It's fine to use out arguments but it's NOT okay to use raw pointers for that. I'm actually surprised that there's no mentioning of this in WebKit's style guideline.

[Pavel Feldman]

(In reply to comment #1)
> It should return PassRefPtr's instead?  PassRefPtr is strictly superior to RefPtr& when possible.

I thought that RefPtr& was the preferred way in case of multiple out parameters. It was allowed in the style:http://trac.webkit.org/changeset/94803 and supported my Darin and Maciej on a recent thread.

> It's fine to use out arguments but it's NOT okay to use raw pointers for that. I'm actually surprised that there's no mentioning of this in WebKit's style guideline.

Although not in the guidelines, references are typically expected to be used, so your original request makes perfect sense.

[Peter Rybin]

Created attachment 119918 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:596:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:84:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:85:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 22 files


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

[Ilya Tikhonovsky]

Comment on attachment 119918 [details]
Patch

Clearing flags on attachment: 119918

Committed r103322: <http://trac.webkit.org/changeset/103322>

[Ilya Tikhonovsky]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 119918 [details])
> Clearing flags on attachment: 119918
> 
> Committed r103322: <http://trac.webkit.org/changeset/103322>

Reopen, because it made 7 inspector tests crash on the Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r103322%20(41262)/results.html

Could you check it, please?

[Csaba Osztrogonác]

Here is the problem from Qt debug bot:

ASSERTION FAILED: value
../../../../Source/WebCore/inspector/InspectorValues.h(321) : void WebCore::InspectorArray::pushObject(WTF::PassRefPtr<WebCore::InspectorObject>)

[Peter Rybin]

Created attachment 120048 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:596:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:84:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:85:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 22 files


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

[Peter Rybin]

(In reply to comment #11)
> Here is the problem from Qt debug bot:
> 
> ASSERTION FAILED: value
> ../../../../Source/WebCore/inspector/InspectorValues.h(321) : void WebCore::InspectorArray::pushObject(WTF::PassRefPtr<WebCore::InspectorObject>)

Csaba, thank you for investigating.

A condition against NULL pointer was lost in the original patch. New patch should fix it.

[Ilya Tikhonovsky]

Committed r103389: <http://trac.webkit.org/changeset/103389>

[Yury Semikhatsky]

Comment on attachment 120048 [details]
Patch

Cleared r? from landed patch.