Selection should be updated after node is moved to another document

FrameSelection::setSelection should validate a given VisibleSelection.
    
When executing editing commands, endingSelection is valid.
However, if undo some commands, endingSelection might be invalid and
FrameSelection might be updated by the invalid selection.
i.e. the selection might be owned by another document without any frames.
    
BUG=368978
TEST=Source/core/editing/FrameSelectionTest

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179003

[tasak, 2014-06-26 11:05:50]

I'm still working in progress.
However, would you check the WIP patch?

I'm afraid that I'm going in the wrong direction.

[yosin_UTC9, 2014-06-27 01:39:24]

Thanks for quick work!

Unfortunately, FrameSelection::firstRange() isn't out friend. It doesn't work as
we expect. It is only for DOM Selection API  compatibility thing.

https://codereview.chromium.org/359593002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/359593002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:3748: if (!frame->selection().isNone()) {
FrameSelection::firstRange() returns faked Range object, updating it doesn't
affect selection boundary points. We know this is confusion but we have it for
DOM Selection API compatibility.

We may want to have FrameSelection::ensureOwnerDocument(), which updates |base|,
|extent| member variables in FrameSelection::m_selection.

Here is sketch:
FrameSelection::validateOwnerDocument() {
  ASSERT(m_frame);
  if (m_selection.isNone())
     return;
  const Position base = m_selection.base();
  const Position extent = m_seleciton.extent();
  bool isBaseValid = base.document() == m_frame->document();
  bool isExtentValid = base.document() == m_frame->document();
  if (isBaseValid && isExtentValid)
     return;
  if (isBaseValid) {
    setSelection(VisibleSelection(base, m_selection.affiniy()), DoNotSetFocus);
    return;
  }
  if (isExtentValid) {
    setSelection(VisibleSelection(extent, m_selection.affiniy()),
DoNotSetFocus);
    return;
  }
  // Both boundary points aren't in this selection.
  setSelection(VisibleSelection(), DoNotSetFocus);
}

[yosin_UTC9, 2014-06-27 01:59:24]

Actual implementation should call VisibleSelection::setWithoutValidation(base,
extent) rather than using VisibleSelection ctor, because VS ctor calls
updateLayout(). Usage of setWithoutValidation is found in
FrameSelection::updateSelectionIfNeeded().

[tasak, 2014-07-01 09:17:32]

On 2014/06/27 01:59:24, Yosi_UTC9 wrote:
> Actual implementation should call VisibleSelection::setWithoutValidation(base,
> extent) rather than using VisibleSelection ctor, because VS ctor calls
> updateLayout(). Usage of setWithoutValidation is found in
> FrameSelection::updateSelectionIfNeeded().

I think, I found the root cause of crbug.com/368978.

When creating editing commands, EditCommandCompoisition's endingSelection is
valid.
However, after reapplied/unapplied the commands, the endingSelection might be no
longer valid.

[tasak, 2014-07-09 01:57:09]

Would you review this CL?

[yosin_UTC9, 2014-07-09 03:27:35]

LGTM w/ nits

