Bug 110263 – Add the default video poster if it doesn't exist in video tag

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

The Android web view application could provide the default poster for a video that doesn't have the poster attribute.

To provide the default poster, the application must set defaultVideoPosterURL setting and return the image in the response of that URL.

Created attachment 189171 [details]
Patch

Created attachment 189174 [details]
Patch

[Eric Carlson]

Comment on attachment 189174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189174&action=review

> Source/WebCore/ChangeLog:15
> +        No new tests, covered by existing tests.

How is this new functionallity covered by existing tests?

Sorry, I just found out I could write a Layout test to test this feature, I am working on it.

Created attachment 189357 [details]
Patch

Created attachment 189368 [details]
Patch

[Philippe Normand]

GTK build fails because the new symbols are not referenced in Source/autotools/symbols.filter

[Eric Carlson]

Comment on attachment 189368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189368&action=review

Marking r- because of the GTK problem.

> LayoutTests/media/video-default-poster.html:17
> +          document.getElementById("result-no-poster").innerText =
> +              poster.value == default_poster ? "PASS" : "FAIL";

Nit: I think the line break + ternary make this more difficult to understand instead of easier.

> LayoutTests/media/video-default-poster.html:20
> +      } else {
> +          document.getElementById("result-no-poster").innerText = "FAIL: poster attribute is null";
> +      }

Nit: single line if conditionals don't need braces.

> LayoutTests/media/video-default-poster.html:27
> +      if (poster) {
> +          document.getElementById("result-has-poster").innerText =
> +              poster.value == "content/abe.png" ? "PASS" : "FAIL: poster was changed";
> +      } else {
> +          document.getElementById("result-has-poster").innerText = "FAIL: poster attribute is null";
> +      }

Dittos.

> LayoutTests/media/video-no-default-poster.html:21
> +      document.getElementById("result-no-poster").innerText =
> +          poster ? "FAIL : poster was added." : "PASS";
> +      poster = document.getElementById("video-has-poster").getAttributeNode("poster");
> +      if (poster) {
> +          document.getElementById("result-has-poster").innerText =
> +              poster.value == "content/abe.png" ? "PASS" : "FAIL: poster was changed";
> +      } else {
> +          document.getElementById("result-has-poster").innerText = "FAIL: poster attribute is null";
> +      }

Dittos.

Created attachment 189562 [details]
Patch

[Eric Carlson]

Comment on attachment 189562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189562&action=review

> Source/WebCore/ChangeLog:22
> +        (WebCore::HTMLVideoElement::attach):
> +        * page/Settings.in:
> +        * testing/InternalSettings.cpp:
> +        (WebCore::InternalSettings::Backup::Backup):
> +        (WebCore::InternalSettings::Backup::restoreTo):
> +        (WebCore::InternalSettings::setDefaultVideoPosterURL):
> +        (WebCore):
> +        * testing/InternalSettings.h:
> +        (Backup):
> +        (InternalSettings):
> +        * testing/InternalSettings.idl:

Sorry I didn't notice this before, but it is helpful to have per-method comments about what changed here.

> LayoutTests/media/video-default-poster.html:11
> +    var default_poster = "about:blank";

Why use an invalid poster?

