Add pixel readback to page popup

Add pixel readback to page popup

BUG=409503
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182027

[keishi, 2014-09-01 04:35:31]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2014-09-01 04:35:32]

CL will be landed in this order
https://codereview.chromium.org/513723003/ (this)
https://codereview.chromium.org/508063002/
https://codereview.chromium.org/530653002/

[tkent, 2014-09-01 05:36:51]

> Add pixel readback to page popup
>
> BUG=409503

The description is too short.  Please write what the CL adds and why.

[tkent, 2014-09-01 05:37:04]

https://codereview.chromium.org/513723003/diff/40001/Source/web/resources/pic...
File Source/web/resources/pickerCommon.js (right):

https://codereview.chromium.org/513723003/diff/40001/Source/web/resources/pic...
Source/web/resources/pickerCommon.js:118: console.log("anchorRect.x:" +
anchorRect.x + " anchorRect.y:" + anchorRect.y + " anchorRect.width:" +
anchorRect.width + " anchorRect.height:" + anchorRect.height);
Remove console.log()s.

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
File public/platform/WebCompositeAndReadbackAsyncCallback.h (right):

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
public/platform/WebCompositeAndReadbackAsyncCallback.h:38: void setIsPopup(bool
isPopup) { m_isPopup = isPopup; };
Because this is a public API, please write comments on each of functions.  e.g.
Who should call the function, what happens by calling it, etc.

https://codereview.chromium.org/513723003/diff/40001/public/web/WebPagePopup.h
File public/web/WebPagePopup.h (right):

https://codereview.chromium.org/513723003/diff/40001/public/web/WebPagePopup....
public/web/WebPagePopup.h:44: virtual WebPoint relativePosition() = 0;
relative to what?

[jamesr, 2014-09-02 19:09:28]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-09-02 19:09:29]

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
File public/platform/WebCompositeAndReadbackAsyncCallback.h (right):

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
public/platform/WebCompositeAndReadbackAsyncCallback.h:38: void setIsPopup(bool
isPopup) { m_isPopup = isPopup; };
nothing in platform/ should know or care about what a popup is. why would this
code need to be different from the normal readback paths?

[keishi, 2014-09-04 13:16:20]

https://codereview.chromium.org/513723003/diff/40001/Source/web/resources/pic...
File Source/web/resources/pickerCommon.js (right):

https://codereview.chromium.org/513723003/diff/40001/Source/web/resources/pic...
Source/web/resources/pickerCommon.js:118: console.log("anchorRect.x:" +
anchorRect.x + " anchorRect.y:" + anchorRect.y + " anchorRect.width:" +
anchorRect.width + " anchorRect.height:" + anchorRect.height);
On 2014/09/01 05:37:03, tkent wrote:
> Remove console.log()s.

Done.

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
File public/platform/WebCompositeAndReadbackAsyncCallback.h (right):

https://codereview.chromium.org/513723003/diff/40001/public/platform/WebCompo...
public/platform/WebCompositeAndReadbackAsyncCallback.h:38: void setIsPopup(bool
isPopup) { m_isPopup = isPopup; };
On 2014/09/02 19:09:29, jamesr wrote:
> nothing in platform/ should know or care about what a popup is. why would this
> code need to be different from the normal readback paths?

Removed. I changed it so I don't stick these data into the callback here.

https://codereview.chromium.org/513723003/diff/40001/public/web/WebPagePopup.h
File public/web/WebPagePopup.h (right):

https://codereview.chromium.org/513723003/diff/40001/public/web/WebPagePopup....
public/web/WebPagePopup.h:44: virtual WebPoint relativePosition() = 0;
On 2014/09/01 05:37:03, tkent wrote:
> relative to what?

Changed to screenPosition

