Attachment 357015 Details for Bug 192575 – Cleanup

[patch] Cleanup

bug-192575-20181210163905.patch (text/plain), 9.85 KB, created by Ryosuke Niwa on 2018-12-10 16:39:06 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-12-10 16:39:06 PST

Size: 9.85 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 239060)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,25 @@
>+2018-12-10  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Use WeakPtr to refer to VTTCue in VTTCueBox
>+        https://bugs.webkit.org/show_bug.cgi?id=192575
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Address the FIXME in VTTCue::~VTTCue by clearing VTTCueBox::m_cue when VTTCue goes away.
>+        This is implemented by simply using WeakPtr.
>+
>+        No new tests since there shoul be no behaivoral change.
>+
>+        * html/track/TextTrackCueGeneric.cpp:
>+        (WebCore::TextTrackCueGenericBoxElement::applyCSSProperties):
>+        * html/track/VTTCue.cpp:
>+        (WebCore::VTTCueBox::VTTCueBox):
>+        (WebCore::VTTCueBox::getCue const):
>+        (WebCore::VTTCueBox::applyCSSProperties):
>+        (WebCore::VTTCue::~VTTCue):
>+        * html/track/VTTCue.h:
>+        (WebCore::VTTCueBox::fontSizeFromCaptionUserPrefs const):
>+
> 2018-12-10  Antti Koivisto  <antti@apple.com>
> 
>         Rename "forced style recalc" to "full style rebuild"
>Index: Source/WebCore/html/track/TextTrackCueGeneric.cpp
>===================================================================
>--- Source/WebCore/html/track/TextTrackCueGeneric.cpp	(revision 239058)
>+++ Source/WebCore/html/track/TextTrackCueGeneric.cpp	(working copy)
>@@ -68,10 +68,13 @@ TextTrackCueGenericBoxElement::TextTrack
> 
> void TextTrackCueGenericBoxElement::applyCSSProperties(const IntSize& videoSize)
> {
>+    RefPtr<TextTrackCueGeneric> cue = static_cast<TextTrackCueGeneric*>(getCue());
>+    if (!cue)
>+        return;
>+
>     setInlineStyleProperty(CSSPropertyPosition, CSSValueAbsolute);
>     setInlineStyleProperty(CSSPropertyUnicodeBidi, CSSValuePlaintext);
>-    
>-    RefPtr<TextTrackCueGeneric> cue = static_cast<TextTrackCueGeneric*>(getCue());
>+
>     Ref<HTMLSpanElement> cueElement = cue->element();
> 
>     double textPosition = cue->calculateComputedTextPosition();
>@@ -92,16 +95,16 @@ void TextTrackCueGenericBoxElement::appl
>         if (cue->fontSizeMultiplier())
>             authorFontSize *= cue->fontSizeMultiplier() / 100;
> 
>-        double multiplier = m_fontSizeFromCaptionUserPrefs / authorFontSize;
>+        double multiplier = fontSizeFromCaptionUserPrefs() / authorFontSize;
>         double newCueSize = std::min(size * multiplier, 100.0);
>         if (cue->getWritingDirection() == VTTCue::Horizontal) {
>             setInlineStyleProperty(CSSPropertyWidth, newCueSize, CSSPrimitiveValue::CSS_PERCENTAGE);
>             if ((alignment == CSSValueMiddle || alignment == CSSValueCenter) && multiplier != 1.0)
>-                setInlineStyleProperty(CSSPropertyLeft, static_cast<double>(textPosition - (newCueSize - m_cue.getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>+                setInlineStyleProperty(CSSPropertyLeft, static_cast<double>(textPosition - (newCueSize - cue->getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>         } else {
>             setInlineStyleProperty(CSSPropertyHeight, newCueSize,  CSSPrimitiveValue::CSS_PERCENTAGE);
>             if ((alignment == CSSValueMiddle || alignment == CSSValueCenter) && multiplier != 1.0)
>-                setInlineStyleProperty(CSSPropertyTop, static_cast<double>(cue->line() - (newCueSize - m_cue.getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>+                setInlineStyleProperty(CSSPropertyTop, static_cast<double>(cue->line() - (newCueSize - cue->getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>         }
>     }
> 
>Index: Source/WebCore/html/track/VTTCue.cpp
>===================================================================
>--- Source/WebCore/html/track/VTTCue.cpp	(revision 239058)
>+++ Source/WebCore/html/track/VTTCue.cpp	(working copy)
>@@ -133,20 +133,25 @@ Ref<VTTCueBox> VTTCueBox::create(Documen
> 
> VTTCueBox::VTTCueBox(Document& document, VTTCue& cue)
>     : HTMLElement(divTag, document)
>-    , m_cue(cue)
>+    , m_cue(makeWeakPtr(cue))
> {
>     setPseudo(vttCueBoxShadowPseudoId());
> }
> 
> VTTCue* VTTCueBox::getCue() const
> {
>-    return &m_cue;
>+    return m_cue.get();
> }
> 
> void VTTCueBox::applyCSSProperties(const IntSize& videoSize)
> {
>+    if (!m_cue)
>+        return;
>+
>+    auto cue = makeRef(*m_cue);
>+
>     // FIXME: Apply all the initial CSS positioning properties. http://wkb.ug/79916
>-    if (!m_cue.regionId().isEmpty()) {
>+    if (!cue->regionId().isEmpty()) {
>         setInlineStyleProperty(CSSPropertyPosition, CSSValueRelative);
>         return;
>     }
>@@ -160,12 +165,12 @@ void VTTCueBox::applyCSSProperties(const
>     setInlineStyleProperty(CSSPropertyUnicodeBidi, CSSValuePlaintext);
> 
>     // the 'direction' property must be set to direction
>-    setInlineStyleProperty(CSSPropertyDirection, m_cue.getCSSWritingDirection());
>+    setInlineStyleProperty(CSSPropertyDirection, cue->getCSSWritingDirection());
> 
>     // the 'writing-mode' property must be set to writing-mode
>-    setInlineStyleProperty(CSSPropertyWritingMode, m_cue.getCSSWritingMode(), false);
>+    setInlineStyleProperty(CSSPropertyWritingMode, cue->getCSSWritingMode(), false);
> 
>-    auto position = m_cue.getCSSPosition();
>+    auto position = cue->getCSSPosition();
> 
>     // the 'top' property must be set to top,
>     setInlineStyleProperty(CSSPropertyTop, position.second, CSSPrimitiveValue::CSS_PERCENTAGE);
>@@ -178,39 +183,39 @@ void VTTCueBox::applyCSSProperties(const
>     if (authorFontSize)
>         multiplier = m_fontSizeFromCaptionUserPrefs / authorFontSize;
> 
>-    double textPosition = m_cue.calculateComputedTextPosition();
>+    double textPosition = cue->calculateComputedTextPosition();
>     double maxSize = 100.0;
>-    CSSValueID alignment = m_cue.getCSSAlignment();
>+    CSSValueID alignment = cue->getCSSAlignment();
>     if (alignment == CSSValueEnd || alignment == CSSValueRight)
>         maxSize = textPosition;
>     else if (alignment == CSSValueStart || alignment == CSSValueLeft)
>         maxSize = 100.0 - textPosition;
> 
>-    double newCueSize = std::min(m_cue.getCSSSize() * multiplier, 100.0);
>+    double newCueSize = std::min(cue->getCSSSize() * multiplier, 100.0);
>     // the 'width' property must be set to width, and the 'height' property  must be set to height
>-    if (m_cue.vertical() == horizontalKeyword()) {
>+    if (cue->vertical() == horizontalKeyword()) {
>         setInlineStyleProperty(CSSPropertyWidth, newCueSize, CSSPrimitiveValue::CSS_PERCENTAGE);
>         setInlineStyleProperty(CSSPropertyHeight, CSSValueAuto);
>         setInlineStyleProperty(CSSPropertyMinWidth, "min-content");
>         setInlineStyleProperty(CSSPropertyMaxWidth, maxSize, CSSPrimitiveValue::CSS_PERCENTAGE);
>         if ((alignment == CSSValueMiddle || alignment == CSSValueCenter) && multiplier != 1.0)
>-            setInlineStyleProperty(CSSPropertyLeft, static_cast<double>(position.first - (newCueSize - m_cue.getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>+            setInlineStyleProperty(CSSPropertyLeft, static_cast<double>(position.first - (newCueSize - cue->getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>     } else {
>         setInlineStyleProperty(CSSPropertyWidth, CSSValueAuto);
>         setInlineStyleProperty(CSSPropertyHeight, newCueSize, CSSPrimitiveValue::CSS_PERCENTAGE);
>         setInlineStyleProperty(CSSPropertyMinHeight, "min-content");
>         setInlineStyleProperty(CSSPropertyMaxHeight, maxSize, CSSPrimitiveValue::CSS_PERCENTAGE);
>         if ((alignment == CSSValueMiddle || alignment == CSSValueCenter) && multiplier != 1.0)
>-            setInlineStyleProperty(CSSPropertyTop, static_cast<double>(position.second - (newCueSize - m_cue.getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>+            setInlineStyleProperty(CSSPropertyTop, static_cast<double>(position.second - (newCueSize - cue->getCSSSize()) / 2), CSSPrimitiveValue::CSS_PERCENTAGE);
>     }
> 
>     // The 'text-align' property on the (root) List of WebVTT Node Objects must
>     // be set to the value in the second cell of the row of the table below
>     // whose first cell is the value of the corresponding cue's text track cue
>     // alignment:
>-    setInlineStyleProperty(CSSPropertyTextAlign, m_cue.getCSSAlignment());
>+    setInlineStyleProperty(CSSPropertyTextAlign, cue->getCSSAlignment());
>     
>-    if (!m_cue.snapToLines()) {
>+    if (!cue->snapToLines()) {
>         // 10.13.1 Set up x and y:
>         // Note: x and y are set through the CSS left and top above.
> 
>@@ -229,7 +234,7 @@ void VTTCueBox::applyCSSProperties(const
> 
>     // Make sure shadow or stroke is not clipped.
>     setInlineStyleProperty(CSSPropertyOverflow, CSSValueVisible);
>-    m_cue.element().setInlineStyleProperty(CSSPropertyOverflow, CSSValueVisible);
>+    cue->element().setInlineStyleProperty(CSSPropertyOverflow, CSSValueVisible);
> }
> 
> const AtomicString& VTTCueBox::vttCueBoxShadowPseudoId()
>@@ -277,9 +282,6 @@ VTTCue::VTTCue(ScriptExecutionContext& c
> 
> VTTCue::~VTTCue()
> {
>-    // FIXME: We should set m_cue in VTTCueBox to nullptr instead.
>-    if (m_displayTree && m_displayTree->document().refCount())
>-        m_displayTree->remove();
> }
> 
> void VTTCue::initialize(ScriptExecutionContext& context)
>Index: Source/WebCore/html/track/VTTCue.h
>===================================================================
>--- Source/WebCore/html/track/VTTCue.h	(revision 239058)
>+++ Source/WebCore/html/track/VTTCue.h	(working copy)
>@@ -64,13 +64,16 @@ protected:
> 
>     RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) final;
> 
>-    VTTCue& m_cue;
>+    int fontSizeFromCaptionUserPrefs() const { return m_fontSizeFromCaptionUserPrefs; }
>+
>+private:
>+    WeakPtr<VTTCue> m_cue;
>     int m_fontSizeFromCaptionUserPrefs;
> };
> 
> // ----------------------------
> 
>-class VTTCue : public TextTrackCue {
>+class VTTCue : public TextTrackCue, public CanMakeWeakPtr<VTTCue> {
> public:
>     static Ref<VTTCue> create(ScriptExecutionContext& context, double start, double end, const String& content)
>     {

Flags:

eric.carlson: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 192575: 357015