Fix user agent overridding with PlzNavigate.

Fix user agent overridding with PlzNavigate.

This fixes
org.chromium.android_webview.test.AwSettingsTest#testUserAgentWithTestServer
with PlzNavigate.

BUG=645983
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2746043003
Cr-Commit-Position: refs/heads/master@{#456603}
Committed: https://chromium.googlesource.com/chromium/src/+/439e9ae5b8b0b3e7a2007ee017ac54fd4b8fefc6

[jam, 2017-03-13 15:48:54]

Description was changed from

==========
Fix user agent overridding with PlzNavigate.

This fixes
org.chromium.android_webview.test.AwSettingsTest#testUserAgentWithTestServer
with PlzNavigate.

BUG=645983
==========

to

==========
Fix user agent overridding with PlzNavigate.

This fixes
org.chromium.android_webview.test.AwSettingsTest#testUserAgentWithTestServer
with PlzNavigate.

BUG=645983
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-13 15:50:21]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 15:50:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-13 16:20:46]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[commit-bot: I haz the power, 2017-03-13 16:38:43]

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

[commit-bot: I haz the power, 2017-03-13 16:38:44]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-03-13 18:47:12]

LGTM with a nit.

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:109: virtual const std::string&
GetNavigationUserAgentOverride();
nit: The "Navigation" part of the name doesn't really add much meaning to the
method. We will use the same user agent override for subresources too, so making
it shorter and easier to read is a win IMHO.

[jam, 2017-03-13 19:32:40]

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:109: virtual const std::string&
GetNavigationUserAgentOverride();
On 2017/03/13 18:47:12, nasko (slow) wrote:
> nit: The "Navigation" part of the name doesn't really add much meaning to the
> method. We will use the same user agent override for subresources too, so
making
> it shorter and easier to read is a win IMHO.

yeah I did it this way because WebContents, which implements, already has a
GetUserAgentOverride. I know I could leave it as is, and the one override will
work for both interfaces, but I find that it's more confusing that way for
readability. wdyt?

[jam, 2017-03-13 20:18:39]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-03-13 20:19:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-03-13 20:35:41]

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:109: virtual const std::string&
GetNavigationUserAgentOverride();
On 2017/03/13 19:32:40, jam wrote:
> On 2017/03/13 18:47:12, nasko (slow) wrote:
> > nit: The "Navigation" part of the name doesn't really add much meaning to
the
> > method. We will use the same user agent override for subresources too, so
> making
> > it shorter and easier to read is a win IMHO.
> 
> yeah I did it this way because WebContents, which implements, already has a
> GetUserAgentOverride. I know I could leave it as is, and the one override will
> work for both interfaces, but I find that it's more confusing that way for
> readability. wdyt?

We have a few cases where the same pure virtual method exist on this delegate
interface and on WebContentsImpl. As long as the function is exactly the same in
both cases, we keep them exactly the same. An example of that is the OpenURL
call above. This case looks identical, so I personally don't find it more
confusing. It is one method on the same object, reachable through different
interfaces.

[jam, 2017-03-13 20:36:06]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-03-13 20:38:25]

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/2746043003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:109: virtual const std::string&
GetNavigationUserAgentOverride();
On 2017/03/13 20:35:41, nasko (slow) wrote:
> On 2017/03/13 19:32:40, jam wrote:
> > On 2017/03/13 18:47:12, nasko (slow) wrote:
> > > nit: The "Navigation" part of the name doesn't really add much meaning to
> the
> > > method. We will use the same user agent override for subresources too, so
> > making
> > > it shorter and easier to read is a win IMHO.
> > 
> > yeah I did it this way because WebContents, which implements, already has a
> > GetUserAgentOverride. I know I could leave it as is, and the one override
will
> > work for both interfaces, but I find that it's more confusing that way for
> > readability. wdyt?
> 
> We have a few cases where the same pure virtual method exist on this delegate
> interface and on WebContentsImpl. As long as the function is exactly the same
in
> both cases, we keep them exactly the same. An example of that is the OpenURL
> call above. This case looks identical, so I personally don't find it more
> confusing. It is one method on the same object, reachable through different
> interfaces.

ah, if there's already precedent with this interface I'll do the same, thanks.

Personally I didn't find this confusing but I got feedback in reviews before
that it it, so happy to go with this approach :)

[jam, 2017-03-13 20:45:30]

The CQ bit was checked by jam@chromium.org

[jam, 2017-03-13 20:45:30]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2746043003/#ps40001
(title: "review comment")

[commit-bot: I haz the power, 2017-03-13 20:46:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 20:54:39]

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

[commit-bot: I haz the power, 2017-03-13 20:54:40]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-03-13 21:00:06]

Patchset #3 (id:40001) has been deleted

[jam, 2017-03-13 21:00:10]

The CQ bit was checked by jam@chromium.org

[jam, 2017-03-13 21:00:10]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2746043003/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2017-03-13 21:01:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 22:27:23]

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

[commit-bot: I haz the power, 2017-03-13 22:27:24]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-03-13 23:00:27]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:01:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 03:05:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489446027160100,
"parent_rev": "4da606b2343d81d63e7c8fc5450e304e1fdba534", "commit_rev":
"439e9ae5b8b0b3e7a2007ee017ac54fd4b8fefc6"}

[commit-bot: I haz the power, 2017-03-14 03:07:20]

Description was changed from

==========
Fix user agent overridding with PlzNavigate.

This fixes
org.chromium.android_webview.test.AwSettingsTest#testUserAgentWithTestServer
with PlzNavigate.

BUG=645983
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix user agent overridding with PlzNavigate.

This fixes
org.chromium.android_webview.test.AwSettingsTest#testUserAgentWithTestServer
with PlzNavigate.

BUG=645983
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2746043003
Cr-Commit-Position: refs/heads/master@{#456603}
Committed:
https://chromium.googlesource.com/chromium/src/+/439e9ae5b8b0b3e7a2007ee017ac...
==========

[commit-bot: I haz the power, 2017-03-14 03:07:22]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/439e9ae5b8b0b3e7a2007ee017ac...