> LayoutTests/media/video-default-poster.html:17
> +      if (poster) {
> +        if (poster.value)
> +          document.getElementById("result-no-poster").innerText = "PASS";

Shouldn't this test the poster value before declaring success?

> ChangeLog:8
> +        * Source/autotools/symbols.filter:

Ditto.

Created attachment 189596 [details]
Patch

Comment on attachment 189562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189562&action=review

>> Source/WebCore/ChangeLog:22
>> +        * testing/InternalSettings.idl:
> 
> Sorry I didn't notice this before, but it is helpful to have per-method comments about what changed here.

Never mind, Done

>> LayoutTests/media/video-default-poster.html:11
>> +    var default_poster = "about:blank";
> 
> Why use an invalid poster?

I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation.
The default poster URL couldn't be related path in practice, so it shouldn't be an issue.

>> LayoutTests/media/video-default-poster.html:17
>> +          document.getElementById("result-no-poster").innerText = "PASS";
> 
> Shouldn't this test the poster value before declaring success?

Done

>> ChangeLog:8
>> +        * Source/autotools/symbols.filter:
> 
> Ditto.

Done

[Build Bot]

Comment on attachment 189596 [details]
Patch

Attachment 189596 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16722088

[Eric Carlson]

(In reply to comment #13)
> (From update of attachment 189596 [details])
> Attachment 189596 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16722088

10>WebCoreTestSupport.lib(InternalSettings.obj) : error LNK2019: unresolved external symbol "public: __thiscall WebCore::KURL::KURL(enum WebCore::ParsedURLStringTag,class WTF::String const &)" (??0KURL@WebCore@@QAE@W4ParsedURLStringTag@1@ABVString@WTF@@@Z) referenced in function "public: void __thiscall WebCore::InternalSettings::Backup::restoreTo(class WebCore::Settings *)" (?restoreTo@Backup@InternalSettings@WebCore@@QAEXPAVSettings@3@@Z)

Created attachment 189829 [details]
Patch

This should fix win bot.

[Eric Carlson]

(In reply to comment #16)
> This should fix win bot.

Looks like you need to rebase the patch:

Last 500 characters of output:
ebKitExports.def.in.rej
patching file Source/autotools/symbols.filter
Hunk #1 succeeded at 272 (offset 10 lines).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/media/video-default-poster-expected.txt
patching file LayoutTests/media/video-default-poster.html
patching file LayoutTests/media/video-no-default-poster-expected.txt
patching file LayoutTests/media/video-no-default-poster.html
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Created attachment 189839 [details]
Patch

Why win bot didn't showup?

Created attachment 190080 [details]
Patch

Rebase again.

[Darin Adler]

(In reply to comment #12)
> >> LayoutTests/media/video-default-poster.html:11
> >> +    var default_poster = "about:blank";
> > 
> > Why use an invalid poster?
> 
> I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation.
> The default poster URL couldn't be related path in practice, so it shouldn't be an issue.

The test can construct an absolute URL for content/abe.png.

(In reply to comment #22)
> (In reply to comment #12)
> > >> LayoutTests/media/video-default-poster.html:11
> > >> +    var default_poster = "about:blank";
> > > 
> > > Why use an invalid poster?
> > 
> > I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation.
> > The default poster URL couldn't be related path in practice, so it shouldn't be an issue.
> 
> The test can construct an absolute URL for content/abe.png.

Would you like point an example to me, thanks?

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #12)
> > > >> LayoutTests/media/video-default-poster.html:11
> > > >> +    var default_poster = "about:blank";
> > > > 
> > > > Why use an invalid poster?
> > > 
> > > I were trying to use content/abe.png, but the test failed because content/abe.png is not a valid URL for KURL's GURL implementation.
> > > The default poster URL couldn't be related path in practice, so it shouldn't be an issue.
> > 
> > The test can construct an absolute URL for content/abe.png.
> 
> Would you like point an example to me, thanks?



I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here.

[Darin Adler]

(In reply to comment #24)
> I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here.

Did you try document.URL?

(In reply to comment #25)
> (In reply to comment #24)
> > I just found out the testcase's URL with window.location, it is the file name itself, like 'video-default-poster.html', so I guess use 'about:blank' is a good way here.
> 
> Did you try document.URL?

It is same, gives file name itself.

[Darin Adler]

(In reply to comment #26)
> It is same, gives file name itself.

Strange, given the many tests that use document.URL such as fast/loader/external-script-URL-location.html.

[Eric Carlson]

(In reply to comment #26)
> It is same, gives file name itself.

Something is strange. I modified your test to just log window.location.href:

<!DOCTYPE html>
<html>
<head>
<script>
  if (window.testRunner) {
    testRunner.dumpAsText();
    testRunner.waitUntilDone();
  }

  if (window.internals) {
    addEventListener("load", function() {
      alert(window.location.href);
      testRunner.notifyDone();
    }, false);
  }

</script>
</head>
<body>
<pre id="result-no-poster"></pre>
<pre id="result-has-poster"></pre>
<video id="video-no-poster" src="content/test.mp4" preload="none" />
<video id="video-has-poster" src="content/test.mp4" poster="content/abe.png" preload="none" />
</body>
</html>

and the result is

ALERT: file:///Volumes/User/WebKit/Sources/OpenSource/LayoutTests/media/temp.html

Would you like me to know how did you run it?

I ran the test with the following command,
perl Tools/Scripts/run-webkit-tests  --chromium --debug LayoutTests/media/video-default-poster.html

I think this might be the reason I only got the file name itself.

(In reply to comment #27)
> (In reply to comment #26)
> > It is same, gives file name itself.
> 
> Strange, given the many tests that use document.URL such as fast/loader/external-script-URL-location.html.


This test also passed when I ran it with

perl Tools/Scripts/run-webkit-tests  --chromium --debug LayoutTests/fast/loader/external-script-URL-location.html

The test passed even the Document.URL doesn't have '/'.

It seemed that console.log only output the relative path. I will provide another patch.

Created attachment 190338 [details]
Patch

Changed to construct URL in Javascript, PTAL

[Eric Carlson]

Comment on attachment 190338 [details]
Patch

Thank you for updating the test!

[Darin Adler]

Comment on attachment 190338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190338&action=review

> Source/WebCore/html/HTMLVideoElement.cpp:90
> +            setAttribute(posterAttr, defaultVideoPosterURL.string());

I don’t know why I didn’t notice this earlier. This is not the right way to do it! We should do this logic when getting the poster attribute. We shouldn’t actually set the poster attribute on an element. It’s not good to have an element that modifies itself during the attach process -- it could even lead to a security bug.

(In reply to comment #35)
> (From update of attachment 190338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190338&action=review
> 
> > Source/WebCore/html/HTMLVideoElement.cpp:90
> > +            setAttribute(posterAttr, defaultVideoPosterURL.string());
> 
> I don’t know why I didn’t notice this earlier. This is not the right way to do it! We should do this logic when getting the poster attribute. We shouldn’t actually set the poster attribute on an element. It’s not good to have an element that modifies itself during the attach process -- it could even lead to a security bug.

Would you like me know why there is a security bug here? What the right way to do this?

Hi Darin,

Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here.

In another word, it has no impact to the rest of the world.

Thanks

Created attachment 190796 [details]
Patch

ping...

[Darin Adler]

(In reply to comment #37)
> Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here.

Your analysis is incorrect. If this Android web view app loads content from the web, the web content could exploit the bug and take advantage of it to run arbitrary code.

(In reply to comment #40)
> (In reply to comment #37)
> > Only Android web view application will enable this feature, beside this there is no API or UI coud access this, the application could do whatever they would like and it is the user's choice to trust the application. It shouldn't have security issue here.
> 
> Your analysis is incorrect. If this Android web view app loads content from the web, the web content could exploit the bug and take advantage of it to run arbitrary code.

Thanks for reply, I still not quite understand how the security issue could happen? Did you mean load the poster image could run arbitrary code?

[Darin Adler]

The correct way to do this is to put this logic into the code that fetches the poster. We should not actually change the poster attribute on the DOM element, instead we should fetch the appropriate poster.

The way to do this would be:

A) Replace the Element::imageSourceAttributeName function with an Element::imageSourceURL function that returns the imageSourceURL as a const AtomicString&. The body will be the same as before, it will just also include a call to getAttribute. Also will need to revise the four classes that override that function.

B) Add a new HTMLVideoElement::posterImageURL function that implements the default poster URL logic.

C) Update the four functions that get the poster attribute to handle poster loading and display to call posterImageURL.

1) HTMLVideoElement::imageSourceURL.
2) HTMLVideoElement::setDisplayMode.
3) HTMLVideoElement::updateDisplayState.
4) HTMLMediaElement::getPluginProxyParams. Will need to cast to HTMLVideoElement after checking isVideo.

That’s the correct way to fix this.

[Darin Adler]

(In reply to comment #41)
> Did you mean load the poster image could run arbitrary code?

Setting a poster DOM attribute will allow handlers on the webpage to run arbitrary JavaScript during the attach process. Running arbitrary JavaScript at that time can result in the engine following stale pointers, which is exploitable to run arbitrary code.

Created attachment 191627 [details]
Patch

Comment on attachment 191627 [details]
Patch

Not for review, just want to try

[Eric Carlson]

Comment on attachment 191627 [details]
Patch

Drive by comment: I would recommend only removing extraneous white-space in functions where you are also changing code. Removing it as you have here makes it harder to pick out what you have actually changed.

Created attachment 191830 [details]
Patch

I should add new symbols in  Source/WebCore/WebCore.order, but don't have corresponding platform, will add them once error appears in bots.

[Stephen Chenney]

Comment on attachment 191830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review

> Source/WebCore/ChangeLog:16
> +

Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch.

(In reply to comment #49)
> (From update of attachment 191830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +
> 
> Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch.

I followed Darin Adler's suggestion (refer comments 42), actually, the return value from imageSourceAttributeName() ends up with getAttribute(imageSourceAttributeName()), it's exactly same as the default implementation of imageSourceURL() does.

[Stephen Chenney]

(In reply to comment #50)
> (In reply to comment #49)
> > (From update of attachment 191830 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191830&action=review
> > 
> > > Source/WebCore/ChangeLog:16
> > > +
> > 
> > Nowhere here do you explain why imageSourceAttributeName must be changed to imageSourceURL, which seems to be a signficant outcome of this patch.
> 
> I followed Darin Adler's suggestion (refer comments 42), actually, the return value from imageSourceAttributeName() ends up with getAttribute(imageSourceAttributeName()), it's exactly same as the default implementation of imageSourceURL() does.

Yes, but I believe the ChangeLog should say something. Probably just repeat Darin's outline of the change.

Created attachment 191856 [details]
Patch

Update ChangeLog, thanks

Created attachment 192167 [details]
Patch

[Stephen Chenney]

Comment on attachment 192167 [details]
Patch

Looks good to me, but I think Darin should give the final word. I'm ultimately not an expert in this area.

[Build Bot]

Comment on attachment 192167 [details]
Patch

Attachment 192167 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17007165

New failing tests:
editing/selection/selection-modify-crash.html

[Darin Adler]

Comment on attachment 192167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192167&action=review

Looks OK, but breaks whitespace handling in video element poster URLs. I’m surprised there was no test for this that is broken. Please fix that.

> Source/WebCore/html/HTMLVideoElement.cpp:312
> +    const AtomicString& url = imageSourceURL();

It’s a shame to make a virtual function call here. No big deal, but not ideal either.

> Source/WebCore/html/HTMLVideoElement.cpp:315
> +    if (url.isEmpty())
> +        return KURL();
> +    return document()->completeURL(url);

This is missing a call to stripLeadingAndTrailingHTMLSpaces.

Created attachment 192318 [details]
Patch

Comment on attachment 192167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192167&action=review

>> Source/WebCore/html/HTMLVideoElement.cpp:315
>> +    return document()->completeURL(url);
> 
> This is missing a call to stripLeadingAndTrailingHTMLSpaces.

Done

[Darin Adler]

Comment on attachment 192318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192318&action=review

> Source/WebCore/html/HTMLVideoElement.cpp:189
> +    const AtomicString& url = getAttribute(posterAttr);
> +    if (!url.isEmpty())
> +        return url;

On reflection, it’s a little strange here to special case only an empty URL without considering whitespace stripping. I could imagine special casing only a null URL, which means we’d only do this when the poster attribute is entirely missing. Or special casing any URL that ends up empty, and therefore is something we won't even try to load.

> Source/WebCore/html/HTMLVideoElement.cpp:316
> +    const AtomicString& url = imageSourceURL();
> +    if (url.isEmpty())
> +        return KURL();
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url));

This is still wrong. The stripping of needs to be done before the isEmpty check. Just look at the code in getNonEmptyURLAttribute and match its behavior exactly. That’s the problem with copied and pasted code, I guess. Hard to get it right!

> Source/WebCore/html/HTMLVideoElement.h:94
> +    AtomicString m_defaultPosterURL;

It’s a little strange to store a copy of this in every video element. There’s no reason we couldn’t fetch it from the document settings when we need it instead of when we create the video element.

Created attachment 192766 [details]
Patch

Comment on attachment 192318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192318&action=review

>> Source/WebCore/html/HTMLVideoElement.cpp:189
>> +        return url;
> 
> On reflection, it’s a little strange here to special case only an empty URL without considering whitespace stripping. I could imagine special casing only a null URL, which means we’d only do this when the poster attribute is entirely missing. Or special casing any URL that ends up empty, and therefore is something we won't even try to load.

Thanks, I striped space for isEmpty() checking, but returned the original value, I think imageSoureURL is supposed to return unstriped value.

>> Source/WebCore/html/HTMLVideoElement.cpp:316
>> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url));
> 
> This is still wrong. The stripping of needs to be done before the isEmpty check. Just look at the code in getNonEmptyURLAttribute and match its behavior exactly. That’s the problem with copied and pasted code, I guess. Hard to get it right!

Done

>> Source/WebCore/html/HTMLVideoElement.h:94
>> +    AtomicString m_defaultPosterURL;
> 
> It’s a little strange to store a copy of this in every video element. There’s no reason we couldn’t fetch it from the document settings when we need it instead of when we create the video element.

The document->settings()->defaultVideoPosterURL() returns value, but imageSourceURL() returns a reference, I didn't find other workaround, so have to hold the value here. :(
I just guess the settings doesn't return reference might because the settings could be changed during the page's life cycle.

PTAL when you have time

[Darin Adler]

Comment on attachment 192766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192766&action=review

review- because the whitespace stripping is still wrong in this patch; I’m guessing you uploaded the wrong one

> Source/WebCore/ChangeLog:37
> +        * WebCore.order: Replace imageSourceAttributeName() with imageSourceURL()

This file does not need to be updated. You can just remove this change from your patch.

> Source/WebCore/ChangeLog:39
> +        * dom/Element.cpp: Replace imageSourceAttributeName() with imageSourceURL()
> +        (WebCore::Element::imageSourceURL):

Comments should go on the functions, not the entire file, unless there is no function name.

> Source/WebCore/ChangeLog:41
> +        (Element):

Bogus lines like this should just be deleted from the ChangeLog. The script generates them.

> Source/WebCore/myChangeLog:1
> [phone]  Tao Bai  <[e-mail]>

Please don’t land this second change log file!

> Source/WebCore/html/HTMLVideoElement.cpp:189
> +    const AtomicString& url = getAttribute(posterAttr);
> +    if (!url.isEmpty())
> +        return url;

As I mentioned in my last patch, it’s not right to check isEmpty here. It would be OK to check isNull, or to check isEmpty after calling stripLeadingAndTrailingHTMLSpaces.

> Source/WebCore/html/HTMLVideoElement.cpp:316
> +    const AtomicString& url = imageSourceURL();
> +    if (url.isEmpty())
> +        return KURL();
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url));

The stripLeadingAndTrailingHTMLSpaces call needs to be *before* calling isEmpty. Isn’t that why I said review- last time? Did you upload the wrong version of the patch?

> Source/WebCore/html/HTMLVideoElement.h:68
> +    KURL posterImageURL();

Should be const.

Created attachment 192945 [details]
Patch

Comment on attachment 192766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192766&action=review

>> Source/WebCore/ChangeLog:37
>> +        * WebCore.order: Replace imageSourceAttributeName() with imageSourceURL()
> 
> This file does not need to be updated. You can just remove this change from your patch.

Done

>> Source/WebCore/ChangeLog:39
>> +        (WebCore::Element::imageSourceURL):
> 
> Comments should go on the functions, not the entire file, unless there is no function name.

Done

>> Source/WebCore/ChangeLog:41
>> +        (Element):
> 
> Bogus lines like this should just be deleted from the ChangeLog. The script generates them.

Done

>> Source/WebCore/myChangeLog:1
>> [phone]  Tao Bai  <[e-mail]>
> 
> Please don’t land this second change log file!

Sorry, it is my bad, this is just wrong patch

>> Source/WebCore/html/HTMLVideoElement.cpp:189
>> +        return url;
> 
> As I mentioned in my last patch, it’s not right to check isEmpty here. It would be OK to check isNull, or to check isEmpty after calling stripLeadingAndTrailingHTMLSpaces.

Sorry for the wrong patch.

>> Source/WebCore/html/HTMLVideoElement.cpp:316
>> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(url));
> 
> The stripLeadingAndTrailingHTMLSpaces call needs to be *before* calling isEmpty. Isn’t that why I said review- last time? Did you upload the wrong version of the patch?

Sorry again.

>> Source/WebCore/html/HTMLVideoElement.h:68
>> +    KURL posterImageURL();
> 
> Should be const.

Done

[Darin Adler]

Comment on attachment 192945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192945&action=review

> Source/WebCore/html/HTMLVideoElement.cpp:188
> +    if (!stripLeadingAndTrailingHTMLSpaces(url).isEmpty())

Some day we may want to replace this by an isAllHTMLSpaces function that does not require copying the string even if the string contains leading or trailing spaces.

> Source/WebCore/html/HTMLVideoElement.h:72
> +    const KURL posterImageURL();

The const here before KURL is unneeded and nearly meaningless. It just means that you can’t call non-const member functions on the return value from this function.

What I meant when I said that this function should be const is that there should be a const *after* the parentheses, allowing this to be called on a const HTMLVideoElement.

Created attachment 192979 [details]
Patch

Comment on attachment 192945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192945&action=review

>> Source/WebCore/html/HTMLVideoElement.h:72
>> +    const KURL posterImageURL();
> 
> The const here before KURL is unneeded and nearly meaningless. It just means that you can’t call non-const member functions on the return value from this function.
> 
> What I meant when I said that this function should be const is that there should be a const *after* the parentheses, allowing this to be called on a const HTMLVideoElement.

Done

[WebKit Review Bot]

Comment on attachment 192979 [details]
Patch

Clearing flags on attachment: 192979

Committed r145750: <http://trac.webkit.org/changeset/145750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 192979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192979&action=review

> Source/WebCore/html/HTMLVideoElement.cpp:320
> +    const AtomicString& url = stripLeadingAndTrailingHTMLSpaces(imageSourceURL());

After this landed, I noticed a mistake here. This should be typed either const String& or String. Using the type "const AtomicString&" means we’ll do an extra hash lookup to make an atomic string, but there is no value to doing that.

We need a followup.

Will do