Bug 146995 – Reduce PassRefPtr in WebKit2 - 3

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 256893 [details]
Patch

[Gyuyoung Kim]

Created attachment 256900 [details]
Patch

[Gyuyoung Kim]

Created attachment 256940 [details]
Patch

[Gyuyoung Kim]

Created attachment 256944 [details]
Patch

[Andreas Kling]

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 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]

(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]

Created attachment 257026 [details]
Patch

[Gyuyoung Kim]

(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]

Created attachment 257028 [details]
Patch

[Gyuyoung Kim]

Created attachment 257029 [details]
Patch

[Daniel Bates]

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]

Created attachment 257033 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 257033 [details]
Patch for landing

Clearing flags on attachment: 257033

Committed r187002: <http://trac.webkit.org/changeset/187002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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)]

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