Remove DumpWithoutCrashing from ShouldAllowOpenURL.

Remove DumpWithoutCrashing from ShouldAllowOpenURL.

The root causes of this call have been understood (see discussion on
issue 696034), and this should not be needed anymore.

BUG=696034, 698709
Review-Url: https://codereview.chromium.org/2787573003
Cr-Commit-Position: refs/heads/master@{#462216}
Committed: https://chromium.googlesource.com/chromium/src/+/7f7bae62d5928d1da3e0d100d5f4215bfce61a87

[alexmos, 2017-03-29 19:04:11]

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

[commit-bot: I haz the power, 2017-03-29 19:04:48]

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

[alexmos, 2017-03-29 21:49:46]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org

[alexmos, 2017-03-29 21:49:47]

Nick, can you please take a look?  I think I understand where most of the
reports from this DWOC were coming from (described in comments on 696034), and I
don't think any of those require changes on our side.  I don't think we need the
DWOC anymore, but I did add metrics for the SiteInstance scheme so that we can
look at any other interesting cases if there's any unexpected data.

For now, I left the other DWOC for blob/filesystem there.  I'm kind of curious
to look at any reports we're getting just for that one alone, as the two DWOCs
were always confused since they share the same signature.

[ncarter (slow), 2017-03-29 22:59:21]

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:115:
// RecordShowAllowOpenURLFailure.
It's possible to add a presubmit that will fail if this winds up mismatching the
xml.

You can also declare these enums in such a way that the .xml can be auto-updated
from the .cc file. We do this for bad_message, see the scripts here: --

https://cs.chromium.org/chromium/src/tools/metrics/histograms/update_histogra...

and the other update_ scripts (which just call into the above).

LMK if you want more guidance here -- I'm very familiar with those scripts.

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:133:
SCHEME_FILESYSTEM,
content://  (that's the android-only file:// like scheme)
chrome-native://
chrome-distiller://

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:134:
SCHEME_LAST,
I recommend adding a unittest here that: grab a set of registered schemes from
somewhere (maybe from the ContentBrowserClient impl, maybe from GURL) and pass
it through your function.

Then use histogram_tester to assert that the histogram has no samples in the
UNKNOWN bucket.

[commit-bot: I haz the power, 2017-03-30 00:35:00]

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

[commit-bot: I haz the power, 2017-03-30 00:35:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alexmos, 2017-03-30 17:21:04]

Description was changed from

==========
Remove DumpWithoutCrashing from ShouldAllowOpenURL.

The root causes of this call have been understood (see discussion on
issue 696034), and this should not be needed anymore.

BUG=696034
==========

to

==========
Remove DumpWithoutCrashing from ShouldAllowOpenURL.

The root causes of this call have been understood (see discussion on
issue 696034), and this should not be needed anymore.

BUG=696034,698709
==========

[alexmos, 2017-03-31 00:56:43]

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

[commit-bot: I haz the power, 2017-03-31 00:57:49]

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

[alexmos, 2017-03-31 01:11:04]

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:115:
// RecordShowAllowOpenURLFailure.
On 2017/03/29 22:59:21, ncarter wrote:
> It's possible to add a presubmit that will fail if this winds up mismatching
the
> xml.
> 
> You can also declare these enums in such a way that the .xml can be
auto-updated
> from the .cc file. We do this for bad_message, see the scripts here: --
> 
>
https://cs.chromium.org/chromium/src/tools/metrics/histograms/update_histogra...
> 
> and the other update_ scripts (which just call into the above).
> 
> LMK if you want more guidance here -- I'm very familiar with those scripts.

Done.  This is awesome, thanks for the pointers!  I had no idea these existed.

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:133:
SCHEME_FILESYSTEM,
On 2017/03/29 22:59:21, ncarter wrote:
> content://  (that's the android-only file:// like scheme)
> chrome-native://
> chrome-distiller://

Done.  content:// should never show up for this uma (no extensions on android),
but still good to have it in the list (and the same is true for things like
view-source).

https://codereview.chromium.org/2787573003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:134:
SCHEME_LAST,
On 2017/03/29 22:59:21, ncarter wrote:
> I recommend adding a unittest here that: grab a set of registered schemes from
> somewhere (maybe from the ContentBrowserClient impl, maybe from GURL) and pass
> it through your function.
> 
> Then use histogram_tester to assert that the histogram has no samples in the
> UNKNOWN bucket.

Done.  Had to move some things around to let the test access the recording
function (and fixed a typo in the name along the way).  I didn't find a good
list of schemes; I ended up pulling out some known schemes out of
ContentBrowserClient (the ones in GURL aren't exposed publicly) in a pretty
arbitrary way, plus adding a few manually, but let me know if you see a better
way.

[commit-bot: I haz the power, 2017-03-31 02:33:54]

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

[commit-bot: I haz the power, 2017-03-31 02:33:54]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ncarter (slow), 2017-04-03 18:34:48]

lgtm

https://codereview.chromium.org/2787573003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_should_allow_open_url_histograms.py
(right):

https://codereview.chromium.org/2787573003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_should_allow_open_url_histograms.py:22:
histogram_enum_name='ShouldAllowOpenURLFailureScheme',
I think it might be generally useful to have a scheme enum (enums can
be shared by several histograms stats).

So, these presubmits lay the groundwork for promoting this to its own header
file (possibly in content) once we want to use it in a second place.

[alexmos, 2017-04-04 18:32:30]

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

[commit-bot: I haz the power, 2017-04-04 18:33:36]

Dry run: 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-04-04 20:59:25]

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

