Issue 10696222: Make chrome::Navigate take in a Browser* that is never NULL. That function doesn't have a context, …

Issue 10696222: Make chrome::Navigate take in a Browser* that is never NULL. That function doesn't have a context, … (Closed)

Created:
8 years, 5 months ago by jam

Modified:
8 years, 5 months ago

Reviewers:
Ben Goodger (Google)

CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, Munjal (Google)

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

Visibility:
Public.

More Reviews

Description

Make chrome::Navigate take in a Browser* that is never NULL. That function doesn't have a context, so it could never figure out which desktop it should open new windows in. Instead, push this out to callers that have more context. BUG=129187 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146681

Patch Set 1 #

Patch Set 2 : fix unittests #

Patch Set 3 : fix cros #

Total comments: 2

Patch Set 4 : sync #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+54 lines, -155 lines)			Patch
M	chrome/browser/extensions/extension_tab_util.cc	View	1 2 3	2 chunks	+1 line, -7 lines	0 comments	Download
M	chrome/browser/platform_util_chromeos.cc	View	1 2 3	2 chunks	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/browser_navigator.h	View	1 2 3	3 chunks	+1 line, -7 lines	0 comments	Download
M	chrome/browser/ui/browser_navigator.cc	View	1 2 3	9 chunks	+39 lines, -68 lines	0 comments	Download
M	chrome/browser/ui/browser_navigator_browsertest.cc	View	1 2 3	1 chunk	+0 lines, -56 lines	0 comments	Download
M	chrome/browser/ui/intents/web_intent_picker_controller.cc	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/web_dialog_web_contents_delegate.h	View	1 2 3	1 chunk	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/ui/webui/web_dialog_web_contents_delegate.cc	View	1 2 3	3 chunks	+3 lines, -9 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Mihai Parparita -not on Chrome

http://codereview.chromium.org/10696222/diff/8004/chrome/browser/ui/browser_navigator.cc File chrome/browser/ui/browser_navigator.cc (right): http://codereview.chromium.org/10696222/diff/8004/chrome/browser/ui/browser_navigator.cc#newcode143 chrome/browser/ui/browser_navigator.cc:143: return Browser::CreateWithParams( Given that NEW_POPUP ends up creating a ...

8 years, 5 months ago (2012-07-13 21:09:27 UTC) #3

jam

8 years, 5 months ago (2012-07-13 21:18:39 UTC) #4

http://codereview.chromium.org/10696222/diff/8004/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/10696222/diff/8004/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:143: return Browser::CreateWithParams(
On 2012/07/13 21:09:27, Mihai Parparita wrote:
> Given that NEW_POPUP ends up creating a Browser, it seems odd to require one
to
> be passed in.

There are a number of ways that this function creates a new Browser. The passed
in Browser is used to give it context when it does.

> 
> We'd like to be able to open a popup while in an app to show the sign-in UI
>
(https://chromiumcodereview.appspot.com/10701041/diff/11002/chrome/browser/ext...),
> and there may not be a Browser that we can pass in.

I'm not familiar with that code. In that case, could there be no Browser* at
all?

Expand Messages | Collapse Messages