Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in inSameLine

Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in inSameLine

BUG=590369
Committed: https://crrev.com/d5a658f547e994cbf85379878361af501bcbf879
Cr-Commit-Position: refs/heads/master@{#418148}

[Xiaocheng, 2016-09-12 10:29:38]

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

[commit-bot: I haz the power, 2016-09-12 10:29:54]

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

[Xiaocheng, 2016-09-12 11:31:03]

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

[Xiaocheng, 2016-09-12 11:31:04]

PTAL.

Can I assume that a function with a |VisiblePosition| parameter must be called
with clean layout? In my intuition, the parameter is not going to make sense
when called with dirty.

[commit-bot: I haz the power, 2016-09-12 11:50:45]

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

[commit-bot: I haz the power, 2016-09-12 11:50:46]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-09-13 01:40:33]

lgtm

On 2016/09/12 at 11:31:04, xiaochengh wrote:
> PTAL.
> 
> Can I assume that a function with a |VisiblePosition| parameter must be called
with clean layout? In my intuition, the parameter is not going to make sense
when called with dirty.

Your assumption is correct but it has more condition.
VP's lifetime is very short and until next layout.
VP requires no DOM tree modification and style changes.
Here is pseudo code for VP usage:

VP::VP(const Position deepEquvialent)
  : m_deepEquivalen(deepEquivalent)
  , m_domTreeVersion(document().domTreeVersion()))
  , m_styleTreeVersion(document().styleTreeVersion)) {
  document().assertLayoutTreeIsClean();
}


bool VP::isValid() const {
  return document().layoutTreeIsClean() 
      && m_domTreeVersion == document().domTreeVersion()
      && m_styleTreeVersion == document().styleTreeVersion()
}

Position VP::deepEquivalen() const {
  DCHECK(isValid()) << *this;
  return m_deepEquivalent;
  
}

Function taking VP should have DCHECK(visiblePosition.isValud()) <<
visiblePosition;

It seems most of execCommand implementations violate this assumption. They keep
VP on stack then use after DOM tree modification.

[Xiaocheng, 2016-09-13 02:01:41]

On 2016/09/13 at 01:40:33, yosin wrote:
> lgtm
> 
> On 2016/09/12 at 11:31:04, xiaochengh wrote:
> > PTAL.
> > 
> > Can I assume that a function with a |VisiblePosition| parameter must be
called with clean layout? In my intuition, the parameter is not going to make
sense when called with dirty.
> 
> Your assumption is correct but it has more condition.
> VP's lifetime is very short and until next layout.
> VP requires no DOM tree modification and style changes.
> Here is pseudo code for VP usage:
> 
> VP::VP(const Position deepEquvialent)
>   : m_deepEquivalen(deepEquivalent)
>   , m_domTreeVersion(document().domTreeVersion()))
>   , m_styleTreeVersion(document().styleTreeVersion)) {
>   document().assertLayoutTreeIsClean();
> }
> 
> 
> bool VP::isValid() const {
>   return document().layoutTreeIsClean() 
>       && m_domTreeVersion == document().domTreeVersion()
>       && m_styleTreeVersion == document().styleTreeVersion()
> }
> 
> Position VP::deepEquivalen() const {
>   DCHECK(isValid()) << *this;
>   return m_deepEquivalent;
>   
> }
> 
> Function taking VP should have DCHECK(visiblePosition.isValud()) <<
visiblePosition;
> 
> It seems most of execCommand implementations violate this assumption. They
keep VP on stack then use after DOM tree modification.

I see. Thanks.

[tkent, 2016-09-13 02:07:05]

lgtm

[Xiaocheng, 2016-09-13 02:47:40]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2016-09-13 02:47:50]

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, 2016-09-13 02:51:43]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-13 02:53:47]

Description was changed from

==========
Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in inSameLine

BUG=590369
==========

to

==========
Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in inSameLine

BUG=590369

Committed: https://crrev.com/d5a658f547e994cbf85379878361af501bcbf879
Cr-Commit-Position: refs/heads/master@{#418148}
==========

[commit-bot: I haz the power, 2016-09-13 02:53:48]

Patchset 1 (id:??) landed as
https://crrev.com/d5a658f547e994cbf85379878361af501bcbf879
Cr-Commit-Position: refs/heads/master@{#418148}