Make to call WebContentsImpl::RenderViewCreated() when we create child window.

Make to call WebContentsImpl::RenderViewCreated() when we create child window.
Actually we do not call WebContentsImpl::RenderViewCreated() when we care child window.
It makes many issues such as addJavascriptInterface() is not working well in child window.

BUG=361418
TEST=WebContentsImplBrowserTest.RenderViewCreatedForChildWindow

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

[Jimmy Jo, 2014-03-13 12:40:48]

The CQ bit was checked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-03-13 12:40:49]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-03-19 02:18:16]

Hi Wez
This issue is about JavaBridgeDispatcherHostManager::RenderViewCreated. We don`t
call this function when we create child Window.
PTAL.

[Wez, 2014-03-19 04:29:39]

On 2014/03/19 02:18:16, Jincheol Jo wrote:
> Hi Wez
> This issue is about JavaBridgeDispatcherHostManager::RenderViewCreated. We
don`t
> call this function when we create child Window.
> PTAL.

Sorry, this is not code that I'm familiar with.

[Jimmy Jo, 2014-03-19 04:40:33]

On 2014/03/19 04:29:39, Wez wrote:
> On 2014/03/19 02:18:16, Jincheol Jo wrote:
> > Hi Wez
> > This issue is about JavaBridgeDispatcherHostManager::RenderViewCreated. We
> don`t
> > call this function when we create child Window.
> > PTAL.
> 
> Sorry, this is not code that I'm familiar with.

No promblem.
Wez, it seems that Sky has many incomming issues for review.
Could you recommend another reviewer?

[sky, 2014-03-19 15:41:31]

https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1380:
new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
This should be invoked from RenderViewHostImpl. Is there a reason it isn't
happening here?

[Charlie Reis, 2014-03-19 23:31:59]

https://codereview.chromium.org/196283013/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
AUTHORS:144: Jincheol Jo <jincheol.jo@navercorp.com>
nit: This should be on the next line (between Jin Yang and Jingwei Liu).

https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1380:
new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
On 2014/03/19 15:41:31, sky wrote:
> This should be invoked from RenderViewHostImpl. Is there a reason it isn't
> happening here?

Agreed, this is not the right place for this call.  Please do not land this.

If you're not seeing it called, it sounds like you may be missing the call to
RenderViewHostImpl::CreateRenderView.

[Jimmy Jo, 2014-03-20 05:04:25]

On 2014/03/19 23:31:59, Charlie Reis wrote:
> https://codereview.chromium.org/196283013/diff/1/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
> AUTHORS:144: Jincheol Jo <mailto:jincheol.jo@navercorp.com>
> nit: This should be on the next line (between Jin Yang and Jingwei Liu).
> 
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:1380:
> new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
> On 2014/03/19 15:41:31, sky wrote:
> > This should be invoked from RenderViewHostImpl. Is there a reason it isn't
> > happening here?
> 
> Agreed, this is not the right place for this call.  Please do not land this.
> 
> If you're not seeing it called, it sounds like you may be missing the call to
> RenderViewHostImpl::CreateRenderView.

I totally agreed.
Generally WebContentsImpl::RenderViewCreated is called by delegate_ in
RenderViewHostImpl::CreateRenderView.

But When we create ChildWindow on WebKit, RenderViewHostImpl::CreateRenderView
is not called.

for example, when we click foo
<a href="foo.html" target="_blank"> foo </a>

CallStack is as following

-------- Render Process ----------     
HTMLAnchorElement::handleClick
FrameLoader::load
createWindowForRequest
createWindow
ChromeClientImpl::createWindow
RenderViewImpl::createView      <----------- we create new RenderViewImpl of
ChildWindow

-------- IPC (ViewHostMsg_CreateWindow)

RenderMessageFilter::OnCreateWindow
RenderWidgetHelper::CreateNewWindow
RenderWidgetHelper::OnCreateWindowOnUI
content::RenderViewHostImpl::CreateNewWindow 
content::WebContentsImpl::CreateNewWindow  <---- we create new WebContentImpl of
ChildWindow but it do not call RenderViewCreated.
content::WebContentsImpl::Init

________ Browser Process __________


so I make to WebContentImpl::RenderViewCreated of childWindow

[Jimmy Jo, 2014-03-26 02:47:37]

On 2014/03/20 05:04:25, Jincheol Jo wrote:
> On 2014/03/19 23:31:59, Charlie Reis wrote:
> > https://codereview.chromium.org/196283013/diff/1/AUTHORS
> > File AUTHORS (right):
> > 
> > https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
> > AUTHORS:144: Jincheol Jo <mailto:jincheol.jo@navercorp.com>
> > nit: This should be on the next line (between Jin Yang and Jingwei Liu).
> > 
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> > content/browser/web_contents/web_contents_impl.cc:1380:
> > new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
> > On 2014/03/19 15:41:31, sky wrote:
> > > This should be invoked from RenderViewHostImpl. Is there a reason it isn't
> > > happening here?
> > 
> > Agreed, this is not the right place for this call.  Please do not land this.
> > 
> > If you're not seeing it called, it sounds like you may be missing the call
to
> > RenderViewHostImpl::CreateRenderView.
> 
> I totally agreed.
> Generally WebContentsImpl::RenderViewCreated is called by delegate_ in
> RenderViewHostImpl::CreateRenderView.
> 
> But When we create ChildWindow on WebKit, RenderViewHostImpl::CreateRenderView
> is not called.
> 
> for example, when we click foo
> <a href="foo.html" target="_blank"> foo </a>
> 
> CallStack is as following
> 
> -------- Render Process ----------     
> HTMLAnchorElement::handleClick
> FrameLoader::load
> createWindowForRequest
> createWindow
> ChromeClientImpl::createWindow
> RenderViewImpl::createView      <----------- we create new RenderViewImpl of
> ChildWindow
> 
> -------- IPC (ViewHostMsg_CreateWindow)
> 
> RenderMessageFilter::OnCreateWindow
> RenderWidgetHelper::CreateNewWindow
> RenderWidgetHelper::OnCreateWindowOnUI
> content::RenderViewHostImpl::CreateNewWindow 
> content::WebContentsImpl::CreateNewWindow  <---- we create new WebContentImpl
of
> ChildWindow but it do not call RenderViewCreated.
> content::WebContentsImpl::Init
> 
> ________ Browser Process __________
> 
> 
> so I make to WebContentImpl::RenderViewCreated of childWindow

PTAL!!

[Charlie Reis, 2014-03-26 16:46:11]

On 2014/03/20 05:04:25, Jincheol Jo wrote:
> On 2014/03/19 23:31:59, Charlie Reis wrote:
> > https://codereview.chromium.org/196283013/diff/1/AUTHORS
> > File AUTHORS (right):
> > 
> > https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
> > AUTHORS:144: Jincheol Jo <mailto:jincheol.jo@navercorp.com>
> > nit: This should be on the next line (between Jin Yang and Jingwei Liu).
> > 
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> > content/browser/web_contents/web_contents_impl.cc:1380:
> > new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
> > On 2014/03/19 15:41:31, sky wrote:
> > > This should be invoked from RenderViewHostImpl. Is there a reason it isn't
> > > happening here?
> > 
> > Agreed, this is not the right place for this call.  Please do not land this.
> > 
> > If you're not seeing it called, it sounds like you may be missing the call
to
> > RenderViewHostImpl::CreateRenderView.
> 
> I totally agreed.
> Generally WebContentsImpl::RenderViewCreated is called by delegate_ in
> RenderViewHostImpl::CreateRenderView.
> 
> But When we create ChildWindow on WebKit, RenderViewHostImpl::CreateRenderView
> is not called.
> 
> for example, when we click foo
> <a href="foo.html" target="_blank"> foo </a>
> 
> CallStack is as following
> 
> -------- Render Process ----------     
> HTMLAnchorElement::handleClick
> FrameLoader::load
> createWindowForRequest
> createWindow
> ChromeClientImpl::createWindow
> RenderViewImpl::createView      <----------- we create new RenderViewImpl of
> ChildWindow
> 
> -------- IPC (ViewHostMsg_CreateWindow)
> 
> RenderMessageFilter::OnCreateWindow
> RenderWidgetHelper::CreateNewWindow
> RenderWidgetHelper::OnCreateWindowOnUI
> content::RenderViewHostImpl::CreateNewWindow 
> content::WebContentsImpl::CreateNewWindow  <---- we create new WebContentImpl
of
> ChildWindow but it do not call RenderViewCreated.
> content::WebContentsImpl::Init
> 
> ________ Browser Process __________
> 
> 
> so I make to WebContentImpl::RenderViewCreated of childWindow

Is there a bug on file for this?  It's better to have a record of the problem
with repro steps like the one you mention than just trying to add a call like
this.  We can discuss the right way to fix it from there.

[sky, 2014-03-26 16:49:15]

Said differently, how about a unit test that fails on tip of tree.

  -Scott

On Wed, Mar 26, 2014 at 9:46 AM,  <creis@chromium.org> wrote:
> On 2014/03/20 05:04:25, Jincheol Jo wrote:
>>
>> On 2014/03/19 23:31:59, Charlie Reis wrote:
>> > https://codereview.chromium.org/196283013/diff/1/AUTHORS
>> > File AUTHORS (right):
>> >
>> > https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
>> > AUTHORS:144: Jincheol Jo <mailto:jincheol.jo@navercorp.com>
>> > nit: This should be on the next line (between Jin Yang and Jingwei Liu).
>> >
>> >
>
>
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
>>
>> > File content/browser/web_contents/web_contents_impl.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
>>
>> > content/browser/web_contents/web_contents_impl.cc:1380:
>> > new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
>> > On 2014/03/19 15:41:31, sky wrote:
>> > > This should be invoked from RenderViewHostImpl. Is there a reason it
>> > > isn't
>> > > happening here?
>> >
>> > Agreed, this is not the right place for this call.  Please do not land
>> > this.
>> >
>> > If you're not seeing it called, it sounds like you may be missing the
>> > call
>
> to
>>
>> > RenderViewHostImpl::CreateRenderView.
>
>
>> I totally agreed.
>> Generally WebContentsImpl::RenderViewCreated is called by delegate_ in
>> RenderViewHostImpl::CreateRenderView.
>
>
>> But When we create ChildWindow on WebKit,
>> RenderViewHostImpl::CreateRenderView
>> is not called.
>
>
>> for example, when we click foo
>> <a href="foo.html" target="_blank"> foo </a>
>
>
>> CallStack is as following
>
>
>> -------- Render Process ----------
>> HTMLAnchorElement::handleClick
>> FrameLoader::load
>> createWindowForRequest
>> createWindow
>> ChromeClientImpl::createWindow
>> RenderViewImpl::createView      <----------- we create new RenderViewImpl
>> of
>> ChildWindow
>
>
>> -------- IPC (ViewHostMsg_CreateWindow)
>
>
>> RenderMessageFilter::OnCreateWindow
>> RenderWidgetHelper::CreateNewWindow
>> RenderWidgetHelper::OnCreateWindowOnUI
>> content::RenderViewHostImpl::CreateNewWindow
>> content::WebContentsImpl::CreateNewWindow  <---- we create new
>> WebContentImpl
>
> of
>>
>> ChildWindow but it do not call RenderViewCreated.
>> content::WebContentsImpl::Init
>
>
>> ________ Browser Process __________
>
>
>
>> so I make to WebContentImpl::RenderViewCreated of childWindow
>
>
> Is there a bug on file for this?  It's better to have a record of the
> problem
> with repro steps like the one you mention than just trying to add a call
> like
> this.  We can discuss the right way to fix it from there.
>
> https://codereview.chromium.org/196283013/

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

[Jimmy Jo, 2014-03-27 00:40:36]

On 2014/03/26 16:49:15, sky wrote:
> Said differently, how about a unit test that fails on tip of tree.
> 
>   -Scott
> 
> On Wed, Mar 26, 2014 at 9:46 AM,  <mailto:creis@chromium.org> wrote:
> > On 2014/03/20 05:04:25, Jincheol Jo wrote:
> >>
> >> On 2014/03/19 23:31:59, Charlie Reis wrote:
> >> > https://codereview.chromium.org/196283013/diff/1/AUTHORS
> >> > File AUTHORS (right):
> >> >
> >> > https://codereview.chromium.org/196283013/diff/1/AUTHORS#newcode144
> >> > AUTHORS:144: Jincheol Jo <mailto:jincheol.jo@navercorp.com>
> >> > nit: This should be on the next line (between Jin Yang and Jingwei Liu).
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> >>
> >> > File content/browser/web_contents/web_contents_impl.cc (right):
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/196283013/diff/1/content/browser/web_contents...
> >>
> >> > content/browser/web_contents/web_contents_impl.cc:1380:
> >> > new_contents->RenderViewCreated(new_contents->GetRenderViewHost());
> >> > On 2014/03/19 15:41:31, sky wrote:
> >> > > This should be invoked from RenderViewHostImpl. Is there a reason it
> >> > > isn't
> >> > > happening here?
> >> >
> >> > Agreed, this is not the right place for this call.  Please do not land
> >> > this.
> >> >
> >> > If you're not seeing it called, it sounds like you may be missing the
> >> > call
> >
> > to
> >>
> >> > RenderViewHostImpl::CreateRenderView.
> >
> >
> >> I totally agreed.
> >> Generally WebContentsImpl::RenderViewCreated is called by delegate_ in
> >> RenderViewHostImpl::CreateRenderView.
> >
> >
> >> But When we create ChildWindow on WebKit,
> >> RenderViewHostImpl::CreateRenderView
> >> is not called.
> >
> >
> >> for example, when we click foo
> >> <a href="foo.html" target="_blank"> foo </a>
> >
> >
> >> CallStack is as following
> >
> >
> >> -------- Render Process ----------
> >> HTMLAnchorElement::handleClick
> >> FrameLoader::load
> >> createWindowForRequest
> >> createWindow
> >> ChromeClientImpl::createWindow
> >> RenderViewImpl::createView      <----------- we create new RenderViewImpl
> >> of
> >> ChildWindow
> >
> >
> >> -------- IPC (ViewHostMsg_CreateWindow)
> >
> >
> >> RenderMessageFilter::OnCreateWindow
> >> RenderWidgetHelper::CreateNewWindow
> >> RenderWidgetHelper::OnCreateWindowOnUI
> >> content::RenderViewHostImpl::CreateNewWindow
> >> content::WebContentsImpl::CreateNewWindow  <---- we create new
> >> WebContentImpl
> >
> > of
> >>
> >> ChildWindow but it do not call RenderViewCreated.
> >> content::WebContentsImpl::Init
> >
> >
> >> ________ Browser Process __________
> >
> >
> >
> >> so I make to WebContentImpl::RenderViewCreated of childWindow
> >
> >
> > Is there a bug on file for this?  It's better to have a record of the
> > problem
> > with repro steps like the one you mention than just trying to add a call
> > like
> > this.  We can discuss the right way to fix it from there.
> >
> > https://codereview.chromium.org/196283013/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

This issue is about that AddJavascriptInterface doesn`t work when we create
child window through WebChromeClient`s onCreateWindow on Android 4.4.2 Kitkat
WebView.
As you know, kitkat webview use chromium. so I wrote this code.

I will make a sample Android App an attach it, if you want it.
Please let me know you want something else I did say.
Thank you for your attention.

[Charlie Reis, 2014-03-27 01:05:17]

On 2014/03/27 00:40:36, Jincheol Jo wrote:
> This issue is about that AddJavascriptInterface doesn`t work when we create
> child window through WebChromeClient`s onCreateWindow on Android 4.4.2 Kitkat
> WebView.
> As you know, kitkat webview use chromium. so I wrote this code.
> 
> I will make a sample Android App an attach it, if you want it.
> Please let me know you want something else I did say.
> Thank you for your attention.

Thanks.  The usual procedure is to file a bug at http://crbug.com with the repro
steps you provided.  The Android app isn't necessary to attach.  You can then
add a "BUG=NNNN" line to this CL description, with the bug's number.

As @sky mentioned, this would be a good thing to have a test for.  You can
search for web_contents_impl_browsertest.cc on http://cs.chromium.org to find
some examples.  For example, there's a test in there with an observer that
listens for RenderViewCreated.  You could write a test that loads a page with a
link that opens in a new window and check that the event was observed.

A lot of this process is summarized on
http://www.chromium.org/developers/contributing-code.  Hope that helps!

[Jimmy Jo, 2014-04-09 01:19:19]

On 2014/03/27 01:05:17, Charlie Reis wrote:
> On 2014/03/27 00:40:36, Jincheol Jo wrote:
> > This issue is about that AddJavascriptInterface doesn`t work when we create
> > child window through WebChromeClient`s onCreateWindow on Android 4.4.2
Kitkat
> > WebView.
> > As you know, kitkat webview use chromium. so I wrote this code.
> > 
> > I will make a sample Android App an attach it, if you want it.
> > Please let me know you want something else I did say.
> > Thank you for your attention.
> 
> Thanks.  The usual procedure is to file a bug at http://crbug.com with the
repro
> steps you provided.  The Android app isn't necessary to attach.  You can then
> add a "BUG=NNNN" line to this CL description, with the bug's number.
> 
> As @sky mentioned, this would be a good thing to have a test for.  You can
> search for web_contents_impl_browsertest.cc on http://cs.chromium.org to find
> some examples.  For example, there's a test in there with an observer that
> listens for RenderViewCreated.  You could write a test that loads a page with
a
> link that opens in a new window and check that the event was observed.
> 
> A lot of this process is summarized on
> http://www.chromium.org/developers/contributing-code.  Hope that helps!

I changed Issue description Charlie Reis`s guide and attached sample app.

