athena: Handle WebContents created for new windows and create activities for these.

Handle WebContents created for new windows and create activities for these.

BUG=391473
TEST=Follow a link from Gmail or hold shift when clicking a link. A new activity following the link is created.

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

[flackr, 2014-07-15 21:41:05]

Hey, this handles following links in a new "window/tab" by starting a new
activity for it, whether created by shift/ctrl+click or target="_blank" (i.e.
gmail links). Please take a look and let me know what you think. Thanks! Please
forgive if it's a little rough, it's my first CL in athena/.

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:135: web_contents->SetDelegate(this);
FYI: The change in
https://codereview.chromium.org/332443005/diff/160001/athena/content/web_acti...
made it so that the AthenaWebView was no longer the delegate of the WebContents
as the WebView WebContents creation was responsible for assigning this. It also
breaks the WebContentsFocused and EmbedsFullscreenWidget methods which assume
that the delegate owns the webcontents so I've overridden these here.

[oshima, 2014-07-16 16:51:08]

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:135: web_contents->SetDelegate(this);
On 2014/07/15 21:41:05, flackr wrote:
> FYI: The change in
>
https://codereview.chromium.org/332443005/diff/160001/athena/content/web_acti...
> made it so that the AthenaWebView was no longer the delegate of the
WebContents
> as the WebView WebContents creation was responsible for assigning this. It
also
> breaks the WebContentsFocused and EmbedsFullscreenWidget methods which assume
> that the delegate owns the webcontents so I've overridden these here.

Thanks for the fix. It was actually https://codereview.chromium.org/358003002,
but I missed that in review.

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:205: case NEW_FOREGROUND_TAB:
don't you have to handle NEW_BACKGROUND_TAB as well?

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:235: // WebContents.
Can't we just call OnWebContentsFocused()?

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:257: // WebContents.
Do you know why webview is checking wc_owner there?

[flackr, 2014-07-18 18:15:08]

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:135: web_contents->SetDelegate(this);
On 2014/07/16 16:51:08, oshima (OOO) until July 21) wrote:
> On 2014/07/15 21:41:05, flackr wrote:
> > FYI: The change in
> >
>
https://codereview.chromium.org/332443005/diff/160001/athena/content/web_acti...
> > made it so that the AthenaWebView was no longer the delegate of the
> WebContents
> > as the WebView WebContents creation was responsible for assigning this. It
> also
> > breaks the WebContentsFocused and EmbedsFullscreenWidget methods which
assume
> > that the delegate owns the webcontents so I've overridden these here.
> 
> Thanks for the fix. It was actually https://codereview.chromium.org/358003002,
> but I missed that in review.

Oops, pasted the wrong link.

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:205: case NEW_FOREGROUND_TAB:
On 2014/07/16 16:51:08, oshima (OOO) until July 21) wrote:
> don't you have to handle NEW_BACKGROUND_TAB as well?

Done.

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:235: // WebContents.
On 2014/07/16 16:51:08, oshima (OOO) until July 21) wrote:
> Can't we just call OnWebContentsFocused()?

Done. Though I'm still not sure why we require that the webview is the owner in
the base class, perhaps just to ensure that it didn't accidentally become the
delegate of a WebContents it doesn't own?

https://codereview.chromium.org/398603002/diff/40001/athena/content/web_activ...
athena/content/web_activity.cc:257: // WebContents.
On 2014/07/16 16:51:08, oshima (OOO) until July 21) wrote:
> Do you know why webview is checking wc_owner there?

