New windows opened in javascript with target = "_blank" should

New windows opened in javascript with target = "_blank" should
have an empty name. 

BUG=88129
TEST=InProcessRenderViewBrowserTest::*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98946

[shinyak (Google), 2011-08-10 05:37:11]

Darin,
I've fixed BUG=88129
http://code.google.com/p/chromium/issues/detail?id=88129

If frame_name is "_blank", we should ignore it.
Could you review it?

[darin (slow to review), 2011-08-10 21:44:14]

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1329: WebString fixed_frame_name =
Hmm... perhaps _blank should be stripped on the WebKit side instead?

[shinyak (Google), 2011-08-11 01:52:33]

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1329: WebString fixed_frame_name =
On 2011/08/10 21:44:14, darin wrote:
> Hmm... perhaps _blank should be stripped on the WebKit side instead?

Hmm... I'm not assure that this is the best position, but Safari can treat with
_blank well.
I try to strip it on the WebKit side, this could break Safari though. If it
breaks Safari, I inform you and we should consider treating _blank on the
Chromium side.

[shinyak (Google), 2011-08-11 06:43:09]

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1329: WebString fixed_frame_name =
Darin, I've uploaded the patch to WebKit.
https://bugs.webkit.org/show_bug.cgi?id=66030

I'm not assure this patch is accepted. If not so, we should consider handling
this problem on Chromium side, I think.

On 2011/08/11 01:52:33, shinyak wrote:
> On 2011/08/10 21:44:14, darin wrote:
> > Hmm... perhaps _blank should be stripped on the WebKit side instead?
> 
> Hmm... I'm not assure that this is the best position, but Safari can treat
with
> _blank well.
> I try to strip it on the WebKit side, this could break Safari though. If it
> breaks Safari, I inform you and we should consider treating _blank on the
> Chromium side.

[darin (slow to review), 2011-08-12 04:12:07]

On Wed, Aug 10, 2011 at 11:43 PM, <shinyak@google.com> wrote:

>
> http://codereview.chromium.**org/7497058/diff/1/content/**
>
renderer/render_view.cc<http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc>
> File content/renderer/render_view.**cc (right):
>
> http://codereview.chromium.**org/7497058/diff/1/content/**
>
renderer/render_view.cc#**newcode1329<http://codereview.chromium.org/7497058/diff/1/content/renderer/render_view.cc#newcode1329>
> content/renderer/render_view.**cc:1329: WebString fixed_frame_name =
> Darin, I've uploaded the patch to WebKit.
>
https://bugs.webkit.org/show_**bug.cgi?id=66030<https://bugs.webkit.org/show_...
>
> I'm not assure this patch is accepted. If not so, we should consider
> handling this problem on Chromium side, I think.
>
>
You could also just patch this in WebKit/chromium/src/ code instead, right?
 You don't necessarily need to fix it in WebCore as you then run the risk of
impacting other ports.

-Darin


>
> On 2011/08/11 01:52:33, shinyak wrote:
>
>> On 2011/08/10 21:44:14, darin wrote:
>> > Hmm... perhaps _blank should be stripped on the WebKit side instead?
>>
>
>  Hmm... I'm not assure that this is the best position, but Safari can
>>
> treat with
>
>> _blank well.
>> I try to strip it on the WebKit side, this could break Safari though.
>>
> If it
>
>> breaks Safari, I inform you and we should consider treating _blank on
>>
> the
>
>> Chromium side.
>>
>
>
http://codereview.chromium.**org/7497058/<http://codereview.chromium.org/7497...
>

[shinyak (Google), 2011-08-22 05:56:24]

From this discussion (https://bugs.webkit.org/show_bug.cgi?id=66030) I removed 2
lines.

[darin (slow to review), 2011-08-23 04:44:30]

LGTM

[commit-bot: I haz the power, 2011-08-24 02:45:06]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 97976, job name
was 7497058-6001 (retry) (retry) (retry) (retry).

[shinyak (Google), 2011-08-24 04:35:08]

Darin, 

Since in the previous patch the test depends on the modules which should not be
depended, try-bots fails again and again.

I have moved the test to renderer_host, which I think is related to the change.

If you think it's OK, could you review it again?
Also, I want you to send this patch try-bot. (I don't have the right to use
try-bot now.)

[shinyak (Google), 2011-08-29 07:50:02]

Darin,
ping?

On 2011/08/24 04:35:08, shinyak wrote:
> Darin, 
> 
> Since in the previous patch the test depends on the modules which should not
be
> depended, try-bots fails again and again.
> 
> I have moved the test to renderer_host, which I think is related to the
change.
> 
> If you think it's OK, could you review it again?
> Also, I want you to send this patch try-bot. (I don't have the right to use
> try-bot now.)

[darin (slow to review), 2011-08-29 17:33:41]

http://codereview.chromium.org/7497058/diff/10002/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

http://codereview.chromium.org/7497058/diff/10002/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_browsertest.cc:92: //
observer.Wait();
why is this code commented out?

[shinyak (Google), 2011-08-30 02:19:02]

Darin,

Sorry, it was my mistake. I've fixed it.

[shinyak (Google), 2011-08-30 18:35:18]

Darin,

I hope you to review this patch again. The main change is that the test is moved
from renderer to renderer_host. Actually this patch fixes render_view, however
this patch fixes not only render_view behavior. So I think it is OK to have
tests not in render_view.

On 2011/08/30 02:19:02, shinyak wrote:
> Darin,
> 
> Sorry, it was my mistake. I've fixed it.

[darin (slow to review), 2011-08-30 20:32:29]

LGTM

http://codereview.chromium.org/7497058/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_browsertest.cc (right):

http://codereview.chromium.org/7497058/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_browsertest.cc:84:
browser()->GetSelectedTabContents()->render_view_host(),
nit: use 4 space indent for line continuation.

[commit-bot: I haz the power, 2011-08-31 06:45:10]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 98927, job name
was 7497058-21005 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-31 09:04:36]

Change committed as 98946