WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
46755
Viewport data change notification
https://bugs.webkit.org/show_bug.cgi?id=46755
Summary
Viewport data change notification
Luiz Agostini
Reported
2010-09-28 14:15:51 PDT
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.
Attachments
patch
(15.24 KB, patch)
2010-09-28 14:24 PDT
,
Luiz Agostini
no flags
Details
Formatted Diff
Diff
Script used for testing
(2.98 KB, text/html)
2010-09-28 14:26 PDT
,
Luiz Agostini
no flags
Details
Script used for testing
(3.01 KB, text/html)
2010-09-28 14:42 PDT
,
Luiz Agostini
no flags
Details
patch
(15.25 KB, patch)
2010-09-28 14:54 PDT
,
Luiz Agostini
kenneth
: review-
Details
Formatted Diff
Diff
patch
(16.20 KB, patch)
2010-10-04 05:37 PDT
,
Luiz Agostini
kenneth
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Kenneth Rohde Christiansen
Comment 1
2010-09-28 14:18:51 PDT
(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.
Luiz Agostini
Comment 2
2010-09-28 14:24:37 PDT
Created
attachment 69104
[details]
patch
Luiz Agostini
Comment 3
2010-09-28 14:26:26 PDT
Created
attachment 69105
[details]
Script used for testing
Kenneth Rohde Christiansen
Comment 4
2010-09-28 14:31:21 PDT
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.
Luiz Agostini
Comment 5
2010-09-28 14:42:45 PDT
Created
attachment 69108
[details]
Script used for testing
Luiz Agostini
Comment 6
2010-09-28 14:54:58 PDT
Created
attachment 69111
[details]
patch rebase
Kenneth Rohde Christiansen
Comment 7
2010-09-28 19:20:28 PDT
Darin, Could you have a look at this one? Especially the HTMLBodyElement.cpp part?
Kenneth Rohde Christiansen
Comment 8
2010-09-28 19:25:21 PDT
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.
Kenneth Rohde Christiansen
Comment 9
2010-09-29 10:44:46 PDT
Darin, Hyatt, ping?
Kenneth Rohde Christiansen
Comment 10
2010-09-30 15:21:08 PDT
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
Kenneth Rohde Christiansen
Comment 11
2010-09-30 15:23:26 PDT
(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()
Kenneth Rohde Christiansen
Comment 12
2010-09-30 15:38:51 PDT
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.
Antonio Gomes
Comment 13
2010-10-02 17:54:30 PDT
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.
Gyuyoung Kim
Comment 14
2010-10-04 00:39:55 PDT
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 ?
Luiz Agostini
Comment 15
2010-10-04 00:52:31 PDT
(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.
Luiz Agostini
Comment 16
2010-10-04 01:13:57 PDT
(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 { }
Gyuyoung Kim
Comment 17
2010-10-04 01:30:47 PDT
(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 { }
Kenneth Rohde Christiansen
Comment 18
2010-10-04 02:49:43 PDT
> 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.
Gyuyoung Kim
Comment 19
2010-10-04 02:52:26 PDT
(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.
Luiz Agostini
Comment 20
2010-10-04 05:37:53 PDT
Created
attachment 69620
[details]
patch
Antonio Gomes
Comment 21
2010-10-04 05:44:26 PDT
(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?
Kenneth Rohde Christiansen
Comment 22
2010-10-04 05:53:15 PDT
(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.
Luiz Agostini
Comment 23
2010-10-04 06:24:41 PDT
Committed
r69009
: <
http://trac.webkit.org/changeset/69009
>
Ademar Reis
Comment 24
2010-10-05 05:19:41 PDT
Revision
r69009
cherry-picked into qtwebkit-2.1 with commit e6e21be <
http://gitorious.org/webkit/qtwebkit/commit/e6e21be
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug