[webkit-reviews] review canceled: [Bug 73235] [Chromium] Support adding/removing page overlay to WebView : [Attachment 117288] Address comments in previous patch.

[bugzilla-daemon at webkit.org]

xiyuan <xiyuan at chromium.org> has canceled xiyuan <xiyuan at chromium.org>'s
request for review:
Bug 73235: [Chromium] Support adding/removing page overlay to WebView
https://bugs.webkit.org/show_bug.cgi?id=73235

Attachment 117288: Address comments in previous patch.
https://bugs.webkit.org/attachment.cgi?id=117288&action=review

------- Additional Comments from xiyuan <xiyuan at chromium.org>
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.