146995 – Reduce PassRefPtr in WebKit2 - 3

RESOLVED FIXED 146995

Reduce PassRefPtr in WebKit2 - 3

https://bugs.webkit.org/show_bug.cgi?id=146995

Summary Reduce PassRefPtr in WebKit2 - 3

Gyuyoung Kim

Reported 2015-07-15 22:56:51 PDT

SSIA

Attachments
Patch (98.76 KB, patch) 2015-07-15 23:04 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (115.05 KB, patch) 2015-07-16 07:23 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (115.13 KB, patch) 2015-07-16 17:51 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (116.17 KB, patch) 2015-07-16 18:51 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (116.22 KB, patch) 2015-07-18 01:49 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (116.22 KB, patch) 2015-07-18 04:17 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch (116.23 KB, patch) 2015-07-18 06:06 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Patch for landing (116.19 KB, patch) 2015-07-18 15:33 PDT, Gyuyoung Kim	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Gyuyoung Kim

Comment 1 2015-07-15 23:04:20 PDT

Created attachment 256893 [details] Patch

Gyuyoung Kim

Comment 2 2015-07-16 07:23:41 PDT

Created attachment 256900 [details] Patch

Gyuyoung Kim

Comment 3 2015-07-16 17:51:05 PDT

Created attachment 256940 [details] Patch

Gyuyoung Kim

Comment 4 2015-07-16 18:51:39 PDT

Created attachment 256944 [details] Patch

Andreas Kling

Comment 5 2015-07-17 13:36:03 PDT

Comment on attachment 256944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256944&action=review > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333 > - return loader.release(); > + return loader.leakRef(); Doesn't this introduce a leak? Same question for every other instance of release() -> leakRef() in this patch.

Daniel Bates

Comment 6 2015-07-17 22:22:01 PDT

Comment on attachment 256944 [details] Patch > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333 > - return loader.release(); > + return loader.leakRef(); This is not the correct idiom. It should be sufficient to write this line as: return loader;

Daniel Bates

Comment 7 2015-07-17 23:02:24 PDT

(In reply to comment #6) > Comment on attachment 256944 [details] > Patch > > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333 > > - return loader.release(); > > + return loader.leakRef(); > > This is not the correct idiom. It should be sufficient to write this line as: > > return loader; err, I meant to write: return WTF::move(loader);

Gyuyoung Kim

Comment 8 2015-07-18 01:49:22 PDT

Created attachment 257026 [details] Patch

Gyuyoung Kim

Comment 9 2015-07-18 01:50:05 PDT

(In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 256944 [details] > > Patch > > > > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1333 > > > - return loader.release(); > > > + return loader.leakRef(); > > > > This is not the correct idiom. It should be sufficient to write this line as: > > > > return loader; > > err, I meant to write: > > return WTF::move(loader); Oops, that was my fault. Thank you for pointing it out.

Gyuyoung Kim

Comment 10 2015-07-18 04:17:36 PDT

Created attachment 257028 [details] Patch

Gyuyoung Kim

Comment 11 2015-07-18 06:06:50 PDT

Created attachment 257029 [details] Patch

Daniel Bates

Comment 12 2015-07-18 08:58:50 PDT

Comment on attachment 257029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257029&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:251 > + return WTF::move(page); It is unnecessary to use WTF::move() here. I would write this line as: return page;

Gyuyoung Kim

Comment 13 2015-07-18 15:33:55 PDT

Created attachment 257033 [details] Patch for landing

WebKit Commit Bot

Comment 14 2015-07-18 17:18:07 PDT

Comment on attachment 257033 [details] Patch for landing Clearing flags on attachment: 257033 Committed r187002: <http://trac.webkit.org/changeset/187002>

WebKit Commit Bot

Comment 15 2015-07-18 17:18:14 PDT

All reviewed patches have been landed. Closing bug.

David Kilzer (:ddkilzer)

Comment 16 2015-07-19 05:42:55 PDT

This broken the Windows build: <https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/71645/steps/compile-webkit/logs/stdio> ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(954): error C2440: 'initializing' : cannot convert from 'WTF::PassRefPtr<T>' to 'WTF::Ref<T>' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] with [ T=WebDocumentLoader ] No constructor could take the source type, or constructor overload resolution was ambiguous ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(956): error C2664: 'WebDataSource *WebDataSource::createInstance(WebDocumentLoader *)' : cannot convert argument 1 from 'WebDocumentLoader' to 'WebDocumentLoader *' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(956): error C2664: 'COMPtr<WebDataSource>::COMPtr(WTF::HashTableDeletedValueType)' : cannot convert argument 1 from 'AdoptCOMTag' to 'WebDataSource *' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1051): error C2556: 'WTF::PassRefPtr<WebCore::Frame> WebFrameLoaderClient::createFrame(const WebCore::URL &,const WTF::String &,WebCore::HTMLFrameOwnerElement *,const WTF::String &,bool,int,int)' : overloaded function differs only by return type from 'WTF::RefPtr<WebCore::Frame> WebFrameLoaderClient::createFrame(const WebCore::URL &,const WTF::String &,WebCore::HTMLFrameOwnerElement *,const WTF::String &,bool,int,int)' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(183) : see declaration of 'WebFrameLoaderClient::createFrame' ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1051): error C2371: 'WebFrameLoaderClient::createFrame' : redefinition; different basic types [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(183) : see declaration of 'WebFrameLoaderClient::createFrame' ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1168): error C2556: 'WTF::PassRefPtr<WebCore::Widget> WebFrameLoaderClient::createPlugin(const WebCore::IntSize &,WebCore::HTMLPlugInElement *,const WebCore::URL &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::String &,bool)' : overloaded function differs only by return type from 'WTF::RefPtr<WebCore::Widget> WebFrameLoaderClient::createPlugin(const WebCore::IntSize &,WebCore::HTMLPlugInElement *,const WebCore::URL &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16> &,const WTF::String &,bool)' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(185) : see declaration of 'WebFrameLoaderClient::createPlugin' ..\..\win\WebCoreSupport\WebFrameLoaderClient.cpp(1168): error C2371: 'WebFrameLoaderClient::createPlugin' : redefinition; different basic types [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webcoresupport\WebFrameLoaderClient.h(185) : see declaration of 'WebFrameLoaderClient::createPlugin'

David Kilzer (:ddkilzer)

Comment 17 2015-07-19 06:18:47 PDT

Attempted build fix for Windows in r187010: <http://trac.webkit.org/r187010>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Gyuyoung Kim

Reported

2015-07-15 22:56 PDT

Modified

2015-07-19 06:18 PDT History

CC List

7 users Show

URL

Keywords

Depends on

Blocks

144092

Dependencies

tree graph