Summary: | Allow elements to register for changes in page scale | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||
Component: | DOM | Assignee: | Dean Jackson <dino> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kangil.han, philipj, sergio, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 131322 | ||||||||||
Attachments: |
|
Description
Dean Jackson
2014-04-07 15:53:02 PDT
Created attachment 228765 [details]
Patch
Created attachment 228773 [details]
Patch
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. 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? 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. Created attachment 228794 [details]
Patch
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 Committed r166937: <http://trac.webkit.org/changeset/166937> |