Allow override of user agent depending on url.

Allow override of user agent depending on url.

BUG=419308
Committed: https://crrev.com/0cf2564781c97d1cd20428bc63f701cfc3a4a001
Cr-Commit-Position: refs/heads/master@{#298216}

[Mathias Hällman, 2014-10-01 10:31:44]

mathiash@opera.com changed reviewers:
+ jamesr@chromium.org, jochen@chromium.org

[Mathias Hällman, 2014-10-01 10:36:06]

mathiash@opera.com changed reviewers:
+ jam@chromium.org

[Mathias Hällman, 2014-10-01 10:36:06]

As I noted in the BTS, Opera would like to be able to look at the URL and
possibly use a specific user agent for that site. Does that sound like something
which would be allowed upstream?

Does this look like a reasonable solution? We've had an earlier version of this
patch where instead of extending the ContentRendererClient, we would set an
object on the RenderFrameImpl. All in all this solution seemed less intrusive.

[jochen (gone - plz use gerrit), 2014-10-02 07:31:57]

we usually use the content clients as a last resort. Why not add a callback to
render frame observer?

[Mathias Hällman, 2014-10-02 09:16:06]

On 2014/10/02 07:31:57, jochen wrote:
> we usually use the content clients as a last resort. Why not add a callback to
> render frame observer?

Thanks for the input, jochen. I have uploaded a new version of the patch with
your suggestion, but I have one issue with this solution: What if the frame has
multiple observers which, for whatever reason, would return different user
agents?

[Mathias Hällman, 2014-10-02 09:17:57]

On 2014/10/02 09:16:06, Mathias Hällman wrote:
> On 2014/10/02 07:31:57, jochen wrote:
> > we usually use the content clients as a last resort. Why not add a callback
to
> > render frame observer?
> 
> Thanks for the input, jochen. I have uploaded a new version of the patch with
> your suggestion, but I have one issue with this solution: What if the frame
has
> multiple observers which, for whatever reason, would return different user
> agents?

Of course that's of purely theoretical concern and if it's a non-issue to you
I'd be happy to see an lgtm. :)

[jochen (gone - plz use gerrit), 2014-10-02 09:24:04]

dunno, IPC handling does the same - the first observer wins.

James/John, what's your take on this?

https://codereview.chromium.org/614343002/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame_observer.cc (right):

https://codereview.chromium.org/614343002/diff/20001/content/public/renderer/...
content/public/renderer/render_frame_observer.cc:37: return "";
nit std::string()

https://codereview.chromium.org/614343002/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/614343002/diff/20001/content/public/renderer/...
content/public/renderer/render_frame_observer.h:14: #include "url/gurl.h"
should be enough to forward declare GURL, no?

[jam, 2014-10-02 15:12:16]

On 2014/10/02 07:31:57, jochen wrote:
> we usually use the content clients as a last resort. Why not add a callback to
> render frame observer?

note: this is true for 'notification style' methods. however we don't put
methods on Observer classes that can change state (i.e. GetFoo), because then
the order of registration can affect things and we wanted to avoid that can of
worms.

[jam, 2014-10-02 15:12:49]

On 2014/10/02 09:24:04, jochen wrote:
> dunno, IPC handling does the same - the first observer wins.
> 
> James/John, what's your take on this?

ah just saw this too.

for IPCs this isn't really an issue because generally each observer looks for
different IPCs.

[Mathias Hällman, 2014-10-03 06:11:49]

On 2014/10/02 15:12:16, jam wrote:
> On 2014/10/02 07:31:57, jochen wrote:
> > we usually use the content clients as a last resort. Why not add a callback
to
> > render frame observer?
> 
> note: this is true for 'notification style' methods. however we don't put
> methods on Observer classes that can change state (i.e. GetFoo), because then
> the order of registration can affect things and we wanted to avoid that can of
> worms.

So is patch set 1 preferable over patch set 3 then? I definitely understand the
content client as a last resort though, what do you think of setting some "user
agent overrider object" on the frame as I mentioned in my first message?

[Mathias Hällman, 2014-10-06 05:23:03]

On 2014/10/03 06:11:49, Mathias Hällman wrote:
> On 2014/10/02 15:12:16, jam wrote:
> > On 2014/10/02 07:31:57, jochen wrote:
> > > we usually use the content clients as a last resort. Why not add a
callback
> to
> > > render frame observer?
> > 
> > note: this is true for 'notification style' methods. however we don't put
> > methods on Observer classes that can change state (i.e. GetFoo), because
then
> > the order of registration can affect things and we wanted to avoid that can
of
> > worms.
> 
> So is patch set 1 preferable over patch set 3 then? I definitely understand
the
> content client as a last resort though, what do you think of setting some
"user
> agent overrider object" on the frame as I mentioned in my first message?

Giving this a bump for the new week!

[jochen (gone - plz use gerrit), 2014-10-06 07:37:06]

i think patchset #1 + my comments is the way to go

[Mathias Hällman, 2014-10-06 08:21:50]

On 2014/10/06 07:37:06, jochen wrote:
> i think patchset #1 + my comments is the way to go

I think this is what you meant then :)
thanks

[jochen (gone - plz use gerrit), 2014-10-06 09:36:33]

lgtm

[Mathias Hällman, 2014-10-06 09:39:43]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-10-06 09:40:33]

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

[commit-bot: I haz the power, 2014-10-06 10:44:27]

Committed patchset #4 (id:60001) as 43284e0b88d87d13fd4e1602f59938f69c1b2956

[commit-bot: I haz the power, 2014-10-06 10:45:06]

Patchset 4 (id:??) landed as
https://crrev.com/0cf2564781c97d1cd20428bc63f701cfc3a4a001
Cr-Commit-Position: refs/heads/master@{#298216}