[Autofill] Add ability to load Risk fingerprint data in AutofillDialogControllerImpl.

[Autofill] Add ability to load Risk fingerprint data in AutofillDialogControllerImpl.

BUG=166596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183714

[Ilya Sherman, 2013-02-07 01:09:56]

Dan, are you ok with the TODO(dbeam)'s added in this CL?  I could change them to
TODO(isherman)'s, but I think most likely you're the one who'll end up
implementing them.

[Evan Stade, 2013-02-07 01:34:53]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
it seems like all this bounds calculation stuff could and should be handled in
the risk code (for better encapsulation). All you need to pass is the
WebContents.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:995: 
^H

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:213: // calls
OnDidLoadRiskFingerprintData() with the data once it's loaded.
this function does not actually call OnDidLoadRiskFingerprintData, so this
comment seems misleading to me. Perhaps:

Asks risk module to asynchronously load fingerprint data. Data will be returned
via OnDidLoadRiskFingerprintData.

and then you can remove the 2 lines between the 2 functions.

https://chromiumcodereview.appspot.com/12212057/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1215: if (GetView()) {
no curlies

[Dan Beam, 2013-02-07 01:36:45]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:968: //
TODO(dbeam): Set the right GAIA id.
this can be obtained via
|base::StringToInt64(wallet_items_.obfuscated_gaia_id())|

[Dan Beam, 2013-02-07 01:37:33]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:968: //
TODO(dbeam): Set the right GAIA id.
On 2013/02/07 01:36:46, Dan Beam wrote:
> this can be obtained via
> |base::StringToInt64(wallet_items_.obfuscated_gaia_id())|

er, I mean... wallet_items_->obfuscated_gaia_id() (assuming wallet_items_.get()
!= NULL, of course)

[Ilya Sherman, 2013-02-07 01:58:29]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:968: //
TODO(dbeam): Set the right GAIA id.
On 2013/02/07 01:36:46, Dan Beam wrote:
> this can be obtained via
> |base::StringToInt64(wallet_items_.obfuscated_gaia_id())|

Done.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:968: //
TODO(dbeam): Set the right GAIA id.
On 2013/02/07 01:37:33, Dan Beam wrote:
> On 2013/02/07 01:36:46, Dan Beam wrote:
> > this can be obtained via
> > |base::StringToInt64(wallet_items_.obfuscated_gaia_id())|
> 
> er, I mean... wallet_items_->obfuscated_gaia_id() (assuming
wallet_items_.get()
> != NULL, of course)

Done.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/07 01:34:53, Evan Stade wrote:
> it seems like all this bounds calculation stuff could and should be handled in
> the risk code (for better encapsulation). All you need to pass is the
> WebContents.

That would require the risk code to have dependencies on chrome/browser/ui code,
which is undesirable.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:995: 
On 2013/02/07 01:34:53, Evan Stade wrote:
> ^H

Done.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:213: // calls
OnDidLoadRiskFingerprintData() with the data once it's loaded.
On 2013/02/07 01:34:53, Evan Stade wrote:
> this function does not actually call OnDidLoadRiskFingerprintData, so this
> comment seems misleading to me. Perhaps:
> 
> Asks risk module to asynchronously load fingerprint data. Data will be
returned
> via OnDidLoadRiskFingerprintData.
> 
> and then you can remove the 2 lines between the 2 functions.

Done.

https://chromiumcodereview.appspot.com/12212057/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:1215: if (GetView()) {
On 2013/02/07 01:34:53, Evan Stade wrote:
> no curlies

Done.

[Evan Stade, 2013-02-07 02:09:15]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
probably worth noting that FindBrowserWithWebContents can fail if the
WebContents is embedded in something besides a browser window. You should handle
that without crashing.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/07 01:58:29, Ilya Sherman wrote:
> On 2013/02/07 01:34:53, Evan Stade wrote:
> > it seems like all this bounds calculation stuff could and should be handled
in
> > the risk code (for better encapsulation). All you need to pass is the
> > WebContents.
> 
> That would require the risk code to have dependencies on chrome/browser/ui
code,
> which is undesirable.

