Bug 46755 – Viewport data change notification

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Luiz Agostini]

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.

[Kenneth Rohde Christiansen]

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

Created attachment 69104 [details]
patch

[Luiz Agostini]

Created attachment 69105 [details]
Script used for testing

[Kenneth Rohde Christiansen]

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]

Created attachment 69108 [details]
Script used for testing

[Luiz Agostini]

Created attachment 69111 [details]
patch

rebase

[Kenneth Rohde Christiansen]

Darin,

Could you have a look at this one? Especially the HTMLBodyElement.cpp part?

[Kenneth Rohde Christiansen]

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]

Darin, Hyatt, ping?

[Kenneth Rohde Christiansen]

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]

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

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]

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]

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]

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

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

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

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

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

Created attachment 69620 [details]
patch

[Antonio Gomes]

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

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

Committed r69009: <http://trac.webkit.org/changeset/69009>

[Ademar Reis]

Revision r69009 cherry-picked into qtwebkit-2.1 with commit e6e21be <http://gitorious.org/webkit/qtwebkit/commit/e6e21be>