Expand FrameSeleciton::isNone() to increase chances of hoisting update layout

Expand FrameSeleciton::isNone() to increase chances of hoisting update layout

This patch expands |FrameSeleciton::isNone()| to increase chance of hoisting
update layout.

Note: This is mechanical change to replace |selection().isNone()|
by |selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()|
Following patch will remove deprecated version.

This patch is apart of expanding expensive member functions in |FrameSelection|:
- rootEditableElement()
- hasEditableStyle()
- isContentEditable()
- isContentRichlyEditable()
- getSelectionType()
- base() 
- extent()
- start()
- end()
- isNone() 
- isCaret()
- isRange()

BUG=694360
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2710883002
Cr-Commit-Position: refs/heads/master@{#452435}
Committed: https://chromium.googlesource.com/chromium/src/+/3032e512f4c50e0275cc2306cf75208ee7c9a2c4

[yosin_UTC9, 2017-02-22 04:01:15]

Description was changed from

==========
2017-02-22T12:59:40

2017-02-22T12:58:42

BUG=
==========

to

==========
Expand FrameSeleciton::isNone() to increase chances of hoisting update layout

This patch expands |FrameSeleciton::isNone()| to increase chance of hoisting
update layout.

Note: This is mechanical change to replace |selection().isNone()|
by |selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()|
Following patch will remove deprecated version.

This patch is apart of expanding expensive member functions in |FrameSelection|:
- rootEditableElement()
- hasEditableStyle()
- isContentEditable()
- isContentRichlyEditable()
- getSelectionType()
- base() 
- extent()
- start()
- end()
- isNone() 
- isCaret()
- isRange()

BUG=694360
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2017-02-22 06:54:08]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 06:55:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 08:39:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 08:39:13]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-22 09:31:35]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-02-22 09:31:36]

PTAL

[Xiaocheng, 2017-02-22 17:45:50]

Is there still any intuitive way to check whether FS is none?

[tkent, 2017-02-22 23:00:34]

lgtm.

On 2017/02/22 at 17:45:50, xiaochengh wrote:
> Is there still any intuitive way to check whether FS is none?

IMO, we should not re-introduce isNone() for VisibleSelection in order to help
hoisting.  We may have isNone() for SelectionInDOMTree on FrameSelection, but it
might not have many users.

[tkent, 2017-02-22 23:00:38]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-02-22 23:01:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 23:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 23:16:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yosin_UTC9, 2017-02-22 23:42:38]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 23:43:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-02-23 00:05:15]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 00:05:18]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 00:05:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2710883002/#ps20001
(title: "2017-02-23T08:39:24")

[commit-bot: I haz the power, 2017-02-23 00:06:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-02-23 00:14:56]

On 2017/02/22 at 23:00:34, tkent wrote:
> lgtm.
> 
> On 2017/02/22 at 17:45:50, xiaochengh wrote:
> > Is there still any intuitive way to check whether FS is none?
> 
> IMO, we should not re-introduce isNone() for VisibleSelection in order to help
hoisting.  We may have isNone() for SelectionInDOMTree on FrameSelection, but it
might not have many users.

At first glance, FS::isVisibleNone() is attractive, but it is not.
Since it does not imply it causes layout updating == slow.
We think "isXXX()" is cheap and does not cause side-effect, but isVisibleNone()
is not.

On rewriting frameSelection().computeVisibleSelectionInDOMTree().isNone(), we
need to think we really need to do so.
I think most of case we can use selectionInDOMTree().isNone() or we should go
toward this route.

Just change frameSelection().computeVisibleSelectionInDOMTree().isNone() to
frameSelection().selectionInDOMTree().isNone() then
which layout tests are failed.

I have not had simple sample of when VS.isNone() != SelectionInDOM.isNone(),
since visible canonicalization moves to visible position
when HTML have some visible thing, positions go there.

[yosin_UTC9, 2017-02-23 00:24:09]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 00:26:39]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-23 00:27:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-02-23 00:29:09]

On 2017/02/23 at 00:14:56, yosin wrote:
> On 2017/02/22 at 23:00:34, tkent wrote:
> > lgtm.
> > 
> > On 2017/02/22 at 17:45:50, xiaochengh wrote:
> > > Is there still any intuitive way to check whether FS is none?
> > 
> > IMO, we should not re-introduce isNone() for VisibleSelection in order to
help hoisting.  We may have isNone() for SelectionInDOMTree on FrameSelection,
but it might not have many users.
> 
> At first glance, FS::isVisibleNone() is attractive, but it is not.
> Since it does not imply it causes layout updating == slow.
> We think "isXXX()" is cheap and does not cause side-effect, but
isVisibleNone() is not.
> 
> On rewriting frameSelection().computeVisibleSelectionInDOMTree().isNone(), we
need to think we really need to do so.
> I think most of case we can use selectionInDOMTree().isNone() or we should go
toward this route.
> 
> Just change frameSelection().computeVisibleSelectionInDOMTree().isNone() to
frameSelection().selectionInDOMTree().isNone() then
> which layout tests are failed.
> 
> I have not had simple sample of when VS.isNone() != SelectionInDOM.isNone(),
since visible canonicalization moves to visible position
> when HTML have some visible thing, positions go there.

Sounds like something doesn't work correctly...

What's the crucial difference between a "visibly none" selection (i.e., none
after canonicalization) and a "truly none" selection (i.e., already none before
canonicalization)? Do many clients care about that?

[yosin_UTC9, 2017-02-23 01:31:34]

The CQ bit was unchecked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 01:38:22]

On 2017/02/23 at 00:29:09, xiaochengh wrote:
> On 2017/02/23 at 00:14:56, yosin wrote:
> > On 2017/02/22 at 23:00:34, tkent wrote:
> > > lgtm.
> > > 
> > > On 2017/02/22 at 17:45:50, xiaochengh wrote:
> > > > Is there still any intuitive way to check whether FS is none?
> > > 
> > > IMO, we should not re-introduce isNone() for VisibleSelection in order to
help hoisting.  We may have isNone() for SelectionInDOMTree on FrameSelection,
but it might not have many users.
> > 
> > At first glance, FS::isVisibleNone() is attractive, but it is not.
> > Since it does not imply it causes layout updating == slow.
> > We think "isXXX()" is cheap and does not cause side-effect, but
isVisibleNone() is not.
> > 
> > On rewriting frameSelection().computeVisibleSelectionInDOMTree().isNone(),
we need to think we really need to do so.
> > I think most of case we can use selectionInDOMTree().isNone() or we should
go toward this route.
> > 
> > Just change frameSelection().computeVisibleSelectionInDOMTree().isNone() to
frameSelection().selectionInDOMTree().isNone() then
> > which layout tests are failed.
> > 
> > I have not had simple sample of when VS.isNone() != SelectionInDOM.isNone(),
since visible canonicalization moves to visible position
> > when HTML have some visible thing, positions go there.
> 
> Sounds like something doesn't work correctly...
> 
> What's the crucial difference between a "visibly none" selection (i.e., none
after canonicalization) and a "truly none" selection (i.e., already none before
canonicalization)? Do many clients care about that?

Let's discuss this in http://crbug.com/695277
I hold this patch examine result of FrameSelection::isNone() { return
selectionInDOMTree().isNone(); }

[yosin_UTC9, 2017-02-23 07:02:49]

