Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 914253004: Fix captive portal histogram in SSL interstitials. (Closed)

Created:
5 years, 9 months ago by meacer
Modified:
5 years, 9 months ago
Reviewers:
felt, jwd
CC:
chromium-reviews, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix captive portal histogram in SSL interstitials. The SSL interstitial currently records captive portal histograms in its constructor, but captive portal detection results arrive after the interstitial is displayed so the histograms are incomplete. This CL moves the histogram recording to the interstitial's destructor. BUG=335933 Committed: https://crrev.com/59114e6e4736489f3a30eb30acccc1b59829c5f1 Cr-Commit-Position: refs/heads/master@{#316018}

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -14 lines) Patch
M chrome/browser/ssl/ssl_blocking_page.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ssl/ssl_error_classification.cc View 2 chunks +2 lines, -0 lines 1 comment Download
M tools/metrics/histograms/histograms.xml View 1 chunk +12 lines, -11 lines 0 comments Download

Messages

Total messages: 11 (2 generated)
meacer
felt, jwd, can you please take a look? felt: chrome/browser/ssl/* jwd: histograms.xml Thanks.
5 years, 9 months ago (2015-02-12 01:21:02 UTC) #2
felt
https://codereview.chromium.org/914253004/diff/1/chrome/browser/ssl/ssl_error_classification.cc File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/914253004/diff/1/chrome/browser/ssl/ssl_error_classification.cc#newcode63 chrome/browser/ssl/ssl_error_classification.cc:63: CAPTIVE_PORTAL_ALL, All older data will get bucketed incorrectly as ...
5 years, 9 months ago (2015-02-12 01:35:37 UTC) #3
meacer
On 2015/02/12 01:35:37, felt wrote: > https://codereview.chromium.org/914253004/diff/1/chrome/browser/ssl/ssl_error_classification.cc > File chrome/browser/ssl/ssl_error_classification.cc (right): > > https://codereview.chromium.org/914253004/diff/1/chrome/browser/ssl/ssl_error_classification.cc#newcode63 > ...
5 years, 9 months ago (2015-02-12 01:40:30 UTC) #4
felt
On 2015/02/12 01:40:30, Mustafa Emre Acer wrote: > On 2015/02/12 01:35:37, felt wrote: > > ...
5 years, 9 months ago (2015-02-12 05:09:40 UTC) #5
jwd
lgtm Same sentiment as felt@
5 years, 9 months ago (2015-02-12 18:01:56 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914253004/1
5 years, 9 months ago (2015-02-12 18:05:18 UTC) #8
meacer
Thanks!
5 years, 9 months ago (2015-02-12 18:58:41 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 9 months ago (2015-02-12 19:20:25 UTC) #10
commit-bot: I haz the power
5 years, 9 months ago (2015-02-12 19:21:26 UTC) #11
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/59114e6e4736489f3a30eb30acccc1b59829c5f1
Cr-Commit-Position: refs/heads/master@{#316018}

Powered by Google App Engine
This is Rietveld 408576698