Make LoadMonitoringExtensionHostQueue remove itself as an ExtensionHost observer at the correct tim…

Make LoadMonitoringExtensionHostQueue remove itself as an ExtensionHost observer at the correct time.

"Extensions.ExtensionHostMonitoring.NumLoaded" UMA is now correctly recorded.
I also cleaned up ExtensionHost a little to make it more obvious.

R=yoz@chromium.org
TBR=sky@chromium.org

Committed: https://crrev.com/4eefc7b64335c6008a1e5218c230dadd733875a7
Cr-Commit-Position: refs/heads/master@{#320536}

[not at google - send to devlin, 2015-03-10 22:26:07]

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
File extensions/browser/load_monitoring_extension_host_queue.cc (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
extensions/browser/load_monitoring_extension_host_queue.cc:59:
host->AddDeferredStartRenderHostObserver(this);
I moved this out of the "if (waiting_loading_.insert(host).second)" block
because it seems irrelevant to whether or not it's being waited for - the
reality is, we unconditionally observe on Add and stop observing on Remove.

[Yoyo Zhou, 2015-03-10 22:48:13]

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/exten...
extensions/browser/extension_host.cc:281: CHECK(!did_stop_loading_);
I think this is wrong. Say the background page navigates to another page - don't
we get the stop loading event again?

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
File extensions/browser/load_monitoring_extension_host_queue.cc (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
extensions/browser/load_monitoring_extension_host_queue.cc:59:
host->AddDeferredStartRenderHostObserver(this);
On 2015/03/10 22:26:07, kalman wrote:
> I moved this out of the "if (waiting_loading_.insert(host).second)" block
> because it seems irrelevant to whether or not it's being waited for - the
> reality is, we unconditionally observe on Add and stop observing on Remove.

sounds good.

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
File extensions/browser/load_monitoring_extension_host_queue.h (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
extensions/browser/load_monitoring_extension_host_queue.h:90: std::set<const
DeferredStartRenderHost*> waiting_loading_;
awaiting_loading_ would be more natural

[not at google - send to devlin, 2015-03-12 20:44:46]

Patchset #3 (id:40001) has been deleted

[not at google - send to devlin, 2015-03-12 21:47:21]

Patchset #3 (id:60001) has been deleted

[not at google - send to devlin, 2015-03-12 22:43:13]

Patchset #3 (id:80001) has been deleted

[not at google - send to devlin, 2015-03-12 22:46:12]

Patchset #3 (id:100001) has been deleted

[not at google - send to devlin, 2015-03-12 22:46:51]

Alright! Done. Made a few more changes along the way to help future confusees.

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/exten...
extensions/browser/extension_host.cc:281: CHECK(!did_stop_loading_);
On 2015/03/10 22:48:13, Yoyo Zhou wrote:
> I think this is wrong. Say the background page navigates to another page -
don't
> we get the stop loading event again?

Yeap, hopefully that explains the epic set of crashes in the tests. I guess we
should be tracking this across DidStartLoading-->DidStopLoading events, not
construction-->DidStopLoading.

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
File extensions/browser/load_monitoring_extension_host_queue.h (right):

https://codereview.chromium.org/995983002/diff/20001/extensions/browser/load_...
extensions/browser/load_monitoring_extension_host_queue.h:90: std::set<const
DeferredStartRenderHost*> waiting_loading_;
On 2015/03/10 22:48:13, Yoyo Zhou wrote:
> awaiting_loading_ would be more natural

Done.

[Yoyo Zhou, 2015-03-12 22:57:29]

LGTM

https://codereview.chromium.org/995983002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_view_host.cc (right):

https://codereview.chromium.org/995983002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_view_host.cc:102: DCHECK(has_loaded_once());
Should this be is_loading? (I mean, it's okay as is because you didn't change
the behavior. Just a little funny this way.)

https://codereview.chromium.org/995983002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/995983002/diff/120001/extensions/browser/exte...
extensions/browser/extension_host.cc:97: // Record UMA now because we're about
to stop observing our WebContents.
We'll be recording incomplete loads? Hope that's okay.

[not at google - send to devlin, 2015-03-12 23:22:36]

Patchset #4 (id:140001) has been deleted

[not at google - send to devlin, 2015-03-12 23:29:22]

Found a little bug, this is pretty similar to the last patch so putting though
CQ, but have another look if you like.

https://codereview.chromium.org/995983002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_view_host.cc (right):

https://codereview.chromium.org/995983002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_view_host.cc:102: DCHECK(has_loaded_once());
On 2015/03/12 22:57:29, Yoyo Zhou wrote:
> Should this be is_loading? (I mean, it's okay as is because you didn't change
> the behavior. Just a little funny this way.)

Yeah, it correlates with is_loading (though perhaps !is_loading, depending) not
has_loaded_once. Pretty pointless assertion though. I'll just delete it.

https://codereview.chromium.org/995983002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/995983002/diff/120001/extensions/browser/exte...
extensions/browser/extension_host.cc:97: // Record UMA now because we're about
to stop observing our WebContents.
On 2015/03/12 22:57:29, Yoyo Zhou wrote:
> We'll be recording incomplete loads? Hope that's okay.

Yeah I couldn't decide whether or not this was a good thing. Maybe it does skew
the results in an overall bad way. Funnily enough, adding this behavior was part
of the reason I touched this file in the first place. That would make this CL
basically a rename of has_loaded and removing some of the unnecessary
assertions.

Well - SGTM. Ok.

[not at google - send to devlin, 2015-03-12 23:29:26]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-03-12 23:29:26]

The patchset sent to the CQ was uploaded after l-g-t-m from yoz@chromium.org
Link to the patchset: https://codereview.chromium.org/995983002/#ps160001
(title: "roll back some stuff")

[commit-bot: I haz the power, 2015-03-12 23:29:48]

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

[commit-bot: I haz the power, 2015-03-12 23:51:32]

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

[commit-bot: I haz the power, 2015-03-12 23:51:32]

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

[not at google - send to devlin, 2015-03-13 16:38:08]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2015-03-13 16:38:34]

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

[commit-bot: I haz the power, 2015-03-13 18:29:22]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-03-13 18:30:15]

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

[not at google - send to devlin, 2015-03-16 17:06:30]

A revert of this CL (patchset #4 id:160001) has been created in
https://codereview.chromium.org/1012823002/ by kalman@chromium.org.

The reason for reverting is: Crashes (see crbug.com/467353)..