[commit-bot: I haz the power, 2017-04-04 20:59:25]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-04-04 21:18:16]

Thanks, Nick.  Note that I made a small tweak in PS3 to fix compile on a couple
of bots.

https://codereview.chromium.org/2787573003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_should_allow_open_url_histograms.py
(right):

https://codereview.chromium.org/2787573003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_should_allow_open_url_histograms.py:22:
histogram_enum_name='ShouldAllowOpenURLFailureScheme',
On 2017/04/03 18:34:48, ncarter wrote:
> I think it might be generally useful to have a scheme enum (enums can
> be shared by several histograms stats).
> 
> So, these presubmits lay the groundwork for promoting this to its own header
> file (possibly in content) once we want to use it in a second place.

Acknowledged, I agree this might be generally useful in other stats.

[alexmos, 2017-04-04 21:21:36]

alexmos@chromium.org changed reviewers:
+ asvitkine@chromium.org, dbeam@chromium.org, rdevlin.cronin@chromium.org

[alexmos, 2017-04-04 21:21:37]

Adding OWNERS:
rdevlin.cronin@: please review chrome/browser/extensions/
asvitkine@: please review tools/metrics/histograms/
dbeam@: please review chrome/browser/PRESUBMIT.py

[Dan Beam, 2017-04-04 22:03:46]

lgtm w/nits

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
File chrome/browser/PRESUBMIT.py (right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
chrome/browser/PRESUBMIT.py:37: import
presubmit_should_allow_open_url_histograms
this identifier is ... long

can we make it shorter?

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
chrome/browser/PRESUBMIT.py:39: PrecheckShouldAllowOpenURLEnums(input_api,
output_api))
this_text_is_kind_of_repetitive.
ThisTextIsKindOfRepetitive()

[Devlin, 2017-04-05 01:11:57]

extensions lgtm, though honestly this is more of a metrics patch than anything
else.  asvitkine@, please take a close look. :)

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:728:
UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure.Scheme",
I don't think I've seen us have Histogram.Foo and Histogram.Foo.Bar be distinct
entries before.  I'd assume it would work, though?

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
(right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:98:
GURL site_url);
const &

[alexmos, 2017-04-05 17:17:15]

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

[commit-bot: I haz the power, 2017-04-05 17:17:42]

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

