Preserve Page::openedByDOM state across process swaps.

Preserve Page::openedByDOM state across process swaps.

Every Page in Blink has an openedByDOM bit which partially determines whether
window.close() may be called. This bit is equivalent to whether the WebContents
was created with an opener. Move this bit up to WebContents and forward it to
new renderers to be preserved across process swaps. Add a suite of tests to
ensure it's preserved correctly.

This also fixes a bug where the bit was not preserved for blocked-then-opened
popups. Add a test for this case.

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

[davidben, 2014-04-04 19:37:41]

creis: Sending this to you for the main review. This depends on
https://codereview.chromium.org/223603005/ and
https://codereview.chromium.org/224173007/ is a followup. I'll get reviewers for
those and add the pile of OWNERS I need afterwards if you think this is
reasonable.

This does add a bit more to RenderViewImpl. I'm not sure what this looks like in
the RenderFrameImpl world since it involves per-Page state. I guess that's still
pending? On the plus side, I added tests, so hopefully those'll be useful in
managing page state.

The main part of this CL is that WebContentsImpl gains a created_with_opener_
bit. It's whether the opener_ was non-NULL at creating time. It can't just be
opener_ because opener_ gets NULL'd if the opener is closed in the meantime, but
the popup is still a popup. That bit gets passed down into
RenderViewHostImpl::CreateRenderView and mirrored into the Page in
RenderViewImpl. The rest is plumbing because some interface gains a parameter.

The prerender bit I can probably split out into a follow-up if that's
preferable. It's small either way.


Why openedByDOM can probably be the same as created_with_opener_:

setOpenedByDOM is called in Blink in Source/core/page/CreateWindow.cpp. The
LocalFrames there come from calls to ChromeClient::createWindow() which
eventually makes its way back up to WebContentsImpl::CreateNewWindow. Working
backwards, that is the only codepath which calls
WebContentsImpl::CreateNewWindow[1]. So we currently set openedByDOM iff that
chain would have returned a route id.

Jumping to RenderWidgetHelper::CreateNewWindow, the renderer gets a route id
unless params.opener_suppressed or no_javascript_access. For the former,
WebContentsImpl::CreateNewWindow passes NULL for the opener, so that matches.
For the latter, we do actually change behavior:

no_javascript_access comes from ContentBrowserClient::CanCreateWindow. Chrome
sets it to true if we are opening a background window from an extension that
doesn't allow JS access? This is the case I'm not sure about. I believe it would
be a behavior change: this CL would allow such a background window to call
window.close() on itself. That doesn't seem terribly interesting although I'm
kind of puzzled it gets an opener. Should we make no_javascript_access imply
opener_suppressed?  Looks like this comes from
https://codereview.chromium.org/9837074

The last potential source of changes (apart from preserving it across process
swaps) is other codepaths that create a WebContents with an opener. If you trust
codesearch's links...

1. The only callers of WebContentsImpl's ctor with a non-NULL opener are
CreateNewWindow (described above) and WebContentsImpl::CreateWithOpener.

2. WebContentsImpl::CreateWithOpener is only called from WebContents::Create
with params.opener.

3. params.opener is only set to non-NULL in CreateTargetContents in
chrome/browser/ui/browser_navigator.cc, if should_set_opener is true on
chrome::NavigateParams.

4. That, in turn, is only set in BlockedWindowParams::CreateNavigateParams in
chrome/browser/ui/blocked_content/blocked_window_params.cc.

I believe this is blocked popups and we actually want this behavior. So this
actually fixes another bug. Open a tab, bring up a JS console and
window.open("http://example.com"). Release the popup if you hit the popup
blocker. Attach a JS console, location.href = "/blahblah" a couple times[2], and
then window.close(). Right now, the result is different depending on whether you
have the popup blocker on or off.

[1] CreateNewWindow is called via RenderViewHostDelgate, called by
RenderViewHostImpl::CreateWindow. That's called through RenderWidgetHelper in
handling ViewHostMsg_CreateWindow in RenderMessageFilter.

That message is only ever send in RenderViewImpl::createView.
WebViewClient::createView is only ever called by ChromeClientImpl::createWindow.
ChromeClient::createWindow is only called by the createWindow functions in
Source/core/page/CreateWindow.cpp. Each of those always sets openedByDOM if they
get a LocalFrame back.

[2] window.close() is allowed if openedByDOM or backForwardListCount <= 1. So
add a few navigations force the latter false.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:1589: view->opener_suppressed_ =
params.opener_suppressed;
Slightly puzzled by this line actually. Judging by
RenderWidgetHelper::CreateNewWindow, opener_suppressed implies we don't return a
routing id and early return, so this is always false.

[Charlie Reis, 2014-04-05 01:41:24]

Nasko: one question for you below about where we think page-level state will end
up.

Thanks for taking this on!  It's too bad we don't have the cache of page-level
state in content/ yet, since this would be useful to put there rather than
adding to RenderView.  No big deal.

Quite the writeup below; I'll try to work through it.

On 2014/04/04 19:37:41, David Benjamin wrote:
> creis: Sending this to you for the main review. This depends on
> https://codereview.chromium.org/223603005/ and
> https://codereview.chromium.org/224173007/ is a followup. I'll get reviewers
for
> those and add the pile of OWNERS I need afterwards if you think this is
> reasonable.
> 
> This does add a bit more to RenderViewImpl. I'm not sure what this looks like
in
> the RenderFrameImpl world since it involves per-Page state. I guess that's
still
> pending? On the plus side, I added tests, so hopefully those'll be useful in
> managing page state.

Yeah, we'll end up with page-level state in content somewhere, and this will be
part of it.

Thanks for the tests, of course.

> 
> The main part of this CL is that WebContentsImpl gains a created_with_opener_
> bit. It's whether the opener_ was non-NULL at creating time. It can't just be
> opener_ because opener_ gets NULL'd if the opener is closed in the meantime,
but
> the popup is still a popup. That bit gets passed down into
> RenderViewHostImpl::CreateRenderView and mirrored into the Page in
> RenderViewImpl. The rest is plumbing because some interface gains a parameter.

Agreed that the bit is necessary and should live on WebContents.

> 
> The prerender bit I can probably split out into a follow-up if that's
> preferable. It's small either way.


Let's move to another CL, as I mention below.

> 
> 
> Why openedByDOM can probably be the same as created_with_opener_:
> 
> setOpenedByDOM is called in Blink in Source/core/page/CreateWindow.cpp. The
> LocalFrames there come from calls to ChromeClient::createWindow() which
> eventually makes its way back up to WebContentsImpl::CreateNewWindow. Working
> backwards, that is the only codepath which calls
> WebContentsImpl::CreateNewWindow[1]. So we currently set openedByDOM iff that
> chain would have returned a route id.
> 
> Jumping to RenderWidgetHelper::CreateNewWindow, the renderer gets a route id
> unless params.opener_suppressed or no_javascript_access. For the former,
> WebContentsImpl::CreateNewWindow passes NULL for the opener, so that matches.
> For the latter, we do actually change behavior:
> 
> no_javascript_access comes from ContentBrowserClient::CanCreateWindow. Chrome
> sets it to true if we are opening a background window from an extension that
> doesn't allow JS access? This is the case I'm not sure about. I believe it
would
> be a behavior change: this CL would allow such a background window to call
> window.close() on itself. That doesn't seem terribly interesting although I'm
> kind of puzzled it gets an opener. Should we make no_javascript_access imply
> opener_suppressed?  Looks like this comes from
> https://codereview.chromium.org/9837074
> 
> The last potential source of changes (apart from preserving it across process
> swaps) is other codepaths that create a WebContents with an opener. If you
trust
> codesearch's links...
> 
> 1. The only callers of WebContentsImpl's ctor with a non-NULL opener are
> CreateNewWindow (described above) and WebContentsImpl::CreateWithOpener.
> 
> 2. WebContentsImpl::CreateWithOpener is only called from WebContents::Create
> with params.opener.
> 
> 3. params.opener is only set to non-NULL in CreateTargetContents in
> chrome/browser/ui/browser_navigator.cc, if should_set_opener is true on
> chrome::NavigateParams.
> 
> 4. That, in turn, is only set in BlockedWindowParams::CreateNavigateParams in
> chrome/browser/ui/blocked_content/blocked_window_params.cc.
> 
> I believe this is blocked popups and we actually want this behavior. So this
> actually fixes another bug. Open a tab, bring up a JS console and
> window.open("http://example.com"). Release the popup if you hit the popup
> blocker. Attach a JS console, location.href = "/blahblah" a couple times[2],
and
> then window.close(). Right now, the result is different depending on whether
you
> have the popup blocker on or off.
> 
> [1] CreateNewWindow is called via RenderViewHostDelgate, called by
> RenderViewHostImpl::CreateWindow. That's called through RenderWidgetHelper in
> handling ViewHostMsg_CreateWindow in RenderMessageFilter.
> 
> That message is only ever send in RenderViewImpl::createView.
> WebViewClient::createView is only ever called by
ChromeClientImpl::createWindow.
> ChromeClient::createWindow is only called by the createWindow functions in
> Source/core/page/CreateWindow.cpp. Each of those always sets openedByDOM if
they
> get a LocalFrame back.
> 
> [2] window.close() is allowed if openedByDOM or backForwardListCount <= 1. So
> add a few navigations force the latter false.

Hmm, all of this is tough to process here.  Maybe it's better to post this as a
comment to the bug and get people to weigh in (e.g., japhet@ for Blink stuff,
jochen@ for blocked popups, etc).

https://codereview.chromium.org/212703005/diff/80001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/212703005/diff/80001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:535: if
(web_contents->CreatedWithOpener()) {
Unfortunately, using the opener for this was the wrong thing in the first place.
 I mentioned this to cbentzel in http://crbug.com/118294 but I think it got
forgotten.

Prerendering should never be used in tabs with other views in the same
SiteInstance.  Otherwise there are same-origin windows with a reference to the
current tab that will break when it moves to the prerendered process.  The
opener is not the right way to detect this, since it doesn't apply for the
opener itself.

Let's move that fix to a different CL.

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
File content/browser/opened_by_dom_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:71: // Wait for the popup to be
created and for it to have navigiated.
nit: navigated

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:89: // the back/forward list
containing only one element.
This comment confuses me.  Is this test about preventing the close because it
wasn't opened by DOM, or because it has multiple navigation entries?

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:100: // Tests that window.close()
works in a popup window that has navigated a few
Wait, the comment above says that window.close is only allowed if the
back/forward list only has one element.  This contradicts that.

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:252: bool
created_with_opener);
Please document this parameter above.

I agree that this adds some work for us in removing RenderView, but we haven't
starting collecting the cache of page-level state yet in the renderer process. 
(Nasko, were we talking about putting that cached data on the top-level
RenderFrame?  I think some separate PageContext class might be cleaner.)

Anyway, we can add it here for now.

https://codereview.chromium.org/212703005/diff/80001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/212703005/diff/80001/content/content_tests.gy...
content/content_tests.gypi:1034: 'browser/opened_by_dom_browsertest.cc',
This should probably be in browser/web_contents/, since it's testing state on
WebContentsImpl.

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

https://codereview.chromium.org/212703005/diff/80001/content/public/browser/w...
content/public/browser/web_contents.h:461: // whether window.close() is
allowed.)
I'm having trouble reading this.  s/calls/false/?

I think both this and HasOpener could probably be rephrased to be clearer:

// Returns whether this WebContents currently has an opener WebContents.
HasOpener()...

// Returns whether this WebContents had an opener WebContents when it was
created.  This affects whether window.close() is allowed.
CreatedWithOpener() ...

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:796: webview()->setOpenedByDOM();
Let's put a TODO to move this state from Blink into content.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:1589: view->opener_suppressed_ =
params.opener_suppressed;
On 2014/04/04 19:37:41, David Benjamin wrote:
> Slightly puzzled by this line actually. Judging by
> RenderWidgetHelper::CreateNewWindow, opener_suppressed implies we don't return
a
> routing id and early return, so this is always false.

Hmm, good catch but let's investigate that separately.  (Sounds like it could be
buggy, but I want to avoid a rathole here.)

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:184: // responsible for creating this
RenderView.
Worth pointing out that |created_with_opener| can be true when |opener_id| is
MSG_ROUTING_NONE, if the opener has been closed by the time this view is
created.

(That fact does make the name a bit confusing.  I wonder if we want to switch
over to something like window_created_with_opener around this point in the
stack.)

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:196: bool created_with_opener,
This should probably be up near opener_id.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl_params.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl_params.h:37: bool created_with_opener,
Again, might be useful to move near opener_id and rename to something like
window_was_created_with_opener.

[davidben, 2014-04-07 17:57:52]

