Add risk data to getrealpan request.

Add risk data to getrealpan request.

BUG=451286
Committed: https://crrev.com/028ecc84877a5973485d9deb80e871155219bc02
Cr-Commit-Position: refs/heads/master@{#315386}

[Evan Stade, 2015-02-02 22:21:41]

estade@chromium.org changed reviewers:
+ brettw@chromium.org, jam@chromium.org

[Evan Stade, 2015-02-02 22:21:41]

+jam for content/
+brettw for the rest

[brettw, 2015-02-02 23:44:10]

https://codereview.chromium.org/896583003/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/risk_util.cc (right):

https://codereview.chromium.org/896583003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/risk_util.cc:45: Browser* browser =
chrome::FindBrowserWithWebContents(web_contents);
This function is really unfortunate. Is there any possible way we can pass this
data to LoadRiskData? It seems like the calling code is frontend UI, so that
should be possible.

[Evan Stade, 2015-02-02 23:47:52]

https://codereview.chromium.org/896583003/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/risk_util.cc (right):

https://codereview.chromium.org/896583003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/risk_util.cc:45: Browser* browser =
chrome::FindBrowserWithWebContents(web_contents);
On 2015/02/02 23:44:10, brettw wrote:
> This function is really unfortunate. Is there any possible way we can pass
this
> data to LoadRiskData? It seems like the calling code is frontend UI, so that
> should be possible.

This code is moved from AutofillDialogControllerImpl (which is one of two
callers of LoadRiskData). I don't see an advantage in moving it back there, or
how we can get around calling it somewhere.

[brettw, 2015-02-03 00:03:56]

Can we pass the BrowserContext through this system to avoid this? Looking up a
random toplevel Window is something we really should be avoiding and various
people have ranted to me when they encounter such code.

[brettw, 2015-02-03 00:03:56]

Can we pass the BrowserContext through this system to avoid this? Looking up a
random toplevel Window is something we really should be avoiding and various
people have ranted to me when they encounter such code.

[brettw, 2015-02-03 00:11:53]

LGTM, I misunderstood how the code works

[jam, 2015-02-03 05:53:43]

https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
content/public/browser/web_contents.h:411: virtual gfx::NativeWindow
GetTopLevelNativeWindow() const = 0;
they don't have const by design. see
http://www.chromium.org/developers/content-module/content-api:  "don't add the
const identifier to interfaces. For interfaces implemented by the embedder, we
can't make assumptions about what the embedder needs to implement it. For
interfaces implemented by content, the implementation details doesn't have to be
exposed."

[Evan Stade, 2015-02-03 17:48:40]

https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
content/public/browser/web_contents.h:411: virtual gfx::NativeWindow
GetTopLevelNativeWindow() const = 0;
On 2015/02/03 05:53:43, jam wrote:
> they don't have const by design. see
> http://www.chromium.org/developers/content-module/content-api:  "don't add the
> const identifier to interfaces. For interfaces implemented by the embedder, we
> can't make assumptions about what the embedder needs to implement it. For
> interfaces implemented by content, the implementation details doesn't have to
be
> exposed."

ok, will remove changes to content. Why do some of the methods in this file have
the const qualifier?

[Evan Stade, 2015-02-03 20:38:12]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:39:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896583003/40001

[commit-bot: I haz the power, 2015-02-03 20:41:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:41:46]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2015-02-03 21:02:21]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:03:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896583003/60001

[commit-bot: I haz the power, 2015-02-03 21:10:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:10:43]

Try jobs failed on following builders:
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Evan Stade, 2015-02-04 20:29:55]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:31:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896583003/80001

[commit-bot: I haz the power, 2015-02-04 20:40:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:40:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Evan Stade, 2015-02-04 21:44:54]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-04 21:46:11]

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

[commit-bot: I haz the power, 2015-02-04 22:13:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 22:13:38]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jam, 2015-02-05 22:58:17]

