Bug 110169 - Encapsulate FloatingObject's constructor inside create
Summary: Encapsulate FloatingObject's constructor inside create
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-18 17:24 PST by Ryosuke Niwa
Modified: 2017-07-18 08:29 PDT (History)
15 users (show)

See Also:


Attachments
Cleanup (5.06 KB, patch)
2013-02-18 17:25 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup after build fixes (2.53 KB, patch)
2013-02-18 23:11 PST, Ryosuke Niwa
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Upload the right file (2.59 KB, patch)
2013-02-19 00:09 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed builds (2.59 KB, patch)
2013-02-19 01:37 PST, Ryosuke Niwa
benjamin: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-02-18 17:24:16 PST
Encapsulate FloatingObject's constructor inside create
Comment 1 Ryosuke Niwa 2013-02-18 17:25:45 PST
Created attachment 188967 [details]
Cleanup
Comment 2 Darin Adler 2013-02-18 19:18:49 PST
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.
Comment 3 Ryosuke Niwa 2013-02-18 19:40:18 PST
Committed r143289: <http://trac.webkit.org/changeset/143289>
Comment 4 Ryosuke Niwa 2013-02-18 22:17:18 PST
Apparently this patch doesn't work on Windows :(

Build fix landed in http://trac.webkit.org/changeset/143293.
Comment 5 Ryosuke Niwa 2013-02-18 22:29:17 PST
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>        ]
Comment 6 Ryosuke Niwa 2013-02-18 22:30:05 PST
I made the class public: http://trac.webkit.org/changeset/143294

This is why I hate nested classes. They never work.
Comment 7 Ryosuke Niwa 2013-02-18 23:11:54 PST
Reopening to attach new patch.
Comment 8 Ryosuke Niwa 2013-02-18 23:11:57 PST
Created attachment 188998 [details]
Cleanup after build fixes
Comment 9 Early Warning System Bot 2013-02-18 23:17:16 PST
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
Comment 10 Early Warning System Bot 2013-02-18 23:18:32 PST
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
Comment 11 WebKit Review Bot 2013-02-18 23:24:41 PST
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
Comment 12 WebKit Review Bot 2013-02-18 23:26:08 PST
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
Comment 13 EFL EWS Bot 2013-02-18 23:29:23 PST
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
Comment 14 Build Bot 2013-02-18 23:44:20 PST
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
Comment 15 Ryosuke Niwa 2013-02-19 00:09:36 PST
Created attachment 189009 [details]
Upload the right file
Comment 16 Early Warning System Bot 2013-02-19 00:14:30 PST
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
Comment 17 Early Warning System Bot 2013-02-19 00:15:01 PST
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
Comment 18 Build Bot 2013-02-19 00:18:21 PST
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
Comment 19 WebKit Review Bot 2013-02-19 00:22:05 PST
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
Comment 20 WebKit Review Bot 2013-02-19 00:26:00 PST
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
Comment 21 EFL EWS Bot 2013-02-19 00:35:12 PST
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
Comment 22 Ryosuke Niwa 2013-02-19 01:37:39 PST
Created attachment 189024 [details]
Fixed builds
Comment 23 Early Warning System Bot 2013-02-19 01:50:38 PST
Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16629325
Comment 24 Early Warning System Bot 2013-02-19 01:53:37 PST
Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16610700
Comment 25 WebKit Review Bot 2013-02-19 01:58:34 PST
Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16627433
Comment 26 WebKit Review Bot 2013-02-19 02:18:40 PST
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
Comment 27 EFL EWS Bot 2013-02-19 02:57:38 PST
Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16610717
Comment 28 Build Bot 2013-02-20 06:17:48 PST
Comment on attachment 189024 [details]
Fixed builds

Attachment 189024 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16653297
Comment 29 Benjamin Poulain 2013-02-26 15:01:38 PST
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?