you could solve that by adding a function to WebContentsDelegate which returns
the bounds of the window the webcontents is embedded in. This is not too
dissimilar in spirit from WebContentsDelegate::IsPopupOrPanel. You might consult
jam to see if that's ok.

Honestly it seems a bit silly that the risk params include both the webview's
size and the window's size. The two are pretty much directly related, except for
small platform differences, details like whether the bookmark bar is open, and
extreme corner cases like the web inspector sucking up space.

[Ilya Sherman, 2013-02-07 02:27:32]

John, what do you think of Evan's suggestion of adding a method to the
WebContentsDelegate to get at the containing browser window's bounds?

[jam, 2013-02-07 17:28:43]

On 2013/02/07 02:27:32, Ilya Sherman wrote:
> John, what do you think of Evan's suggestion of adding a method to the
> WebContentsDelegate to get at the containing browser window's bounds?

one of the guiding principles of the content api is that any embedder interface
(like WebContentsDelegate) should only have methods that are called by content.
i.e. WCD shouldn't have a method that is there because it's called by chrome. we
had gone through and removed all such instances.

i can appreciate that the autofill code is being componentized. doesn't
autofill, as part of splitting it off from the rest of chrome/browser, have a
delegate interface that chrome/browser implements? if so, can a method be added
to that which takes a WebContents and returns the window bounds from Browser?

[Ilya Sherman, 2013-02-14 09:00:40]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/07 02:09:16, Evan Stade wrote:
> probably worth noting that FindBrowserWithWebContents can fail if the
> WebContents is embedded in something besides a browser window. You should
handle
> that without crashing.

Aren't we guaranteed to be inside a browser window whenever the rAc UI is
showing?  If not, this probably isn't the right way to compute the window
bounds.

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/07 02:09:16, Evan Stade wrote:
> On 2013/02/07 01:58:29, Ilya Sherman wrote:
> > On 2013/02/07 01:34:53, Evan Stade wrote:
> > > it seems like all this bounds calculation stuff could and should be
handled
> in
> > > the risk code (for better encapsulation). All you need to pass is the
> > > WebContents.
> > 
> > That would require the risk code to have dependencies on chrome/browser/ui
> code,
> > which is undesirable.
> 
> you could solve that by adding a function to WebContentsDelegate which returns
> the bounds of the window the webcontents is embedded in. This is not too
> dissimilar in spirit from WebContentsDelegate::IsPopupOrPanel. You might
consult
> jam to see if that's ok.

Per John's comment, I did not do this.  However, I did change the code to accept
a WebContents rather than the content_bounds rect and screen_info.

[Evan Stade, 2013-02-19 21:18:18]

lgtm

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/14 09:00:40, Ilya Sherman wrote:
> On 2013/02/07 02:09:16, Evan Stade wrote:
> > probably worth noting that FindBrowserWithWebContents can fail if the
> > WebContents is embedded in something besides a browser window. You should
> handle
> > that without crashing.
> 
> Aren't we guaranteed to be inside a browser window whenever the rAc UI is
> showing?  If not, this probably isn't the right way to compute the window
> bounds.

I can't remember if platform apps have a Browser object.

https://chromiumcodereview.appspot.com/12212057/diff/18001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/18001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1094: //
TODO(dbeam): Add a DCHECK that the ToS have been accepted prior to
due to the sensitivity of the issue, perhaps something stronger than a DCHECK is
wise.

[Ilya Sherman, 2013-02-20 00:05:19]

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/1/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:972:
chrome::FindBrowserWithWebContents(web_contents())->window()->GetBounds();
On 2013/02/19 21:18:18, Evan Stade wrote:
> On 2013/02/14 09:00:40, Ilya Sherman wrote:
> > On 2013/02/07 02:09:16, Evan Stade wrote:
> > > probably worth noting that FindBrowserWithWebContents can fail if the
> > > WebContents is embedded in something besides a browser window. You should
> > handle
> > > that without crashing.
> > 
> > Aren't we guaranteed to be inside a browser window whenever the rAc UI is
> > showing?  If not, this probably isn't the right way to compute the window
> > bounds.
> 
> I can't remember if platform apps have a Browser object.

