Merge NOTIFICATION_RENDER_VIEW_HOST_CHANGED into NOTIFICATION_WEB_CONTENTS_SWAPPED.

Merge NOTIFICATION_WEB_CONTENTS_SWAPPED into NOTIFICATION_RENDER_VIEW_HOST_CHANGED.

NOTIFICATION_WEB_CONTENTS_SWAPPED was fired in a strict subset of times that NOTIFICATION_RENDER_VIEW_HOST_CHANGED was fired, but everyone was listening for NOTIFICATION_WEB_CONTENTS_SWAPPED since it showed up in the types file first.

BUG=170921, 123007
TEST=no functional change

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226637

[Avi (use Gerrit), 2013-09-06 21:22:54]

John: FYI
Jonathan: Take a look at the Android stuff
Charlie: I'm sending this to you because of your bug crbug.com/123007

I'm not yet ready to commit this (I'm running the tests now) but I see this as a
cleanup. NOTIFICATION_RENDER_VIEW_HOST_CHANGED has always been fired as a
superset of NOTIFICATION_WEB_CONTENTS_SWAPPED.

NOTIFICATION_WEB_CONTENTS_SWAPPED was originally meant as a one-off notification
to refresh the Task Manager, but I think people re-used it because it was higher
up in the file than NOTIFICATION_RENDER_VIEW_HOST_CHANGED.

[joth, 2013-09-06 21:33:25]

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:295:
Java_ContentViewCore_onWebContentsSwapped(env, obj.obj());
onWebContentsSwapped would never be called after this patch. So needs a java
side cleanup too?

+tedchoc maybe more familiar with this flow.

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:222:
Source<WebContents>(web_contents_));
you're registering this twice now

[Avi (use Gerrit), 2013-09-06 21:39:06]

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (left):

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:295:
Java_ContentViewCore_onWebContentsSwapped(env, obj.obj());
On 2013/09/06 21:33:25, joth wrote:
> onWebContentsSwapped would never be called after this patch. So needs a java
> side cleanup too?

Note that onWebContentsSwapped was never called without being preceded by
onRenderProcessSwap. I have no idea what onWebContentsSwapped does. That's why I
need your help.

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23618036/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:222:
Source<WebContents>(web_contents_));
On 2013/09/06 21:33:25, joth wrote:
> you're registering this twice now

Yep; fixed.

[Charlie Reis, 2013-09-06 22:22:40]

Great!  I'd always wondered why we had both.  LGTM.

Note: I probably would have picked RENDER_VIEW_HOST_CHANGED as the name to keep,
since it's a bit more accurate/descriptive than WEB_CONTENTS_SWAPPED.  (We're
not actually swapping to a different WebContents, which does happen in cases
like prerendering.)  That said, if this is just a step toward removing both of
them, it may not be worth the effort to change it.

https://codereview.chromium.org/23618036/diff/14001/content/public/browser/no...
File content/public/browser/notification_types.h (right):

https://codereview.chromium.org/23618036/diff/14001/content/public/browser/no...
content/public/browser/notification_types.h:121: // std::pair::<old
RenderViewHost, new RenderViewHost>.
Should we keep the sentence about WEB_CONTENTS_DISCONNECTED?

[joth, 2013-09-06 22:28:26]

On 6 September 2013 14:39, <avi@chromium.org> wrote:

>
> https://codereview.chromium.**org/23618036/diff/1/content/**
>
browser/android/content_view_**core_impl.cc<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc>
> File content/browser/android/**content_view_core_impl.cc (left):
>
> https://codereview.chromium.**org/23618036/diff/1/content/**
>
browser/android/content_view_**core_impl.cc#oldcode295<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc#oldcode295>
> content/browser/android/**content_view_core_impl.cc:295:
> Java_ContentViewCore_**onWebContentsSwapped(env, obj.obj());
> On 2013/09/06 21:33:25, joth wrote:
>
>> onWebContentsSwapped would never be called after this patch. So needs
>>
> a java
>
>> side cleanup too?
>>
>
> Note that onWebContentsSwapped was never called without being preceded
> by onRenderProcessSwap. I have no idea what onWebContentsSwapped does.
> That's why I need your help.
>
>
Looks like you can splice content of onWebContentsSwapped() into the end of
onRenderProcessSwap()

+boliu,yusuf who  wrote & reviewed onWebContentsSwapped(), as an FYI (
https://chromiumcodereview.appspot.com/12218029)


>
> https://codereview.chromium.**org/23618036/diff/1/content/**
>
browser/android/content_view_**core_impl.cc<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc>
> File content/browser/android/**content_view_core_impl.cc (right):
>
> https://codereview.chromium.**org/23618036/diff/1/content/**
>
browser/android/content_view_**core_impl.cc#newcode222<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc#newcode222>
> content/browser/android/**content_view_core_impl.cc:222:
> Source<WebContents>(web_**contents_));
> On 2013/09/06 21:33:25, joth wrote:
>
>> you're registering this twice now
>>
>
> Yep; fixed.
>
>
https://codereview.chromium.**org/23618036/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ted C, 2013-09-06 22:36:27]

On 2013/09/06 22:28:26, joth wrote:
> On 6 September 2013 14:39, <mailto:avi@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/23618036/diff/1/content/**
> >
>
browser/android/content_view_**core_impl.cc<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc>
> > File content/browser/android/**content_view_core_impl.cc (left):
> >
> > https://codereview.chromium.**org/23618036/diff/1/content/**
> >
>
browser/android/content_view_**core_impl.cc#oldcode295<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc#oldcode295>
> > content/browser/android/**content_view_core_impl.cc:295:
> > Java_ContentViewCore_**onWebContentsSwapped(env, obj.obj());
> > On 2013/09/06 21:33:25, joth wrote:
> >
> >> onWebContentsSwapped would never be called after this patch. So needs
> >>
> > a java
> >
> >> side cleanup too?
> >>
> >
> > Note that onWebContentsSwapped was never called without being preceded
> > by onRenderProcessSwap. I have no idea what onWebContentsSwapped does.
> > That's why I need your help.
> >
> >
> Looks like you can splice content of onWebContentsSwapped() into the end of
> onRenderProcessSwap()
> 
> +boliu,yusuf who  wrote & reviewed onWebContentsSwapped(), as an FYI (
> https://chromiumcodereview.appspot.com/12218029)

I think it was actually this change (adding aurimas as he knows the most about
IME these days):
https://chromiumcodereview.appspot.com/14129003

> 
> 
> >
> > https://codereview.chromium.**org/23618036/diff/1/content/**
> >
>
browser/android/content_view_**core_impl.cc<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc>
> > File content/browser/android/**content_view_core_impl.cc (right):
> >
> > https://codereview.chromium.**org/23618036/diff/1/content/**
> >
>
browser/android/content_view_**core_impl.cc#newcode222<https://codereview.chromium.org/23618036/diff/1/content/browser/android/content_view_core_impl.cc#newcode222>
> > content/browser/android/**content_view_core_impl.cc:222:
> > Source<WebContents>(web_**contents_));
> > On 2013/09/06 21:33:25, joth wrote:
> >
> >> you're registering this twice now
> >>
> >
> > Yep; fixed.
> >
> >
>
https://codereview.chromium.**org/23618036/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2013-09-07 01:57:32]

On 2013/09/06 22:22:40, creis wrote:
> Note: I probably would have picked RENDER_VIEW_HOST_CHANGED as the name to
keep,
> since it's a bit more accurate/descriptive than WEB_CONTENTS_SWAPPED.

Well, the notifications are all going away anyway. Do you like the new API,
WebContentsObserver::RenderViewHostSwapped()?

> content/public/browser/notification_types.h:121: // std::pair::<old
> RenderViewHost, new RenderViewHost>.
> Should we keep the sentence about WEB_CONTENTS_DISCONNECTED?

I don't know what that means. Can you give me more context as to what
WEB_CONTENTS_DISCONNECTED would mean in relation to RENDER_VIEW_HOST_CHANGED?

[Avi (use Gerrit), 2013-09-07 02:05:35]

FYI everyone:

I am not yet convinced that this completely works yet, but it seems like
everyone likes this direction so I'll keep at it.

[Avi (use Gerrit), 2013-09-09 16:25:39]

OK, let's get some reviews.

jam@: content/ except android and browser plugin
Ted, joth, aurimas: There seems to be a debate who is best to handle Android;
pick the best person
creis@: How does this affect your bug crbug.com/123007 ?
asargent@: apps/, chrome/browser/extensions/
hans@: chrome/browser/speech/
dimich@: chrome/browser/ui/panels/
fsamuel@: content/browser/browser_plugin/
sky@: chrome/browser/task_manager/ and ui/views/

[Fady Samuel, 2013-09-09 16:27:40]

browser_plugin LGTM

[hans, 2013-09-09 16:42:04]

chrome/browser/speech/ LGTM

I noticed that the code review subject line and the first line of the summary
seems reversed though.

[Avi (use Gerrit), 2013-09-09 16:44:38]

On 2013/09/09 16:42:04, hans wrote:
> chrome/browser/speech/ LGTM
> 
> I noticed that the code review subject line and the first line of the summary
> seems reversed though.

If you don't mind the gmail threading breaking I'll change it :)

[Charlie Reis, 2013-09-09 16:54:06]

On 2013/09/07 01:57:32, Avi wrote:
> On 2013/09/06 22:22:40, creis wrote:
> > Note: I probably would have picked RENDER_VIEW_HOST_CHANGED as the name to
> keep,
> > since it's a bit more accurate/descriptive than WEB_CONTENTS_SWAPPED.
> 
> Well, the notifications are all going away anyway. Do you like the new API,
> WebContentsObserver::RenderViewHostSwapped()?

Looks like it's now WebContentsObserver::RenderViewHostChanged.  Either one is
fine by me, since they correctly refer to the RVH changing rather than the
WebContents.

> > content/public/browser/notification_types.h:121: // std::pair::<old
> > RenderViewHost, new RenderViewHost>.
> > Should we keep the sentence about WEB_CONTENTS_DISCONNECTED?
> 
> I don't know what that means. Can you give me more context as to what
> WEB_CONTENTS_DISCONNECTED would mean in relation to RENDER_VIEW_HOST_CHANGED?

My hypothesis is that it made it safe to hold a reference to the WebContents
from the source pointer, since the caller was guaranteed to be notified before
the WebContents was deleted.  But you can disregard my question for two reasons:
there's a big DEPRECATED notice on WEB_CONTENTS_DISCONNECTED, and all these
notifications are going away anyway.

So, still LGTM.

[asargent_no_longer_on_chrome, 2013-09-09 17:12:14]

apps/, chrome/browser/extensions/ LGTM

+cc mpcomplete as FYI, since he's a little more familiar with some of this code
than I am.

[sky, 2013-09-09 19:20:38]

LGTM

[Ted C, 2013-09-09 23:51:00]

lgtm for android

[Dmitry Titov, 2013-09-11 17:13:54]

lgtm for panels

[commit-bot: I haz the power, 2013-09-12 15:12:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23618036/27001

[commit-bot: I haz the power, 2013-09-12 15:54:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Avi (use Gerrit), 2013-09-25 16:04:55]

Matt:

I make the NativeAppWindow change as you suggested; please take a look.

[Matt Perry, 2013-09-25 18:51:18]

Much better! LGTM

[commit-bot: I haz the power, 2013-09-30 18:30:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23618036/50001

[commit-bot: I haz the power, 2013-10-01 01:10:03]

Retried try job too often on mac_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-10-02 16:42:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23618036/99001

[commit-bot: I haz the power, 2013-10-02 17:40:17]

Retried try job too often on linux_aura for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-10-02 22:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/23618036/131001

[commit-bot: I haz the power, 2013-10-03 00:57:33]

Change committed as 226637