Bug 49396 - Make WebWidget actively notify client when compositing enables
Summary: Make WebWidget actively notify client when compositing enables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-11 11:55 PST by Nat Duca
Modified: 2010-11-15 21:13 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (2.85 KB, patch)
2010-11-11 12:05 PST, Nat Duca
fishd: review-
Details | Formatted Diff | Diff
Round 2 (5.44 KB, patch)
2010-11-15 12:03 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Better name (5.34 KB, patch)
2010-11-15 14:12 PST, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2010-11-11 11:55:15 PST
Right now, RenderWidget pasively determines when compositing enables based on checking isAcceleratedCompositingActive.

This patch adds active notification for when compositing enables and disables.
Comment 1 Nat Duca 2010-11-11 12:05:55 PST
Created attachment 73633 [details]
Proposed patch
Comment 2 Darin Fisher (:fishd, Google) 2010-11-12 16:26:33 PST
Comment on attachment 73633 [details]
Proposed patch

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

> WebKit/chromium/src/WebViewImpl.cpp:2383
> +        m_layerRenderer->finish(); // finish all GL rendering before we hide the window? TODO(nduca) fixthisshit

TODO(nduca) -> FIXME and leave a more descriptive comment for someone else who may be reading this code.

> WebKit/chromium/src/WebViewImpl.cpp:2390
> +        m_layerRenderer->resizeOnscreenContent(WebCore::IntSize(std::max(1, m_size.width),

no need for the WebCore prefix.

> WebKit/chromium/public/WebWidgetClient.h:54
> +    virtual void didChangeAcceleratedCompositingActive(bool active) { }

nit: how about didActivateAcceleratedCompositing(bool)?
Comment 3 Nat Duca 2010-11-15 12:03:25 PST
Created attachment 73919 [details]
Round 2
Comment 4 Darin Fisher (:fishd, Google) 2010-11-15 13:57:17 PST
Comment on attachment 73919 [details]
Round 2

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

> WebKit/chromium/src/WebViewImpl.cpp:-1022
> -        m_layerRenderer->present();

nice to see this cleanup!

> WebKit/chromium/public/WebWidgetClient.h:54
> +    virtual void didAcceleratedCompositingEnable(bool active) { }

why this over didActivateAcceleratedCompositing?  when vangelis added the isAcceleratedCompositingActive,
i suggested using "enabled" instead of "active" as the suffix, but he argued against "enable" since that
sounds like the compile-time option (is the code enabled at all).  so, we went with "active", and he used
that throughout the codebase (both webkit and chrome).  can we stick with "active"/"activate"?
Comment 5 Nat Duca 2010-11-15 14:06:33 PST
> why this over didActivateAcceleratedCompositing?

My bad. I made this change last Friday before I got your emailed ocmmets; the name I jotted down on the post-it was different. Will upload a new patch shortly.
Comment 6 Nat Duca 2010-11-15 14:12:55 PST
Created attachment 73929 [details]
Better name
Comment 7 WebKit Commit Bot 2010-11-15 21:13:01 PST
Comment on attachment 73929 [details]
Better name

Clearing flags on attachment: 73929

Committed r72058: <http://trac.webkit.org/changeset/72058>
Comment 8 WebKit Commit Bot 2010-11-15 21:13:07 PST
All reviewed patches have been landed.  Closing bug.