https://codereview.chromium.org/212703005/diff/80001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/212703005/diff/80001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:535: if
(web_contents->CreatedWithOpener()) {
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Unfortunately, using the opener for this was the wrong thing in the first
place.
>  I mentioned this to cbentzel in http://crbug.com/118294 but I think it got
> forgotten.
> 
> Prerendering should never be used in tabs with other views in the same
> SiteInstance.  Otherwise there are same-origin windows with a reference to the
> current tab that will break when it moves to the prerendered process.  The
> opener is not the right way to detect this, since it doesn't apply for the
> opener itself.
> 
> Let's move that fix to a different CL.

Hrm, yeah, I'd been wondering if the opener check was a little too lax...

Moving prerender bits out. Also unexposing CreatedWithOpener() outside of
content/ for now. If we end up doing the prerender thing per 118294, it might
not be needed anyway.

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
File content/browser/opened_by_dom_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:71: // Wait for the popup to be
created and for it to have navigiated.
On 2014/04/05 01:41:25, Charlie Reis wrote:
> nit: navigated

Done.

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:89: // the back/forward list
containing only one element.
On 2014/04/05 01:41:25, Charlie Reis wrote:
> This comment confuses me.  Is this test about preventing the close because it
> wasn't opened by DOM, or because it has multiple navigation entries?

Sorry, this wasn't clear. The condition is an OR not an AND.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So all of these tests navigate a few times to get the back/forward list longer.
Reworded the comments to be clearer.

https://codereview.chromium.org/212703005/diff/80001/content/browser/opened_b...
content/browser/opened_by_dom_browsertest.cc:100: // Tests that window.close()
works in a popup window that has navigated a few
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Wait, the comment above says that window.close is only allowed if the
> back/forward list only has one element.  This contradicts that.

See reply above.

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:252: bool
created_with_opener);
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Please document this parameter above.
> 
> I agree that this adds some work for us in removing RenderView, but we haven't
> starting collecting the cache of page-level state yet in the renderer process.

> (Nasko, were we talking about putting that cached data on the top-level
> RenderFrame?  I think some separate PageContext class might be cleaner.)
> 
> Anyway, we can add it here for now.

Done. I can also do the PageContext thing if those plans are concrete enough
that there's a place to add it that would be useful.

https://codereview.chromium.org/212703005/diff/80001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/212703005/diff/80001/content/content_tests.gy...
content/content_tests.gypi:1034: 'browser/opened_by_dom_browsertest.cc',
On 2014/04/05 01:41:25, Charlie Reis wrote:
> This should probably be in browser/web_contents/, since it's testing state on
> WebContentsImpl.

Done.

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

https://codereview.chromium.org/212703005/diff/80001/content/public/browser/w...
content/public/browser/web_contents.h:461: // whether window.close() is
allowed.)
On 2014/04/05 01:41:25, Charlie Reis wrote:
> I'm having trouble reading this.  s/calls/false/?
> 
> I think both this and HasOpener could probably be rephrased to be clearer:
> 
> // Returns whether this WebContents currently has an opener WebContents.
> HasOpener()...
> 
> // Returns whether this WebContents had an opener WebContents when it was
> created.  This affects whether window.close() is allowed.
> CreatedWithOpener() ...

Removed the additional method for now since we're splitting prerender out.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:796: webview()->setOpenedByDOM();
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Let's put a TODO to move this state from Blink into content.

Done.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:184: // responsible for creating this
RenderView.
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Worth pointing out that |created_with_opener| can be true when |opener_id| is
> MSG_ROUTING_NONE, if the opener has been closed by the time this view is
> created.
> 
> (That fact does make the name a bit confusing.  I wonder if we want to switch
> over to something like window_created_with_opener around this point in the
> stack.)

Done.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:196: bool created_with_opener,
On 2014/04/05 01:41:25, Charlie Reis wrote:
> This should probably be up near opener_id.

Done.

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl_params.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/renderer/render_...
content/renderer/render_view_impl_params.h:37: bool created_with_opener,
On 2014/04/05 01:41:25, Charlie Reis wrote:
> Again, might be useful to move near opener_id and rename to something like
> window_was_created_with_opener.

Done. (Hrm, window_was_created_with_opener is quite a mouthful.)

[nasko, 2014-04-08 22:43:24]

Answering Charlie's question and a few drive-by comments.

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/212703005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:252: bool
created_with_opener);
On 2014/04/07 17:57:52, David Benjamin wrote:
> On 2014/04/05 01:41:25, Charlie Reis wrote:
> > Please document this parameter above.
> > 
> > I agree that this adds some work for us in removing RenderView, but we
haven't
> > starting collecting the cache of page-level state yet in the renderer
process.
> 
> > (Nasko, were we talking about putting that cached data on the top-level
> > RenderFrame?  I think some separate PageContext class might be cleaner.)
> > 
> > Anyway, we can add it here for now.
> 
> Done. I can also do the PageContext thing if those plans are concrete enough
> that there's a place to add it that would be useful.

I don't think we have clear enough plan for page-level context in the renderer
process. On the browser side, it should be on WebContentsImpl.
It should be fine to add the new param for now, since we aren't close to
removing RenderView and this will be needed even after we remove it.

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/opened_by_dom_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/opened_by_dom_browsertest.cc:64:
CHECK(ExecuteScriptAndExtractInt(web_contents, kCloseWindowScript, &dummy));
nit: EXPECT_TRUE?

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:311:
created_with_opener_(opener != NULL),
nit: Most places I've seen use !!ptr for setting boolean value using a pointer
value.

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:878: // True if this tab was
opened by another tab. Is not unset when opener is
nit: "It is not ..."

