Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
240273002:
Close the ntp during startup when opening a link on Mac. (Closed)
|
Created:
6 years, 8 months ago by wjywbs Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionClose the ntp during startup when opening a link on Mac.
If there's only 1 tab and the tab is NTP, close this NTP tab and open all
startup urls in new tabs, because the omnibar will stay focused if we
load url in NTP tab.
R=asvitkine@chromium.org,viettrungluu@chromium.org,thakis@chromium.org
TBR=sergeyberezin@chromium.org
BUG=43520
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266810
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : fix comment #
Total comments: 4
Patch Set 5 : #Patch Set 6 : reorder test header #
Total comments: 6
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 5
Patch Set 9 : #
Total comments: 2
Messages
Total messages: 44 (0 generated)
Please see the bug for details. PTAL. Thanks.
Is it possible to create a browser test for this? I am worried that something may change (e.g. the initial tab count changing from 1 to 2) and this logic not being correct anymore when that does (and no one noticing without a test). https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:732: // The browser is already loaded at this point with the new tab page. Could you make a new method wrapping all of this logic? e.g. -openStartupUrls https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:733: int ntp_index; Nit: Init this to a dummy value. https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:736: DCHECK(browser->tab_strip_model()->count() == 1); DCHECK_EQ? Also, are you sure this will always be true? For example, I think on first launch I remember there could be two tabs open. Is there a way to make this logic more robust (e.g. checking if a tab is actually the NTP?). Or perhaps it should remove all tabs other than the new ones opened? https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:743: // Close the new tab page after opening urls to prevent from closing the Nit: Remove "from". https://codereview.chromium.org/240273002/diff/1/chrome/browser/app_controlle... chrome/browser/app_controller_mac.mm:745: if (browser) Nit: add {}'s
On 2014/04/16 14:35:30, Alexei Svitkine wrote: > Is it possible to create a browser test for this? > > I am worried that something may change (e.g. the initial tab count changing from > 1 to 2) and this logic not being correct anymore when that does (and no one > noticing without a test). > I'm thinking about adding a test, but I didn't find any existing tests to set the browser as default or open a shortcut in Mac. Also it may be hard to trigger this code path via browser_tests, since the data is passed via NSAppleEventManager in Mac system. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/app... I will change the logic tonight to load the first startup url in the NTP tab, and open new tabs for the rest startup urls, only if there's only 1 tab and the tab is NTP during startup. This way we won't lose any messages shown above the NTP tab, such as "Restore last tabs from crash" or "Google API keys are missing". If there are more than 1 tabs during startup, open all startup urls in new tabs.
Please take another look. I updated the logic and added a test. The test is really complex. Hopefully we can say a goodbye to this 5 years old bug. Peter: Please review the infobar changes to move the bars to other tabs. If Alexei and you agree with the changes, I'm willing to submit the infobar CL separately and then this one. Thanks.
This is precisely the sort of thing the InfoBar system is designed to avoid, because reparenting infobars can cause a ton of different problems. Infobars may rely in arbitrary ways on their initial owner and cannot safely be reparented to a new one. Besides this, the fact that Windows does not need this code tells me that this is the wrong fix. How does Windows prevent this from occurring?
To my understanding, url are passed in as command line arguments in Windows: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... The first browser is created in PreMainMessageLoop and the urls in the command line is processed. But on Mac, the urls are passed in via cocoa, not command line. The chrome NSApplication is created in MainMessageLoop, and then the shortcut urls are passed in via apple events. At this point, the first browser is already opened with NTP. Initializing NSApplication before PreMainMessageLoop to capture shortcut url events may cause more problems because it relies on things created in PreMainMessageLoop and may break existing message loop design.
+viettrungluu You may have some better ideas since you mentioned and introduced this bug in http://crrev.com/36902 Thanks.
On 2014/04/18 22:21:58, wjywbs wrote: > To my understanding, url are passed in as command line arguments in Windows: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > The first browser is created in PreMainMessageLoop and the urls in the command > line is processed. > > But on Mac, the urls are passed in via cocoa, not command line. The chrome > NSApplication is created in MainMessageLoop, and then the shortcut urls are > passed in via apple events. At this point, the first browser is already opened > with NTP. Initializing NSApplication before PreMainMessageLoop to capture > shortcut url events may cause more problems because it relies on things created > in PreMainMessageLoop and may break existing message loop design. If you're in a case where we're opening the browser on a specific URL or URLs, why are you trying to move the NTP infobars over anyway? Just navigating the NTP tab to the new tab, or closing it in favor of a new tab, both seem more correct.
On 2014/04/21 21:10:41, Peter Kasting wrote: > If you're in a case where we're opening the browser on a specific URL or URLs, > why are you trying to move the NTP infobars over anyway? Just navigating the > NTP tab to the new tab, or closing it in favor of a new tab, both seem more > correct. The only infobar that I thought worthy to move was restoring last sessions from crash. If it's ok for not having this infobar when opening url shortcuts after a crash, I can remove the infobar changes.
On 2014/04/21 22:11:58, wjywbs wrote: > On 2014/04/21 21:10:41, Peter Kasting wrote: > > If you're in a case where we're opening the browser on a specific URL or URLs, > > why are you trying to move the NTP infobars over anyway? Just navigating the > > NTP tab to the new tab, or closing it in favor of a new tab, both seem more > > correct. > > The only infobar that I thought worthy to move was restoring last sessions from > crash. If it's ok for not having this infobar when opening url shortcuts after a > crash, I can remove the infobar changes. If you're opening a specific URL, it seems like you're going to want that URL, not whatever was leftover from the previous session. So I think not preserving the infobars is fine.
I removed the infobar changes. PTAL. Thanks.
https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:693: // startup urls in new tabs, because the omnibar will stay focused if we Nit: omnibox https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:706: [self clearStartupUrls]; Doesn't seem like -clearStartupUrls is used from anywhere else. Can you just inline its impl here and remove the method? https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:710: startupContent->GetVisibleURL() == GURL(chrome::kChromeUINewTabURL)) { I see other code doing the following check instead for NTP: url.SchemeIs(content::kChromeUIScheme) && url.host() == chrome::kChromeUINewTabHost But, I'm not sure if even this is correct. People can use extensions to override the NTP or can choose to have other pages open at start up. It seems like this logic wouldn't work in those cases, right? What about using the simpler logic of opening the new URLs and then closing all the ones that were there before? (i.e. save the tab count, open the new URLs and then close the tabs that were opened before)
https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:710: startupContent->GetVisibleURL() == GURL(chrome::kChromeUINewTabURL)) { On 2014/04/22 15:42:00, Alexei Svitkine wrote: > I see other code doing the following check instead for NTP: > > url.SchemeIs(content::kChromeUIScheme) && > url.host() == chrome::kChromeUINewTabHost > > But, I'm not sure if even this is correct. People can use extensions to override > the NTP or can choose to have other pages open at start up. It seems like this > logic wouldn't work in those cases, right? > > What about using the simpler logic of opening the new URLs and then closing all > the ones that were there before? (i.e. save the tab count, open the new URLs and > then close the tabs that were opened before) I tested this current method with a NTP override extension, and it still works. I think the extensions are not loaded yet at this point. Should I change this logic or keep it? Thanks.
On 2014/04/22 21:51:50, wjywbs wrote: > https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... > File chrome/browser/app_controller_mac.mm (right): > > https://codereview.chromium.org/240273002/diff/60001/chrome/browser/app_contr... > chrome/browser/app_controller_mac.mm:710: startupContent->GetVisibleURL() == > GURL(chrome::kChromeUINewTabURL)) { > On 2014/04/22 15:42:00, Alexei Svitkine wrote: > > I see other code doing the following check instead for NTP: > > > > url.SchemeIs(content::kChromeUIScheme) && > > url.host() == chrome::kChromeUINewTabHost > > > > But, I'm not sure if even this is correct. People can use extensions to > override > > the NTP or can choose to have other pages open at start up. It seems like this > > logic wouldn't work in those cases, right? > > > > What about using the simpler logic of opening the new URLs and then closing > all > > the ones that were there before? (i.e. save the tab count, open the new URLs > and > > then close the tabs that were opened before) > > I tested this current method with a NTP override extension, and it still works. > I think the extensions are not loaded yet at this point. Should I change this > logic or keep it? Thanks. Did you also try it with "Continue where you left off" functionality (e.g. where closing a browser with a few tabs and re-opening it would normally restore those tabs). How does that work with this change? I'm okay with not changing the logic if this achieves the desired behavior. But it would be especially good to expand test coverage to cover those cases as well, if possible.
On 2014/04/23 14:42:42, Alexei Svitkine wrote: > Did you also try it with "Continue where you left off" functionality (e.g. where > closing a browser with a few tabs and re-opening it would normally restore those > tabs). How does that work with this change? > > I'm okay with not changing the logic if this achieves the desired behavior. But > it would be especially good to expand test coverage to cover those cases as > well, if possible. I tested this "Continue where you left off" settings on chrome 34 stable and chromium with this patch on Mac. If this setting is enabled, both browsers will not open the url in the shortcut. They only restore the tabs in the previous session. I think this should be filed as a separate bug, such as "url shortcut is not opened when 'Continue where you left off' is enabled on Mac". I also tested the "Open a specific page or set of pages". The browser with this patch worked correctly to append a tab with the url in the shortcut after the specified tabs.
On 2014/04/24 00:36:06, wjywbs wrote: > On 2014/04/23 14:42:42, Alexei Svitkine wrote: > > Did you also try it with "Continue where you left off" functionality (e.g. > where > > closing a browser with a few tabs and re-opening it would normally restore > those > > tabs). How does that work with this change? > > > > I'm okay with not changing the logic if this achieves the desired behavior. > But > > it would be especially good to expand test coverage to cover those cases as > > well, if possible. > > I tested this "Continue where you left off" settings on chrome 34 stable and > chromium with this patch on Mac. If this setting is enabled, both browsers will > not open the url in the shortcut. They only restore the tabs in the previous > session. I think this should be filed as a separate bug, such as "url shortcut > is not opened when 'Continue where you left off' is enabled on Mac". I'm okay with filing a separate bug for that if this fix doesn't make anything worse in that area. > > I also tested the "Open a specific page or set of pages". The browser with this > patch worked correctly to append a tab with the url in the shortcut after the > specified tabs.
https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac.mm:685: // TODO(viettrungluu): This is very temporary, since this should be done "in" Nit: Remove the "This is very temporary" part of the comment, since clearly it's not temporary anymore. But... does this comment even makes sense anymore? It's suggesting that perhaps we could handle the URLs much earlier than this. Can we? If I understand your explanation correctly, it sounds like we can't since we get them pretty late. If so, perhaps we should replace this comment with an explanation of why this must be done here (and not earlier). https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac_browsertest.mm:40: GURL openShortcutUrl = GURL::EmptyGURL(); Nit: Use c++ naming convention here. https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac_browsertest.mm:71: forKeyword:keyDirectObject]; Nit: Align :'s https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac_browsertest.mm:284: // send the event before applicationDidFinishLaunching is called, but Nit: "applicationDidFinishLaunching" -> "-applicationDidFinishLaunching". Same below. https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac_browsertest.mm:303: @selector(applicationWillFinishLaunching:)); Nit: Extract to local SEL variable and re-use below. https://codereview.chromium.org/240273002/diff/90001/chrome/browser/app_contr... chrome/browser/app_controller_mac_browsertest.mm:325: bool constructor_success_ = true; Instead of exposing this as a protected variable, can you just add ASSERT_TRUE() statements to SetUpInProcessBrowserTestFixture()?
I fixed the code per comments. PTAL, thanks. https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:70: [NSString stringWithUTF8String:open_shortcut_url.spec().c_str()]] I aligned the :'s, but this line is >80 chars. What should I do?
https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:685: // On Mac, the URLs are passed in via cocoa, not command line. The chrome Nit: Capitalize Cocoa and Chrome. https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:687: // are passed in via apple events. At this point, the first browser is Nit: Apple events https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/110001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:70: [NSString stringWithUTF8String:open_shortcut_url.spec().c_str()]] On 2014/04/24 21:26:46, wjywbs wrote: > I aligned the :'s, but this line is >80 chars. What should I do? Can you make a local variable for [NSString stringWithUTF8String:open_shortcut_url.spec().c_str()]?
I updated the code. PTAL, thanks.
https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:40: GURL open_shortcut_url = GURL::EmptyGURL(); Nit: g_open_shortcut_url https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:54: AppController* controller = (AppController*)[NSApp delegate]; Use ObjCCast() from foundation_util.h instead, which does a isKindOfClass: check. https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:72: forKeyword:keyDirectObject]; Nit: Sorry, maybe my previous comment was confusing. The actual style guide rule is to align :'s with a single call. So forKeyword: should be aligned with the : of setParamDescriptor: https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:297: ASSERT_TRUE(appControllerClass != nil && openShortcutClass != nil); Nit: Can you split these into separate ASSERT lines? This would make it easier to debug when the test fails. Same for below.
https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/130001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:72: forKeyword:keyDirectObject]; On 2014/04/25 14:47:08, Alexei Svitkine wrote: > Nit: Sorry, maybe my previous comment was confusing. The actual style guide rule > is to align :'s with a single call. So forKeyword: should be aligned with the : > of setParamDescriptor: *within a single call, rather
I updated the test. PTAL, thanks.
lgtm Can you also file the bug for edge case this is not handling (i.e. "Continue where you left of" case)? Thanks!
On 2014/04/25 16:29:40, Alexei Svitkine wrote: > lgtm > > Can you also file the bug for edge case this is not handling (i.e. "Continue > where you left of" case)? Thanks! The "Continue where you left off" case is filed as Issue 367169.
The CQ bit was checked by wjywbs@gmail.com
This patch does not affect the behavior of "Continue where you left off". I just found out that the URL in the shortcut is opened in a new window behind the window with the tabs from previous session in both chrome stable and chromium with this patch. Sorry that I didn't notice the window was hidden behind days ago.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/240273002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
thakis@: Missing owner LGTM. Thanks.
lgtm https://codereview.chromium.org/240273002/diff/150001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/150001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:41: GURL g_open_shortcut_url = GURL::EmptyGURL(); Can you pass this around instead of making this a global?
https://codereview.chromium.org/240273002/diff/150001/chrome/browser/app_cont... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/240273002/diff/150001/chrome/browser/app_cont... chrome/browser/app_controller_mac_browsertest.mm:41: GURL g_open_shortcut_url = GURL::EmptyGURL(); On 2014/04/28 21:17:21, Nico wrote: > Can you pass this around instead of making this a global? It's used by the outer @implementation TestOpenShortcutOnStartup and the inner class AppControllerOpenShortcutBrowserTest in line 71 and 314, and they don't communicate directly with each other.
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/240273002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/240273002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/240273002/150001
Message was sent while issue was closed.
Change committed as 266810 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
