Bug 131319 – Allow elements to register for changes in page scale

• Component: DOM
• Status: RESOLVED FIXED

[Dean Jackson]

Some HTML elements might want to react to changes in the page scale.

[Radar WebKit Bug Importer]

<rdar://problem/16545804>

[Dean Jackson]

Created attachment 228765 [details]
Patch

[Dean Jackson]

Created attachment 228773 [details]
Patch

[Eric Carlson]

Comment on attachment 228773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228773&action=review

> Source/WebCore/ChangeLog:25
> +        * html/HTMLMediaElement.cpp:
> +        (WebCore::HTMLMediaElement::HTMLMediaElement):
> +        (WebCore::HTMLMediaElement::~HTMLMediaElement): Remove ourselves from the
> +        callback if necessary.

Nit: you forgot "setMediaControlsDependOnPageScaleFactor".

> Source/WebCore/ChangeLog:28
> +        (WebCore::HTMLMediaElement::mediaControlsDependOnPageScaleFactor): Add ourselves to

Nit: This is the accessor.

> Source/WebCore/ChangeLog:31
> +        (WebCore::Page::setPageScaleFactor): Tell the document that the user has zoomed.

Nit: "the document" ->  "all documents"

> Source/WebCore/dom/Document.cpp:4199
> +void Document::registerForPageScaleFactorChangedCallbacks(HTMLMediaElement* e)

Nit: might as well spell out "element".

> Source/WebCore/dom/Document.cpp:4204
> +void Document::unregisterForPageScaleFactorChangedCallbacks(HTMLMediaElement* e)

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:422
> +    if (m_mediaControlsDependOnPageScaleFactor)
> +        document().unregisterForPageScaleFactorChangedCallbacks(this);

Can this go in HTMLMediaElement::unregisterWithDocument?

> Source/WebCore/html/HTMLMediaElement.cpp:5986
> +void HTMLMediaElement::setMediaControlsDependOnPageScaleFactor(bool v)

Nit: you might as well use a descriptive variable name.

> Source/WebCore/html/HTMLMediaElement.h:892
> +    bool m_mediaControlsDependOnPageScaleFactor : 1;

Nit: this should be grouped with the other 1-bit bools to make the element slightly smaller.

[Simon Fraser (smfr)]

Comment on attachment 228773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228773&action=review

r- for the "moving between documents" issue.

> Source/WebCore/dom/Document.cpp:4199
> +void Document::registerForPageScaleFactorChangedCallbacks(HTMLMediaElement* e)

e -> element.

> Source/WebCore/html/HTMLMediaElement.cpp:423
> +#if ENABLE(MEDIA_CONTROLS_SCRIPT)
> +    if (m_mediaControlsDependOnPageScaleFactor)
> +        document().unregisterForPageScaleFactorChangedCallbacks(this);
> +#endif

What happens when an element moves between documents?

> Source/WebCore/html/HTMLMediaElement.cpp:5986
> +void HTMLMediaElement::setMediaControlsDependOnPageScaleFactor(bool v)

v -> something.

> Source/WebCore/page/Page.cpp:737
> +#if ENABLE(MEDIA_CONTROLS_SCRIPT)
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext())
> +        frame->document()->pageScaleFactorChanged();
> +#endif

Don't we already do this somewhere, e.g. to tell compositing that the scale changed?

Can this happen to pages in the page cache?

[Dean Jackson]

Comment on attachment 228773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228773&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:422
>> +        document().unregisterForPageScaleFactorChangedCallbacks(this);
> 
> Can this go in HTMLMediaElement::unregisterWithDocument?

Yes, and that will solve Simon's issue.

>> Source/WebCore/html/HTMLMediaElement.cpp:423
>> +#endif
> 
> What happens when an element moves between documents?

I'll fix that.

>> Source/WebCore/html/HTMLMediaElement.h:892
>> +    bool m_mediaControlsDependOnPageScaleFactor : 1;
> 
> Nit: this should be grouped with the other 1-bit bools to make the element slightly smaller.

OK.

>> Source/WebCore/page/Page.cpp:737
>> +#endif
> 
> Don't we already do this somewhere, e.g. to tell compositing that the scale changed?
> 
> Can this happen to pages in the page cache?

We do similar stuff in this method. e.g.

    if (view && view->fixedElementsLayoutRelativeToFrame())
        view->setViewportConstrainedObjectsNeedLayout();

This is basically the same thing but there is no point going up to the view for it.

[Dean Jackson]

Created attachment 228794 [details]
Patch

[Eric Carlson]

Comment on attachment 228794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228794&action=review

> Source/WebCore/ChangeLog:29
> +        (WebCore::HTMLMediaElement::setMediaControlsDependOnPageScaleFactor): Add ourselves to
> +        the callback if necessary.

Nit: Add -> Add/remove

[Dean Jackson]

Committed r166937: <http://trac.webkit.org/changeset/166937>