Bug 110169 – Encapsulate FloatingObject's constructor inside create

• Status: REOPENED
• Component: New Bugs

[Ryosuke Niwa]

Encapsulate FloatingObject's constructor inside create

[Ryosuke Niwa]

Created attachment 188967 [details]
Cleanup

[Darin Adler]

Comment on attachment 188967 [details]
Cleanup

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

> Source/WebCore/rendering/RenderBlock.cpp:7714
> +void RenderBlock::ensureFloatingObjects()

I’m not such a big fan of the naming here. I understand, though, that we’re using the word “ensure” for this in many places in WebKit now. But I have two thoughts:

1) We should find a better name or short phrase for this that is less like jargon if we can.
2) In other places, I think this is only used when the function has a return value.

To me this is just createFloatingObjects. The fact that it's smart enough not to recreate it if it already exists seems like a detail that need not be in its name.

> Source/WebCore/rendering/RenderBlock.cpp:7717
> +    if (!m_floatingObjects)
> +        m_floatingObjects = FloatingObjects::create(this, isHorizontalWritingMode());

I like early return when we already have it better than nesting like this, but it's just a matter of taste.

> Source/WebCore/rendering/RenderBlock.cpp:7720
> +inline PassOwnPtr<RenderBlock::FloatingObjects> RenderBlock::FloatingObjects::create(const RenderBlock* renderer, bool horizontalWritingMode)

With older compilers, the inline needs to before the code that uses it. Thus, I suggest reordering these three things in exactly the reverse order they currently appear.

[Ryosuke Niwa]

Committed r143289: <http://trac.webkit.org/changeset/143289>

[Ryosuke Niwa]

Apparently this patch doesn't work on Windows :(

Build fix landed in http://trac.webkit.org/changeset/143293.

[Ryosuke Niwa]

Still doesn't work:

6>C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtrCommon.h(63) : error C2248: 'WebCore::RenderBlock::FloatingObjects' : cannot access protected class declared in class 'WebCore::RenderBlock'
6>        c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(1140) : see declaration of 'WebCore::RenderBlock::FloatingObjects'
6>        c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(83) : see declaration of 'WebCore::RenderBlock'
6>        C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtr.h(63) : see reference to function template instantiation 'void WTF::deleteOwnedPtr<WebCore::RenderBlock::FloatingObjects>(T *)' being compiled
6>        with
6>        [
6>            T=WebCore::RenderBlock::FloatingObjects
6>        ]
6>        C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\include\private\wtf/OwnPtr.h(63) : while compiling class template member function 'WTF::OwnPtr<T>::~OwnPtr(void)'
6>        with
6>        [
6>            T=WebCore::RenderBlock::FloatingObjects
6>        ]
6>        c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderBlock.h(1180) : see reference to class template instantiation 'WTF::OwnPtr<T>' being compiled
6>        with
6>        [
6>            T=WebCore::RenderBlock::FloatingObjects
6>        ]

[Ryosuke Niwa]

I made the class public: http://trac.webkit.org/changeset/143294

This is why I hate nested classes. They never work.

[Ryosuke Niwa]

Reopening to attach new patch.

[Ryosuke Niwa]

Created attachment 188998 [details]
Cleanup after build fixes

[Early Warning System Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16627369

[Early Warning System Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16613540

[WebKit Review Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16627371

[WebKit Review Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16627370

[EFL EWS Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16627382

[Build Bot]

Comment on attachment 188998 [details]
Cleanup after build fixes

Attachment 188998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16612585

[Ryosuke Niwa]

Created attachment 189009 [details]
Upload the right file

[Early Warning System Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16621547

[Early Warning System Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16628344

[Build Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16608735

[WebKit Review Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16622002

[WebKit Review Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16632001

[EFL EWS Bot]

Comment on attachment 189009 [details]
Upload the right file

Attachment 189009 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16620003

[Ryosuke Niwa]

Created attachment 189024 [details]
Fixed builds

[Early Warning System Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16629325

[Early Warning System Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16610700

[WebKit Review Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16627433

[WebKit Review Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16614661

[EFL EWS Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16610717

[Build Bot]

Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16653297

[Benjamin Poulain]

Comment on attachment 189024 [details]
Fixed builds

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

> Source/WebCore/rendering/RenderBlock.cpp:7752
> +inline PassOwnPtr<RenderBlock::FloatingObjects> RenderBlock::FloatingObjects::create(const RenderBlock* renderer, bool horizontalWritingMode)
> +{
> +    return adoptPtr(new FloatingObjects(renderer, horizontalWritingMode));
> +}

For better encapsulation, remove horizontalWritingMode and do?:
    return adoptPtr(new FloatingObjects(renderer, renderer->isHorizontalWritingMode()));

> Source/WebCore/rendering/RenderBlock.h:1178
>          friend void RenderBlock::createFloatingObjects();

You should remove this.

> Source/WebCore/rendering/RenderBlock.h:1181
> +    friend OwnPtr<FloatingObjects>;
> +    friend PassOwnPtr<FloatingObjects>;

friend class xxx

For which scope was this needed?