[sky, 2014-04-09 04:08:23]

Can't you write a test and not a sample app?

  -Scott

On Tue, Apr 8, 2014 at 6:19 PM,  <jincheol.jo@navercorp.com> wrote:
> On 2014/03/27 01:05:17, Charlie Reis wrote:
>>
>> On 2014/03/27 00:40:36, Jincheol Jo wrote:
>> > This issue is about that AddJavascriptInterface doesn`t work when we
>> > create
>> > child window through WebChromeClient`s onCreateWindow on Android 4.4.2
>
> Kitkat
>>
>> > WebView.
>> > As you know, kitkat webview use chromium. so I wrote this code.
>> >
>> > I will make a sample Android App an attach it, if you want it.
>> > Please let me know you want something else I did say.
>> > Thank you for your attention.
>
>
>> Thanks.  The usual procedure is to file a bug at http://crbug.com with the
>
> repro
>>
>> steps you provided.  The Android app isn't necessary to attach.  You can
>> then
>> add a "BUG=NNNN" line to this CL description, with the bug's number.
>
>
>> As @sky mentioned, this would be a good thing to have a test for.  You can
>> search for web_contents_impl_browsertest.cc on http://cs.chromium.org to
>> find
>> some examples.  For example, there's a test in there with an observer that
>> listens for RenderViewCreated.  You could write a test that loads a page
>> with
>
> a
>>
>> link that opens in a new window and check that the event was observed.
>
>
>> A lot of this process is summarized on
>> http://www.chromium.org/developers/contributing-code.  Hope that helps!
>
>
> I changed Issue description Charlie Reis`s guide and attached sample app.
>
> https://codereview.chromium.org/196283013/

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

[Jimmy Jo, 2014-04-09 04:25:26]

On 2014/04/09 04:08:23, sky wrote:
> Can't you write a test and not a sample app?
> 
>   -Scott
> 
> On Tue, Apr 8, 2014 at 6:19 PM,  <mailto:jincheol.jo@navercorp.com> wrote:
> > On 2014/03/27 01:05:17, Charlie Reis wrote:
> >>
> >> On 2014/03/27 00:40:36, Jincheol Jo wrote:
> >> > This issue is about that AddJavascriptInterface doesn`t work when we
> >> > create
> >> > child window through WebChromeClient`s onCreateWindow on Android 4.4.2
> >
> > Kitkat
> >>
> >> > WebView.
> >> > As you know, kitkat webview use chromium. so I wrote this code.
> >> >
> >> > I will make a sample Android App an attach it, if you want it.
> >> > Please let me know you want something else I did say.
> >> > Thank you for your attention.
> >
> >
> >> Thanks.  The usual procedure is to file a bug at http://crbug.com with the
> >
> > repro
> >>
> >> steps you provided.  The Android app isn't necessary to attach.  You can
> >> then
> >> add a "BUG=NNNN" line to this CL description, with the bug's number.
> >
> >
> >> As @sky mentioned, this would be a good thing to have a test for.  You can
> >> search for web_contents_impl_browsertest.cc on http://cs.chromium.org to
> >> find
> >> some examples.  For example, there's a test in there with an observer that
> >> listens for RenderViewCreated.  You could write a test that loads a page
> >> with
> >
> > a
> >>
> >> link that opens in a new window and check that the event was observed.
> >
> >
> >> A lot of this process is summarized on
> >> http://www.chromium.org/developers/contributing-code.  Hope that helps!
> >
> >
> > I changed Issue description Charlie Reis`s guide and attached sample app.
> >
> > https://codereview.chromium.org/196283013/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

