Attachment 350158 Details for Bug 189767 – Patch

[patch] Patch

bug-189767-20180919162755.patch (text/plain), 14.02 KB, created by Per Arne Vollan on 2018-09-19 16:27:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Per Arne Vollan

Created: 2018-09-19 16:27:56 PDT

Size: 14.02 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 236230)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2018-09-19  Per Arne Vollan  <pvollan@apple.com>
>+
>+        [WebVTT] Update the parser according to the new region syntax.
>+        https://bugs.webkit.org/show_bug.cgi?id=189767
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The majority of the code added in this patch is adopted from the Chromium project, which has added
>+        support for the new region syntax. The complete parser specification can be found at
>+        https://w3c.github.io/webvtt/#file-parsing. One small difference in behavior is that the new parser
>+        will not add regions with empty id.
>+
>+        No new tests, covered by existing tests.
>+
>+        * html/track/WebVTTParser.cpp:
>+        (WebCore::WebVTTParser::getNewRegions):
>+        (WebCore::WebVTTParser::parse):
>+        (WebCore::WebVTTParser::collectRegionSettings):
>+        (WebCore::WebVTTParser::collectWebVTTBlock):
>+        (WebCore::WebVTTParser::checkAndRecoverCue):
>+        (WebCore::WebVTTParser::checkAndCreateRegion):
>+        (WebCore::WebVTTParser::checkAndStoreRegion):
>+        (WebCore::WebVTTParser::collectMetadataHeader): Deleted.
>+        (WebCore::WebVTTParser::createNewRegion): Deleted.
>+        * html/track/WebVTTParser.h:
>+
> 2018-09-19  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         REGRESSION(r235917): 2% regression in Dromaeo CSS selector on MacBookPro11,4
>Index: Source/WebCore/html/track/WebVTTParser.cpp
>===================================================================
>--- Source/WebCore/html/track/WebVTTParser.cpp	(revision 236212)
>+++ Source/WebCore/html/track/WebVTTParser.cpp	(working copy)
>@@ -51,6 +51,7 @@ const double secondsPerMinute = 60;
> const double secondsPerMillisecond = 0.001;
> const char* fileIdentifier = "WEBVTT";
> const unsigned fileIdentifierLength = 6;
>+const unsigned regionIdentifierLength = 6;
> 
> bool WebVTTParser::parseFloatPercentageValue(VTTScanner& valueScanner, float& percentage)
> {
>@@ -101,8 +102,7 @@ void WebVTTParser::getNewCues(Vector<Ref
> 
> void WebVTTParser::getNewRegions(Vector<RefPtr<VTTRegion>>& outputRegions)
> {
>-    outputRegions = m_regionList;
>-    m_regionList.clear();
>+    outputRegions = WTFMove(m_regionList);
> }
> 
> void WebVTTParser::parseFileHeader(String&& data)
>@@ -166,20 +166,12 @@ void WebVTTParser::parse()
>             break;
> 
>         case Header:
>-            collectMetadataHeader(*line);
>-
>-            if (line->isEmpty()) {
>-                // Steps 10-14 - Allow a header (comment area) under the WEBVTT line.
>-                if (m_client && m_regionList.size())
>-                    m_client->newRegionsParsed();
>-                m_state = Id;
>-                break;
>-            }
>-            // Step 15 - Break out of header loop if the line could be a timestamp line.
>-            if (line->contains("-->"))
>-                m_state = recoverCue(*line);
>+            // Steps 11 - 14 - Collect WebVTT block
>+            m_state = collectWebVTTBlock(*line);
>+            break;
> 
>-            // Step 16 - Line is not the empty string and does not contain "-->".
>+        case Region:
>+            m_state = collectRegionSettings(*line);
>             break;
> 
>         case Id:
>@@ -249,28 +241,90 @@ bool WebVTTParser::hasRequiredFileIdenti
>     return true;
> }
> 
>-void WebVTTParser::collectMetadataHeader(const String& line)
>+WebVTTParser::ParseState WebVTTParser::collectRegionSettings(const String& line)
> {
>-    // WebVTT header parsing (WebVTT parser algorithm step 12)
>-    static NeverDestroyed<const AtomicString> regionHeaderName("Region", AtomicString::ConstructFromLiteral);
>+    // End of region block
>+    if (checkAndStoreRegion(line))
>+        return checkAndRecoverCue(line);
>+    
>+    m_currentRegion->setRegionSettings(line);
>+    return Region;
>+}
> 
>-    // Step 12.4 If line contains the character ":" (A U+003A COLON), then set metadata's
>-    // name to the substring of line before the first ":" character and
>-    // metadata's value to the substring after this character.
>-    size_t colonPosition = line.find(':');
>-    if (colonPosition == notFound)
>-        return;
>-
>-    String headerName = line.substring(0, colonPosition);
>-
>-    // Step 12.5 If metadata's name equals "Region":
>-    if (headerName == regionHeaderName) {
>-        String headerValue = line.substring(colonPosition + 1, line.length() - 1);
>-        // Steps 12.5.1 - 12.5.11 Region creation: Let region be a new text track region [...]
>-        createNewRegion(headerValue);
>+WebVTTParser::ParseState WebVTTParser::collectWebVTTBlock(const String& line)
>+{
>+    // collect a WebVTT block parsing. (WebVTT parser algorithm step 14)
>+    
>+    // If Region support is enabled.
>+    if (checkAndCreateRegion(line))
>+        return Region;
>+    
>+    // Handle cue block.
>+    ParseState state = checkAndRecoverCue(line);
>+    if (state != Header) {
>+        if (m_client && !m_regionList.isEmpty())
>+            m_client->newRegionsParsed();
>+        if (!m_previousLine.isEmpty() && !m_previousLine.contains("-->"))
>+            m_currentId = AtomicString(m_previousLine);
>+        
>+        return state;
>+    }
>+    
>+    // store previous line for cue id.
>+    // length is more than 1 line clear m_previousLine_ and ignore line.
>+    if (m_previousLine.isEmpty())
>+        m_previousLine = line;
>+    else
>+        m_previousLine = "";
>+
>+    return state;
>+}
>+
>+WebVTTParser::ParseState WebVTTParser::checkAndRecoverCue(const String& line)
>+{
>+    // parse cue timings and settings
>+    if (line.contains("-->")) {
>+        ParseState state = recoverCue(line);
>+        if (state != BadCue)
>+            return state;
>     }
>+    return Header;
> }
> 
>+bool WebVTTParser::checkAndCreateRegion(const String& line)
>+{
>+    if (m_previousLine.contains("-->"))
>+        return false;
>+    // line starts with the substring "REGION" and remaining characters
>+    // zero or more U+0020 SPACE characters or U+0009 CHARACTER TABULATION
>+    // (tab) characters expected other than these charecters it is invalid.
>+    if (line.startsWith("REGION") && line.substring(regionIdentifierLength).isAllSpecialCharacters<isASpace>()) {
>+        m_currentRegion = VTTRegion::create(*m_scriptExecutionContext);
>+        return true;
>+    }
>+    return false;
>+}
>+
>+bool WebVTTParser::checkAndStoreRegion(const String& line)
>+{
>+    if (!line.isEmpty() && !line.contains("-->"))
>+        return false;
>+    
>+    if (!m_currentRegion->id().isEmpty()) {
>+        // Step 12.5.10 If the text track list of regions regions contains a region
>+        // with the same region identifier value as region, remove that region.
>+        for (size_t i = 0; i < m_regionList.size(); ++i) {
>+            if (m_regionList[i]->id() == m_currentRegion->id()) {
>+                m_regionList.remove(i);
>+                break;
>+            }
>+        }
>+        m_regionList.append(m_currentRegion);
>+    }
>+    m_currentRegion = nullptr;
>+    return true;
>+}
>+    
> WebVTTParser::ParseState WebVTTParser::collectCueId(const String& line)
> {
>     if (line.contains("-->"))
>@@ -422,27 +476,6 @@ void WebVTTParser::resetCueValues()
>     m_currentContent.clear();
> }
> 
>-void WebVTTParser::createNewRegion(const String& headerValue)
>-{
>-    if (headerValue.isEmpty())
>-        return;
>-
>-    // Steps 12.5.1 - 12.5.9 - Construct and initialize a WebVTT Region object.
>-    RefPtr<VTTRegion> region = VTTRegion::create(*m_scriptExecutionContext);
>-    region->setRegionSettings(headerValue);
>-
>-    // Step 12.5.10 If the text track list of regions regions contains a region
>-    // with the same region identifier value as region, remove that region.
>-    for (size_t i = 0; i < m_regionList.size(); ++i)
>-        if (m_regionList[i]->id() == region->id()) {
>-            m_regionList.remove(i);
>-            break;
>-        }
>-
>-    // Step 12.5.11
>-    m_regionList.append(region);
>-}
>-
> bool WebVTTParser::collectTimeStamp(const String& line, MediaTime& timeStamp)
> {
>     if (line.isEmpty())
>Index: Source/WebCore/html/track/WebVTTParser.h
>===================================================================
>--- Source/WebCore/html/track/WebVTTParser.h	(revision 236212)
>+++ Source/WebCore/html/track/WebVTTParser.h	(working copy)
>@@ -103,6 +103,7 @@ public:
>         Id,
>         TimingsAndSettings,
>         CueText,
>+        Region,
>         BadCue,
>         Finished
>     };
>@@ -118,6 +119,14 @@ public:
>             || tagName == rtTag;
>     }
> 
>+    static inline bool isASpace(UChar c)
>+    {
>+        // WebVTT space characters are U+0020 SPACE, U+0009 CHARACTER
>+        // TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF), and
>+        // U+000D CARRIAGE RETURN (CR).
>+        return c == ' ' || c == '\t' || c == '\n' || c == '\f' || c == '\r';
>+    }
>+
>     static inline bool isValidSettingDelimiter(UChar c)
>     {
>         // ... a WebVTT cue consists of zero or more of the following components, in any order, separated from each other by one or more 
>@@ -157,13 +166,15 @@ private:
>     ParseState collectCueText(const String&);
>     ParseState recoverCue(const String&);
>     ParseState ignoreBadCue(const String&);
>-
>+    ParseState collectRegionSettings(const String&);
>+    ParseState collectWebVTTBlock(const String&);
>+    ParseState checkAndRecoverCue(const String& line);
>+    bool checkAndCreateRegion(const String& line);
>+    bool checkAndStoreRegion(const String& line);
>+    
>     void createNewCue();
>     void resetCueValues();
> 
>-    void collectMetadataHeader(const String&);
>-    void createNewRegion(const String& headerValue);
>-
>     static bool collectTimeStamp(VTTScanner& input, MediaTime& timeStamp);
> 
>     BufferedLineReader m_lineReader;
>@@ -172,8 +183,10 @@ private:
>     MediaTime m_currentStartTime;
>     MediaTime m_currentEndTime;
>     StringBuilder m_currentContent;
>+    String m_previousLine;
>     String m_currentSettings;
>-
>+    RefPtr<VTTRegion> m_currentRegion;
>+    
>     WebVTTParserClient* m_client;
> 
>     Vector<RefPtr<WebVTTCueData>> m_cuelist;
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 236212)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-09-19  Per Arne Vollan  <pvollan@apple.com>
>+
>+        [WebVTT] Update the parser according to the new region syntax.
>+        https://bugs.webkit.org/show_bug.cgi?id=189767
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * media/track/captions-webvtt/captions-regions.vtt:
>+        * media/track/captions-webvtt/header-regions.vtt:
>+        * media/track/regions-webvtt/vtt-region-parser-expected.txt:
>+        * media/track/regions-webvtt/vtt-region-parser.html:
>+
> 2018-09-19  Youenn Fablet  <youenn@apple.com>
> 
>         Layout Test webrtc/video-mute.html is flaky.
>Index: LayoutTests/media/track/captions-webvtt/captions-regions.vtt
>===================================================================
>--- LayoutTests/media/track/captions-webvtt/captions-regions.vtt	(revision 236212)
>+++ LayoutTests/media/track/captions-webvtt/captions-regions.vtt	(working copy)
>@@ -1,5 +1,11 @@
> WEBVTT FILE
>-Region: id=fred width=50% height=2 regionanchor=50%,100% viewportanchor=10%,40%
>+
>+REGION
>+id=fred
>+width=50%
>+height=2
>+regionanchor=50%,100%
>+viewportanchor=10%,40%
> 
> 1
> 00:00:00.000 --> 00:00:01.500 region:fred
>@@ -16,4 +22,4 @@ Cue
> 
> 4
> 00:00:02.000 --> 00:00:03.000 region:fred
>-Fourth cue!
>\ No newline at end of file
>+Fourth cue!
>Index: LayoutTests/media/track/captions-webvtt/header-regions.vtt
>===================================================================
>--- LayoutTests/media/track/captions-webvtt/header-regions.vtt	(revision 236212)
>+++ LayoutTests/media/track/captions-webvtt/header-regions.vtt	(working copy)
>@@ -1,10 +1,31 @@
> WEBVTT FILE
>-Region: id=region_without_settings
>-Region: id=region_with_all_settings width=32% height=5 regionanchor=41%,20% viewportanchor=31%,84% scroll=up
>-Region: id=region_floating_point_anchor regionanchor=41.133%,20.42% viewportanchor=32.33%,32.44%
>-Region: id=not_unique_id width=42%
>-Region: id=not_unique_id width=67%
>-Region: invalid_settings values but region still created
>+
>+REGION
>+id=region_without_settings
>+
>+REGION
>+id=region_with_all_settings
>+width=32%
>+height=5
>+regionanchor=41%,20%
>+viewportanchor=31%,84%
>+scroll=up
>+
>+REGION
>+id=region_floating_point_anchor
>+regionanchor=41.133%,20.42%
>+viewportanchor=32.33%,32.44%
>+
>+REGION
>+id=not_unique_id
>+width=42%
>+
>+REGION
>+id=not_unique_id
>+width=67%
>+
>+REGION
>+invalid_settings values but region still created
> : Invalid Header
> 
> 1
>Index: LayoutTests/media/track/regions-webvtt/vtt-region-parser-expected.txt
>===================================================================
>--- LayoutTests/media/track/regions-webvtt/vtt-region-parser-expected.txt	(revision 236212)
>+++ LayoutTests/media/track/regions-webvtt/vtt-region-parser-expected.txt	(working copy)
>@@ -1,7 +1,7 @@
> Tests proper parsing of various regions present in WebVTT header area.
> 
> 
>-EXPECTED (video.textTracks[0].regions.length == '5') OK
>+EXPECTED (video.textTracks[0].regions.length == '4') OK
> 
> EXPECTED (region.id == 'region_without_settings') OK
> 
>@@ -23,7 +23,5 @@ EXPECTED (Math.round(region.viewportAnch
> EXPECTED (region.id == 'not_unique_id') OK
> EXPECTED (region.width == '67') OK
> 
>-EXPECTED (region.id == '') OK
>-
> END OF TEST
> 
>Index: LayoutTests/media/track/regions-webvtt/vtt-region-parser.html
>===================================================================
>--- LayoutTests/media/track/regions-webvtt/vtt-region-parser.html	(revision 236212)
>+++ LayoutTests/media/track/regions-webvtt/vtt-region-parser.html	(working copy)
>@@ -16,7 +16,7 @@
>             }
> 
>             findMediaElement();
>-            testExpected("video.textTracks[0].regions.length", 5);
>+            testExpected("video.textTracks[0].regions.length", 4);
> 
>             consoleWrite("");
>             region = video.textTracks[0].regions[0];
>@@ -47,10 +47,6 @@
>             testExpected("region.width", 67);
> 
>             consoleWrite("");
>-            region = video.textTracks[0].regions[4];
>-            testExpected("region.id", "");
>-
>-            consoleWrite("");
>             endTest();
>         }
>

Flags:

eric.carlson: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189767: 350158 | 350206 | 350217