Bug 107325 – [Chromium] Expose didCommitCompositorFrame in WebWidgetClient.

• Component: WebKit API
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

The didCommit event is currently exposed via the didBecomeReadyForAdditionalInput method, but re-utilizing this for purposes other than input is misleading and can lead to issues in the future if this code changes. To solve this, we should expose didCommit as a separate event.

[Leandro Graciá Gil]

Created attachment 183546 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 183546 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidgetClient.h:88
> +    virtual void didCommit() { }

nit: can you be more specific with the name of this method?  based on didCommitAndDrawCompositorFrame, perhaps the name of this method should be didCommitCompositorFrame?

there are many things that use the term "commit" in web browsers, and it could be rather confusing to see a didCommit method implemented in a WebWidgetClient subclass, which may be concerned with many things beyond just page rendering.

[Leandro Graciá Gil]

Comment on attachment 183546 [details]
Patch

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

>> Source/WebKit/chromium/public/WebWidgetClient.h:88
>> +    virtual void didCommit() { }
> 
> nit: can you be more specific with the name of this method?  based on didCommitAndDrawCompositorFrame, perhaps the name of this method should be didCommitCompositorFrame?
> 
> there are many things that use the term "commit" in web browsers, and it could be rather confusing to see a didCommit method implemented in a WebWidgetClient subclass, which may be concerned with many things beyond just page rendering.

Good point. I'll be renaming didCommit to didCommitCompositorFrame as suggested. Thanks.

[Leandro Graciá Gil]

Created attachment 184063 [details]
Patch

[James Robinson]

Comment on attachment 184063 [details]
Patch

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

Code is good, comments are not.  Documentation is important, so I think it's worth going another review round.

> Source/WebKit/chromium/ChangeLog:8
> +        Expose didCommitCompositorFrame as a separate event to not mix it with didBecomeReadyForAdditionalInput.

Please describe what this patch is adding, i.e. "Add a notification on WebWidgetClient called when ...."

> Source/WebKit/chromium/public/WebWidgetClient.h:87
> +    // Called for compositing mode when a thread commit operation has finished.

This notification will be called in single-threaded mode as well, so this comment isn't entirely accurate.

[Leandro Graciá Gil]

Comment on attachment 184063 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:8
>> +        Expose didCommitCompositorFrame as a separate event to not mix it with didBecomeReadyForAdditionalInput.
> 
> Please describe what this patch is adding, i.e. "Add a notification on WebWidgetClient called when ...."

Done.

>> Source/WebKit/chromium/public/WebWidgetClient.h:87
>> +    // Called for compositing mode when a thread commit operation has finished.
> 
> This notification will be called in single-threaded mode as well, so this comment isn't entirely accurate.

Done.

[Leandro Graciá Gil]

Created attachment 184087 [details]
Patch

[James Robinson]

Comment on attachment 184087 [details]
Patch

Eh, ok

[WebKit Review Bot]

Comment on attachment 184087 [details]
Patch

Clearing flags on attachment: 184087

Committed r140495: <http://trac.webkit.org/changeset/140495>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.