No prolem. but I need the time for analyzing unit test.
I hope you consider that I`m beginner.
I will try to write unit test code.
Thank you for your response.

[Charlie Reis, 2014-04-09 22:33:12]

On 2014/04/09 04:25:26, Jincheol Jo wrote:
> On 2014/04/09 04:08:23, sky wrote:
> > Can't you write a test and not a sample app?
> > 
> >   -Scott
> > 
> No prolem. but I need the time for analyzing unit test.
> I hope you consider that I`m beginner.
> I will try to write unit test code.
> Thank you for your response.

Thanks for your effort so far.  The bug report is helping as a place to discuss
it further.

It seems plausible that this is a bug that affects more than Android.  It might
be possible to write a browsertest that creates a new window (like your link
example) and watches whether the RenderViewCreated observer method is called. 
You can see an example of something similar to this in
web_contents_impl_browsertest.cc with RenderViewSizeObserver and the
GetSizeForNewRenderView test:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

[Jimmy Jo, 2014-04-10 00:36:27]

On 2014/04/09 22:33:12, Charlie Reis wrote:
> On 2014/04/09 04:25:26, Jincheol Jo wrote:
> > On 2014/04/09 04:08:23, sky wrote:
> > > Can't you write a test and not a sample app?
> > > 
> > >   -Scott
> > > 
> > No prolem. but I need the time for analyzing unit test.
> > I hope you consider that I`m beginner.
> > I will try to write unit test code.
> > Thank you for your response.
> 
> Thanks for your effort so far.  The bug report is helping as a place to
discuss
> it further.
> 
> It seems plausible that this is a bug that affects more than Android.  It
might
> be possible to write a browsertest that creates a new window (like your link
> example) and watches whether the RenderViewCreated observer method is called. 
> You can see an example of something similar to this in
> web_contents_impl_browsertest.cc with RenderViewSizeObserver and the
> GetSizeForNewRenderView test:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

Thank you for your kind guidance.
I will try it.

[Jimmy Jo, 2014-04-12 04:46:10]

On 2014/04/10 00:36:27, Jincheol Jo wrote:
> On 2014/04/09 22:33:12, Charlie Reis wrote:
> > On 2014/04/09 04:25:26, Jincheol Jo wrote:
> > > On 2014/04/09 04:08:23, sky wrote:
> > > > Can't you write a test and not a sample app?
> > > > 
> > > >   -Scott
> > > > 
> > > No prolem. but I need the time for analyzing unit test.
> > > I hope you consider that I`m beginner.
> > > I will try to write unit test code.
> > > Thank you for your response.
> > 
> > Thanks for your effort so far.  The bug report is helping as a place to
> discuss
> > it further.
> > 
> > It seems plausible that this is a bug that affects more than Android.  It
> might
> > be possible to write a browsertest that creates a new window (like your link
> > example) and watches whether the RenderViewCreated observer method is
called. 
> > You can see an example of something similar to this in
> > web_contents_impl_browsertest.cc with RenderViewSizeObserver and the
> > GetSizeForNewRenderView test:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
> 
> Thank you for your kind guidance.
> I will try it.
It seems to below link about Blink Contribution. so I try it but not as well.
http://www.chromium.org/blink/conversion-cheatsheet
Is there right?