https://codereview.chromium.org/212703005/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/100001/content/renderer/render...
content/renderer/render_view_impl.h:185: // has been closed,
|window_was_created_with_opener| while be true while
nit: This sentence doesn't read very easily and I'm not sure what it actually
means.

[davidben, 2014-04-08 23:48:22]

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/opened_by_dom_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/opened_by_dom_browsertest.cc:64:
CHECK(ExecuteScriptAndExtractInt(web_contents, kCloseWindowScript, &dummy));
On 2014/04/08 22:43:25, nasko wrote:
> nit: EXPECT_TRUE?

Done. The other one might be better to keep a CHECK. If we fail to call
window.open, the later calls will hang, so failing early may be better.

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:311:
created_with_opener_(opener != NULL),
On 2014/04/08 22:43:25, nasko wrote:
> nit: Most places I've seen use !!ptr for setting boolean value using a pointer
> value.

Done. (Huh, didn't know that was our style. Okay.)

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/212703005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:878: // True if this tab was
opened by another tab. Is not unset when opener is
On 2014/04/08 22:43:25, nasko wrote:
> nit: "It is not ..."

Done. (Well, s/It/This/. "It" seems a little weird given the previous
sentence-noun-phrase-thing.)

https://codereview.chromium.org/212703005/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/100001/content/renderer/render...
content/renderer/render_view_impl.h:185: // has been closed,
|window_was_created_with_opener| while be true while
On 2014/04/08 22:43:25, nasko wrote:
> nit: This sentence doesn't read very easily and I'm not sure what it actually
> means.

It means I'm very bad at typing. :-) Fixed.

[nasko, 2014-04-08 23:57:19]

Ah, I didn't see this typo on the previous round : (.

https://codereview.chromium.org/212703005/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:186: // |opened_id| will be
MSG_ROUTING_NONE.
nit: opener_id

[davidben, 2014-04-09 16:43:18]

https://codereview.chromium.org/212703005/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/212703005/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:186: // |opened_id| will be
MSG_ROUTING_NONE.
On 2014/04/08 23:57:20, nasko wrote:
> nit: opener_id

Done.

[Charlie Reis, 2014-04-11 22:57:56]

Thanks for the cleanup, and sorry for the delay.  LGTM!

https://codereview.chromium.org/212703005/diff/140001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/212703005/diff/140001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:878: // True if this tab was
opened by another tab. This is not unset when opener
nit: if the opener is closed.

[Charlie Reis, 2014-04-11 23:00:36]

Note: Please have jochen@ weigh in on comment 4 of the bug
(http://crbug.com/357579) before landing, since you mentioned it involves a
blocked popup behavior change.

[jochen (gone - plz use gerrit), 2014-04-14 07:28:06]

commented on the bug. assuming I understood the question correctly lgtm

[davidben, 2014-04-14 21:26:39]

Added a test for the popup blocker case.

[davidben, 2014-04-14 21:29:46]

Oops, missed this one.

https://codereview.chromium.org/212703005/diff/140001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/212703005/diff/140001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:878: // True if this tab was
opened by another tab. This is not unset when opener
On 2014/04/11 22:57:56, Charlie Reis wrote:
> nit: if the opener is closed.

Done.

[davidben, 2014-04-15 21:26:14]

jochen: How does the popup_blocker_browsertest.cc change look?

[jochen (gone - plz use gerrit), 2014-04-16 08:58:30]

lgtm, thanks

https://codereview.chromium.org/212703005/diff/200001/chrome/browser/ui/block...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/200001/chrome/browser/ui/block...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:91: };
nit. DISALLOW_COPY_AND_ASSIGN()

[davidben, 2014-04-16 16:15:09]

https://codereview.chromium.org/212703005/diff/200001/chrome/browser/ui/block...
File chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc (right):

https://codereview.chromium.org/212703005/diff/200001/chrome/browser/ui/block...
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc:91: };
On 2014/04/16 08:58:30, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN()

Done.

[davidben, 2014-04-16 16:17:26]

Oh whoops, still need a view_messages.h OWNER. +jschuh.

[nasko, 2014-04-16 16:20:24]

IPC messages LGTM, since I've already reviewed the CL earlier.

[davidben, 2014-04-16 18:27:56]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-16 18:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/212703005/240001

[commit-bot: I haz the power, 2014-04-16 20:19:54]

Change committed as 264291