Bug 152013

Summary: Picture element needs to respond to dynamic viewport changes
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jer.noble, jonlee, ossy, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=152140
Attachments:
Description Flags
Patch
none
Patch dino: review+

Dave Hyatt
Reported 2015-12-08 14:05:24 PST
Picture element needs to respond to dynamic viewport changes
Attachments
Patch (12.76 KB, patch)
2015-12-08 14:06 PST, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch (32.20 KB, patch)
2015-12-09 12:44 PST, Dave Hyatt 		dino: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dave Hyatt
Comment 1 2015-12-08 14:06:02 PST
Created attachment 266939 [details] Patch
Jon Lee
Comment 2 2015-12-08 14:18:48 PST
rdar://problem/23766375
Simon Fraser (smfr)
Comment 3 2015-12-08 17:15:07 PST
Comment on attachment 266939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266939&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:188 > + // iterate through expressions, stop if any of them eval to false > + // (AND semantics) Sentence case. > Source/WebCore/css/MediaQueryEvaluator.cpp:199 > + // assume true if we are at the end of the list, > + // otherwise assume false Ditto. > Source/WebCore/dom/Document.cpp:3510 > + Vector<HTMLPictureElement*> changedPictures; reserveCapacity? > Source/WebCore/dom/Document.h:1310 > + void addViewportDependentPicture(HTMLPictureElement*); > + void removeViewportDependentPicture(HTMLPictureElement*); Can these take references? > Source/WebCore/html/HTMLPictureElement.cpp:69 > + unsigned s = m_viewportDependentMediaQueryResults.size(); > + for (unsigned i = 0; i < s; i++) { s -> numResults?
Dave Hyatt
Comment 4 2015-12-09 12:44:09 PST
Created attachment 267036 [details] Patch
WebKit Commit Bot
Comment 5 2015-12-09 12:45:58 PST
Attachment 267036 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:55: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dean Jackson
Comment 6 2015-12-09 12:54:31 PST
Comment on attachment 267036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267036&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:187 > + size_t j = 0; Since you use j outside the loop, maybe give it a better name? > Source/WebCore/dom/Document.cpp:3512 > + HashSet<HTMLPictureElement*>::iterator end = m_viewportDependentPictures.end(); > + for (HashSet<HTMLPictureElement*>::iterator it = m_viewportDependentPictures.begin(); it != end; ++it) { I thought we could do this with a modern for loop now? > Source/WebCore/html/HTMLPictureElement.cpp:44 > +HTMLPictureElement::~HTMLPictureElement() > +{ > + if (hasViewportDependentResults()) > + document().removeViewportDependentPicture(*this); > +} Is it possible for a picture to start with a viewport dependent query, then have that source removed - leaving it in a state where it's in the hashset but will no longer return true from hasViewportDependentResults?
Dave Hyatt
Comment 7 2015-12-09 13:42:54 PST
Landed in r193859.
Csaba Osztrogonác
Comment 8 2016-01-05 00:55:12 PST
(In reply to comment #7) > Landed in r193859. The commit log was repeated 47 times. :( Dave, could you check your script?
Note You need to log in before you can comment on or make changes to this bug.