Ensure the new navigation classifier works in the real world.

Ensure the new navigation classifier works in the real world.

BUG=369661
TEST=No whammies!

Committed: https://crrev.com/39825ed5795d26cc69167c1fe774f23117cc8f77
Cr-Commit-Position: refs/heads/master@{#333755}

[Avi (use Gerrit), 2015-06-09 21:44:02]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Avi (use Gerrit), 2015-06-09 22:24:32]

avi@chromium.org changed reviewers:
+ rsesek@chromium.org

[Avi (use Gerrit), 2015-06-09 22:24:33]

Robert, keys.
Charlie, can you think of anything else to check?

[Charlie Reis, 2015-06-09 23:32:14]

LGTM with nits (and if we record process ID + routing ID rather than pointer).

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:834:
base::debug::SetCrashKeyValue("369661-spp", "yes");
This is unreachable, given line 822.

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2089: std::string result =
base::Int64ToString(reinterpret_cast<int64_t>(this));
This seems very suspicious to me (signed int, reinterpret_cast, etc).  I'm not
sure the actual pointer value will help us much.  Can you just put the process
ID + routing ID instead?

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:444: // Temporary for
http://crbug.com/369661
nit: End comment with period (here and elsewhere).

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:737: int commit_count_ = 0;
*shakes fist at C++11*

I'm not sure what language we're writing in anymore...  :)

[Avi (use Gerrit), 2015-06-10 16:34:53]

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl.cc:834:
base::debug::SetCrashKeyValue("369661-spp", "yes");
On 2015/06/09 23:32:13, Charlie Reis wrote:
> This is unreachable, given line 822.

Good point. (I just copied the old set of keys.)

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:2089: std::string result =
base::Int64ToString(reinterpret_cast<int64_t>(this));
On 2015/06/09 23:32:13, Charlie Reis wrote:
> This seems very suspicious to me (signed int, reinterpret_cast, etc).  I'm not
> sure the actual pointer value will help us much.  Can you just put the process
> ID + routing ID instead?

The *actual* value of the pointer is completely irrelevant. I only need to
correlate the commits per RFH (e.g. for renderframe X we get this stuff logged
for commit 1 and commit 2. I need some value, and whether it's the "this"
pointer or the routing id doesn't matter.

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:444: // Temporary for
http://crbug.com/369661
On 2015/06/09 23:32:13, Charlie Reis wrote:
> nit: End comment with period (here and elsewhere).

Done.

https://codereview.chromium.org/1171973004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:737: int commit_count_ = 0;
On 2015/06/09 23:32:13, Charlie Reis wrote:
> *shakes fist at C++11*
> 
> I'm not sure what language we're writing in anymore...  :)

Hahahaha

[Robert Sesek, 2015-06-10 16:36:01]

keys.cc LGTM

[Avi (use Gerrit), 2015-06-10 16:38:41]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-06-10 16:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1171973004/#ps20001
(title: "nits")

[commit-bot: I haz the power, 2015-06-10 16:39:37]

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

[commit-bot: I haz the power, 2015-06-10 17:30:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-10 17:31:03]

Patchset 2 (id:??) landed as
https://crrev.com/39825ed5795d26cc69167c1fe774f23117cc8f77
Cr-Commit-Position: refs/heads/master@{#333755}

[Avi (use Gerrit), 2015-06-11 20:18:20]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1180933002/ by avi@chromium.org.

The reason for reverting is: We have an explanation..