You're right, they don't.  Added code to handle platform apps as well.

https://chromiumcodereview.appspot.com/12212057/diff/18001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12212057/diff/18001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1094: //
TODO(dbeam): Add a DCHECK that the ToS have been accepted prior to
On 2013/02/19 21:18:18, Evan Stade wrote:
> due to the sensitivity of the issue, perhaps something stronger than a DCHECK
is
> wise.

Do you mean something like a CHECK(), or handling the failure by means of an
early return?

[Ilya Sherman, 2013-02-20 00:05:39]

John, could you review the changes to content/ ?

[jam, 2013-02-20 00:58:33]

On 2013/02/20 00:05:39, Ilya Sherman wrote:
> John, could you review the changes to content/ ?

I was still waiting for a reply to my previous question:
"one of the guiding principles of the content api is that any embedder interface
(like WebContentsDelegate) should only have methods that are called by content.
i.e. WCD shouldn't have a method that is there because it's called by chrome. we
had gone through and removed all such instances.

i can appreciate that the autofill code is being componentized. doesn't
autofill, as part of splitting it off from the rest of chrome/browser, have a
delegate interface that chrome/browser implements? if so, can a method be added
to that which takes a WebContents and returns the window bounds from Browser?"

[Ilya Sherman, 2013-02-20 01:19:23]

On 2013/02/20 00:58:33, jam wrote:
> On 2013/02/20 00:05:39, Ilya Sherman wrote:
> > John, could you review the changes to content/ ?
> 
> I was still waiting for a reply to my previous question:
> "one of the guiding principles of the content api is that any embedder
interface
> (like WebContentsDelegate) should only have methods that are called by
content.
> i.e. WCD shouldn't have a method that is there because it's called by chrome.
we
> had gone through and removed all such instances.
> 
> i can appreciate that the autofill code is being componentized. doesn't
> autofill, as part of splitting it off from the rest of chrome/browser, have a
> delegate interface that chrome/browser implements? if so, can a method be
added
> to that which takes a WebContents and returns the window bounds from Browser?"

That would be possible, but it would just mean that the code currently in
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc would move to
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc, which IMO isn't
worth it.  The content/ changes in the latest patch set aren't about surfacing
the containing window's bounds, but rather about exposing the WebScreenInfo
details to embedders.  Previously, WebScreenInfo was only exposed to WebKit; but
we now need to access this same info from chrome/.

[jam, 2013-02-20 01:34:49]

On 2013/02/20 01:19:23, Ilya Sherman wrote:
> On 2013/02/20 00:58:33, jam wrote:
> > On 2013/02/20 00:05:39, Ilya Sherman wrote:
> > > John, could you review the changes to content/ ?
> > 
> > I was still waiting for a reply to my previous question:
> > "one of the guiding principles of the content api is that any embedder
> interface
> > (like WebContentsDelegate) should only have methods that are called by
> content.
> > i.e. WCD shouldn't have a method that is there because it's called by
chrome.
> we
> > had gone through and removed all such instances.
> > 
> > i can appreciate that the autofill code is being componentized. doesn't
> > autofill, as part of splitting it off from the rest of chrome/browser, have
a
> > delegate interface that chrome/browser implements? if so, can a method be
> added
> > to that which takes a WebContents and returns the window bounds from
Browser?"
> 
> That would be possible, but it would just mean that the code currently in
> chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc would move to
> chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc, which IMO isn't
> worth it.

can you expand on this some more, why is it not worth it? (I'm not familiar with
that code)

>  The content/ changes in the latest patch set aren't about surfacing
> the containing window's bounds, but rather about exposing the WebScreenInfo
> details to embedders.  Previously, WebScreenInfo was only exposed to WebKit;
but
> we now need to access this same info from chrome/.

the change surfaces stuff in content that is set from chrome. why wouldn't
chrome figure out this information itself? this is the sort of stuff we don't
put in the content API, i.e. the embedder shouldn't have to loop through chrome.

also, note that there is an effort to remove WebKit usage in the browser
process: http://code.google.com/p/chromium/issues/detail?id=146251

[Ilya Sherman, 2013-02-20 02:02:41]

