Bug 179386 – Implement VisualViewport API events

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ali Juma]

Implement the events defined by the VisualViewport spec: https://wicg.github.io/visual-viewport/#events

[Ali Juma]

Created attachment 329944 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 329944 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        testing.

I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request?

(I'm not familiar at all with the rules regarding experimental flags)

[Ali Juma]

Comment on attachment 329944 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        testing.
> 
> I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request?
> 
> (I'm not familiar at all with the rules regarding experimental flags)

With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here.

[Frédéric Wang (:fredw)]

Comment on attachment 329944 [details]
Patch

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

>>> Source/WebKit/ChangeLog:10
>>> +        testing.
>> 
>> I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request?
>> 
>> (I'm not familiar at all with the rules regarding experimental flags)
> 
> With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here.

OK, then maybe the ChangeLog should indicate that this patch finishes the implementation of the spec and so the feature is ready for wider testing.

[Ali Juma]

Created attachment 330486 [details]
Patch

[Ali Juma]

Comment on attachment 329944 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:10
>>>> +        testing.
>>> 
>>> I'm probably missing some context, but can you explain this change? Do the new APIs implemented here make this feature "ready for wider testing" or can this change be done independently? Was it discussed elsewhere with other people or are you just proposing to do it now with that review request?
>>> 
>>> (I'm not familiar at all with the rules regarding experimental flags)
>> 
>> With the new APIs implemented here, the implementation of the spec is complete. This hasn't been discussed elsewhere, I'm just proposing it here.
> 
> OK, then maybe the ChangeLog should indicate that this patch finishes the implementation of the spec and so the feature is ready for wider testing.

Done.

[Frédéric Wang (:fredw)]

Comment on attachment 330486 [details]
Patch

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

> Source/WebCore/ChangeLog:45
> +        (WebCore::getFrameViewAndLayoutIfNonNull): Deleted.

I believe WebKit people like when some explanation of per-function/file change is provided in the ChangeLog.

> Source/WebCore/page/FrameView.cpp:1679
> +        }

So IIUC, this is triggered by the call to updateLayoutIgnorePendingStylesheets, right? So we are sure that the member variables are up-to-date when we call offsetLeft(), scale() etc.

> Source/WebCore/page/VisualViewport.cpp:71
>          ASSERT(frame->pageZoomFactor());

It looks like the

ASSERT(frame->pageZoomFactor())

is no longer really necessary now. I guess you should just move it to update().

> Source/WebCore/page/VisualViewport.cpp:76
>  }

I think the helper function is really simplified now and I wonder whether it's still worth factoring out the "m_frame != nullptr" check.

How about just adding a member function

void VisualViewport::updateFrameLayout()
{
  ASSERT(m_frame);
  m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously)
}

? Then you could use it in visualViewport::scale() too.

> Source/WebCore/page/VisualViewport.cpp:83
> +    return m_offsetLeft;

With the above suggestion this would become

if (!m_frame)
  return 0.0;

updateFrameLayout();
return m_offsetLeft;

> Source/WebCore/page/VisualViewport.cpp:132
> +    m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously);

This could be just be updateFrameLayout().

> Source/WebCore/page/VisualViewport.cpp:145
> +

I think you don't need the ".0"
https://webkit.org/code-style-guidelines/#floating-point-literals

> Source/WebCore/page/VisualViewport.cpp:148
> +        if (view) {

if (auto* view = m_frame->view())

> Source/WebCore/page/VisualViewport.cpp:151
> +            auto pageZoomFactor = m_frame->pageZoomFactor();

Maybe move the ASSERT(pageZoomFactor) here?

> Source/WebCore/page/VisualViewport.h:73
> +    double m_scale { 1.0 };

I think you don't need the ".0"
https://webkit.org/code-style-guidelines/#floating-point-literals

> LayoutTests/ChangeLog:8
> +        Add tests for Visual Viewport API events.

Maybe repeat here that they check resize events (after pinch zoom and window resize) and scroll events.

[Ali Juma]

Created attachment 330722 [details]
Patch

[Ali Juma]

Comment on attachment 330486 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/ChangeLog:45
>> +        (WebCore::getFrameViewAndLayoutIfNonNull): Deleted.
> 
> I believe WebKit people like when some explanation of per-function/file change is provided in the ChangeLog.

Done.

>> Source/WebCore/page/FrameView.cpp:1679
>> +        }
> 
> So IIUC, this is triggered by the call to updateLayoutIgnorePendingStylesheets, right? So we are sure that the member variables are up-to-date when we call offsetLeft(), scale() etc.

Right.

>> Source/WebCore/page/VisualViewport.cpp:71
>>          ASSERT(frame->pageZoomFactor());
> 
> It looks like the
> 
> ASSERT(frame->pageZoomFactor())
> 
> is no longer really necessary now. I guess you should just move it to update().

Done.

>> Source/WebCore/page/VisualViewport.cpp:76
>>  }
> 
> I think the helper function is really simplified now and I wonder whether it's still worth factoring out the "m_frame != nullptr" check.
> 
> How about just adding a member function
> 
> void VisualViewport::updateFrameLayout()
> {
>   ASSERT(m_frame);
>   m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously)
> }
> 
> ? Then you could use it in visualViewport::scale() too.

Done.

>> Source/WebCore/page/VisualViewport.cpp:132
>> +    m_frame->document()->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously);
> 
> This could be just be updateFrameLayout().

Done.

>> Source/WebCore/page/VisualViewport.cpp:145
>> +
> 
> I think you don't need the ".0"
> https://webkit.org/code-style-guidelines/#floating-point-literals

Thanks, done.

>> Source/WebCore/page/VisualViewport.cpp:148
>> +        if (view) {
> 
> if (auto* view = m_frame->view())

Done.

>> Source/WebCore/page/VisualViewport.cpp:151
>> +            auto pageZoomFactor = m_frame->pageZoomFactor();
> 
> Maybe move the ASSERT(pageZoomFactor) here?

Done.

>> Source/WebCore/page/VisualViewport.h:73
>> +    double m_scale { 1.0 };
> 
> I think you don't need the ".0"
> https://webkit.org/code-style-guidelines/#floating-point-literals

Done.

>> LayoutTests/ChangeLog:8
>> +        Add tests for Visual Viewport API events.
> 
> Maybe repeat here that they check resize events (after pinch zoom and window resize) and scroll events.

Done.

[Frédéric Wang (:fredw)]

Comment on attachment 330722 [details]
Patch

LGTM, thanks!

[WebKit Commit Bot]

Comment on attachment 330722 [details]
Patch

Clearing flags on attachment: 330722

Committed r226622: <https://trac.webkit.org/changeset/226622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36372751>

[Frédéric Wang (:fredw)]

@Ali: This test fails on Apple El Capitan Debug WK1:

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r226622%20(5688)/results.html