Bug 177713 – RenderView does not need to be a SelectionSubtreeRoot

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

It's the only selection root (it's not even a thing)

[alan]

Created attachment 322307 [details]
Patch

[alan]

Created attachment 322311 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34758519>

[alan]

Created attachment 322323 [details]
Patch

[alan]

Created attachment 322324 [details]
Patch

[alan]

Created attachment 322327 [details]
Patch

[Antti Koivisto]

Comment on attachment 322327 [details]
Patch

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

r=me

> Source/WebCore/rendering/SelectionRangeData.cpp:56
> +struct SelectionIterator {

This looks more like a class than a struct.

[Darin Adler]

Comment on attachment 322327 [details]
Patch

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

Feels like the code might need a struct with start and end in it. Lots of code manipulating the two together.

> Source/WebCore/editing/FrameSelection.cpp:2117
> +        view->selection().set({startRenderer, endRenderer, startOffset, endOffset});

We normally put spaces after "{" and before "}". Not sure I would chose that style for my own project, but I like the idea of staying consistent in WebKit.

> Source/WebCore/editing/FrameSelection.cpp:2237
> +    auto* renderView = m_frame->contentRenderer();
> +    auto* view = m_frame->view();
> +    if (!renderView || !view)
>          return LayoutRect();

You can get the FrameView from a RenderView. It returns a reference and never returns null. So there is no reason to go back to the Frame to get the view and just have to null check it. We can remove view and the one place below that says "view->" could instead just say "renderView->frameView()." instead. Or you could have a local variable, but it could be a reference rather than a pointer.

> Source/WebCore/platform/DragImage.cpp:186
> +    view->selection().set({startRenderer, endRenderer, startOffset, endOffset}, SelectionRangeData::RepaintMode::Nothing);

Same braces thing.

> Source/WebCore/rendering/InlineTextBox.cpp:148
> +        auto startPos = renderer().view().selection().startPosition();
> +        auto endPos = renderer().view().selection().endPosition();

If it was me, I would put renderer().view().selectIon() into a local variable rather than evaluating it twice. No idea if that’s helpful or not. Maybe one of those three functions is not inlined? If so, then it will pay off, I think.

> Source/WebCore/rendering/InlineTextBox.cpp:643
> +    auto start = renderer().view().selection().startPosition();
> +    auto end = renderer().view().selection().endPosition();

Ditto.

> Source/WebCore/rendering/SelectionRangeData.cpp:3
> + * Copyright (C) 2015, 2017 Apple Inc. All rights reserved.

Should be 2015-2017 at least. I think most of this code is older and should have additional publication dates from those older times it was published (checked in).

> Source/WebCore/rendering/SelectionRangeData.cpp:48
> +    typedef HashMap<RenderObject*, std::unique_ptr<RenderSelectionInfo>> RendererMap;
> +    typedef HashMap<const RenderBlock*, std::unique_ptr<RenderBlockSelectionInfo>> RenderBlockMap;

In our modern code we use "using" instead of "typedef".

> Source/WebCore/rendering/SelectionRangeData.cpp:56
> +struct SelectionIterator {

This looks like a class, not a struct. It has public function members and private data members.

> Source/WebCore/rendering/SelectionRangeData.cpp:93
> +    Vector<RenderMultiColumnSpannerPlaceholder*> m_spannerStack;

Comment on the moved, not new, code: Do we really want a stack here? Maybe a deque instead?

> Source/WebCore/rendering/SelectionRangeData.cpp:106
> +    return (renderer.canBeSelectionLeaf()
> +        || &renderer == selection.start
> +        || &renderer == selection.end)

This part would read better on one line.

> Source/WebCore/rendering/SelectionRangeData.cpp:117
> +    SelectionData oldSelectionData;
> +    // Record the old selected objects. These will be used later
> +    // when we compare against the new selected objects.
> +    oldSelectionData.startPosition = selection.startPosition;
> +    oldSelectionData.endPosition = selection.endPosition;

Could do this with initialization rather than assignment:

    SelectionData oldSelectionData { selection.startPosition, selection.endPosition, { } { } };

But also, the comment (old, moved here not just written) says we record the old "selected objects", but we don’t seem to quite do that; we just record the old start and end positions, not the maps of renderers and blocks.

> Source/WebCore/rendering/SelectionRangeData.cpp:127
> +    SelectionIterator selectionIterator(start);
> +    while (start && start != stop) {

Should be a for loop.

> Source/WebCore/rendering/SelectionRangeData.cpp:133
> +                RenderBlock* block = start->containingBlock();
> +                while (block && !is<RenderView>(*block)) {

I’d like to write this loop better at some point. Probably as a for loop. Here are four versions:

Try 1:

    for (auto* block = start->containingBlock(); block && !is<RenderView>(*block); block = block->containingBlock()) {

Try 2:

    for (auto* block = start; (block = block->containingBlock()) && !is<RenderView>(block); ) {

Try 3:

    auto containingBlockBelowView = [] (RenderObject& block)
    {
        auto* containingBlock = block.containingBlock();
        return is<RenderView>(containingBlock) ? nullptr : containingBlock;
    }

    for (auto* block = start; (block = containingBlockBelowView(*block)); ) {

Try 4:

    auto containingBlockBelowView = [] (RenderObject& block)
    {
        auto* containingBlock = block.containingBlock();
        return is<RenderView>(containingBlock) ? nullptr : containingBlock;
    }

    for (auto* block = containingBlockBelowView(*start); block; block = containingBlockBelowView(*block)) {

> Source/WebCore/rendering/SelectionRangeData.cpp:134
> +                    std::unique_ptr<RenderBlockSelectionInfo>& blockInfo = oldSelectionData.blocks.add(block, nullptr).iterator->value;

Using auto& here would be good.

> Source/WebCore/rendering/SelectionRangeData.cpp:168
> +    std::unique_ptr<SelectionRectGatherer::Notifier> rectNotifier = m_selectionRectGatherer.clearAndCreateNotifier();

I suggest using auto instead of writing out the type here.

> Source/WebCore/rendering/SelectionRangeData.cpp:169
> +#endif // ENABLE(SERVICE_CONTROLS)

I would remove this comment. Makes it harder to read.

> Source/WebCore/rendering/SelectionRangeData.cpp:194
> +        for (RenderBlock* block = renderer->containingBlock(); block && !is<RenderView>(*block); block = block->containingBlock()) {

Same loop as above, only this time written as a for loop (my Try 1, in fact).

> Source/WebCore/rendering/SelectionRangeData.cpp:210
> +    SelectionIterator selectionIterator(start);
> +    while (start && start != stop) {

Should be a for loop.

> Source/WebCore/rendering/SelectionRangeData.cpp:222
> +            // Blocks are responsible for painting line gaps and margin gaps. They must be examined as well.
> +            renderers.set(start, std::make_unique<RenderSelectionInfo>(*start, clipToVisibleContent));
> +            auto* block = start->containingBlock();
> +            while (block && !is<RenderView>(*block)) {
> +                std::unique_ptr<RenderSelectionInfo>& blockInfo = renderers.add(block, nullptr).iterator->value;
> +                if (blockInfo)
> +                    break;
> +                blockInfo = std::make_unique<RenderSelectionInfo>(*block, clipToVisibleContent);
> +                block = block->containingBlock();
> +            }

This code is so close to the code from collect above. Would be good to find a way to share more if we can do that in a clean way.

> Source/WebCore/rendering/SelectionRangeData.cpp:231
> +    auto end = renderers.end();
> +    for (auto i = renderers.begin(); i != end; ++i) {
> +        auto* info = i->value.get();

Should be a modern for loop:

    for (auto& info : renderers.values()) {

> Source/WebCore/rendering/SelectionRangeData.h:3
> + * Copyright (C) 2017 Apple Inc.

Need "All rights reserved."

> Source/WebCore/rendering/SelectionRangeData.h:33
> +#include "RenderObject.h"

Don’t need this include. RenderSelectionInfo.h includes RenderBlock.h which includes RenderObject.h.

> Source/WebCore/rendering/SelectionRangeData.h:37
> +#if ENABLE(SERVICE_CONTROLS)
> +#include "SelectionRectGatherer.h"
> +#endif

Should have a blank line before so it can be a separate paragraph.

> Source/WebCore/rendering/SelectionRangeData.h:51
> +        bool operator==(const Context& other) const
> +        {
> +            return start == other.start && end == other.end && startPosition == other.startPosition && endPosition == other.endPosition;
> +        }

I suggest putting this after the data members. In classes we put functions first, but in a struct it makes sense to have the data first since that’s the public interface.

> Source/WebCore/rendering/SelectionRangeData.h:61
> +    Context get() const { return m_selectionContext; }

Should consider having this return const Context& so it doesn’t have to copy.

> Source/WebCore/rendering/SelectionRangeData.h:69
> +    IntRect bounds(bool clipToVisibleContent = true) const;

Can we use two separate functions here instead of a function with a boolean argument? The boolean argument version could be a separate implementation detail.

[alan]

Created attachment 322343 [details]
Patch

[alan]

Created attachment 322352 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322352 [details]
Patch

Clearing flags on attachment: 322352

Committed r222697: <http://trac.webkit.org/changeset/222697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Builds break for Apple Win and WinCairo port:

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/6775

> C:\WebKit-BuildSlave\win-cairo-release\build\Source\WebCore\editing\FrameSelection.cpp(2117): error C2397: conversion from 'int' to 'std::optional<unsigned int>' requires a narrowing conversion

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/4835

> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\FrameSelection.cpp(2117): error C2664: 'void WebCore::SelectionRangeData::set(const WebCore::SelectionRangeData::Context &,WebCore::SelectionRangeData::RepaintMode)': cannot convert argument 1 from 'initializer list' to 'const WebCore::SelectionRangeData::Context &' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\EditingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\FrameSelection.cpp(2117): note: Reason: cannot convert from 'initializer list' to 'const WebCore::SelectionRangeData::Context' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\EditingAllInOne.cpp)
> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\FrameSelection.cpp(2117): note: No constructor could take the source type, or constructor overload resolution was ambiguous (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing\EditingAllInOne.cpp)

> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\DragImage.cpp(188): error C2664: 'void WebCore::SelectionRangeData::set(const WebCore::SelectionRangeData::Context &,WebCore::SelectionRangeData::RepaintMode)': cannot convert argument 1 from 'initializer list' to 'const WebCore::SelectionRangeData::Context &' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\DragImage.cpp(188): note: Reason: cannot convert from 'initializer list' to 'const WebCore::SelectionRangeData::Context'
> C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\DragImage.cpp(188): note: No constructor could take the source type, or constructor overload resolution was ambiguous

> c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\SelectionRangeData.cpp(175): error C2664: 'void WebCore::SelectionRangeData::set(const WebCore::SelectionRangeData::Context &,WebCore::SelectionRangeData::RepaintMode)': cannot convert argument 1 from 'initializer list' to 'const WebCore::SelectionRangeData::Context &' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\SelectionRangeData.cpp(175): note: Reason: cannot convert from 'initializer list' to 'const WebCore::SelectionRangeData::Context' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\rendering\RenderingAllInOne.cpp)
> c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\rendering\SelectionRangeData.cpp(175): note: No constructor could take the source type, or constructor overload resolution was ambiguous (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\rendering\RenderingAllInOne.cpp)

[Ryosuke Niwa]

Windows build fix attempt: https://trac.webkit.org/changeset/222700

[Ryosuke Niwa]

Fixed Windows build in https://trac.webkit.org/changeset/222701.