Summary: | REGRESSION(r89004): Video pauses and never resumes playing if scrubbed during playback | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||||
Component: | Media | Assignee: | Kent Tamura <tkent> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, hyatt, jer.noble, sam, sjl, tkent, tonikitoo | ||||||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://media.w3.org/2010/05/bunny/movie.mp4 | ||||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2011-07-11 13:32:20 PDT
The video gets stuck because after r89004 MediaControlTimelineElement::defaultEventHandler no longer sees the mouse up event. This means that endScrubbing is no longer called on the MediaElement. For ease of clickability: This broke in <http://trac.webkit.org/changeset/89004>. Created attachment 100432 [details]
Patch
Comment on attachment 100432 [details]
Patch
Why is mousedown different?
(In reply to comment #6) > (From update of attachment 100432 [details]) > Why is mousedown different? setDefaultHandled() for mousedown is required to fix Bug 62683, and mousedown is unrelated to this media control issue. I understand that changing it is not required to fix the bug. What I’m asking is why does calling setDefaultHandled cause a problem for mousemove / mouseup but not for mousedown? Why is there an inconsistency in behavior? (In reply to comment #8) I think it's because SliderThumbElement starts mouse capturing since a mousedown event. (In reply to comment #9) > (In reply to comment #8) > > I think it's because SliderThumbElement starts mouse capturing since a mousedown event. MediaControlTimelineElement::defaultEventHandler() needs to receive a mouseDown event to initiate scrubbing. Calling setDefaultHandled() in SliderThumbElement means that event will never be handled by MediaControlTimelineElement. The setDefaultHandled() call for mouseDown must be removed as well to fix media scrubbing, so this patch is not complete. Is there another way to fix the Qt layout test that doesn't involve calling setDefaultHandled() on mouseDown events? FYI: This problem has broken Chromium too. Any ETA? Thx :-) Jer, Steve, Would you try my patch and tell me what's wrong please? I thought my patch fixed the bug. A code change can fix a bug yet not be correct. The patch does fix the symptom (i.e. media timeline now works, for chromium), but I don't understand why the mouseDown case should be different? (In reply to comment #12) > Jer, Steve, > Would you try my patch and tell me what's wrong please? I thought my patch fixed the bug. Your patch sets defaultHandled for mouseDown events. This means that MediaControlTimelineElement will never see the mouseDown event, and HTMLMediaElement::beginScrubbing() will never be called. HTMLMediaElement::beginScrubbing() sets some internal flags and pauses the movie if necessary. HTMLMediaElement::endScrubbing() checks those flags, and begins to play the movie if it was paused in beginScrubbing(). With your patch, beginScrubbing() will never get called, and endScrubbing() will. While your patch fixes the obvious bug, it creates another less obvious one. And testing more closely, pausing/resuming after scrubbing is now buggy (with the patch). Thank you for the comments. It seems I tested with a wrong way. I thought I clicked on the thumb of the timeline slider but probably I clicked on the slider track near the thumb. My patch doesn't pause the media by mouse clicking. Created attachment 101272 [details]
Patch 2
Comment on attachment 101272 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=101272&action=review This patch looks much better. Just one comment: > Source/WebCore/html/RangeInputType.cpp:164 > + // Mouse click on the thumb was already handled in SliderThumbElement::defaultEventHandler(). > + if (targetNode == thumb) > + return; > thumb->dragFrom(event->absoluteLocation()); > event->setDefaultHandled(); Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314? (In reply to comment #19) > Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314? Whoops, wrong bug number. Is that line still needed to fix bug #62683? Comment on attachment 101272 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=101272&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:283 > + // We don't call event->setDefaultHandled() here intentionally. > + // http://webkit.org/b/64314 We typically don’t put a comment at every location that we don’t call a given function. Is the lack of call to setDefaultHandled here so surprising that the comment is necessary? Created attachment 101282 [details]
Patch 3
Comment on attachment 101272 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=101272&action=review >> Source/WebCore/html/RangeInputType.cpp:164 >> event->setDefaultHandled(); > > Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314? It is unnecessary. Removed. >> Source/WebCore/html/shadow/SliderThumbElement.cpp:283 >> + // http://webkit.org/b/64314 > > We typically don’t put a comment at every location that we don’t call a given function. Is the lack of call to setDefaultHandled here so surprising that the comment is necessary? The lack surprised me and I added setDefaultHandled() blindly in r89004. Almost all of event handling code of HTMLInputElement and InputTypes call setDefaultHandled() if they do something for events. Comment on attachment 101282 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=101282&action=review > Source/WebCore/html/shadow/SliderThumbElement.cpp:283 > + // We don't call event->setDefaultHandled() here intentionally. > + // http://webkit.org/b/64314 A better way to phrase this would be "We intentionally do not call event->setDefaultHandled() here. It would probably be of more use to explain why we don't call it rather than pointing to a bug. (In reply to comment #24) > (From update of attachment 101282 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101282&action=review > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:283 > > + // We don't call event->setDefaultHandled() here intentionally. > > + // http://webkit.org/b/64314 > > A better way to phrase this would be "We intentionally do not call event->setDefaultHandled() here. It would probably be of more use to explain why we don't call it rather than pointing to a bug. Thanks! I'll change the comment as follows and will land the patch. // We intentionally do not call event->setDefaultHandled() here because // MediaControlTimelineElement::defaultEventHandler() wants to handle these // mouse events. Committed r91333: <http://trac.webkit.org/changeset/91333> (In reply to comment #26) > Committed r91333: <http://trac.webkit.org/changeset/91333> Kent, is this really the right thing to do for "input type=range" case. Now no events get handled at all on this case for range input. More specially, getting the mouse-move set as 'defaultHandled' when draggin the slider is specially usefull for touch-based input devices, if they want to make this slider draggable. (In reply to comment #27) > (In reply to comment #26) > > Committed r91333: <http://trac.webkit.org/changeset/91333> > > Kent, is this really the right thing to do for "input type=range" case. Now no events get handled at all on this case for range input. > > More specially, getting the mouse-move set as 'defaultHandled' when draggin the slider is specially usefull for touch-based input devices, if they want to make this slider draggable. I think the current behavior is same as the behavior before r89004. Would you explain the detail of the use-case in another bug please? |