On 2015/02/03 17:48:40, Evan Stade wrote:
>
https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
> File content/public/browser/web_contents.h (right):
> 
>
https://codereview.chromium.org/896583003/diff/20001/content/public/browser/w...
> content/public/browser/web_contents.h:411: virtual gfx::NativeWindow
> GetTopLevelNativeWindow() const = 0;
> On 2015/02/03 05:53:43, jam wrote:
> > they don't have const by design. see
> > http://www.chromium.org/developers/content-module/content-api:  "don't add
the
> > const identifier to interfaces. For interfaces implemented by the embedder,
we
> > can't make assumptions about what the embedder needs to implement it. For
> > interfaces implemented by content, the implementation details doesn't have
to
> be
> > exposed."
> 
> ok, will remove changes to content. Why do some of the methods in this file
have
> the const qualifier?

that should be fixed..

[Evan Stade, 2015-02-06 00:15:45]

estade@chromium.org changed reviewers:
+ aruslan@chromium.org

[Evan Stade, 2015-02-06 00:15:46]

Had to make a couple minor changes for Android. aruslan, please review changes
to:
- content/content_browser.gypi
- content/browser/BUILD.gn

also, I have another question for you inline.

https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
File chrome/browser/autofill/risk_util.cc (right):

https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
chrome/browser/autofill/risk_util.cc:66: // No easy way to get window bounds on
Android, and that signal isn't very
aruslan@, is there an easy way to get the browser window bounds on Android?

Risk team says it's not necessary (not particularly useful on mobile), so if
not, we can just skip over it.

[Evan Stade, 2015-02-06 21:42:34]

On 2015/02/06 00:15:46, Evan Stade wrote:
> Had to make a couple minor changes for Android. aruslan, please review changes
> to:
> - content/content_browser.gypi
> - content/browser/BUILD.gn
> 
> also, I have another question for you inline.
> 
>
https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
> File chrome/browser/autofill/risk_util.cc (right):
> 
>
https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
> chrome/browser/autofill/risk_util.cc:66: // No easy way to get window bounds
on
> Android, and that signal isn't very
> aruslan@, is there an easy way to get the browser window bounds on Android?
> 
> Risk team says it's not necessary (not particularly useful on mobile), so if
> not, we can just skip over it.

ping aruslan

[Evan Stade, 2015-02-09 20:46:48]

On 2015/02/06 21:42:34, Evan Stade wrote:
> On 2015/02/06 00:15:46, Evan Stade wrote:
> > Had to make a couple minor changes for Android. aruslan, please review
changes
> > to:
> > - content/content_browser.gypi
> > - content/browser/BUILD.gn
> > 
> > also, I have another question for you inline.
> > 
> >
>
https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
> > File chrome/browser/autofill/risk_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
> > chrome/browser/autofill/risk_util.cc:66: // No easy way to get window bounds
> on
> > Android, and that signal isn't very
> > aruslan@, is there an easy way to get the browser window bounds on Android?
> > 
> > Risk team says it's not necessary (not particularly useful on mobile), so if
> > not, we can just skip over it.
> 
> ping aruslan

aruslan tbr'd

[Evan Stade, 2015-02-09 20:46:53]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-02-09 20:47:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896583003/140001

[commit-bot: I haz the power, 2015-02-09 20:53:35]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-09 21:00:21]

Patchset 8 (id:??) landed as
https://crrev.com/028ecc84877a5973485d9deb80e871155219bc02
Cr-Commit-Position: refs/heads/master@{#315386}

[aruslan, 2015-02-10 00:18:21]

(lgtm android)

https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
File chrome/browser/autofill/risk_util.cc (right):

https://codereview.chromium.org/896583003/diff/140001/chrome/browser/autofill...
chrome/browser/autofill/risk_util.cc:66: // No easy way to get window bounds on
Android, and that signal isn't very
On 2015/02/06 00:15:46, Evan Stade wrote:
> aruslan@, is there an easy way to get the browser window bounds on Android?
> 
> Risk team says it's not necessary (not particularly useful on mobile), so if
> not, we can just skip over it.

You might use the size of the display with and without window decorations using
DeviceDisplayInfo().GetPhysicalDisplayHeight() etc.

It wouldn't work in all cases (e.g. in the case of multi-window devices etc),
but it's a good first approximation and it's an easy way.