A crash fix: the entire implicated section seems obsolete. Removed.

A crash fix: removed the entire implicated section as obsolete. 

Note that the TODO points to a crbug which is declared obsolete / irrelevant.

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

[motek., 2012-09-18 22:16:48]

Could I trouble you for a review, Robert?

[robertshield, 2012-09-18 23:52:37]

Code change lgtm, but I'm unsure as to whether this will cause visible
side-effects in the non-crashing case. 

CC'ing jcampan to also take a look.

[motek., 2012-09-19 14:53:48]

sky@,

Could I trouble you with this very simple CL? I believe setfocus call to have no
effect (at least I could not notice one), so instead of "if !null" it might be
better to simply remove it. What do you think?

On 2012/09/18 23:52:37, robertshield wrote:
> Code change lgtm, but I'm unsure as to whether this will cause visible
> side-effects in the non-crashing case. 
> 
> CC'ing jcampan to also take a look.
This may not work... -jcampan +sky

[sky, 2012-09-19 17:04:53]

How did you verify this is no longer needed?

[motek., 2012-09-19 18:32:18]

I obviously do not have conclusive proof, but: 

I followed the code path and tried out that behaviour under gtk and under win
for both branches (focusing on the omnibox and focusing on the content). No
noticeable difference. 
I debugged a bit, too. In each of 3 scenarios (session restore, newtab, selected
URL) SetInitialFocus is called after a notification (active tab changed) when
content is created. So, it would seem this call is redundant and in some edge
cases harmful. 

+ circumstantial evidence: the bug mentioned in the TODO before change went
'obsolete' over 2 years ago, with a comment that this is not a problem anymore. 


On 2012/09/19 17:04:53, sky wrote:
> How did you verify this is no longer needed?

[sky, 2012-09-19 21:03:54]

Jay is still around. Jay, any ideas?

[Jay Civelli, 2012-09-19 21:27:47]

Sorry for the delay.
I don't remember precisely, but if you can still move the focus around
pressing tab (from page to omnibox) then I think that's OK.

Jay


On Wed, Sep 19, 2012 at 2:03 PM, <sky@chromium.org> wrote:

> Jay is still around. Jay, any ideas?
>
>
https://chromiumcodereview.**appspot.com/10948021/<https://chromiumcodereview...
>

[motek., 2012-09-20 14:57:03]

On 2012/09/19 21:27:47, Jay Civelli wrote:
> Sorry for the delay.
> I don't remember precisely, but if you can still move the focus around
> pressing tab (from page to omnibox) then I think that's OK.
Tab navigation appears the same in both situation. In particular, you can
tab-navigate in-page and between the omnibox and tabs without first clicking on
anything (which is the point).

[sky, 2012-09-20 15:31:55]

LGTM

[commit-bot: I haz the power, 2012-09-20 18:04:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/motek@chromium.org/10948021/1

[commit-bot: I haz the power, 2012-09-21 05:21:37]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-21 12:11:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/motek@chromium.org/10948021/1

[commit-bot: I haz the power, 2012-09-21 13:56:12]

Change committed as 157974