Issue 1679002: Possible fix / workaround for issue where infobar fails to show...

Keyboard Shortcuts

	File
u :	up to issue
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line

	Issue
u :	up to list of issues
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue

Issue 1679002: Possible fix / workaround for issue where infobar fails to show... (Closed)

Created:
10 years, 8 months ago by joth

Modified:
9 years, 7 months ago

Reviewers:
rohitrao (ping after 24h)

CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

Possible fix / workaround for issue where infobar fails to show BUG=41712 TEST=as per bug comment 8 http://crbug.com/41712#c8 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45295

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 1

Created: 10 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+2 lines, -1 line)			Patch
	M	chrome/browser/cocoa/infobar_container_controller.mm	View	1	1 chunk	+2 lines, -1 line	1 comment	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

joth

Hi rohitrao I think you're the expert on this code, would you have a look ...

10 years, 8 months ago (2010-04-20 03:23:39 UTC) #1

rohitrao (ping after 24h)

Heh, has this been broken forever? Wonder why no one noticed it before. LGTM http://codereview.chromium.org/1679002/diff/2001/3001 ...

10 years, 8 months ago (2010-04-21 14:00:52 UTC) #2

joth

10 years, 8 months ago (2010-04-22 11:27:15 UTC) #3

Thanks! Committed.

Yeah, I guess this was always there. I suppose not so many flows completely rely
on infobars compared so geolocation, so it was just not noticed.

See your point about the logic differing, but this seems the simplest way to do
it here and as you say, it should work fine.

Cheers

On 2010/04/21 14:00:52, rohitrao wrote:
> Heh, has this been broken forever?  Wonder why no one noticed it before.
> 
> LGTM
> 
> http://codereview.chromium.org/1679002/diff/2001/3001
> File chrome/browser/cocoa/infobar_container_controller.mm (right):
> 
> http://codereview.chromium.org/1679002/diff/2001/3001#newcode126
> chrome/browser/cocoa/infobar_container_controller.mm:126: if
> (currentTabContents_ == contents)
> This doesn't quite match the logic in browser_view.cc, but it should work
fine.

Expand Messages | Collapse Messages