Can I know processes for Blink Contribution?

[Jimmy Jo, 2014-04-12 04:48:42]

On 2014/04/12 04:46:10, Jincheol Jo wrote:
> On 2014/04/10 00:36:27, Jincheol Jo wrote:
> > On 2014/04/09 22:33:12, Charlie Reis wrote:
> > > On 2014/04/09 04:25:26, Jincheol Jo wrote:
> > > > On 2014/04/09 04:08:23, sky wrote:
> > > > > Can't you write a test and not a sample app?
> > > > > 
> > > > >   -Scott
> > > > > 
> > > > No prolem. but I need the time for analyzing unit test.
> > > > I hope you consider that I`m beginner.
> > > > I will try to write unit test code.
> > > > Thank you for your response.
> > > 
> > > Thanks for your effort so far.  The bug report is helping as a place to
> > discuss
> > > it further.
> > > 
> > > It seems plausible that this is a bug that affects more than Android.  It
> > might
> > > be possible to write a browsertest that creates a new window (like your
link
> > > example) and watches whether the RenderViewCreated observer method is
> called. 
> > > You can see an example of something similar to this in
> > > web_contents_impl_browsertest.cc with RenderViewSizeObserver and the
> > > GetSizeForNewRenderView test:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
> > 
> > Thank you for your kind guidance.
> > I will try it.
> It seems to below link about Blink Contribution. so I try it but not as well.
> http://www.chromium.org/blink/conversion-cheatsheet
> Is there right?
> 
> Can I know processes for Blink Contribution?

Oh I made mistakes. Sorry. I sent the wrong message.

[Jimmy Jo, 2014-04-27 07:07:09]

I proposed test code. PTAL.

[Charlie Reis, 2014-04-29 00:37:57]

Thanks for adding the test!  We can discuss style issues in a bit, but I'd like
to get John's opinion on where the new call to RenderViewCreated belongs now
that we have the context for which case is broken.

John: Do you know whether there's a better way to trigger RenderViewCreated via
CreateNewWindow?

Side note: We are deprecating the notification service, so we'll probably end up
using a different way to monitor for the new window in the test.  I think
there's probably an existing technique we can suggest.

[Jimmy Jo, 2014-04-29 01:06:45]

On 2014/04/29 00:37:57, Charlie Reis wrote:
> Thanks for adding the test!  We can discuss style issues in a bit, but I'd
like
> to get John's opinion on where the new call to RenderViewCreated belongs now
> that we have the context for which case is broken.
> 
> John: Do you know whether there's a better way to trigger RenderViewCreated
via
> CreateNewWindow?
> 
> Side note: We are deprecating the notification service, so we'll probably end
up
> using a different way to monitor for the new window in the test.  I think
> there's probably an existing technique we can suggest.

Thanks for your response. I will find a different way to monitor for the new
window but I think it`s not easy.
Could you give me hint for it please?

