Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

The RenderFrameCreated and RenderFrameDeleted observer methods aren't
matching the lifetime of the renderer-side RenderFrame object. This CL
fixes the methods to be called at the right times to match.

BUG=450799, 425397
Committed: https://crrev.com/b21fe487807106de1344974db463ec27a370b995
Cr-Commit-Position: refs/heads/master@{#317792}

[nasko, 2015-02-11 18:15:43]

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

[nasko, 2015-02-11 18:15:44]

Hey Charlie,
Can you review this CL for me? It only fixes RenderFrameCreated/Deleted and
disables checking for consistency of RenderFrameHostChanged, which will come in
subsequent CL.
Thanks in advance!
Nasko

[Charlie Reis, 2015-02-12 00:29:31]

Wow, this is heroic.  I can see why it was so difficult!

I've got some questions below, but on the whole it looks pretty good.  I'm very
much looking forward to these methods being sane.

Also, does this fix http://crbug.com/425397 as well?  If so, let's list that bug
number in the CL description.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:72: // TODO(nasko): Re-enable
this logging once RenderFrameHostChanged observer
I don't see these logging messages checked anywhere, so I'm not sure what breaks
if they're on.  Is there a need to disable them?  (Maybe they started showing up
in the logs when they didn't before?)

(If we do need this, let's list a bug number in the TODO.)

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:624: if (delegate_ &&
(created ^ render_frame_created_)) {
I feel like != would be clearer than ^.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1053: // before
RenderViewTerminated because observers expect the subframes of any
I'm not sure why these lines were moved lower.  If it's for a similar reason as
this comment about RenderViewTerminated, let's update the comment.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1420:
render_frame_delegate_->RenderFrameCreated(new_render_frame_host.get());
Woo hoo!  Glad to see these calls move within RFH.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:90: // Release the WebUI
prior to resetting the current RenderFrameHost, as it
"it accesses it" is ambiguous.  (I think it means "as the WebUI accesses the
RenderFrameHost"?)

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3658:
SetAccessibilityModeOnFrame(accessibility_mode_, main_frame);
Where does SetAccessibilityModeOnFrame get called now?

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3695:
ClearPowerSaveBlockers(rvh->GetMainFrame());
Just to confirm, we now get RenderFrameDeleted in addition to
RenderViewTerminated on a crash?

(It'd be nice to have a test that shows that.  Maybe we can log
RenderViewTerminated in FrameTreeTest.ObserverWalksTreeAfterCrash?)

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:505: ->is_swapped_out()) {
I'm worried about regressions from this change.  There's a bunch of IPCs in
CanHandleWhileSwappedOut that might get broken due to this.

What made this necessary?  I'm wondering if we can get by without it until we
kill swappedout://.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1270: // corresponding
RenderView and main RenderFrame have already been created.
Wow.  There are so many ways to create RenderFrames!!

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1272: if
(params.renderer_created) {
Maybe params.renderer_initiated_creation?

https://codereview.chromium.org/921443003/diff/20001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/browser/w...
content/public/browser/web_contents.h:127: bool renderer_created;
Hmm, this is kind of difficult to understand without looking at the
implementation.  CreateParams are instructions to the WebContents constructor,
so let's clarify when callers should set this to true.

As is, it might mean any of the following (or something else):
 - This WC will definitely be put into an existing renderer process.
 - A renderer process asked for this WC to be created, but it could end up in a
new/different process.
 - A renderer process asked for this WC to be created, and thus its main
RenderFrame has already been created (?).

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:85: #if 0
Can we enable this check now, or is it still broken in some cases?

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:117:
AssertRenderFrameExists(render_frame_host);
Interesting.  Does this mean we don't call WCO::FrameDeleted if a subframe
crashes and then we later remove the frame from the frame tree?  In that case,
RenderFrameDeleted would be called at the time of the crash, and this assert
would fail if we later removed the FTN.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:204: // the message
first. This shouldn't happen, but for now handle it here.
Let's list a bug number here.

Any ideas on what the right behavior is?

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.h (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.h:94:
base::hash_map<std::pair<int, int>, std::string> render_frame_created_stacks_;
I think we should leave this out of this CL.  If we want to add it for
debug-only logging, let's do that in a follow-up CL so that it's easier to read
in git blame.

https://codereview.chromium.org/921443003/diff/20001/content/test/test_render...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/test/test_render...
content/test/test_render_frame_host.cc:117: SetRenderFrameCreated(true);
Should this call go in SendNavigateWithParameters, which is called here, below,
and by SendNavigateWithFile?

[nasko, 2015-02-12 17:52:27]

Yes, indeed this should fix 425397, at least the consistency checker says so : )

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:72: // TODO(nasko): Re-enable
this logging once RenderFrameHostChanged observer
On 2015/02/12 00:29:31, Charlie Reis wrote:
> I don't see these logging messages checked anywhere, so I'm not sure what
breaks
> if they're on.  Is there a need to disable them?  (Maybe they started showing
up
> in the logs when they didn't before?)
> 
> (If we do need this, let's list a bug number in the TODO.)

It starts showing up, which violates the expectations. Since this is for the
RenderFrameHostChanged, I want to disable it and fix it with the work of fixing
RenderFrameHostChanged itself.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:624: if (delegate_ &&
(created ^ render_frame_created_)) {
On 2015/02/12 00:29:31, Charlie Reis wrote:
> I feel like != would be clearer than ^.

I think so too :)

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1053: // before
RenderViewTerminated because observers expect the subframes of any
On 2015/02/12 00:29:31, Charlie Reis wrote:
> I'm not sure why these lines were moved lower.  If it's for a similar reason
as
> this comment about RenderViewTerminated, let's update the comment.

The reason I moved them is that it doesn't make sense to invalidate the node
which we are currently on before invalidating its children. I don't recall if
I've seen an actual problem.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1420:
render_frame_delegate_->RenderFrameCreated(new_render_frame_host.get());
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Woo hoo!  Glad to see these calls move within RFH.

Acknowledged.

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:90: // Release the WebUI
prior to resetting the current RenderFrameHost, as it
On 2015/02/12 00:29:31, Charlie Reis wrote:
> "it accesses it" is ambiguous.  (I think it means "as the WebUI accesses the
> RenderFrameHost"?)

Done.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3658:
SetAccessibilityModeOnFrame(accessibility_mode_, main_frame);
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Where does SetAccessibilityModeOnFrame get called now?

WebContentsImpl::RenderFrameCreated has pretty much this exact code - notify
observers and set accessibility.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3695:
ClearPowerSaveBlockers(rvh->GetMainFrame());
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Just to confirm, we now get RenderFrameDeleted in addition to
> RenderViewTerminated on a crash?

Correct.
 
> (It'd be nice to have a test that shows that.  Maybe we can log
> RenderViewTerminated in FrameTreeTest.ObserverWalksTreeAfterCrash?)

This is already tested in ObserverWalksTreeAfterCrash, it is just
RenderProcessGone, not RenderViewTerminated, as no such method exists in the
observer interface.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:505: ->is_swapped_out()) {
On 2015/02/12 00:29:31, Charlie Reis wrote:
> I'm worried about regressions from this change.  There's a bunch of IPCs in
> CanHandleWhileSwappedOut that might get broken due to this.
> 
> What made this necessary?  I'm wondering if we can get by without it until we
> kill swappedout://.

Hmm, don't quite recall, but after all the followup changes, it isn't needed
anymore.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1270: // corresponding
RenderView and main RenderFrame have already been created.
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Wow.  There are so many ways to create RenderFrames!!

Acknowledged.

https://codereview.chromium.org/921443003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1272: if
(params.renderer_created) {
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Maybe params.renderer_initiated_creation?

Done.

https://codereview.chromium.org/921443003/diff/20001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/browser/w...
content/public/browser/web_contents.h:127: bool renderer_created;
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Hmm, this is kind of difficult to understand without looking at the
> implementation.  CreateParams are instructions to the WebContents constructor,
> so let's clarify when callers should set this to true.
> 
> As is, it might mean any of the following (or something else):
>  - This WC will definitely be put into an existing renderer process.
>  - A renderer process asked for this WC to be created, but it could end up in
a
> new/different process.
>  - A renderer process asked for this WC to be created, and thus its main
> RenderFrame has already been created (?).

It is the last one of them. I've renamed the parameter and added a better
comment.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:85: #if 0
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Can we enable this check now, or is it still broken in some cases?

There are still some failures.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:117:
AssertRenderFrameExists(render_frame_host);
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Interesting.  Does this mean we don't call WCO::FrameDeleted if a subframe
> crashes and then we later remove the frame from the frame tree?  In that case,
> RenderFrameDeleted would be called at the time of the crash, and this assert
> would fail if we later removed the FTN.

I think you are right that we don't do the right thing. I think we need explicit
test for it and change of behavior in the sanity checker, though I think it
should go into a separate CL, as this one is big as it is.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:204: // the message
first. This shouldn't happen, but for now handle it here.
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Let's list a bug number here.

Done

> Any ideas on what the right behavior is?

I don't quite know yet, need to think about it, but probably at least suppress
this specific message, as we already have an WCO method for it.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.h (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.h:94:
base::hash_map<std::pair<int, int>, std::string> render_frame_created_stacks_;
On 2015/02/12 00:29:31, Charlie Reis wrote:
> I think we should leave this out of this CL.  If we want to add it for
> debug-only logging, let's do that in a follow-up CL so that it's easier to
read
> in git blame.

Done.

https://codereview.chromium.org/921443003/diff/20001/content/test/test_render...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/test/test_render...
content/test/test_render_frame_host.cc:117: SetRenderFrameCreated(true);
On 2015/02/12 00:29:31, Charlie Reis wrote:
> Should this call go in SendNavigateWithParameters, which is called here,
below,
> and by SendNavigateWithFile?

It should stay before calls to OnDidStartProvisionalLoadForFrame, which sends an
observer notification. If the frame didn't notify for creation, the
DidStartProvisionalLoad will assert.

[Charlie Reis, 2015-02-12 23:51:26]

Thanks!  LGTM.

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/921443003/diff/20001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:117:
AssertRenderFrameExists(render_frame_host);
On 2015/02/12 17:52:26, nasko wrote:
> On 2015/02/12 00:29:31, Charlie Reis wrote:
> > Interesting.  Does this mean we don't call WCO::FrameDeleted if a subframe
> > crashes and then we later remove the frame from the frame tree?  In that
case,
> > RenderFrameDeleted would be called at the time of the crash, and this assert
> > would fail if we later removed the FTN.
> 
> I think you are right that we don't do the right thing. I think we need
explicit
> test for it and change of behavior in the sanity checker, though I think it
> should go into a separate CL, as this one is big as it is.

Agreed.

https://codereview.chromium.org/921443003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/921443003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:73: // methods are fixed. See
https://crbug.com/450799
nit: End with period.

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:127: // renderer. In this case, the
renderer-side objects, such as
nit: Let's try to say "renderer process" when we can, to avoid ambiguity about
process vs RenderView vs RenderFrame.

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:128: // RenderFrameHost, have already been
created on the renderer side, and
RenderFrame  :)

[nasko, 2015-02-13 00:17:18]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[nasko, 2015-02-13 00:18:51]

nasko@chromium.org changed reviewers:
+ atwilson@chromium.org

[nasko, 2015-02-13 00:18:52]

Nits fixed.

Adding OWNERS reviewers
atwilson@ - chrome/browser/background/background_contents.cc
mvanouwerkerk@ -
chrome/browser/geolocation/geolocation_permission_context_unittest.cc

https://codereview.chromium.org/921443003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/921443003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:73: // methods are fixed. See
https://crbug.com/450799
On 2015/02/12 23:51:26, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:127: // renderer. In this case, the
renderer-side objects, such as
On 2015/02/12 23:51:26, Charlie Reis wrote:
> nit: Let's try to say "renderer process" when we can, to avoid ambiguity about
> process vs RenderView vs RenderFrame.

Good catch! I've been trying to avoid using just renderer, so fixed!

https://codereview.chromium.org/921443003/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:128: // RenderFrameHost, have already been
created on the renderer side, and
On 2015/02/12 23:51:26, Charlie Reis wrote:
> RenderFrame  :)

Done.

[nasko, 2015-02-13 00:19:35]

nasko@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[nasko, 2015-02-13 00:19:35]

Really adding mvanouwerkerk@ for OWNERS review.

[Michael van Ouwerkerk, 2015-02-13 10:56:39]

geolocation_permission_context_unittest.cc lgtm

[Andrew T Wilson (Slow), 2015-02-13 13:22:41]

lgtm

[nasko, 2015-02-13 15:16:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-02-13 15:16:54]

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

[commit-bot: I haz the power, 2015-02-13 16:14:40]

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

[commit-bot: I haz the power, 2015-02-13 16:14:41]

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

[nasko, 2015-02-13 16:56:49]

New patchsets have been uploaded after l-g-t-m from
mvanouwerkerk@chromium.org,atwilson@chromium.org

[Charlie Reis, 2015-02-13 17:15:19]

The change to web_contents_observer_sanity_checker.cc in patch set 5 LGTM, with
a note.

https://codereview.chromium.org/921443003/diff/80001/content/public/test/web_...
File content/public/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/921443003/diff/80001/content/public/test/web_...
content/public/test/web_contents_observer_sanity_checker.cc:95: // A RenderFrame
should either exist or have been deleted.
I think there's room for improvement here, but we can address that later in the
RFHChanged/FrameDeleted CL.

(It seems like any FrameDeleted call for a live RenderFrame would be accompanied
by RenderFrameDeleted.  If it's possible to send RenderFrameDeleted before
FrameDeleted, then this method can simply CHECK(render_frame_deleted_happened). 
It looks like that's just a matter of changing the order of
FrameTree::RemoveFrame, but it could have implications on other code.  Let's
leave it for now, perhaps with a TODO if you agree.)

[nasko, 2015-02-13 20:45:52]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[nasko, 2015-02-13 22:07:49]

nasko@chromium.org changed reviewers:
+ estade@chromium.org

[nasko, 2015-02-13 22:07:50]

Adding estade@ for OWNERS review of
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc

[Evan Stade, 2015-02-13 22:09:21]

autofill lgtm

https://codereview.chromium.org/921443003/diff/120001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/921443003/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:93:
// destroy it when the frame is deleted.
nit: I sorta liked the old comment better

[nasko, 2015-02-13 22:11:31]

New patchsets have been uploaded after l-g-t-m from estade@chromium.org

[nasko, 2015-02-13 22:11:50]

https://codereview.chromium.org/921443003/diff/120001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/921443003/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc:93:
// destroy it when the frame is deleted.
On 2015/02/13 22:09:20, Evan Stade wrote:
> nit: I sorta liked the old comment better

Reverted.

[nasko, 2015-02-13 22:11:55]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-02-13 22:12:48]

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

[commit-bot: I haz the power, 2015-02-13 23:59:43]

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

[commit-bot: I haz the power, 2015-02-13 23:59:45]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nasko, 2015-02-23 22:11:39]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-02-23 22:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
mvanouwerkerk@chromium.org, atwilson@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/921443003/#ps200001
(title: "Remove superclass call in OnPopupHidden.")

[commit-bot: I haz the power, 2015-02-23 22:13:01]

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

[nasko, 2015-02-24 02:32:40]

nasko@chromium.org changed reviewers:
+ mlamouri@chromium.org

[nasko, 2015-02-24 02:32:41]

Adding mlamouri@ for FYI about changes in
content/browser/manifest/manifest_manager_host.cc.

[mlamouri (slow - plz ping), 2015-02-24 13:56:01]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-02-24 13:56:02]

lgtm

[mlamouri (slow - plz ping), 2015-02-24 13:56:03]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
mvanouwerkerk@chromium.org, atwilson@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/921443003/#ps220001
(title: "Delete pending callbacks in ManifestManagerHost destructor.")

[commit-bot: I haz the power, 2015-02-24 13:57:02]

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

[commit-bot: I haz the power, 2015-02-24 14:16:06]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-02-24 14:16:53]

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