Add OWNERS for committing.

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:1: /*
nit: You can use shorter copyright header:
http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:47: #define LOREM_IPSUM \
Can we use shorter text? It seems we need to set nodes in different documents to
selection.

[yosin_UTC9, 2014-07-09 03:32:35]

On 2014/07/09 03:27:35, Yosi_UTC9 wrote:
> LGTM w/ nits
> 
> Add OWNERS for committing.
> 
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> File Source/core/editing/FrameSelectionTest.cpp (right):
> 
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> Source/core/editing/FrameSelectionTest.cpp:1: /*
> nit: You can use shorter copyright header:
> http://dev.chromium.org/blink/coding-style#TOC-License
> 
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> Source/core/editing/FrameSelectionTest.cpp:47: #define LOREM_IPSUM \
> Can we use shorter text? It seems we need to set nodes in different documents
to
> selection.

Please update following tests:
-
w3c/web-platform-tests/shadow-dom/elements-and-dom-objects/shadowroot-object/shadowroot-methods/test-004.html
-
w3c/web-platform-tests/shadow-dom/user-interaction/ranges-and-selections/test-002.html
It seems these tests are wrong, because it set elements in another documents to
selection.

[tasak, 2014-07-10 03:02:50]

Thank you for reviewing.

On 2014/07/09 03:32:35, Yosi_UTC9 wrote:
> On 2014/07/09 03:27:35, Yosi_UTC9 wrote:
> > LGTM w/ nits
> > 
> > Add OWNERS for committing.
> > 
> >
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> > File Source/core/editing/FrameSelectionTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> > Source/core/editing/FrameSelectionTest.cpp:1: /*
> > nit: You can use shorter copyright header:
> > http://dev.chromium.org/blink/coding-style#TOC-License
> > 
> >
>
https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
> > Source/core/editing/FrameSelectionTest.cpp:47: #define LOREM_IPSUM \
> > Can we use shorter text? It seems we need to set nodes in different
documents
> to
> > selection.
> 
> Please update following tests:
> -
>
w3c/web-platform-tests/shadow-dom/elements-and-dom-objects/shadowroot-object/shadowroot-methods/test-004.html
> -
>
w3c/web-platform-tests/shadow-dom/user-interaction/ranges-and-selections/test-002.html
> It seems these tests are wrong, because it set elements in another documents
to
> selection.

I updated TestExpectations, because the tests cannot be fixed in this CL. I have
to update tests under github.

[tasak, 2014-07-10 03:16:02]

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:1: /*
On 2014/07/09 03:27:35, Yosi_UTC9 wrote:
> nit: You can use shorter copyright header:
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/359593002/diff/60001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:47: #define LOREM_IPSUM \
On 2014/07/09 03:27:35, Yosi_UTC9 wrote:
> Can we use shorter text? It seems we need to set nodes in different documents
to
> selection.

Done.

[yosin_UTC9, 2014-07-10 03:48:02]

SLGTM

[Yuta Kitamura, 2014-07-14 10:04:43]

Mostly looking fine, just spotted some minor issues.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1863: return newSelection;
I think you can shrink lines 1855-1863 to something
looking like:

    if (isBaseValid)
        newSelection.set...(...);
    else if (isExtentValid)
        newSelection.set...(...);
    return newSelection;

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:21: #include "wtf/Vector.h"
You aren't using a Vector, are you?

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:25: #define HELLO_WORLD "Hello,
World!"
Urm, well, I don't think you have to define this constant.
You can just use the string literal directly in line 52.

If you want this to be a constant, you should declare it
as a C++ constant, instead of a C macro.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:55: m_frameSelection =
FrameSelection::create(&m_dummyPageHolder->frame());
You shouldn't create a FrameSelection instance yourself.
LocalFrame creates one, so you should be using it everywhere.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:86: RefPtr<Document>
documentWithoutFrame = DOMImplementation::createDocument("text/html", dummy,
false);
Just calling Document::create() (with no argument) should be fine.

[tkent, 2014-07-15 00:41:22]

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:42: HTMLDocument* m_document;
This should be RawPtrWillBePersistent<HTMLDocument>

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:43: RefPtr<Text> m_textNode;
RefPtrWillBePersistent<Text>

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:44: OwnPtr<FrameSelection>
m_frameSelection;
OwnPtrWillBePersistent<FrameSelection>

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:86: RefPtr<Document>
documentWithoutFrame = DOMImplementation::createDocument("text/html", dummy,
false);
RefPtrWillBeRawPtr<Document>

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:87: RefPtr<Element> body =
documentWithoutFrame->createElement(HTMLNames::bodyTag, false);
RefPtrWillBeRawPtr<Element>

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:89: RefPtr<Text> anotherText =
documentWithoutFrame->createTextNode("Hello, another world");
RefPtrWillBeRawPtr<Text>

[tasak, 2014-07-25 09:23:35]

Thank you for reviewing.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.cpp:1863: return newSelection;
On 2014/07/14 10:04:43, Yuta Kitamura wrote:
> I think you can shrink lines 1855-1863 to something
> looking like:
> 
>     if (isBaseValid)
>         newSelection.set...(...);
>     else if (isExtentValid)
>         newSelection.set...(...);
>     return newSelection;

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:21: #include "wtf/Vector.h"
On 2014/07/14 10:04:43, Yuta Kitamura wrote:
> You aren't using a Vector, are you?

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:25: #define HELLO_WORLD "Hello,
World!"
On 2014/07/14 10:04:43, Yuta Kitamura wrote:
> Urm, well, I don't think you have to define this constant.
> You can just use the string literal directly in line 52.
> 
> If you want this to be a constant, you should declare it
> as a C++ constant, instead of a C macro.

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:42: HTMLDocument* m_document;
On 2014/07/15 00:41:22, tkent wrote:
> This should be RawPtrWillBePersistent<HTMLDocument>

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:43: RefPtr<Text> m_textNode;
On 2014/07/15 00:41:22, tkent wrote:
> RefPtrWillBePersistent<Text>

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:44: OwnPtr<FrameSelection>
m_frameSelection;
On 2014/07/15 00:41:22, tkent wrote:
> OwnPtrWillBePersistent<FrameSelection>

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:55: m_frameSelection =
FrameSelection::create(&m_dummyPageHolder->frame());
On 2014/07/14 10:04:43, Yuta Kitamura wrote:
> You shouldn't create a FrameSelection instance yourself.
> LocalFrame creates one, so you should be using it everywhere.

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:86: RefPtr<Document>
documentWithoutFrame = DOMImplementation::createDocument("text/html", dummy,
false);
On 2014/07/15 00:41:22, tkent wrote:
> RefPtrWillBeRawPtr<Document>

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:86: RefPtr<Document>
documentWithoutFrame = DOMImplementation::createDocument("text/html", dummy,
false);
On 2014/07/14 10:04:43, Yuta Kitamura wrote:
> Just calling Document::create() (with no argument) should be fine.

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:87: RefPtr<Element> body =
documentWithoutFrame->createElement(HTMLNames::bodyTag, false);
On 2014/07/15 00:41:22, tkent wrote:
> RefPtrWillBeRawPtr<Element>

Done.

https://codereview.chromium.org/359593002/diff/100001/Source/core/editing/Fra...
Source/core/editing/FrameSelectionTest.cpp:89: RefPtr<Text> anotherText =
documentWithoutFrame->createTextNode("Hello, another world");
On 2014/07/15 00:41:22, tkent wrote:
> RefPtrWillBeRawPtr<Text>

Done.

[Yuta Kitamura, 2014-07-25 09:53:55]

lgtm

[tasak, 2014-07-28 04:04:20]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-07-28 04:04:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/359593002/120001

[commit-bot: I haz the power, 2014-07-28 04:08:38]

Change committed as 179003