[Webkit-unassigned] [Bug 73235] [Chromium] Support adding/removing page overlay to WebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73235





--- Comment #18 from James Robinson <jamesr at chromium.org>  2011-11-30 16:17:15 PST ---
(From update of attachment 117288)
View in context: https://bugs.webkit.org/attachment.cgi?id=117288&action=review

> Source/WebKit/chromium/public/WebPageOverlayClient.h:48
> +    virtual WebRect getDirtyRect() const = 0;

webkit style is to omit the 'get' prefix for functions like this

> Source/WebKit/chromium/public/WebPageOverlayClient.h:49
> +};

can you please define a protected, virtual d'tor with an empty body?

> Source/WebKit/chromium/public/WebView.h:416
> +    // graphcical apperance of the WebView. WebPageOverlayClient paints the

typos: graphcical -> graphical, apperance -> appearance

> Source/WebKit/chromium/public/WebView.h:423
> +    virtual void addWebPageOverlay(WebPageOverlayClient*) = 0;

it looks like the z-order value is only checked in this add call, unless I'm missing something. in that case it seems like the z-order should be provided as a parameter here instead of being a client callback

> Source/WebKit/chromium/src/PageOverlay.cpp:54
> +    return canvas;

the way this is written is a bit odd - could you just return directly from inside the #if blocks?

> Source/WebKit/chromium/src/PageOverlayList.cpp:68
> +    bool zorderChanged = false;

nit: zOrderChanged, please. "zorder" isn't a word

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:285
> +    return 99;

could you add a comment explaining this value (it looks like it's just picked to be bigger than any normal overlays, correct?)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.