Regarding viewport meta tags, what matters for browser developers is to know when the viewport data has changed and its current value. Viewport data belongs to the document, but it is useful to keep the current viewport data in Page as a reference, to be able to send notifications only when the current viewport has changed.
(In reply to comment #0) > Regarding viewport meta tags, what matters for browser developers is to know when the viewport data has changed and its current value. > Viewport data belongs to the document, but it is useful to keep the current viewport data in Page as a reference, to be able to send notifications only when the current viewport has changed. Which can happen for instance when the document is taken out of the session history.
Created attachment 69104 [details] patch
Created attachment 69105 [details] Script used for testing
Comment on attachment 69104 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=69104&action=review > WebCore/dom/Document.cpp:2599 > + frame->page()->updateViewportArguments(); I think we should rename this to updateViewportData in a future patch, ie rename ViewportArguments to ViewportData all over. > WebCore/html/HTMLBodyElement.cpp:47 > ASSERT(hasTagName(bodyTag)); > + if (document && document->page()) > + document->page()->updateViewportArguments(); I do not know if this is considered a layering violation, but it was suggested by Hyatt. Maybe there is a better way to do it? Darin? > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:647 > emit m_webPage->viewportChangeRequested(); Looks good. We need to document this in the future.
Created attachment 69108 [details] Script used for testing
Created attachment 69111 [details] patch rebase
Darin, Could you have a look at this one? Especially the HTMLBodyElement.cpp part?
Comment on attachment 69111 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=69111&action=review > WebCore/ChangeLog:5 > + Viewport change notifications viewport *data* change notification > WebCore/page/Chrome.cpp:429 > +void Chrome::viewportDataChanged() const dispatchViewportDataDidChange is probably a better name and more in line with the other function names in Chrome/ChromeClient. > WebKit/qt/Api/qwebpage.cpp:2388 > + WebCore::ViewportConfiguration conf = WebCore::findConfigurationForViewportData(d->viewportArguments(), desktopWidth, deviceWidth, deviceHeight, deviceDPI, availableSize); in the near future, I think we should rename this one to computeConfigurationForViewportData.
Darin, Hyatt, ping?
Comments from IRC: [7:17pm] dhyatt: kenneth: that patch looks fine to me [7:18pm] kenneth: great, I wasn't sure if it was a layering violation calling out from the constructor [7:18pm] dhyatt: kenneth: only weird part about that is that you can have multiple bodies [7:18pm] dhyatt: or someone could just make one via createElement [7:18pm] dhyatt: so it might not even be relevant [7:18pm] dhyatt: so in that sense it's kind of wasteful [7:19pm] kenneth: yes, but at least it will verify if the viewport data changed [7:19pm] dhyatt: insertedIntoDocumnt would be a better place for that code [7:19pm] dhyatt: than the constructor [7:19pm] dhyatt: that way you'll only run it when a real body goes into the document
(In reply to comment #10) > Comments from IRC: > > [7:17pm] dhyatt: kenneth: that patch looks fine to me > [7:18pm] kenneth: great, I wasn't sure if it was a layering violation calling out from the constructor > [7:18pm] dhyatt: kenneth: only weird part about that is that you can have multiple bodies > [7:18pm] dhyatt: or someone could just make one via createElement > [7:18pm] dhyatt: so it might not even be relevant > [7:18pm] dhyatt: so in that sense it's kind of wasteful > [7:19pm] kenneth: yes, but at least it will verify if the viewport data changed > [7:19pm] dhyatt: insertedIntoDocumnt would be a better place for that code > [7:19pm] dhyatt: than the constructor > [7:19pm] dhyatt: that way you'll only run it when a real body goes into the document That is HTMLBodyElement::insertedIntoDocument()
Generally looks good, but r- due to the below: 1. Change the Changelog to reflect the right name of the bug report "viewport *data* change notification" 2. Change viewportDataChanged to something like dispatchViewportDataDidChange 3. Call into the document from HTMLBodyElement::insertedIntoDocument() instead of the constructor.
maybe we should not have another m_viewportArguments in Page? why not refer document()->viewportArguments instead? Just trying to not duplicate it now, and someone in the future make a way they can get out of sync. Btw, we could have an ASSERT somewhere to guarantee that.
I am trying to adjust the changes of this patch to efl port. It seems to me that existed "didReceiveViewportArguments()" is changed to the "viewportDataChanged()". And, we can get viewport data by viewportArguments() of the Document class. However, there is a problem for efl port. In order to get viewport arguments, ChromeClientEfl needs to have the frame instance. But, the ChromeClientEfl class doesn't maintain frame instance. So, to adjust this patch to efl port, we need to get "frame" as parameter as below, >> virtual void viewportDataChanged(Frame* frame) const { } If this is difficult to modify the function, I need to find alternative way. But,in my opinion, to add parameter is easy. Is it possible to modify the function ?
(In reply to comment #13) > maybe we should not have another m_viewportArguments in Page? why not refer document()->viewportArguments instead? > > Just trying to not duplicate it now, and someone in the future make a way they can get out of sync. Btw, we could have an ASSERT somewhere to guarantee that. The idea is to only send notifications when the current viewport arguments has changed. Page keeps the current viewport so that we can compare the previous and the new ones and avoid notifying a viewport change if the new document has the same viewport as the previous one.
(In reply to comment #14) > I am trying to adjust the changes of this patch to efl port. It seems to me that existed "didReceiveViewportArguments()" is changed to the "viewportDataChanged()". And, we can get viewport data by viewportArguments() of the Document class. > > However, there is a problem for efl port. In order to get viewport arguments, ChromeClientEfl needs to have the frame instance. But, the ChromeClientEfl class doesn't maintain frame instance. So, to adjust this patch to efl port, we need to get "frame" as parameter as below, > > >> virtual void viewportDataChanged(Frame* frame) const { } > > If this is difficult to modify the function, I need to find alternative way. But,in my opinion, to add parameter is easy. > > Is it possible to modify the function ? What if we had the viewport data as parameter. It would be: virtual void viewportDataChanged(const ViewportArgument&) const { }
(In reply to comment #16) > (In reply to comment #14) > > I am trying to adjust the changes of this patch to efl port. It seems to me that existed "didReceiveViewportArguments()" is changed to the "viewportDataChanged()". And, we can get viewport data by viewportArguments() of the Document class. > > > > However, there is a problem for efl port. In order to get viewport arguments, ChromeClientEfl needs to have the frame instance. But, the ChromeClientEfl class doesn't maintain frame instance. So, to adjust this patch to efl port, we need to get "frame" as parameter as below, > > > > >> virtual void viewportDataChanged(Frame* frame) const { } > > > > If this is difficult to modify the function, I need to find alternative way. But,in my opinion, to add parameter is easy. > > > > Is it possible to modify the function ? > > What if we had the viewport data as parameter. It would be: > > virtual void viewportDataChanged(const ViewportArgument&) const { } Yes, if viewport argument is passed by pamameter, it is good. However, existed viewport implementation of efl port also have used the Frame to invoke a function of FrameLoaderClientEfl. FrameLoaderClientEfl* client = static_cast<FrameLoaderClientEfl*>(frame->loader()->client()); if (client->getInitLayoutCompleted()) return; So, if Frame parameter can be added to pamameter as well, it is good for me. virtual void viewportDataChanged(Frame*, const ViewportArgument&) const { }
> Yes, if viewport argument is passed by pamameter, it is good. However, existed viewport implementation of efl port also have used the Frame to invoke a function of FrameLoaderClientEfl. > > FrameLoaderClientEfl* client = static_cast<FrameLoaderClientEfl*>(frame->loader()->client()); > if (client->getInitLayoutCompleted()) > return; You will have to modify the above code anyway, as that should not be needed anymore. Your client side code should be a lot more simple after this change.
(In reply to comment #18) > > FrameLoaderClientEfl* client = static_cast<FrameLoaderClientEfl*>(frame->loader()->client()); > > if (client->getInitLayoutCompleted()) > > return; > > You will have to modify the above code anyway, as that should not be needed anymore. Your client side code should be a lot more simple after this change. Ok, I will look into further how to change it.
Created attachment 69620 [details] patch
(In reply to comment #15) > (In reply to comment #13) > > maybe we should not have another m_viewportArguments in Page? why not refer document()->viewportArguments instead? > > > > Just trying to not duplicate it now, and someone in the future make a way they can get out of sync. Btw, we could have an ASSERT somewhere to guarantee that. > > The idea is to only send notifications when the current viewport arguments has changed. Page keeps the current viewport so that we can compare the previous and the new ones and avoid notifying a viewport change if the new document has the same viewport as the previous one. I see now, ok. I would had named it in Page more descriptive to self-explain its propose. Maybe something like: m_lastDispatchedViewportArguments , or something like that. What do you think?
(In reply to comment #21) > (In reply to comment #15) > > (In reply to comment #13) > > > maybe we should not have another m_viewportArguments in Page? why not refer document()->viewportArguments instead? > > > > > > Just trying to not duplicate it now, and someone in the future make a way they can get out of sync. Btw, we could have an ASSERT somewhere to guarantee that. > > > > The idea is to only send notifications when the current viewport arguments has changed. Page keeps the current viewport so that we can compare the previous and the new ones and avoid notifying a viewport change if the new document has the same viewport as the previous one. > > I see now, ok. I would had named it in Page more descriptive to self-explain its propose. Maybe something like: m_lastDispatchedViewportArguments , or something like that. > > What do you think? It is not really the last. It is just the viewport arguemtns for the current document in the page.
Committed r69009: <http://trac.webkit.org/changeset/69009>
Revision r69009 cherry-picked into qtwebkit-2.1 with commit e6e21be <http://gitorious.org/webkit/qtwebkit/commit/e6e21be>