Audit use of updateStyleAndLayoutIgnorePendingStylesheets in EditingStyle::removeStyleFromRulesAndC…

Audit use of updateStyleAndLayoutIgnorePendingStylesheets in EditingStyle::removeStyleFromRulesAndContext

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

[Xiaocheng, 2016-10-17 07:53:44]

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

[commit-bot: I haz the power, 2016-10-17 07:54:12]

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

[Xiaocheng, 2016-10-17 09:03:22]

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

[Xiaocheng, 2016-10-17 09:03:23]

PTAL.

[commit-bot: I haz the power, 2016-10-17 09:26:53]

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

[commit-bot: I haz the power, 2016-10-17 09:26:54]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-10-17 09:45:18]

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingStyle.cpp:1417:
DCHECK_GE(element->document().lifecycle().state(),
Could you add DCHECK(element->document().lifecylce().isActive()) for just in
case?
Since, Stopping and Stopped is grater than StyleClean. :-<

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
(right):

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:612:
document().updateStyleAndLayoutIgnorePendingStylesheets();
Should we call Document::updateStyle() instead of
updateStyleAndLayoutIgnorePendingStyleSheets() since a comment above says it for
StyleResolver.

[Xiaocheng, 2016-10-17 10:06:47]

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

[commit-bot: I haz the power, 2016-10-17 10:07:10]

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

[Xiaocheng, 2016-10-17 10:07:27]

Updated. PTAL.

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingStyle.cpp:1417:
DCHECK_GE(element->document().lifecycle().state(),
On 2016/10/17 at 09:45:18, Yosi_UTC9 wrote:
> Could you add DCHECK(element->document().lifecylce().isActive()) for just in
case?
> Since, Stopping and Stopped is grater than StyleClean. :-<

Thanks for the catch. Done.

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
(right):

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:612:
document().updateStyleAndLayoutIgnorePendingStylesheets();
On 2016/10/17 at 09:45:18, Yosi_UTC9 wrote:
> Should we call Document::updateStyle() instead of
updateStyleAndLayoutIgnorePendingStyleSheets() since a comment above says it for
StyleResolver.

Document::updateStyle is private...

[commit-bot: I haz the power, 2016-10-17 11:42:53]

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

[commit-bot: I haz the power, 2016-10-17 11:42:54]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-10-18 01:40:19]

lgtm after adding TODO regarding update style.

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
(right):

https://codereview.chromium.org/2422133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp:612:
document().updateStyleAndLayoutIgnorePendingStylesheets();
On 2016/10/17 at 10:07:27, Xiaocheng wrote:
> On 2016/10/17 at 09:45:18, Yosi_UTC9 wrote:
> > Should we call Document::updateStyle() instead of
updateStyleAndLayoutIgnorePendingStyleSheets() since a comment above says it for
StyleResolver.
> 
> Document::updateStyle is private...
Could you leave TODO comment that we need to update only style w/ ignoring
pending style sheets?

It seems current implementation of Document::updateStyle() isn't ready for
calling out side Document::updateStyleAndLayout().
We need to do following before calling updateStyle():
  DocumentAnimations::updateAnimationTimingIfNeeded(*this);
  evaluateMediaQueryListIfNeeded();
  updateUseShadowTreesIfNeeded();
  updateDistribution();
  updateStyleInvalidationIfNeeded();

We might want to have updateLifcycleToStyleClean() similar to
FrameView::updateLifecylceToLayoutClean() in Document.

[Xiaocheng, 2016-10-18 02:12:17]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-10-18 02:12:17]

Committing with more notes added.

[Xiaocheng, 2016-10-18 02:12:18]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2422133002/#ps40001
(title: "Add further notes")

[commit-bot: I haz the power, 2016-10-18 02:12:51]

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-10-18 03:33:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-18 03:36:10]

Description was changed from

==========
Audit use of updateStyleAndLayoutIgnorePendingStylesheets in
EditingStyle::removeStyleFromRulesAndContext

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

to

==========
Audit use of updateStyleAndLayoutIgnorePendingStylesheets in
EditingStyle::removeStyleFromRulesAndContext

BUG=590369

Committed: https://crrev.com/7b5e3814db7dc05daa6522663ab9f2c4452a805c
Cr-Commit-Position: refs/heads/master@{#425885}
==========

[commit-bot: I haz the power, 2016-10-18 03:36:11]

Patchset 3 (id:??) landed as
https://crrev.com/7b5e3814db7dc05daa6522663ab9f2c4452a805c
Cr-Commit-Position: refs/heads/master@{#425885}