[tkent, 2014-09-05 01:59:05]

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:54: class WebPagePopup;
nit: Please insert this between WebMouseEvent and WebString.

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:64: virtual WebPoint screenPosition() { return
WebPoint(); }
This should not be necessary because we already have
WebWidgetClient::windowRect().

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:262: virtual WebPagePopup* popup() const { return 0; }
nit: |popup| sounds too generic. Maybe |pagePopup|, and please add comment that
this is for calendar / datalist pickers and not for <select>.

[keishi, 2014-09-11 12:53:26]

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:54: class WebPagePopup;
On 2014/09/05 01:59:05, tkent (overloaded) wrote:
> nit: Please insert this between WebMouseEvent and WebString.

Done.

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:64: virtual WebPoint screenPosition() { return
WebPoint(); }
On 2014/09/05 01:59:05, tkent (overloaded) wrote:
> This should not be necessary because we already have
> WebWidgetClient::windowRect().

Done.

https://codereview.chromium.org/513723003/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:262: virtual WebPagePopup* popup() const { return 0; }
On 2014/09/05 01:59:04, tkent (overloaded) wrote:
> nit: |popup| sounds too generic. Maybe |pagePopup|, and please add comment
that
> this is for calendar / datalist pickers and not for <select>.

Done.

[tkent, 2014-09-11 23:31:50]

On 2014/09/11 12:53:26, keishi wrote:
> Done.

You said done, but didn't upload new patch?

[keishi, 2014-09-12 02:06:28]

On 2014/09/11 23:31:50, tkent (overloaded) wrote:
> On 2014/09/11 12:53:26, keishi wrote:
> > Done.
> 
> You said done, but didn't upload new patch?

Sorry. I'm sorry I uploaded to the other issue by mistake.

[tkent, 2014-09-12 02:17:49]

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
public/web/WebWidget.h:266: virtual WebWidgetClient* client() const { return 0;
}
Why do you need this?  An embedder should know who's the client for this
WebWidget.

[keishi, 2014-09-12 02:27:22]

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
public/web/WebWidget.h:266: virtual WebWidgetClient* client() const { return 0;
}
On 2014/09/12 02:17:49, tkent (overloaded) wrote:
> Why do you need this?  An embedder should know who's the client for this
> WebWidget.

I can get the WebWidgetClient for the main view by adding
WebTestProxyBase::set_widget_client, but how should I get the client for
PagePopup?

Should I add WebWidget::pagePopupClient?
Or add WebPagePopup::client()?

[tkent, 2014-09-12 03:08:28]

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
public/web/WebWidget.h:266: virtual WebWidgetClient* client() const { return 0;
}
On 2014/09/12 02:27:22, keishi wrote:
> On 2014/09/12 02:17:49, tkent (overloaded) wrote:
> > Why do you need this?  An embedder should know who's the client for this
> > WebWidget.
> 
> I can get the WebWidgetClient for the main view by adding
> WebTestProxyBase::set_widget_client, but how should I get the client for
> PagePopup?

I don't know.  Don't we have a way to get content::RenderWidget from
blink::WebWidget in the content layer?

[keishi, 2014-09-12 04:39:40]

On 2014/09/12 03:08:28, tkent (overloaded) wrote:
> https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
> File public/web/WebWidget.h (right):
> 
>
https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
> public/web/WebWidget.h:266: virtual WebWidgetClient* client() const { return
0;
> }
> On 2014/09/12 02:27:22, keishi wrote:
> > On 2014/09/12 02:17:49, tkent (overloaded) wrote:
> > > Why do you need this?  An embedder should know who's the client for this
> > > WebWidget.
> > 
> > I can get the WebWidgetClient for the main view by adding
> > WebTestProxyBase::set_widget_client, but how should I get the client for
> > PagePopup?
> 
> I don't know.  Don't we have a way to get content::RenderWidget from
> blink::WebWidget in the content layer?

WebWidgetClients have a pointer to WebWidgets to manage lifetime and to send
events and stuff.
WebWidget implementations keep a pointer to WebWidgets to ask it to do stuff.
But it doesn't look like we can get a WebWidgetClient from a WebWidget.

