[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


xiyuan <xiyuan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117288|review?                     |
               Flag|                            |




--- Comment #19 from xiyuan <xiyuan at chromium.org>  2011-11-30 16:55:00 PST ---
(From update of attachment 117288)
View in context: https://bugs.webkit.org/attachment.cgi?id=117288&action=review

All done. I will upload updated patch shortly.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:48
>> +    virtual WebRect getDirtyRect() const = 0;
> 
> webkit style is to omit the 'get' prefix for functions like this

Done.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:49
>> +};
> 
> can you please define a protected, virtual d'tor with an empty body?

Done.

>> Source/WebKit/chromium/public/WebView.h:416
>> +    // graphcical apperance of the WebView. WebPageOverlayClient paints the
> 
> typos: graphcical -> graphical, apperance -> appearance

oops, Fixed.

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

Removed zorder client callback and passing it in the add call. The zorder is now stored as part of PageOverlay so that we could add an overlay to proper position.

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

Done.

>> Source/WebKit/chromium/src/PageOverlayList.cpp:68
>> +    bool zorderChanged = false;
> 
> nit: zOrderChanged, please. "zorder" isn't a word

Renamed to zOrderChanged

>> 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?)

Done.

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