add flag to disable link disambiguation popup

We previously had a setting in chrome defaults but this deprecates that for a command line flag and utility function that always defers to the flag but otherwise returns a default based on operating system. The CL then removes some of the code that depended on the default for a more robust direct check by the render_view_impl. This should also help with some of the CrOS UI bugs that link disambiguation created with views that didn't necessarily initialize their RendererPreferences.

BUG=433007, 428163
TBR=jar

Committed: https://crrev.com/e23094a2fb86eb15f76caf5b55927d0b7e9ba8f5
Cr-Commit-Position: refs/heads/master@{#305385}

[luken, 2014-11-14 19:57:01]

Find a home for a function ShouldShowLinkDisambiguationPopup() which first
checks command line for both options, and then returns the appropriate default.

https://codereview.chromium.org/725243002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/725243002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:1661:
SINGLE_VALUE_TYPE(switches::kEnableLinkDisambiguationPopup)
ENABLE_DISABLE_VALUE_TYPE

https://codereview.chromium.org/725243002/diff/1/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/725243002/diff/1/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:482: "enable-link-disambiguation-popup";
need a disable-link-disambiguation-popup as well.

[luken, 2014-11-23 02:47:54]

luken@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2014-11-23 04:57:34]

https://codereview.chromium.org/725243002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/725243002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:48619: <int value="909439558"
label="disable-device-discovery"/>
not understanding what the xml change is all about but not enough fear to stop
me.

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
ui/base/ui_base_switches.h:20: UI_BASE_EXPORT extern const char
kDisableLinkDisambiguationPopup[];
are we propagating this switch to renderers?

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
File ui/base/ui_base_switches_util.cc (right):

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
ui/base/ui_base_switches_util.cc:23: 
yeah, that is pretty nice way to do this.

[luken, 2014-11-23 05:00:50]

https://codereview.chromium.org/725243002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/725243002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:48619: <int value="909439558"
label="disable-device-discovery"/>
On 2014/11/23 04:57:34, cpu wrote:
> not understanding what the xml change is all about but not enough fear to stop
> me.

I was failing a unit test until I added this. That's as much as I know too :).

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
ui/base/ui_base_switches.h:20: UI_BASE_EXPORT extern const char
kDisableLinkDisambiguationPopup[];
On 2014/11/23 04:57:34, cpu wrote:
> are we propagating this switch to renderers?

I'm not sure. It makes it to render_view_impl.cc, which is as far as I need it
to get.

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
File ui/base/ui_base_switches_util.cc (right):

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
ui/base/ui_base_switches_util.cc:23: 
On 2014/11/23 04:57:34, cpu wrote:
> yeah, that is pretty nice way to do this.

Scott's suggestion, credit goes to him.

[cpu_(ooo_6.6-7.5), 2014-11-23 06:05:02]

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/725243002/diff/80001/ui/base/ui_base_switches...
ui/base/ui_base_switches.h:20: UI_BASE_EXPORT extern const char
kDisableLinkDisambiguationPopup[];
On 2014/11/23 05:00:50, luken wrote:
> On 2014/11/23 04:57:34, cpu wrote:
> > are we propagating this switch to renderers?
> 
> I'm not sure. It makes it to render_view_impl.cc, which is as far as I need it
> to get.

strange, I was talking about

render_process_host_impl.cc line 1264.

[cpu_(ooo_6.6-7.5), 2014-11-23 06:05:40]

assuming switch propagation is magical. lgtm

[cpu_(ooo_6.6-7.5), 2014-11-23 06:05:53]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-11-23 06:06:15]

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

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

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

[commit-bot: I haz the power, 2014-11-23 06:10:07]

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

[luken, 2014-11-23 06:18:41]

The CQ bit was checked by luken@chromium.org

[commit-bot: I haz the power, 2014-11-23 06:19:34]

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

[commit-bot: I haz the power, 2014-11-23 14:12:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-23 14:14:05]

Patchset 5 (id:??) landed as
https://crrev.com/e23094a2fb86eb15f76caf5b55927d0b7e9ba8f5
Cr-Commit-Position: refs/heads/master@{#305385}

[jdduke (slow), 2014-11-23 20:00:59]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-11-23 20:01:01]

On 2014/11/23 06:05:40, cpu wrote:
> assuming switch propagation is magical. lgtm

Switch propagation is not magical, it must be explicitly included here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

How did this patch fix the problem locally?!

https://codereview.chromium.org/725243002/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/725243002/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:4054: if
(!switches::IsLinkDisambiguationPopupEnabled())
Could we either cache this switch or push it to Blink?

[jdduke (slow), 2014-11-23 20:05:54]

Not that I have any say in this matter, but my honest opinion is that this
feature is not ready to ship and we should disable it immediately in M39 (and
M40) until the kinks can be ironed out. At the least, it should be opt-in for
beta/stable users until such time.

[jar (doing other things), 2014-11-24 22:41:12]

Not sure why this had my TBR.... (I don't think I was connected with feature)...
but happy to see cpu LG.. it.
If you needed the histograms.xml reviewed (and that looked fine to me), then
asvitkine@ would be a good choice.

[Rick Byers, 2014-11-25 15:00:06]

On 2014/11/23 20:05:54, jdduke wrote:
> Not that I have any say in this matter, but my honest opinion is that this
> feature is not ready to ship and we should disable it immediately in M39 (and
> M40) until the kinks can be ironed out. At the least, it should be opt-in for
> beta/stable users until such time.

+1.

Note that my concerns in http://crbug.com/385249 (such as sites having no way to
disable the feature, the way they do on Android) were never addressed.