74123 – Move MediaDocument styles into CSS. Set black background for chromium.

RESOLVED FIXED 74123

Move MediaDocument styles into CSS. Set black background for chromium.

https://bugs.webkit.org/show_bug.cgi?id=74123

Summary Move MediaDocument styles into CSS. Set black background for chromium.

Dale Curtis

Reported 2011-12-08 13:45:14 PST

Move MediaDocument styles into CSS. Set black background for chromium.

Attachments
Patch (3.84 KB, patch) 2011-12-08 13:46 PST, Dale Curtis	no flags	Details Formatted Diff Diff
Patch (3.62 KB, patch) 2011-12-09 15:07 PST, Dale Curtis	no flags	Details Formatted Diff Diff
Patch (3.58 KB, patch) 2011-12-12 14:31 PST, Dale Curtis	no flags	Details Formatted Diff Diff
Patch (11.04 KB, patch) 2012-01-03 17:08 PST, Dale Curtis	no flags	Details Formatted Diff Diff
Patch (11.62 KB, patch) 2012-01-05 19:03 PST, Dale Curtis	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Dale Curtis

Comment 1 2011-12-08 13:46:38 PST

Created attachment 118461 [details] Patch

Dale Curtis

Comment 2 2011-12-08 13:50:17 PST

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.

Dimitri Glazkov (Google)

Comment 3 2011-12-08 14:12:10 PST

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.

Dimitri Glazkov (Google)

Comment 4 2011-12-08 14:13:09 PST

Mayb Eric has good ideas. Eric, it seems Dale has to force style recalc to get the CSS to apply. That seems wrong.

Andrew Scherkus

Comment 5 2011-12-08 14:24:21 PST

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"

Darin Adler

Comment 6 2011-12-08 14:32:25 PST

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.

Dale Curtis

Comment 7 2011-12-08 15:29:02 PST

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

Dimitri Glazkov (Google)

Comment 8 2011-12-08 16:04:45 PST

(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.

Dale Curtis

Comment 9 2011-12-09 15:07:06 PST

Created attachment 118651 [details] Patch

Dale Curtis

Comment 10 2011-12-09 15:08:23 PST

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?

Eric Carlson

Comment 11 2011-12-09 15:39:26 PST

(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.

Simon Fraser (smfr)

Comment 12 2011-12-09 16:28:22 PST

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.

Dale Curtis

Comment 13 2011-12-09 16:33:49 PST

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.

WebKit Review Bot

Comment 14 2011-12-09 20:39:04 PST

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

Dimitri Glazkov (Google)

Comment 15 2011-12-12 10:09:37 PST

(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.

Dale Curtis

Comment 16 2011-12-12 12:51:38 PST

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.

Dimitri Glazkov (Google)

Comment 17 2011-12-12 12:58:36 PST

(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.

Dale Curtis

Comment 18 2011-12-12 14:31:44 PST

Created attachment 118849 [details] Patch

Dale Curtis

Comment 19 2011-12-12 14:32:11 PST

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.

WebKit Review Bot

Comment 20 2011-12-12 16:07:57 PST

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

Eric Seidel (no email)

Comment 21 2011-12-21 12:12:23 PST

Comment on attachment 118849 [details] Patch LGTM.

Dale Curtis

Comment 22 2011-12-21 12:29:42 PST

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.

Eric Carlson

Comment 23 2011-12-21 12:36:18 PST

(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.

Dale Curtis

Comment 24 2011-12-21 13:14:04 PST

@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?

Eric Carlson

Comment 25 2011-12-21 13:31:13 PST

(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.

Dale Curtis

Comment 26 2011-12-21 13:38:13 PST

Comment on attachment 118849 [details] Patch Thanks for your patience Eric! I'll follow up and update the baseline once results are available.

WebKit Review Bot

Comment 27 2011-12-21 13:38:40 PST

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.

Eric Seidel (no email)

Comment 28 2011-12-21 15:05:08 PST

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-.

Dale Curtis

Comment 29 2011-12-21 15:23:42 PST

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.

Dale Curtis

Comment 30 2012-01-03 17:08:32 PST

Created attachment 121021 [details] Patch

Dale Curtis

Comment 31 2012-01-03 17:12:33 PST

@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.

Eric Seidel (no email)

Comment 32 2012-01-05 17:39:47 PST

Comment on attachment 121021 [details] Patch OK. Looks right.

WebKit Review Bot

Comment 33 2012-01-05 18:15:04 PST

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

Dale Curtis

Comment 34 2012-01-05 19:03:44 PST

Created attachment 121384 [details] Patch

Dale Curtis

Comment 35 2012-01-05 19:07:55 PST

Arg. Fixed conflicts. PTAL.

WebKit Review Bot

Comment 36 2012-01-06 01:44:00 PST

Comment on attachment 121384 [details] Patch Clearing flags on attachment: 121384 Committed r104278: <http://trac.webkit.org/changeset/104278>

WebKit Review Bot

Comment 37 2012-01-06 01:44:07 PST

All reviewed patches have been landed. Closing bug.

epoger

Comment 38 2012-01-06 12:00:36 PST

(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.

epoger

Comment 39 2012-01-06 12:21:21 PST

(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 .

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component WebCore Misc.

Assignee

Nobody

Reported

2011-12-08 13:45 PST

Modified

2012-01-06 12:21 PST History

CC List

8 users Show

URL

Keywords

Depends on

Blocks