Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
398623004:
DevTools: Open new tab with correct url rather than navigate from default page (Closed)
|
Created:
6 years, 5 months ago by vkuzkokov Modified:
6 years, 4 months ago CC:
chromium-reviews, vsevik, yurys, paulirish+reviews_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Open new tab with correct url rather than navigate from default page
BUG=374462
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288035
Patch Set 1 #
Total comments: 3
Patch Set 2 : Used chrome::Navigate #Patch Set 3 : Simplified #
Total comments: 7
Patch Set 4 : Minor fixes #Messages
Total messages: 13 (0 generated)
devtools lgtm https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:342: Browser* OpenEmptyWindow(Profile* profile, HostDesktopType desktop_type) { I think, this method is not used anymore.
https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.h:51: void NewWindow(Profile* profile, HostDesktopType desktop_type, const GURL& url); Can you use browser_navigator.h to create and navigate in a new browser? It handles all the cases you should care about as well as providing an out parameter for the browser that was created.
https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/398623004/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.h:51: void NewWindow(Profile* profile, HostDesktopType desktop_type, const GURL& url); On 2014/07/21 16:35:54, sky wrote: > Can you use browser_navigator.h to create and navigate in a new browser? It > handles all the cases you should care about as well as providing an out > parameter for the browser that was created. Done.
https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right): https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:106: url, content::PAGE_TRANSITION_TYPED); Why is this PAGE_TRANSITION_TYPED?
On 2014/08/05 17:47:00, sky wrote: > https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... > File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right): > > https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... > chrome/browser/devtools/browser_list_tabcontents_provider.cc:106: url, > content::PAGE_TRANSITION_TYPED); > Why is this PAGE_TRANSITION_TYPED? Better question: why it used to be PAGE_TRANSITION_TYPED for new window and PAGE_TRANSITION_LINK for existing? This method is used in remote debugging as a back-end for "Open tab with url" on chrome://inspect so in a way it is typed.
https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right): https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:105: chrome::NavigateParams params(ProfileManager::GetLastUsedProfile(), Are you sure this will be a new tab? Let's set params.disposition = NEW_FOREGROUND_TAB. https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:106: url, content::PAGE_TRANSITION_TYPED); I think this is more like PAGE_TRANSITION_AUTO_TOPLEVEL. https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:107: Navigate(¶ms); nit: chrome::Navigate for better readability.
https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right): https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:105: chrome::NavigateParams params(ProfileManager::GetLastUsedProfile(), On 2014/08/06 08:21:07, dgozman wrote: > Are you sure this will be a new tab? Let's set > params.disposition = NEW_FOREGROUND_TAB. I looked up in source. Adding anyway. https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:106: url, content::PAGE_TRANSITION_TYPED); On 2014/08/06 08:21:07, dgozman wrote: > I think this is more like PAGE_TRANSITION_AUTO_TOPLEVEL. Done. https://codereview.chromium.org/398623004/diff/40001/chrome/browser/devtools/... chrome/browser/devtools/browser_list_tabcontents_provider.cc:107: Navigate(¶ms); On 2014/08/06 08:21:07, dgozman wrote: > nit: chrome::Navigate for better readability. Done.
lgtm
lgtm
The CQ bit was checked by vkuzkokov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/398623004/60001
Message was sent while issue was closed.
Change committed as 288035 |