On 2013/02/20 01:34:49, jam wrote:
> On 2013/02/20 01:19:23, Ilya Sherman wrote:
> > On 2013/02/20 00:58:33, jam wrote:
> > > On 2013/02/20 00:05:39, Ilya Sherman wrote:
> > > > John, could you review the changes to content/ ?
> > > 
> > > I was still waiting for a reply to my previous question:
> > > "one of the guiding principles of the content api is that any embedder
> > interface
> > > (like WebContentsDelegate) should only have methods that are called by
> > content.
> > > i.e. WCD shouldn't have a method that is there because it's called by
> chrome.
> > we
> > > had gone through and removed all such instances.
> > > 
> > > i can appreciate that the autofill code is being componentized. doesn't
> > > autofill, as part of splitting it off from the rest of chrome/browser,
have
> a
> > > delegate interface that chrome/browser implements? if so, can a method be
> > added
> > > to that which takes a WebContents and returns the window bounds from
> Browser?"
> > 
> > That would be possible, but it would just mean that the code currently in
> > chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc would move to
> > chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc, which IMO isn't
> > worth it.
> 
> can you expand on this some more, why is it not worth it? (I'm not familiar
with
> that code)

 I just don't see any advantage to moving this code further away from the one
place where it's actually used.

> >  The content/ changes in the latest patch set aren't about surfacing
> > the containing window's bounds, but rather about exposing the WebScreenInfo
> > details to embedders.  Previously, WebScreenInfo was only exposed to WebKit;
> but
> > we now need to access this same info from chrome/.
> 
> the change surfaces stuff in content that is set from chrome. why wouldn't
> chrome figure out this information itself? this is the sort of stuff we don't
> put in the content API, i.e. the embedder shouldn't have to loop through
chrome.

I don't think this all is being set from Chrome.  For example, here are the
Windows [1] and GTK [2] implementations:
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

It looks like most of the work should be done by WebKit's WebScreenInfoFactory
class, but some of the work is done within content/.  I don't see anywhere that
chrome/ code is setting this data.  So, in order for Chrome to figure this out,
we'd have to duplicate the code, which seems pretty counter-productive...

Maybe there's an even lower layer than content/ where this code should live? 
Suggestions?

> also, note that there is an effort to remove WebKit usage in the browser
> process: http://code.google.com/p/chromium/issues/detail?id=146251

WebKit::WebScreenInfo is a POD type that seems to be blessed by comments 1 and
2.

[jam, 2013-02-20 02:32:18]

On Tue, Feb 19, 2013 at 6:02 PM, <isherman@chromium.org> wrote:

