Bug 152013 – Picture element needs to respond to dynamic viewport changes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Picture element needs to respond to dynamic viewport changes

[Dave Hyatt]

Created attachment 266939 [details]
Patch

[Jon Lee]

rdar://problem/23766375

[Simon Fraser (smfr)]

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]

Created attachment 267036 [details]
Patch

[WebKit Commit Bot]

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

Landed in r193859.

[Csaba Osztrogonác]

(In reply to comment #7)
> Landed in r193859.

The commit log was repeated 47 times.  :(  Dave, could you check your script?