Looking at the original CL it was always checked there. It was likely copied
from WebContentsFocused (i.e. not requested in original review
https://chromiumcodereview.appspot.com/23477051).

I've removed the DCHECK and cc'd miu.

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
ui/views/controls/webview/webview.cc:230: DCHECK(wc_owner_.get());
Do we need this check? We are violating this in athena because the subclass
AthenaWebView owns the WebContents, but it still makes perfect sense to focus
this view.

Alternately, would it make sense to have a virtual bool
OwnsWebContents(WebContents*) method which AthenaWebView could override to say
that it does indeed own the WebContents?

[flackr, 2014-07-18 18:16:02]

+ben for web_view.cc comment.

CC +miu regarding removed DCHECK in EmbedsFullscreenWidget.

[miu, 2014-07-22 00:18:23]

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
ui/views/controls/webview/webview.cc:230: DCHECK(wc_owner_.get());
On 2014/07/18 18:15:08, flackr wrote:
> Do we need this check? We are violating this in athena because the subclass
> AthenaWebView owns the WebContents, but it still makes perfect sense to focus
> this view.
> 
> Alternately, would it make sense to have a virtual bool
> OwnsWebContents(WebContents*) method which AthenaWebView could override to say
> that it does indeed own the WebContents?

The assumption is that WebView is only used as a WebContentsDelegate if it owns
the WebContents.  If AthenaWebView is supposed to work the same way, why not let
the base class (WebView) own the WebContents?

[flackr, 2014-07-22 00:27:37]

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
ui/views/controls/webview/webview.cc:230: DCHECK(wc_owner_.get());
On 2014/07/22 00:18:22, miu wrote:
> On 2014/07/18 18:15:08, flackr wrote:
> > Do we need this check? We are violating this in athena because the subclass
> > AthenaWebView owns the WebContents, but it still makes perfect sense to
focus
> > this view.
> > 
> > Alternately, would it make sense to have a virtual bool
> > OwnsWebContents(WebContents*) method which AthenaWebView could override to
say
> > that it does indeed own the WebContents?
> 
> The assumption is that WebView is only used as a WebContentsDelegate if it
owns
> the WebContents.  If AthenaWebView is supposed to work the same way, why not
let
> the base class (WebView) own the WebContents?

I'm guessing at the intentions of Stefan's CL, but we want to swap out the
WebContents to be able to suspend activities for memory management purposes.
That being said, perhaps I should just add a method which lets you swap out the
WebContents from a subclass so these checks which should pass still do.

[miu, 2014-07-22 01:11:29]

On 2014/07/22 00:27:37, flackr wrote:
>
https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
> File ui/views/controls/webview/webview.cc (right):
> 
>
https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
> ui/views/controls/webview/webview.cc:230: DCHECK(wc_owner_.get());
> On 2014/07/22 00:18:22, miu wrote:
> > On 2014/07/18 18:15:08, flackr wrote:
> > > Do we need this check? We are violating this in athena because the
subclass
> > > AthenaWebView owns the WebContents, but it still makes perfect sense to
> focus
> > > this view.
> > > 
> > > Alternately, would it make sense to have a virtual bool
> > > OwnsWebContents(WebContents*) method which AthenaWebView could override to
> say
> > > that it does indeed own the WebContents?
> > 
> > The assumption is that WebView is only used as a WebContentsDelegate if it
> owns
> > the WebContents.  If AthenaWebView is supposed to work the same way, why not
> let
> > the base class (WebView) own the WebContents?
> 
> I'm guessing at the intentions of Stefan's CL, but we want to swap out the
> WebContents to be able to suspend activities for memory management purposes.
> That being said, perhaps I should just add a method which lets you swap out
the
> WebContents from a subclass so these checks which should pass still do.

sgtm

[flackr, 2014-07-22 13:41:40]

On 2014/07/22 01:11:29, miu wrote:
> On 2014/07/22 00:27:37, flackr wrote:
> >
>
https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
> > File ui/views/controls/webview/webview.cc (right):
> > 
> >
>
https://codereview.chromium.org/398603002/diff/60001/ui/views/controls/webvie...
> > ui/views/controls/webview/webview.cc:230: DCHECK(wc_owner_.get());
> > On 2014/07/22 00:18:22, miu wrote:
> > > On 2014/07/18 18:15:08, flackr wrote:
> > > > Do we need this check? We are violating this in athena because the
> subclass
> > > > AthenaWebView owns the WebContents, but it still makes perfect sense to
> > focus
> > > > this view.
> > > > 
> > > > Alternately, would it make sense to have a virtual bool
> > > > OwnsWebContents(WebContents*) method which AthenaWebView could override
to
> > say
> > > > that it does indeed own the WebContents?
> > > 
> > > The assumption is that WebView is only used as a WebContentsDelegate if it
> > owns
> > > the WebContents.  If AthenaWebView is supposed to work the same way, why
not
> > let
> > > the base class (WebView) own the WebContents?
> > 
> > I'm guessing at the intentions of Stefan's CL, but we want to swap out the
> > WebContents to be able to suspend activities for memory management purposes.
> > That being said, perhaps I should just add a method which lets you swap out
> the
> > WebContents from a subclass so these checks which should pass still do.
> 
> sgtm

Done, PTAL.

[oshima, 2014-07-22 17:58:54]

nice! athena/ lgtm

[miu, 2014-07-22 19:27:53]

WebView changes lgtm, but please address this:

https://codereview.chromium.org/398603002/diff/80001/ui/views/controls/webvie...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/398603002/diff/80001/ui/views/controls/webvie...
ui/views/controls/webview/webview.cc:131: wc_owner_->SetDelegate(this);
This should be guarded by: if (wc_owner_)

[flackr, 2014-07-22 20:01:33]

https://codereview.chromium.org/398603002/diff/80001/ui/views/controls/webvie...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/398603002/diff/80001/ui/views/controls/webvie...
ui/views/controls/webview/webview.cc:131: wc_owner_->SetDelegate(this);
On 2014/07/22 19:27:53, miu wrote:
> This should be guarded by: if (wc_owner_)

Done.

[Ben Goodger (Google), 2014-07-23 16:06:58]

webview lgtm

[flackr, 2014-07-23 16:32:19]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-07-23 16:33:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/flackr@chromium.org/398603002/100001

[commit-bot: I haz the power, 2014-07-23 22:00:21]

Change committed as 285049