Issue 9390015: Get rid of ShowPageInfo on WebContents and WebContentsDelegate. Chrome shouldn't have to go throu...

Issue 9390015: Get rid of ShowPageInfo on WebContents and WebContentsDelegate. Chrome shouldn't have to go throu... (Closed)

Created:
8 years, 10 months ago by jam

Modified:
8 years, 10 months ago

Reviewers:
Avi (use Gerrit), Ben Goodger (Google)

CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Get rid of ShowPageInfo on WebContents and WebContentsDelegate. Chrome shouldn't have to go through content to put up its own UI dialog. BUG=98716 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121736

Patch Set 1 : '' #

Patch Set 2 : fix gtk+mac #

Total comments: 2

Created: 8 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+31 lines, -48 lines)			Patch
M	chrome/browser/tab_contents/render_view_context_menu.cc	View		3 chunks	+8 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/browser.h	View		2 chunks	+3 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/browser.cc	View		1 chunk	+2 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm	View	1	3 chunks	+6 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/gtk/location_bar_view_gtk.cc	View	1	2 chunks	+5 lines, -2 lines	2 comments	Download
M	chrome/browser/ui/views/location_bar/click_handler.cc	View		3 chunks	+7 lines, -2 lines	0 comments	Download
M	content/browser/tab_contents/tab_contents.h	View		1 chunk	+0 lines, -3 lines	0 comments	Download
M	content/browser/tab_contents/tab_contents.cc	View		1 chunk	+0 lines, -9 lines	0 comments	Download
M	content/public/browser/web_contents.h	View		2 chunks	+0 lines, -6 lines	0 comments	Download
M	content/public/browser/web_contents_delegate.h	View		1 chunk	+0 lines, -9 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

jam

On 2012/02/13 20:12:35, Avi wrote: > Haha awesome. Drive-by LGTM? thanks for the review, fixed ...

8 years, 10 months ago (2012-02-13 21:03:02 UTC) #3

Ben Goodger (Google)

lgtm http://codereview.chromium.org/9390015/diff/8001/chrome/browser/ui/gtk/location_bar_view_gtk.cc File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/9390015/diff/8001/chrome/browser/ui/gtk/location_bar_view_gtk.cc#newcode1051 chrome/browser/ui/gtk/location_bar_view_gtk.cc:1051: Browser* browser = Browser::GetBrowserForController(&controller, NULL); You shouldn't need ...

8 years, 10 months ago (2012-02-14 17:52:55 UTC) #4

jam

8 years, 10 months ago (2012-02-14 18:06:08 UTC) #5

http://codereview.chromium.org/9390015/diff/8001/chrome/browser/ui/gtk/locati...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/9390015/diff/8001/chrome/browser/ui/gtk/locati...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1051: Browser* browser =
Browser::GetBrowserForController(&controller, NULL);
On 2012/02/14 17:52:56, Ben Goodger (Google) wrote:
> You shouldn't need to do this. This object has a browser_ field that
corresponds
> to the browser_ it's attached to.

thanks, I missed that, i'll send you a cl updating this

Expand Messages | Collapse Messages