On 2017/02/23 at 01:38:22, yosin_UTC9 wrote:
> On 2017/02/23 at 00:29:09, xiaochengh wrote:
> > On 2017/02/23 at 00:14:56, yosin wrote:
> > > On 2017/02/22 at 23:00:34, tkent wrote:
> > > > lgtm.
> > > > 
> > > > On 2017/02/22 at 17:45:50, xiaochengh wrote:
> > > > > Is there still any intuitive way to check whether FS is none?
> > > > 
> > > > IMO, we should not re-introduce isNone() for VisibleSelection in order
to help hoisting.  We may have isNone() for SelectionInDOMTree on
FrameSelection, but it might not have many users.
> > > 
> > > At first glance, FS::isVisibleNone() is attractive, but it is not.
> > > Since it does not imply it causes layout updating == slow.
> > > We think "isXXX()" is cheap and does not cause side-effect, but
isVisibleNone() is not.
> > > 
> > > On rewriting frameSelection().computeVisibleSelectionInDOMTree().isNone(),
we need to think we really need to do so.
> > > I think most of case we can use selectionInDOMTree().isNone() or we should
go toward this route.
> > > 
> > > Just change frameSelection().computeVisibleSelectionInDOMTree().isNone()
to frameSelection().selectionInDOMTree().isNone() then
> > > which layout tests are failed.
> > > 
> > > I have not had simple sample of when VS.isNone() !=
SelectionInDOM.isNone(), since visible canonicalization moves to visible
position
> > > when HTML have some visible thing, positions go there.
> > 
> > Sounds like something doesn't work correctly...
> > 
> > What's the crucial difference between a "visibly none" selection (i.e., none
after canonicalization) and a "truly none" selection (i.e., already none before
canonicalization)? Do many clients care about that?
> 
> Let's discuss this in http://crbug.com/695277
> I hold this patch examine result of FrameSelection::isNone() { return
selectionInDOMTree().isNone(); }

TL;DR: We commit this patch as is then we change computeVS().isNone() to
selectionInDOMtree().isNone() case by case based on what a call site does.
(This is what tkent@ suggested in #9,
https://codereview.chromium.org/2710883002#msg9)

The purpose of this patch doesn't change behavior. There are cases that
VS.isNone() != S.isNone() since VP(p) can be null event if P is not null.

[yosin_UTC9, 2017-02-23 07:02:54]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-23 07:03:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 07:08:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 07:08:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-02-23 07:30:47]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-02-23 07:30:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2710883002/#ps40001
(title: "2017-02-23T16:27:43 rebase to resolve conflict with DOMSelection in
r452402")

[commit-bot: I haz the power, 2017-02-23 07:31:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 08:56:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487835047033020,
"parent_rev": "109a96913ee46eb93872ebc1333ad370894c39a6", "commit_rev":
"3032e512f4c50e0275cc2306cf75208ee7c9a2c4"}

[commit-bot: I haz the power, 2017-02-23 08:57:28]

Description was changed from

==========
Expand FrameSeleciton::isNone() to increase chances of hoisting update layout

This patch expands |FrameSeleciton::isNone()| to increase chance of hoisting
update layout.

Note: This is mechanical change to replace |selection().isNone()|
by |selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()|
Following patch will remove deprecated version.

This patch is apart of expanding expensive member functions in |FrameSelection|:
- rootEditableElement()
- hasEditableStyle()
- isContentEditable()
- isContentRichlyEditable()
- getSelectionType()
- base() 
- extent()
- start()
- end()
- isNone() 
- isCaret()
- isRange()

BUG=694360
TEST=n/a; no behavior changes
==========

to

==========
Expand FrameSeleciton::isNone() to increase chances of hoisting update layout

This patch expands |FrameSeleciton::isNone()| to increase chance of hoisting
update layout.

Note: This is mechanical change to replace |selection().isNone()|
by |selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()|
Following patch will remove deprecated version.

This patch is apart of expanding expensive member functions in |FrameSelection|:
- rootEditableElement()
- hasEditableStyle()
- isContentEditable()
- isContentRichlyEditable()
- getSelectionType()
- base() 
- extent()
- start()
- end()
- isNone() 
- isCaret()
- isRange()

BUG=694360
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2710883002
Cr-Commit-Position: refs/heads/master@{#452435}
Committed:
https://chromium.googlesource.com/chromium/src/+/3032e512f4c50e0275cc2306cf75...
==========

[commit-bot: I haz the power, 2017-02-23 08:57:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3032e512f4c50e0275cc2306cf75...