Move MediaDocument styles into CSS. Set black background for chromium.
Created attachment 118461 [details] Patch
PTAL. I'm unsure if forcing a full document style recalc is the right approach, but couldn't find another method which worked. Without it the body tag styles don't apply until another event forces a recalc (such as opening web inspector). I.e. when loading a MediaDocument the background color will be the default white until opening web inspector and then it suddenly switches to the styled black.
Comment on attachment 118461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118461&action=review > Source/WebCore/ChangeLog:11 > + No new tests. (OOPS!) Probably need to figure out how to test this change. > Source/WebCore/html/MediaDocument.cpp:97 > + document()->recalcStyle(Node::Force); So, as I was saying earlier, this should not be necessary. I don't have a build in front of me, but perhaps you could take a peek and see if the style is _ever_ resolved on this document. And if it is, when. It seems weird that we have to kludge this like so.
Mayb Eric has good ideas. Eric, it seems Dale has to force style recalc to get the CSS to apply. That seems wrong.
When I tested out this patch it took a serious amount of fuddling to get <body> to pick up the updated background color. If you refreshed the page w/ Web Inspector open it said that <body> had the proper style set, but as dalecurtis mentions you had to go beyond that and close/open Web Inspector again to get the style to "stick"
Comment on attachment 118461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118461&action=review >> Source/WebCore/html/MediaDocument.cpp:97 >> + document()->recalcStyle(Node::Force); > > So, as I was saying earlier, this should not be necessary. I don't have a build in front of me, but perhaps you could take a peek and see if the style is _ever_ resolved on this document. And if it is, when. It seems weird that we have to kludge this like so. Yes, that is absolutely unnecessary and if adding it has any benefit then there is a bug elsewhere.
Sounds good. Any ideas where to look for a bug though? Dimitri, I haven't found a time when the style gets updated without forcing it through web inspector. As far as I can tell the styles are applied to the document prior to creation and should get picked up by the elements as they're added to the document. While the audio, video tag styles update appropriately those on the body tag do not. Is it possible HTMLMediaElement is forcing a style recalculation on its element while the body element has no such actor? I noticed the class makes several style related calls: http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L251 http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L2577 http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L3008
(In reply to comment #7) > Sounds good. Any ideas where to look for a bug though? > > Dimitri, I haven't found a time when the style gets updated without forcing it through web inspector. > > As far as I can tell the styles are applied to the document prior to creation and should get picked up by the elements as they're added to the document. > > While the audio, video tag styles update appropriately those on the body tag do not. Is it possible HTMLMediaElement is forcing a style recalculation on its element while the body element has no such actor? I noticed the class makes several style related calls: > > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L251 > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L2577 > http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp#L3008 I would try to understand why the style recalc is not reaching the body. Is it not ever marked as needing a style recalc? Why so? The MediaDocument and friends are a bit quirky that way, and definitely have bugs. It's a bit of a yak shave, but WebKit will thank you for fixing them.
Created attachment 118651 [details] Patch
So, adding "body->setNeedsStyleRecalc(SyntheticStyleChange);" also solves the problem and is how HTMLMediaElement seems to be getting the style to update. I've updated the patch to reflect this. A test for this behavior is pretty simple but would just end up comparing the values set in the CSS sheet w/ those that are actually applied. If that's worthwhile, I'll add that test. Going through blame, it looks like Eric Carlson added the original style recalc to HTMLMediaElement.cpp in this revision: http://trac.webkit.org/changeset/54826 Eric, can you comment if this is the right fix for getting the style to update on the body tag?
(In reply to comment #10) > So, adding "body->setNeedsStyleRecalc(SyntheticStyleChange);" also solves the problem and is how HTMLMediaElement seems to be getting the style to update. > The media element calls this from HTMLMediaElement::mediaPlayerRenderingModeChanged, which is only called when the media player change from software to hardware rendering, or vice-versa. I don't know if it is appropriate here, but Simon will.
SyntheticStyleChange shouldn't be used to force style recalc in this case. It seems like there's just a bug that needs to be found.
Okay. I'll keep digging. For the record, any setting of setNeedsStyleRecalc() works fine, not just SyntheticStyleChange. I chose that one because HTMLMediaElement was using it.
Comment on attachment 118651 [details] Patch Attachment 118651 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10825530 New failing tests: media/media-document-audio-repaint.html
(In reply to comment #13) > Okay. I'll keep digging. For the record, any setting of setNeedsStyleRecalc() works fine, not just SyntheticStyleChange. I chose that one because HTMLMediaElement was using it. I think you're on to something here. The MediaDocument doesn't run any JS, and the full-page-media pseudoclass is just a property of a page (in other words, there's no recalc triggered for setting it, since it's never set). So what's happening is that the style for HTMLBodyElement is probably just not recalculated to the right value. The problem is then that somehow, the initial style we create for HTMLBodyElement is wrong, since it doesn't contain the rules that clearly match it.
The style issue doesn't appear limited to the <body> element, as far as I can tell no element except the video/audio elements gets their styles applied properly. To be specific, in earlier testing I tried adding <div> and <input> tags plus some styles without selector restrictions and they were not picked up until Web Inspector was launched.
(In reply to comment #16) > The style issue doesn't appear limited to the <body> element, as far as I can tell no element except the video/audio elements gets their styles applied properly. > > To be specific, in earlier testing I tried adding <div> and <input> tags plus some styles without selector restrictions and they were not picked up until Web Inspector was launched. Yes, but why? Ultimately, I think that the correct fix is body->setNeedsStyleRecalc();, but we need to confirm that this is the case.
Created attachment 118849 [details] Patch
So I dug on the pseudo class stuff not resulting in a recalc and found Document::setCSSTarget(<element>). Calling "document()->setCSSTarget(rootElement);" appears to fix the psuedo class issues mentioned by Dimitri, allowing all elements to pick up user agent styles appropriately. PTAL. I've updated the patch set with this fix.
Comment on attachment 118849 [details] Patch Attachment 118849 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10849102 New failing tests: media/media-document-audio-repaint.html
Comment on attachment 118849 [details] Patch LGTM.
Thanks for the review! media-document-audio-repaint needs a rebaseline on the Chromium platforms. Should I do that in this patch? What's the preferred method for collecting all the platform expected files? Apologies if I'm missing some documentation somewhere.
(In reply to comment #22) > Thanks for the review! > > media-document-audio-repaint needs a rebaseline on the Chromium platforms. Should I do that in this patch? What's the preferred method for collecting all the platform expected files? Apologies if I'm missing some documentation somewhere. I try to make my patches self contained whenever possible, so I would include the new test results.
@Eric Carlson: This test uses a pixel comparison and has results for a lot of platforms I don't have. All the documentation I've found says the change needs to be committed first and then the results are pulled from the Buildbots: http://trac.webkit.org/wiki/Rebaseline Is there a way I'm missing to do this before submission? Otherwise should I just add the test to the Skipped / test_expectations file for the chromium platform right now?
(In reply to comment #24) > @Eric Carlson: This test uses a pixel comparison and has results for a lot of platforms I don't have. All the documentation I've found says the change needs to be committed first and then the results are pulled from the Buildbots: > > http://trac.webkit.org/wiki/Rebaseline > > Is there a way I'm missing to do this before submission? Otherwise should I just add the test to the Skipped / test_expectations file for the chromium platform right now? You said the test "needs a rebaseline on the Chromium platforms" so I assumed you could generate the new results before committing. If you can't, you will obviously have to commit what you have now and then commit the rest once you can get results from the bots.
Comment on attachment 118849 [details] Patch Thanks for your patience Eric! I'll follow up and update the baseline once results are available.
Comment on attachment 118849 [details] Patch Rejecting attachment 118849 [details] from commit-queue. dalecurtis@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 118849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118849&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) The cq will reject it because of this line. Since your'e not a commtiter, setting this r-.
Thanks again for your comments Eric! As mentioned above, a new test would just be a comparison of styles set in the CSS files versus the rendered style. I generally avoid tests which just validate constants, but if that's desirable I'm happy to add one.
Created attachment 121021 [details] Patch
@Eric Seidel: Can you please take another look? I've added a test for the new behavior. Rebaselined the text portions of media-document-audio-repaint, updated the chromium test expectations to expect an IMAGE failure, and filed https://bugs.webkit.org/show_bug.cgi?id=75505 to track the image rebaseline.
Comment on attachment 121021 [details] Patch OK. Looks right.
Comment on attachment 121021 [details] Patch Rejecting attachment 121021 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ile LayoutTests/platform/chromium/media/video-black-bg-in-media-document-expected.txt patching file LayoutTests/platform/chromium/media/video-black-bg-in-media-document.html patching file LayoutTests/platform/chromium/test_expectations.txt Hunk #1 FAILED at 3908. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/11146164
Created attachment 121384 [details] Patch
Arg. Fixed conflicts. PTAL.
Comment on attachment 121384 [details] Patch Clearing flags on attachment: 121384 Committed r104278: <http://trac.webkit.org/changeset/104278>
All reviewed patches have been landed. Closing bug.
(In reply to comment #37) > All reviewed patches have been landed. Closing bug. I think this change broke lint for test_expectations: $ Tools/Scripts/new-run-webkit-tests --chromium --lint FAILURES FOR <leopard, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <leopard, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <snowleopard, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <snowleopard, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lion, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lion, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <xp, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <xp, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <vista, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <vista, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <win7, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <win7, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lucid, x86, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lucid, x86, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lucid, x86_64, debug, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html FAILURES FOR <lucid, x86_64, release, gpu> in LayoutTests/platform/chromium/test_expectations.txt Line:3879 More specific entry on line 2707 overrides line 3879 media/media-document-audio-repaint.html Lint failed.
(In reply to comment #38) > (In reply to comment #37) > > All reviewed patches have been landed. Closing bug. > > I think this change broke lint for test_expectations: Looks like it was fixed in http://trac.webkit.org/changeset/104316 .