[Jimmy Jo, 2014-04-29 02:00:29]

On 2014/04/29 01:06:45, Jincheol Jo wrote:
> On 2014/04/29 00:37:57, Charlie Reis wrote:
> > Thanks for adding the test!  We can discuss style issues in a bit, but I'd
> like
> > to get John's opinion on where the new call to RenderViewCreated belongs now
> > that we have the context for which case is broken.
> > 
> > John: Do you know whether there's a better way to trigger RenderViewCreated
> via
> > CreateNewWindow?
> > 
> > Side note: We are deprecating the notification service, so we'll probably
end
> up
> > using a different way to monitor for the new window in the test.  I think
> > there's probably an existing technique we can suggest.
> 
> Thanks for your response. I will find a different way to monitor for the new
> window but I think it`s not easy.
> Could you give me hint for it please?
In Addition, I have a question.
Is it really deprecated? Notification service is used in many places.
Could I know the code to replace? if it exists.

[jam, 2014-04-29 04:25:32]

On 2014/04/29 02:00:29, Jincheol Jo wrote:
> On 2014/04/29 01:06:45, Jincheol Jo wrote:
> > On 2014/04/29 00:37:57, Charlie Reis wrote:
> > > Thanks for adding the test!  We can discuss style issues in a bit, but I'd
> > like
> > > to get John's opinion on where the new call to RenderViewCreated belongs
now
> > > that we have the context for which case is broken.
> > > 
> > > John: Do you know whether there's a better way to trigger
RenderViewCreated
> > via
> > > CreateNewWindow?
> > > 
> > > Side note: We are deprecating the notification service, so we'll probably
> end
> > up
> > > using a different way to monitor for the new window in the test.  I think
> > > there's probably an existing technique we can suggest.
> > 
> > Thanks for your response. I will find a different way to monitor for the new
> > window but I think it`s not easy.
> > Could you give me hint for it please?
> In Addition, I have a question.
> Is it really deprecated? Notification service is used in many places.
> Could I know the code to replace? if it exists.

thanks for the patch, the non-test code changes are fine. just please don't add
a notification, since as mentioned we are trying to remove all the
notifications.

Perhaps the test can implement a WebContentsObserver and check that
RenderViewCreated is being called?

[Charlie Reis, 2014-04-29 16:55:24]

https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:441:
NewChildWindowNotificationObserver observer(shell()->web_contents());
You can use a ShellAddedObserver here instead, without adding the new
notification.  See content_browser_test_utils, and
render_frame_host_manager_browsertest.cc for examples.

https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_browsertest.cc:447:
RenderViewCreatedObserver::current_->call_render_view_created_ == true);
nit: No need for "== true"

https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
File content/test/data/openChildWindow.html (right):

https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
content/test/data/openChildWindow.html:1: <html>
This file should be use a similar naming scheme to other files in the directory,
like open_child_window.html.

https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
content/test/data/openChildWindow.html:6: a.href='http://www.google.com';
Let's not use a network link here.  Would title1.html work?

[Jimmy Jo, 2014-04-30 00:25:54]

On 2014/04/29 16:55:24, Charlie Reis wrote:
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl_browsertest.cc:441:
> NewChildWindowNotificationObserver observer(shell()->web_contents());
> You can use a ShellAddedObserver here instead, without adding the new
> notification.  See content_browser_test_utils, and
> render_frame_host_manager_browsertest.cc for examples.
> 
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl_browsertest.cc:447:
> RenderViewCreatedObserver::current_->call_render_view_created_ == true);
> nit: No need for "== true"
> 
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> File content/test/data/openChildWindow.html (right):
> 
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> content/test/data/openChildWindow.html:1: <html>
> This file should be use a similar naming scheme to other files in the
directory,
> like open_child_window.html.
> 
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> content/test/data/openChildWindow.html:6: a.href='http://www.google.com';
> Let's not use a network link here.  Would title1.html work?

I got it. Thanks Charlie.

[Jimmy Jo, 2014-05-06 05:41:56]