> On 2013/02/20 01:34:49, jam wrote:
>
>> On 2013/02/20 01:19:23, Ilya Sherman wrote:
>> > On 2013/02/20 00:58:33, jam wrote:
>> > > On 2013/02/20 00:05:39, Ilya Sherman wrote:
>> > > > John, could you review the changes to content/ ?
>> > >
>> > > I was still waiting for a reply to my previous question:
>> > > "one of the guiding principles of the content api is that any embedder
>> > interface
>> > > (like WebContentsDelegate) should only have methods that are called by
>> > content.
>> > > i.e. WCD shouldn't have a method that is there because it's called by
>> chrome.
>> > we
>> > > had gone through and removed all such instances.
>> > >
>> > > i can appreciate that the autofill code is being componentized.
>> doesn't
>> > > autofill, as part of splitting it off from the rest of chrome/browser,
>>
> have
>
>> a
>> > > delegate interface that chrome/browser implements? if so, can a
>> method be
>> > added
>> > > to that which takes a WebContents and returns the window bounds from
>> Browser?"
>> >
>> > That would be possible, but it would just mean that the code currently
>> in
>> > chrome/browser/ui/autofill/**autofill_dialog_controller_**impl.cc
>> would move to
>> > chrome/browser/ui/autofill/**tab_autofill_manager_delegate.**cc, which
>> IMO isn't
>> > worth it.
>>
>
>  can you expand on this some more, why is it not worth it? (I'm not
>> familiar
>>
> with
>
>> that code)
>>
>
>  I just don't see any advantage to moving this code further away from the
> one
> place where it's actually used.


the flip side is that we only add stuff to the content api if there's no
other way to get it. it seems that what you need is the size of the render
view, which is something that the browser should know.


>
>
>  >  The content/ changes in the latest patch set aren't about surfacing
>> > the containing window's bounds, but rather about exposing the
>> WebScreenInfo
>> > details to embedders.  Previously, WebScreenInfo was only exposed to
>> WebKit;
>> but
>> > we now need to access this same info from chrome/.
>>
>
>  the change surfaces stuff in content that is set from chrome. why wouldn't
>> chrome figure out this information itself? this is the sort of stuff we
>> don't
>> put in the content API, i.e. the embedder shouldn't have to loop through
>>
> chrome.
>
> I don't think this all is being set from Chrome.  For example, here are the
> Windows [1] and GTK [2] implementations:
> [1]
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/content/browser/renderer_**host/render_widget_host_view_**
>
win.cc&l=308-313<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_view_win.cc&l=308-313>
> [2]
> https://code.google.com/p/**chromium/codesearch#chromium/**
>
src/content/browser/renderer_**host/gtk_window_utils.cc&l=49-**68<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/gtk_window_utils.cc&l=49-68>
>
> It looks like most of the work should be done by WebKit's
> WebScreenInfoFactory
> class, but some of the work is done within content/.  I don't see anywhere
> that
> chrome/ code is setting this data.  So, in order for Chrome to figure this
> out,
> we'd have to duplicate the code, which seems pretty counter-productive...
>


from your previous email, it seems that what you need is the "containing
browser window's bounds". That is data which chrome is the one that knows
about.


> Maybe there's an even lower layer than content/ where this code should
> live?
> Suggestions?
>
>
>  also, note that there is an effort to remove WebKit usage in the browser
>> process:
http://code.google.com/p/**chromium/issues/detail?id=**146251<http://code.goo...
>>
>
> WebKit::WebScreenInfo is a POD type that seems to be blessed by comments 1
> and
> 2.
>

ah, i wasn't aware of that.


>
>
https://codereview.chromium.**org/12212057/<https://codereview.chromium.org/1...
>

[Ilya Sherman, 2013-02-20 03:44:19]

On 2013/02/20 02:32:18, jam wrote:
> On Tue, Feb 19, 2013 at 6:02 PM, <mailto:isherman@chromium.org> wrote:
> 
> > On 2013/02/20 01:34:49, jam wrote:
> >
> >> On 2013/02/20 01:19:23, Ilya Sherman wrote:
> >> > On 2013/02/20 00:58:33, jam wrote:
> >> > > On 2013/02/20 00:05:39, Ilya Sherman wrote:
> >> > > > John, could you review the changes to content/ ?
> >> > >
> >> > > I was still waiting for a reply to my previous question:
> >> > > "one of the guiding principles of the content api is that any embedder
> >> > interface
> >> > > (like WebContentsDelegate) should only have methods that are called by
> >> > content.
> >> > > i.e. WCD shouldn't have a method that is there because it's called by
> >> chrome.
> >> > we
> >> > > had gone through and removed all such instances.
> >> > >
> >> > > i can appreciate that the autofill code is being componentized.
> >> doesn't
> >> > > autofill, as part of splitting it off from the rest of chrome/browser,
> >>
> > have
> >
> >> a
> >> > > delegate interface that chrome/browser implements? if so, can a
> >> method be
> >> > added
> >> > > to that which takes a WebContents and returns the window bounds from
> >> Browser?"
> >> >
> >> > That would be possible, but it would just mean that the code currently
> >> in
> >> > chrome/browser/ui/autofill/**autofill_dialog_controller_**impl.cc
> >> would move to
> >> > chrome/browser/ui/autofill/**tab_autofill_manager_delegate.**cc, which
> >> IMO isn't
> >> > worth it.
> >>
> >
> >  can you expand on this some more, why is it not worth it? (I'm not
> >> familiar
> >>
> > with
> >
> >> that code)
> >>
> >
> >  I just don't see any advantage to moving this code further away from the
> > one
> > place where it's actually used.
> 
> 
> the flip side is that we only add stuff to the content api if there's no
> other way to get it. it seems that what you need is the size of the render
> view, which is something that the browser should know.

The code for getting the size of the render view is remaining purely within the
chrome/ layer either way.  Evan was pushing for moving the code down into
content/ so that we could just pass in a WebContents instance where we're
currently passing both a WebContents and a separate gfx::Rect for the window
size, but since you expressed concerns about pushing this code into content/, I
haven't done so.

> >  >  The content/ changes in the latest patch set aren't about surfacing
> >> > the containing window's bounds, but rather about exposing the
> >> WebScreenInfo
> >> > details to embedders.  Previously, WebScreenInfo was only exposed to
> >> WebKit;
> >> but
> >> > we now need to access this same info from chrome/.
> >>
> >
> >  the change surfaces stuff in content that is set from chrome. why wouldn't
> >> chrome figure out this information itself? this is the sort of stuff we
> >> don't
> >> put in the content API, i.e. the embedder shouldn't have to loop through
> >>
> > chrome.
> >
> > I don't think this all is being set from Chrome.  For example, here are the
> > Windows [1] and GTK [2] implementations:
> > [1]
> > https://code.google.com/p/**chromium/codesearch#chromium/**
> > src/content/browser/renderer_**host/render_widget_host_view_**
> >
>
win.cc&l=308-313<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/render_widget_host_view_win.cc&l=308-313>
> > [2]
> > https://code.google.com/p/**chromium/codesearch#chromium/**
> >
>
src/content/browser/renderer_**host/gtk_window_utils.cc&l=49-**68<https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/gtk_window_utils.cc&l=49-68>
> >
> > It looks like most of the work should be done by WebKit's
> > WebScreenInfoFactory
> > class, but some of the work is done within content/.  I don't see anywhere
> > that
> > chrome/ code is setting this data.  So, in order for Chrome to figure this
> > out,
> > we'd have to duplicate the code, which seems pretty counter-productive...
> >
> 
> 
> from your previous email, it seems that what you need is the "containing
> browser window's bounds". That is data which chrome is the one that knows
> about.

We want multiple pieces of data.  The content/ changes in the latest patch set
are for two other pieces of data:
(1) The screen's color depth.
(2) The size of the screen that's unavailable to web page content, i.e. the
Taskbar size on Windows.

These two pieces of data are exposed to JavaScript via the WebScreenInfo object
in WebKit.  The content/ layer currently populates the WebScreenInfo data, and
passes it off to the renderer process.  The change I've made to content/ moves
this from a private API to a public one, so that we can access the data from
Chrome's browser process as well.  There is no functionality change; just a
change in visibility.

I'm 100% open to suggestions for better ways to access this data if you know of
any.  My two primary goals are (a) to match the values that we expose to
JavaScript; and (b) to not end up with multiple implementations of the code.

> > Maybe there's an even lower layer than content/ where this code should
> > live?
> > Suggestions?
> >
> >
> >  also, note that there is an effort to remove WebKit usage in the browser
> >> process:
>
http://code.google.com/p/**chromium/issues/detail?id=**146251%3Chttp://code.g...>
> >>
> >
> > WebKit::WebScreenInfo is a POD type that seems to be blessed by comments 1
> > and
> > 2.
> >
> 
> ah, i wasn't aware of that.
> 
> 
> >
> >
>
https://codereview.chromium.**org/12212057/%3Chttps://codereview.chromium.org...>
> >

[jam, 2013-02-20 19:13:19]

lgtm

I see, I misunderstood then what was being discussed to move to content with
what's there now.

[commit-bot: I haz the power, 2013-02-20 19:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12212057/24001

[commit-bot: I haz the power, 2013-02-20 20:21:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-20 23:56:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12212057/37003

[commit-bot: I haz the power, 2013-02-21 01:54:57]

Change committed as 183714

[jam, 2013-02-25 19:30:03]

hi, I didn't look at fingerprint_browsertest.cc before, but now I just saw it
while refactoring some other code. WebContents/WebContentsView shouldn't be
implemented inside chrome. these are content only interfaces. doing that in
chrome adds a burden because every time the interface needs to change people
will then get a build error for this browser test to update it.