Bug 195962 – Remove copyRef() calls added in r243163

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

I added some calls to copyRef() in r243163 that Chris wants removed, see bug #195920.

[Michael Catanzaro]

Created attachment 365245 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[EWS Watchlist]

Comment on attachment 365245 [details]
Patch

Attachment 365245 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11573478

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 365286 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 365245 [details]
Patch

Attachment 365245 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11574224

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365297 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 365245 [details]
Patch

Clearing flags on attachment: 365245

Committed r243204: <https://trac.webkit.org/changeset/243204>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49063670>

[Darin Adler]

Comment on attachment 365245 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:235
> +            Ref<JSON::Object> jsonResult = m_result.releaseNonNull();
> +            auto result = WTFMove(*reinterpret_cast<Ref<${objectType}>*>(&jsonResult));
> +            return result;

I would think we’d need to add a comment explaining that this is to work around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the local variable. Another way to protect against that is to make sure we have at least EWS testing completion with GCC 9.

[Michael Catanzaro]

(In reply to Darin Adler from comment #10)
> I would think we’d need to add a comment explaining that this is to work
> around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the
> local variable. Another way to protect against that is to make sure we have
> at least EWS testing completion with GCC 9.

I almost added a comment, but in the end I decided it was low-value since if it's ever refactored, we'll either get a warning again and know right away, or else we won't (if GCC stops warning there) and it will be fine.

EWS won't catch it unless we enable -Werror. There are pros and cons to enabling -Werror on the EWS. The risk is that it will probably significantly increase the number of build failures. The benefit is that it will surely reduce the number of compiler warnings introduced.

(I agree it would be nice to have EWS testing more supported versions of GCC.)