On 2014/04/30 00:25:54, Jincheol Jo wrote:
> On 2014/04/29 16:55:24, Charlie Reis wrote:
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> > File content/browser/web_contents/web_contents_impl_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> > content/browser/web_contents/web_contents_impl_browsertest.cc:441:
> > NewChildWindowNotificationObserver observer(shell()->web_contents());
> > You can use a ShellAddedObserver here instead, without adding the new
> > notification.  See content_browser_test_utils, and
> > render_frame_host_manager_browsertest.cc for examples.
> > 
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/browser/web_cont...
> > content/browser/web_contents/web_contents_impl_browsertest.cc:447:
> > RenderViewCreatedObserver::current_->call_render_view_created_ == true);
> > nit: No need for "== true"
> > 
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> > File content/test/data/openChildWindow.html (right):
> > 
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> > content/test/data/openChildWindow.html:1: <html>
> > This file should be use a similar naming scheme to other files in the
> directory,
> > like open_child_window.html.
> > 
> >
>
https://codereview.chromium.org/196283013/diff/80001/content/test/data/openCh...
> > content/test/data/openChildWindow.html:6: a.href='http://www.google.com';
> > Let's not use a network link here.  Would title1.html work?
> 
> I got it. Thanks Charlie.

Charlie. ShellAddedObserver is observer for Shell Constructor.
WebContentsImpl`s constructor is called and called RenderViewCreated before
calling Shell Constructor in this case.
Refer to WebContentsImpl::CreateNewWindow and WebContentsImpl:ShowCreatedWindow.
It seems not match.
I need the observer for WebContentsImpl Constructor.
so I find out WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback, but
according to comments, it is deprecated.
Can I use it?
If it disable, it seems need to move RenderViewCreated`s position to Shell
Constructor.
Thanks.

[Charlie Reis, 2014-05-08 01:01:38]

On 2014/05/06 05:41:56, Jincheol Jo wrote:
> Charlie. ShellAddedObserver is observer for Shell Constructor.
> WebContentsImpl`s constructor is called and called RenderViewCreated before
> calling Shell Constructor in this case.
> Refer to WebContentsImpl::CreateNewWindow and
WebContentsImpl:ShowCreatedWindow.
> It seems not match.
> I need the observer for WebContentsImpl Constructor.
> so I find out WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback, but
> according to comments, it is deprecated.
> Can I use it?
> If it disable, it seems need to move RenderViewCreated`s position to Shell
> Constructor.
> Thanks.

@jam: It looks like you're planning to remove those per http://crrev.com/234570.
 How would you recommend adding a WebContentsObserver to a WebContents that is
about to be created (e.g., via a targeted link)?  Here, we want the observer to
hear the RenderViewCreated call right after the WebContents is constructed.

[Jimmy Jo, 2014-05-12 07:38:26]

On 2014/05/08 01:01:38, Charlie Reis wrote:
> On 2014/05/06 05:41:56, Jincheol Jo wrote:
> > Charlie. ShellAddedObserver is observer for Shell Constructor.
> > WebContentsImpl`s constructor is called and called RenderViewCreated before
> > calling Shell Constructor in this case.
> > Refer to WebContentsImpl::CreateNewWindow and
> WebContentsImpl:ShowCreatedWindow.
> > It seems not match.
> > I need the observer for WebContentsImpl Constructor.
> > so I find out WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback, but
> > according to comments, it is deprecated.
> > Can I use it?
> > If it disable, it seems need to move RenderViewCreated`s position to Shell
> > Constructor.
> > Thanks.
> 
> @jam: It looks like you're planning to remove those per
http://crrev.com/234570.
>  How would you recommend adding a WebContentsObserver to a WebContents that is
> about to be created (e.g., via a targeted link)?  Here, we want the observer
to
> hear the RenderViewCreated call right after the WebContents is constructed.

@jam : Please take a look?

[jam, 2014-05-12 15:32:40]

On 2014/05/08 01:01:38, Charlie Reis wrote:
> On 2014/05/06 05:41:56, Jincheol Jo wrote:
> > Charlie. ShellAddedObserver is observer for Shell Constructor.
> > WebContentsImpl`s constructor is called and called RenderViewCreated before
> > calling Shell Constructor in this case.
> > Refer to WebContentsImpl::CreateNewWindow and
> WebContentsImpl:ShowCreatedWindow.
> > It seems not match.
> > I need the observer for WebContentsImpl Constructor.
> > so I find out WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback, but
> > according to comments, it is deprecated.
> > Can I use it?
> > If it disable, it seems need to move RenderViewCreated`s position to Shell
> > Constructor.
> > Thanks.
> 
> @jam: It looks like you're planning to remove those per
http://crrev.com/234570.
>  How would you recommend adding a WebContentsObserver to a WebContents that is
> about to be created (e.g., via a targeted link)?  Here, we want the observer
to
> hear the RenderViewCreated call right after the WebContents is constructed.

ah, sorry I missed that subtlety earlier. WebContents::AddCreatedCallback is
fine to call only from test code, which is why it's private. For background, I
hid it because when it was public it allowed layering violations from code in
say src\apps observing all the WebContents' created in src\chrome. So how about:
1) add a WebContentsAddedObserver in content/public/test/browser_test_utils that
is just like ShellAddedObserver but for WebContents. make that class a friend of
WCImpl so it can access AddCreatedCallback 
2) bonus points if you convert TestNavigationObserver to use that new class

[Jimmy Jo, 2014-05-13 00:29:23]