[alexmos, 2017-04-05 17:24:51]

Thanks for reviewing!

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
File chrome/browser/PRESUBMIT.py (right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
chrome/browser/PRESUBMIT.py:37: import
presubmit_should_allow_open_url_histograms
On 2017/04/04 22:03:46, Dan Beam wrote:
> this identifier is ... long
> 
> can we make it shorter?

Renamed to presubmit_scheme_histograms.py

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/PRESUBMI...
chrome/browser/PRESUBMIT.py:39: PrecheckShouldAllowOpenURLEnums(input_api,
output_api))
On 2017/04/04 22:03:46, Dan Beam wrote:
> this_text_is_kind_of_repetitive.
> ThisTextIsKindOfRepetitive()

With the rename, this is hopefully not repetitive anymore.  I could make the
presubmit function more generic and remove ShouldAllowOpenURL from this
altogether.  I was thinking this can wait until we have a second use case for
the scheme enum (as Nick pointed out), at which point the enum will probably
move out to a more public place anyway.

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:728:
UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure.Scheme",
On 2017/04/05 01:11:57, Devlin wrote:
> I don't think I've seen us have Histogram.Foo and Histogram.Foo.Bar be
distinct
> entries before.  I'd assume it would work, though?

Yes, it'd be good to get confirmation from asvitkine@ that this is ok.

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
(right):

https://codereview.chromium.org/2787573003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:98:
GURL site_url);
On 2017/04/05 01:11:57, Devlin wrote:
> const &

Done.

[commit-bot: I haz the power, 2017-04-05 18:33:27]

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

[commit-bot: I haz the power, 2017-04-05 18:33:28]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-04-05 19:13:58]

lgtm

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:693:
const char* const kSchemeNames[] = {
Nit: Add static

Else, compilers will emit a bunch of code to initialize this at runtime very
time this is called.

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/chrome_content_browser_client_extensions_part_unittest.cc
(right):

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part_unittest.cc:9:
#include
"chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
This should be the first include.

[alexmos, 2017-04-05 19:53:41]

Thanks!

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:693:
const char* const kSchemeNames[] = {
On 2017/04/05 19:13:58, Alexei Svitkine (slow) wrote:
> Nit: Add static
> 
> Else, compilers will emit a bunch of code to initialize this at runtime very
> time this is called.

Done.

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/chrome_content_browser_client_extensions_part_unittest.cc
(right):

https://codereview.chromium.org/2787573003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part_unittest.cc:9:
#include
"chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
On 2017/04/05 19:13:58, Alexei Svitkine (slow) wrote:
> This should be the first include.

Done.

[alexmos, 2017-04-05 19:55:20]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2017-04-05 19:55:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
rdevlin.cronin@chromium.org, dbeam@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2787573003/#ps80001
(title: "Address asvitkine's comments")

[commit-bot: I haz the power, 2017-04-05 19:55:56]

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-04-05 21:15:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491422120964610,
"parent_rev": "bb0b3ae9224e1883dd491b6b6810b6bfe0da8480", "commit_rev":
"7f7bae62d5928d1da3e0d100d5f4215bfce61a87"}

[commit-bot: I haz the power, 2017-04-05 21:16:01]

Description was changed from

==========
Remove DumpWithoutCrashing from ShouldAllowOpenURL.

The root causes of this call have been understood (see discussion on
issue 696034), and this should not be needed anymore.

BUG=696034,698709
==========

to

==========
Remove DumpWithoutCrashing from ShouldAllowOpenURL.

The root causes of this call have been understood (see discussion on
issue 696034), and this should not be needed anymore.

BUG=696034,698709

Review-Url: https://codereview.chromium.org/2787573003
Cr-Commit-Position: refs/heads/master@{#462216}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f7bae62d5928d1da3e0d100d5f4...
==========

[commit-bot: I haz the power, 2017-04-05 21:16:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7f7bae62d5928d1da3e0d100d5f4...