Allow zoom-in as alternative to popup when tapping multiple targets.

Adds strategy for deciding whether to show popup or zoom in when having tapped multiple targets.

Related review: https://codereview.chromium.org/19281007/

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

[jochen (gone - plz use gerrit), 2013-07-16 10:05:53]

how is the strategy going to be changed?

https://codereview.chromium.org/19256012/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/19256012/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6739: } else
nit. in chromium, if one if branch has curly braces, all should have

[jochen (gone - plz use gerrit), 2013-07-16 10:06:12]

(and please also reference the bug you've filed from this CL)

[Mathias Hällman, 2013-07-16 12:36:30]

On 2013/07/16 10:05:53, jochen wrote:
> how is the strategy going to be changed?
Which would be the preferred way to change a setting like this? Via
WebPreferences?

[jochen (gone - plz use gerrit), 2013-07-16 12:37:46]

On 2013/07/16 12:36:30, Mathias Hällman wrote:
> On 2013/07/16 10:05:53, jochen wrote:
> > how is the strategy going to be changed?
> Which would be the preferred way to change a setting like this? Via
> WebPreferences?

will the state change during runtime?

[Mathias Hällman, 2013-07-16 12:59:19]

On 2013/07/16 12:37:46, jochen wrote:
> On 2013/07/16 12:36:30, Mathias Hällman wrote:
> > On 2013/07/16 10:05:53, jochen wrote:
> > > how is the strategy going to be changed?
> > Which would be the preferred way to change a setting like this? Via
> > WebPreferences?
> 
> will the state change during runtime?

It won't for our use case at Opera anyhow.

[jochen (gone - plz use gerrit), 2013-07-17 12:03:28]

On 2013/07/16 12:59:19, Mathias Hällman wrote:
> On 2013/07/16 12:37:46, jochen wrote:
> > On 2013/07/16 12:36:30, Mathias Hällman wrote:
> > > On 2013/07/16 10:05:53, jochen wrote:
> > > > how is the strategy going to be changed?
> > > Which would be the preferred way to change a setting like this? Via
> > > WebPreferences?
> > 
> > will the state change during runtime?
> 
> It won't for our use case at Opera anyhow.

Maybe put it into the renderer preferences then?

[Mathias Hällman, 2013-07-19 13:18:21]

On 2013/07/17 12:03:28, jochen wrote:
> On 2013/07/16 12:59:19, Mathias Hällman wrote:
> > On 2013/07/16 12:37:46, jochen wrote:
> > > On 2013/07/16 12:36:30, Mathias Hällman wrote:
> > > > On 2013/07/16 10:05:53, jochen wrote:
> > > > > how is the strategy going to be changed?
> > > > Which would be the preferred way to change a setting like this? Via
> > > > WebPreferences?
> > > 
> > > will the state change during runtime?
> > 
> > It won't for our use case at Opera anyhow.
> 
> Maybe put it into the renderer preferences then?

I moved the strategy setting to the render preferences and added the necessary
IPC message traits/members, does it look right to you?

[jochen (gone - plz use gerrit), 2013-07-22 07:46:33]

ok, looks better

You'll need an review from a content/public owner though (e.g. jam@). I'd
probably wait until the blink side is finalized before sending the review out

https://codereview.chromium.org/19256012/diff/10001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/19256012/diff/10001/content/common/view_messa...
content/common/view_messages.h:89:
IPC_ENUM_TRAITS(content::DidTapMultipleTargetsStrategy)
plz use IPC_ENUM_TRAITS_MAX_VALUE()

[Mathias Hällman, 2013-07-30 12:10:14]

jam, since the blink side seems about done, could you have a look at the
content/public changes?

[jam, 2013-07-30 16:12:40]

https://codereview.chromium.org/19256012/diff/19001/content/public/common/ren...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/19256012/diff/19001/content/public/common/ren...
content/public/common/renderer_preferences.h:41: MULTITARGET_ZOOM = 0,
nit: use content api convention for enums value names, i.e.

enum TapMultipleTargetsStrategy {
TAP_MULTIPLE_TARGETS_STRATEGY_ZOOM,
TAP_MULTIPLE_TARGETS_STRATEGY_POPUP,
};

[Mathias Hällman, 2013-07-31 07:25:34]

On 2013/07/30 16:12:40, jam wrote:
>
https://codereview.chromium.org/19256012/diff/19001/content/public/common/ren...
> File content/public/common/renderer_preferences.h (right):
> 
>
https://codereview.chromium.org/19256012/diff/19001/content/public/common/ren...
> content/public/common/renderer_preferences.h:41: MULTITARGET_ZOOM = 0,
> nit: use content api convention for enums value names, i.e.
> 
> enum TapMultipleTargetsStrategy {
> TAP_MULTIPLE_TARGETS_STRATEGY_ZOOM,
> TAP_MULTIPLE_TARGETS_STRATEGY_POPUP,
> };

Renamed in patch set 4/5. (missed removing the "did" in patch set 4..)

[jam, 2013-07-31 15:06:00]

lgtm

[commit-bot: I haz the power, 2013-07-31 15:58:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/19256012/33001

[commit-bot: I haz the power, 2013-07-31 19:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/19256012/33001

[commit-bot: I haz the power, 2013-08-01 06:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/19256012/47001

[Mathias Hällman, 2013-08-01 06:31:02]

On 2013/08/01 06:24:35, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/mathiash%40opera.com/19256012/47001

Oh, missing lgtm from an owner of view_messages.h. So that's why the commit was
failing..

cevans, do you mind having a look?

[Chris Evans, 2013-08-06 01:53:27]

On 2013/08/01 06:31:02, Mathias Hällman wrote:
> On 2013/08/01 06:24:35, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > https://chromium-status.appspot.com/cq/mathiash%2540opera.com/19256012/47001
> 
> Oh, missing lgtm from an owner of view_messages.h. So that's why the commit
was
> failing..
> 
> cevans, do you mind having a look?

LGTM

[commit-bot: I haz the power, 2013-08-06 05:40:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/19256012/47001

[commit-bot: I haz the power, 2013-08-06 07:45:55]

Change committed as 215843