On 2014/05/12 15:32:40, jam wrote:
> On 2014/05/08 01:01:38, Charlie Reis wrote:
> > On 2014/05/06 05:41:56, Jincheol Jo wrote:
> > > Charlie. ShellAddedObserver is observer for Shell Constructor.
> > > WebContentsImpl`s constructor is called and called RenderViewCreated
before
> > > calling Shell Constructor in this case.
> > > Refer to WebContentsImpl::CreateNewWindow and
> > WebContentsImpl:ShowCreatedWindow.
> > > It seems not match.
> > > I need the observer for WebContentsImpl Constructor.
> > > so I find out WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback,
but
> > > according to comments, it is deprecated.
> > > Can I use it?
> > > If it disable, it seems need to move RenderViewCreated`s position to Shell
> > > Constructor.
> > > Thanks.
> > 
> > @jam: It looks like you're planning to remove those per
> http://crrev.com/234570.
> >  How would you recommend adding a WebContentsObserver to a WebContents that
is
> > about to be created (e.g., via a targeted link)?  Here, we want the observer
> to
> > hear the RenderViewCreated call right after the WebContents is constructed.
> 
> ah, sorry I missed that subtlety earlier. WebContents::AddCreatedCallback is
> fine to call only from test code, which is why it's private. For background, I
> hid it because when it was public it allowed layering violations from code in
> say src\apps observing all the WebContents' created in src\chrome. So how
about:
> 1) add a WebContentsAddedObserver in content/public/test/browser_test_utils
that
> is just like ShellAddedObserver but for WebContents. make that class a friend
of
> WCImpl so it can access AddCreatedCallback 
> 2) bonus points if you convert TestNavigationObserver to use that new class

I got it. Thanks Jam!!. It is very kind helpful for me.

[Jimmy Jo, 2014-05-13 04:41:50]

> > ah, sorry I missed that subtlety earlier. WebContents::AddCreatedCallback is
> > fine to call only from test code, which is why it's private. For background,
I
> > hid it because when it was public it allowed layering violations from code
in
> > say src\apps observing all the WebContents' created in src\chrome. So how
> about:
> > 1) add a WebContentsAddedObserver in content/public/test/browser_test_utils
> that
> > is just like ShellAddedObserver but for WebContents. make that class a
friend
> of
> > WCImpl so it can access AddCreatedCallback 
> > 2) bonus points if you convert TestNavigationObserver to use that new class
> 
I proposed new patch.
Please take a look.

[Jimmy Jo, 2014-05-14 10:17:06]

On 2014/05/13 04:41:50, Jincheol Jo wrote:
> > > ah, sorry I missed that subtlety earlier. WebContents::AddCreatedCallback
is
> > > fine to call only from test code, which is why it's private. For
background,
> I
> > > hid it because when it was public it allowed layering violations from code
> in
> > > say src\apps observing all the WebContents' created in src\chrome. So how
> > about:
> > > 1) add a WebContentsAddedObserver in
content/public/test/browser_test_utils
> > that
> > > is just like ShellAddedObserver but for WebContents. make that class a
> friend
> > of
> > > WCImpl so it can access AddCreatedCallback 
> > > 2) bonus points if you convert TestNavigationObserver to use that new
class
> > 
> I proposed new patch.
> Please take a look.

ping ....

[Charlie Reis, 2014-05-14 17:04:18]

John, I think this mostly matches what you requested, though I'm not sure why it
needs a MessageLoopRunner.  Can you take a look?

Some style and naming nits below.

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

https://codereview.chromium.org/196283013/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:395:
WebContentsAddedObserver new_web_contents_observer;;
nit: Remove the extra semicolon.

https://codereview.chromium.org/196283013/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:397: "var a =
document.createElement('a');\
nit: We tend to use quotes on each line instead of backslashes.  For example:

                            "var a = document.createElement('a');"
                             "a.href='./title2.html';"
...

You can see an example in
RenderFrameHostManagerTest.DontShowLoadingURLIfNotInitialNav.

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
File content/test/content_browser_test_utils.cc (left):

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.cc:6: 
Please don't remove this blank line.  It distinguishes between the header file
for this .cc file and the other headers it's including.

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
File content/test/content_browser_test_utils.cc (right):

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.cc:13: //#include
"content/public/browser/web_contents.h"
Please uncomment this.  It's ok to include both web_contents.h and
web_contents_impl.h.

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.cc:97: bool call_render_view_created_;
nit: render_view_created_called_

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.cc:124: if (web_contents_) {
nit: No braces needed on a one-line body.

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.cc:128: runner_ = new
MessageLoopRunner();
Why is this needed?

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
File content/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.h:79: class RenderViewCreatedObserver;
nit: All class forward declarations should be together at the top of the file.

https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
content/test/content_browser_test_utils.h:86: bool CallRenderViewCreated();
nit: RenderViewCreatedCalled()

[Jimmy Jo, 2014-05-15 01:16:52]

On 2014/05/14 17:04:18, Charlie Reis(OOO as of May 17) wrote:
> John, I think this mostly matches what you requested, though I'm not sure why
it
> needs a MessageLoopRunner.  Can you take a look?
> 
> Some style and naming nits below.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/browser/web_con...
> File content/browser/web_contents/web_contents_impl_browsertest.cc (right):
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl_browsertest.cc:395:
> WebContentsAddedObserver new_web_contents_observer;;
> nit: Remove the extra semicolon.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl_browsertest.cc:397: "var a =
> document.createElement('a');\
> nit: We tend to use quotes on each line instead of backslashes.  For example:
> 
>                             "var a = document.createElement('a');"
>                              "a.href='./title2.html';"
> ...
> 
> You can see an example in
> RenderFrameHostManagerTest.DontShowLoadingURLIfNotInitialNav.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> File content/test/content_browser_test_utils.cc (left):
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.cc:6: 
> Please don't remove this blank line.  It distinguishes between the header file
> for this .cc file and the other headers it's including.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> File content/test/content_browser_test_utils.cc (right):
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.cc:13: //#include
> "content/public/browser/web_contents.h"
> Please uncomment this.  It's ok to include both web_contents.h and
> web_contents_impl.h.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.cc:97: bool call_render_view_created_;
> nit: render_view_created_called_
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.cc:124: if (web_contents_) {
> nit: No braces needed on a one-line body.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.cc:128: runner_ = new
> MessageLoopRunner();
> Why is this needed?
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> File content/test/content_browser_test_utils.h (right):
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.h:79: class RenderViewCreatedObserver;
> nit: All class forward declarations should be together at the top of the file.
> 
>
https://codereview.chromium.org/196283013/diff/100001/content/test/content_br...
> content/test/content_browser_test_utils.h:86: bool CallRenderViewCreated();
> nit: RenderViewCreatedCalled()

Thanks Charlie, I got many tip from you.
I modified a patch by your guidance.

In addition, I think the reason for using MessageLoopRunner.
When we use it, it seems more safely in this case.
Test code request web_contents object. but if web_contents do not exist, we
should wait it using MessageLoopRunner.

[jam, 2014-05-15 19:30:19]

lgtm with nits

sorry for the delay, i didn't see the earlier pings

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
File content/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:29: class RenderViewHost;
nit: not needed

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:85: bool RenderViewCreatedCalled();
nit: please document these two methods

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:86: private:
nit: blank line above this per style guide

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:89: public:
nit: put a public section and then private, and don't add a public section again

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:92: RenderViewCreatedObserver*
child_observer_;
nit: put this in a scoped_ptr so it doesn't leak

[Jimmy Jo, 2014-05-16 01:15:46]

Thanks Jam.
I modified nits by your guidance.

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
File content/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:29: class RenderViewHost;
On 2014/05/15 19:30:19, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:85: bool RenderViewCreatedCalled();
On 2014/05/15 19:30:19, jam wrote:
> nit: please document these two methods

Done.

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:86: private:
On 2014/05/15 19:30:19, jam wrote:
> nit: blank line above this per style guide

Done.

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:89: public:
On 2014/05/15 19:30:19, jam wrote:
> nit: put a public section and then private, and don't add a public section
again

Done.

https://codereview.chromium.org/196283013/diff/120001/content/test/content_br...
content/test/content_browser_test_utils.h:92: RenderViewCreatedObserver*
child_observer_;
On 2014/05/15 19:30:19, jam wrote:
> nit: put this in a scoped_ptr so it doesn't leak

Done.

[Jimmy Jo, 2014-05-16 01:15:59]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-16 01:16:41]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/14...

[Jimmy Jo, 2014-05-16 01:30:58]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-05-16 04:29:26]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-16 04:29:54]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/20...

