Element should not use confusingAndOftenMisusedAttached

Element should not use confusingAndOftenMisusedAttached

Instead introduce Node::inActiveDocument() and use that. To do this I had
to rename a method of the same name on HTMLMediaElement. All callers in
Element.cpp that were checking attached really just want to know if the
element is in an active document and participating in recalcStyle.

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

[ojan, 2013-10-07 21:34:15]

lgtm


On Mon, Oct 7, 2013 at 2:31 PM, <esprehn@chromium.org> wrote:

> Reviewers: abarth, ojan,
>
> Description:
> Element should not use confusingAndOftenMisusedAttach**ed
>
> Instead introduce Node::inActiveDocument() and use that. To do this I had
> to rename a method of the same name on HTMLMediaElement. All callers in
> Element.cpp that were checking attached really just want to know if the
> element is in an active document and participating in recalcStyle.
>
> Please review this at
https://codereview.chromium.**org/26270004/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/blink/**trunk<http://svn.chromium.org/blink/trunk>
>
> Affected files (+22, -14 lines):
>   M Source/core/dom/Element.cpp
>   M Source/core/dom/Node.h
>   M Source/core/dom/Node.cpp
>   M Source/core/html/**HTMLMediaElement.h
>   M Source/core/html/**HTMLMediaElement.cpp
>   M Source/core/rendering/**RenderVideo.cpp
>
>
> Index: Source/core/dom/Element.cpp
> diff --git a/Source/core/dom/Element.cpp b/Source/core/dom/Element.cpp
> index 2a63e21ece950eac77f074f0b97f2b**eaaabcdf34..**
> e011dc8331a816c95b9060f33d2398**b60acaa181 100644
> --- a/Source/core/dom/Element.cpp
> +++ b/Source/core/dom/Element.cpp
> @@ -983,7 +983,7 @@ void Element::attributeChanged(**const QualifiedName&
> name, const AtomicString& ne
>      document().incDOMTreeVersion()**;
>
>      StyleResolver* styleResolver = document().**styleResolverIfExists();
> -    bool testShouldInvalidateStyle = confusingAndOftenMisusedAttach**ed()
> && styleResolver && styleChangeType() < SubtreeStyleChange;
> +    bool testShouldInvalidateStyle = inActiveDocument() && styleResolver
> && styleChangeType() < SubtreeStyleChange;
>      bool shouldInvalidateStyle = false;
>
>      if (isStyledElement() && name == styleAttr) {
> @@ -1098,7 +1098,7 @@ static bool checkSelectorForClassChange(**const
> SpaceSplitString& oldClasses, cons
>  void Element::**classAttributeChanged(const AtomicString& newClassString)
>  {
>      StyleResolver* styleResolver = document().**styleResolverIfExists();
> -    bool testShouldInvalidateStyle = confusingAndOftenMisusedAttach**ed()
> && styleResolver && styleChangeType() < SubtreeStyleChange;
> +    bool testShouldInvalidateStyle = inActiveDocument() && styleResolver
> && styleChangeType() < SubtreeStyleChange;
>      bool shouldInvalidateStyle = false;
>
>      if (classStringHasClassName(**newClassString)) {
> @@ -1770,7 +1770,7 @@ static void inline checkForEmptyStyleChange(**Element*
> element, RenderStyle* style
>  static void checkForSiblingStyleChanges(**Element* e, RenderStyle*
> style, bool finishedParsingCallback,
>                                          Node* beforeChange, Node*
> afterChange, int childCountDelta)
>  {
> -    if (!e->**confusingAndOftenMisusedAttach**ed() ||
e->document().**hasPendingForcedStyleRecalc()
> || e->styleChangeType() >= SubtreeStyleChange)
> +    if (!e->inActiveDocument() ||
e->document().**hasPendingForcedStyleRecalc()
> || e->styleChangeType() >= SubtreeStyleChange)
>          return;
>
>      // :empty selector.
> @@ -2317,7 +2317,7 @@ RenderStyle* Element::computedStyle(**PseudoId
> pseudoElementSpecifier)
>              return usedStyle;
>      }
>
> -    if (!**confusingAndOftenMisusedAttach**ed())
> +    if (!inActiveDocument())
>          // FIXME: Try to do better than this. Ensure that
> styleForElement() works for elements that are not in the
>          // document tree and figure out when to destroy the computed
> style for such elements.
>          return 0;
> @@ -2935,7 +2935,7 @@ void Element::willModifyAttribute(**const
> QualifiedName& name, const AtomicString&
>      }
>
>      if (oldValue != newValue) {
> -        if (**confusingAndOftenMisusedAttach**ed() &&
> hasSelectorForAttribute(&**document(), name.localName()))
> +        if (inActiveDocument() && hasSelectorForAttribute(&**document(),
> name.localName()))
>             setNeedsStyleRecalc();
>
>          if (isUpgradedCustomElement())
> Index: Source/core/dom/Node.cpp
> diff --git a/Source/core/dom/Node.cpp b/Source/core/dom/Node.cpp
> index cb1d7d4a3e911627a66f7e71a9c44a**92ce2cb11b..**
> 351bed8d336d0adfb12385b630dddc**a6452e45ef 100644
> --- a/Source/core/dom/Node.cpp
> +++ b/Source/core/dom/Node.cpp
> @@ -744,6 +744,11 @@ void Node::setNeedsStyleRecalc(**StyleChangeType
> changeType, StyleChangeSource sou
>          markAncestorsWithChildNeedsSty**leRecalc();
>  }
>
> +bool Node::inActiveDocument() const
> +{
> +    return inDocument() && document().isActive();
> +}
> +
>  void Node::lazyAttach()
>  {
>      markAncestorsWithChildNeedsSty**leRecalc();
> Index: Source/core/dom/Node.h
> diff --git a/Source/core/dom/Node.h b/Source/core/dom/Node.h
> index 20b17d7d44bbe4b31a46218ec010e6**3a766fa001..**
> 8a1921f8ce03258d08c18d3e39e05f**1274918ce8 100644
> --- a/Source/core/dom/Node.h
> +++ b/Source/core/dom/Node.h
> @@ -485,6 +485,8 @@ public:
>
>      TreeScope& treeScope() const { return *m_treeScope; }
>
> +    bool inActiveDocument() const;
> +
>      // Returns true if this node is associated with a document and is in
> its associated document's
>      // node tree, false otherwise.
>      bool inDocument() const
> Index: Source/core/html/**HTMLMediaElement.cpp
> diff --git a/Source/core/html/**HTMLMediaElement.cpp b/Source/core/html/**
> HTMLMediaElement.cpp
> index a468ec66144353d748003969e3be15**220a16cdee..**
> 24eea6beeaee06e505a3535b56b63c**08771a80a6 100644
> --- a/Source/core/html/**HTMLMediaElement.cpp
> +++ b/Source/core/html/**HTMLMediaElement.cpp
> @@ -273,7 +273,7 @@ HTMLMediaElement::**HTMLMediaElement(const
> QualifiedName& tagName, Document& docum
>      , m_playing(false)
>      , m_shouldDelayLoadEvent(false)
>      , m_haveFiredLoadedData(false)
> -    , m_inActiveDocument(true)
> +    , m_active(true)
>      , m_autoplaying(true)
>      , m_muted(false)
>      , m_paused(true)
> @@ -532,7 +532,7 @@ Node::**InsertionNotificationRequest
> HTMLMediaElement::**insertedInto(ContainerNode*
>
>      HTMLElement::insertedInto(**insertionPoint);
>      if (insertionPoint->inDocument()) {
> -        m_inActiveDocument = true;
> +        m_active = true;
>
>          if (!getAttribute(srcAttr).**isEmpty() && m_networkState ==
> NETWORK_EMPTY)
>              scheduleDelayedAction(**LoadMediaResource);
> @@ -546,7 +546,7 @@ void HTMLMediaElement::removedFrom(**ContainerNode*
> insertionPoint)
>  {
>      LOG(Media, "HTMLMediaElement::**removedFrom");
>
> -    m_inActiveDocument = false;
> +    m_active = false;
>      if (insertionPoint->inDocument()) {
>          configureMediaControls();
>          if (m_networkState > NETWORK_EMPTY)
> @@ -1270,7 +1270,7 @@ void HTMLMediaElement::**
> endIgnoringTrackDisplayUpdateR**equests()
>  {
>      ASSERT(m_**ignoreTrackDisplayUpdate);
>      --m_ignoreTrackDisplayUpdate;
> -    if (!m_ignoreTrackDisplayUpdate && m_inActiveDocument)
> +    if (!m_ignoreTrackDisplayUpdate && m_active)
>          updateActiveTextTrackCues(**currentTime());
>  }
>
> @@ -3485,7 +3485,7 @@ void HTMLMediaElement::stop()
>  {
>      LOG(Media, "HTMLMediaElement::stop");
>
> -    m_inActiveDocument = false;
> +    m_active = false;
>      userCancelledLoad();
>
>      // Stop the playback without generating events
> @@ -3521,7 +3521,7 @@ void HTMLMediaElement::resume()
>  {
>      LOG(Media, "HTMLMediaElement::resume");
>
> -    m_inActiveDocument = true;
> +    m_active = true;
>      setPausedInternal(false);
>
>      if (m_error && m_error->code() == MediaError::MEDIA_ERR_ABORTED) {
> Index: Source/core/html/**HTMLMediaElement.h
> diff --git a/Source/core/html/**HTMLMediaElement.h b/Source/core/html/**
> HTMLMediaElement.h
> index 3e16fc54369b52b0c4e07529b3979c**1021dfec06..**
> 9a24381ab8f9c696ae3f2a6364e5e1**68a7337bdc 100644
> --- a/Source/core/html/**HTMLMediaElement.h
> +++ b/Source/core/html/**HTMLMediaElement.h
> @@ -96,7 +96,7 @@ public:
>      };
>      void scheduleDelayedAction(**DelayedActionType);
>
> -    bool inActiveDocument() const { return m_inActiveDocument; }
> +    bool isActive() const { return m_active; }
>
>  // DOM API
>  // error state
> @@ -526,10 +526,11 @@ private:
>      typedef unsigned PendingActionFlags;
>      PendingActionFlags m_pendingActionFlags;
>
> +    // FIXME: MediaElement has way too many state bits.
>      bool m_playing : 1;
>      bool m_shouldDelayLoadEvent : 1;
>      bool m_haveFiredLoadedData : 1;
> -    bool m_inActiveDocument : 1;
> +    bool m_active : 1;
>      bool m_autoplaying : 1;
>      bool m_muted : 1;
>      bool m_paused : 1;
> Index: Source/core/rendering/**RenderVideo.cpp
> diff --git a/Source/core/rendering/**RenderVideo.cpp
> b/Source/core/rendering/**RenderVideo.cpp
> index 5971212b1a626b7b9954a27a232764**1abf041d6e..**
> 073785c22ccd21b0e6d03b3aaf9e30**a8f86fb40f 100644
> --- a/Source/core/rendering/**RenderVideo.cpp
> +++ b/Source/core/rendering/**RenderVideo.cpp
> @@ -210,7 +210,7 @@ void RenderVideo::updatePlayer()
>      if (!mediaPlayer)
>          return;
>
> -    if (!videoElement()->**inActiveDocument())
> +    if (!videoElement()->isActive())
>          return;
>
>      contentChanged(VideoChanged);
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-07 21:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/1

[commit-bot: I haz the power, 2013-10-07 23:43:37]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-08 17:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/1

[commit-bot: I haz the power, 2013-10-08 19:16:22]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[esprehn, 2013-10-08 23:25:59]

On 2013/10/08 19:16:22, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Blah, this apparently breaks a bunch of svg/dynamic-updates. I suspect what's
happened is that svg is calling into computedStyle() at a weird time.

[commit-bot: I haz the power, 2013-10-26 16:34:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/26001

[esprehn, 2013-10-26 19:00:11]

This is ready to land now that https://codereview.chromium.org/43223002/ landed
fixing SVG.

[commit-bot: I haz the power, 2013-10-26 19:00:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/26001

[commit-bot: I haz the power, 2013-10-26 19:42:28]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-26 20:06:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/26001

[commit-bot: I haz the power, 2013-10-26 20:43:04]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-26 20:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/26001

[commit-bot: I haz the power, 2013-10-26 21:49:40]

Retried try job too often on win_blink_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[esprehn, 2013-10-26 22:03:27]

Strangely this is failing AnimatedWebPTests.truncatedInBetweenFrame in
webkit_unit_tests but only on windows? Seems crazy...

[esprehn, 2013-10-26 22:06:36]

On 2013/10/26 22:03:27, esprehn wrote:
> Strangely this is failing AnimatedWebPTests.truncatedInBetweenFrame in
> webkit_unit_tests but only on windows? Seems crazy...

Blah, tree is hosed: build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7

[commit-bot: I haz the power, 2013-10-28 17:27:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/26270004/26001

[commit-bot: I haz the power, 2013-10-28 18:30:42]

Change committed as 160733