[jamesr, 2014-09-12 04:40:32]

On 2014/09/12 04:39:40, keishi wrote:
> On 2014/09/12 03:08:28, tkent (overloaded) wrote:
> > https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
> > File public/web/WebWidget.h (right):
> > 
> >
>
https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
> > public/web/WebWidget.h:266: virtual WebWidgetClient* client() const { return
> 0;
> > }
> > On 2014/09/12 02:27:22, keishi wrote:
> > > On 2014/09/12 02:17:49, tkent (overloaded) wrote:
> > > > Why do you need this?  An embedder should know who's the client for this
> > > > WebWidget.
> > > 
> > > I can get the WebWidgetClient for the main view by adding
> > > WebTestProxyBase::set_widget_client, but how should I get the client for
> > > PagePopup?
> > 
> > I don't know.  Don't we have a way to get content::RenderWidget from
> > blink::WebWidget in the content layer?
> 
> WebWidgetClients have a pointer to WebWidgets to manage lifetime and to send
> events and stuff.
> WebWidget implementations keep a pointer to WebWidgets to ask it to do stuff.
> But it doesn't look like we can get a WebWidgetClient from a WebWidget.

The WebWidget is pretty much always owned by the WebWidgetClient.  When would
you have only a WebWidget* and want to get to its client?  If you have a
WebWidget* and live on the embedder side, you probably know the real type of the
WebWidget as well.

[keishi, 2014-09-12 14:20:58]

On 2014/09/12 04:40:32, jamesr wrote:
> On 2014/09/12 04:39:40, keishi wrote:
> > On 2014/09/12 03:08:28, tkent (overloaded) wrote:
> > >
https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h
> > > File public/web/WebWidget.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/513723003/diff/80001/public/web/WebWidget.h#n...
> > > public/web/WebWidget.h:266: virtual WebWidgetClient* client() const {
return
> > 0;
> > > }
> > > On 2014/09/12 02:27:22, keishi wrote:
> > > > On 2014/09/12 02:17:49, tkent (overloaded) wrote:
> > > > > Why do you need this?  An embedder should know who's the client for
this
> > > > > WebWidget.
> > > > 
> > > > I can get the WebWidgetClient for the main view by adding
> > > > WebTestProxyBase::set_widget_client, but how should I get the client for
> > > > PagePopup?
> > > 
> > > I don't know.  Don't we have a way to get content::RenderWidget from
> > > blink::WebWidget in the content layer?
> > 
> > WebWidgetClients have a pointer to WebWidgets to manage lifetime and to send
> > events and stuff.
> > WebWidget implementations keep a pointer to WebWidgets to ask it to do
stuff.
> > But it doesn't look like we can get a WebWidgetClient from a WebWidget.
> 
> The WebWidget is pretty much always owned by the WebWidgetClient.  When would
> you have only a WebWidget* and want to get to its client?  If you have a
> WebWidget* and live on the embedder side, you probably know the real type of
the
> WebWidget as well.



I tried to use WebWidgetClient in WebTestProxyBase but then I couldn't get to
the WebWIdget (because content::RenderWidget has the pointer to WebWidget and
WebWidgetClient can't be cast to content::RenderWidget). So for this new patch I
added WebPagePopup::positionRelativeToOwner and let the blink side handle the
position calculation.

[tkent, 2014-09-15 23:36:49]

> I tried to use WebWidgetClient in WebTestProxyBase but then I couldn't get to
> the WebWIdget (because content::RenderWidget has the pointer to WebWidget and
> WebWidgetClient can't be cast to content::RenderWidget). So for this new patch
I
> added WebPagePopup::positionRelativeToOwner and let the blink side handle the
> position calculation.

ok, this is better than adding client().
lgtm

[tkent, 2014-09-15 23:36:53]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-09-15 23:37:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/513723003/100001

[commit-bot: I haz the power, 2014-09-16 00:49:40]

Committed patchset #4 (id:100001) as 182027