[Jimmy Jo, 2014-05-16 04:32:21]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-05-16 04:45:16]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-16 04:45:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/20...

[Jimmy Jo, 2014-05-16 06:06:53]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-05-16 06:25:04]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-16 06:25:18]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/22...

[commit-bot: I haz the power, 2014-05-16 09:29:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-16 09:45:55]

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

[commit-bot: I haz the power, 2014-05-16 09:45:56]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[Jimmy Jo, 2014-05-16 09:49:59]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-16 09:50:13]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/22...

[commit-bot: I haz the power, 2014-05-16 10:01:11]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[Jimmy Jo, 2014-05-16 10:01:42]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-05-16 12:32:04]

I am facing compile error as following when I sync to latest source.

In file included from
../../content/browser/web_contents/web_contents_impl.h:27:0,
                 from
../../content/public/test/content_browser_test_utils.cc:11:
../../content/public/browser/ax_event_notification_details.h:11:39: fatal error:
ui/accessibility/ax_enums.h: No such file or directory
content_browser_test_utils.cc was moved /content/test --> /content/public/test.

----

ax_enums.h exists in out/Relese/obj/ui/... 

so I removed ax_event_notification_details.h in web_contents_imp.h because
defined "struct AXEventNotificationDetails" in web_contents_impl.h
It works well in android build.

Can I remove "ax_event_notification_details.h" in web_contents_impl.h?

[Charlie Reis, 2014-05-16 23:03:46]

On 2014/05/16 12:32:04, Jincheol Jo wrote:
> I am facing compile error as following when I sync to latest source.
> 
> In file included from
> ../../content/browser/web_contents/web_contents_impl.h:27:0,
>                  from
> ../../content/public/test/content_browser_test_utils.cc:11:
> ../../content/public/browser/ax_event_notification_details.h:11:39: fatal
error:
> ui/accessibility/ax_enums.h: No such file or directory
> content_browser_test_utils.cc was moved /content/test -->
/content/public/test.
> 
> ----
> 
> ax_enums.h exists in out/Relese/obj/ui/... 
> 
> so I removed ax_event_notification_details.h in web_contents_imp.h because
> defined "struct AXEventNotificationDetails" in web_contents_impl.h
> It works well in android build.
> 
> Can I remove "ax_event_notification_details.h" in web_contents_impl.h?

Oh, this error is because you're including web_contents_impl.h from
content_browser_test_utils.cc?  I agree that the include seems unnecessary.  I'm
CC'ing dtseng@ as FYI, but otherwise this still LGTM.

[Jimmy Jo, 2014-05-17 00:52:33]

On 2014/05/16 23:03:46, Charlie Reis (OOO) wrote:
> On 2014/05/16 12:32:04, Jincheol Jo wrote:
> > I am facing compile error as following when I sync to latest source.
> > 
> > In file included from
> > ../../content/browser/web_contents/web_contents_impl.h:27:0,
> >                  from
> > ../../content/public/test/content_browser_test_utils.cc:11:
> > ../../content/public/browser/ax_event_notification_details.h:11:39: fatal
> error:
> > ui/accessibility/ax_enums.h: No such file or directory
> > content_browser_test_utils.cc was moved /content/test -->
> /content/public/test.
> > 
> > ----
> > 
> > ax_enums.h exists in out/Relese/obj/ui/... 
> > 
> > so I removed ax_event_notification_details.h in web_contents_imp.h because
> > defined "struct AXEventNotificationDetails" in web_contents_impl.h
> > It works well in android build.
> > 
> > Can I remove "ax_event_notification_details.h" in web_contents_impl.h?
> 
> Oh, this error is because you're including web_contents_impl.h from
> content_browser_test_utils.cc?  I agree that the include seems unnecessary. 
I'm
> CC'ing dtseng@ as FYI, but otherwise this still LGTM.

Yes. I include web_contents_impl.h in content_browser_test_utils.cc.
Content_browser_test_utils is moved and the section including it changed in
content_test.gypi during code review.
Can I push commit button?

[Jimmy Jo, 2014-05-17 01:44:08]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-17 01:46:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/24...

[commit-bot: I haz the power, 2014-05-17 07:49:24]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-17 08:39:27]

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

[commit-bot: I haz the power, 2014-05-17 08:39:28]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Jimmy Jo, 2014-05-17 08:41:58]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-17 08:43:47]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/24...

[Jimmy Jo, 2014-05-17 13:12:40]

The CQ bit was unchecked by jincheol.jo@navercorp.com

[Jimmy Jo, 2014-05-17 13:12:41]

The CQ bit was checked by jincheol.jo@navercorp.com

[commit-bot: I haz the power, 2014-05-17 13:14:16]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jincheol.jo@navercorp.com/196283013/24...

[commit-bot: I haz the power, 2014-05-17 17:17:29]

Change committed as 271240