Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798143004:
Fix for URL opening code (Closed)
DescriptionFix for tab opening code
When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user.
This fixes opening URL for cases:
1. When Chromium is not started yet.
2. When Chromium is started, has window(s).
3. When chromium app is started, but has no windows.
for different startup settings:
1. Default startup.
2. Restore session on startup.
3. Open pre-defined set of URLs on startup.
R=sky@chromium.org,gab@chromium.org,avi@chromium.org
BUG=708873
Review-Url: https://codereview.chromium.org/2798143004
Cr-Commit-Position: refs/heads/master@{#479684}
Committed: https://chromium.googlesource.com/chromium/src/+/7afbc36eeee75f34c8ad3aa2f90cb6fc1f345463
Patch Set 1 #
Total comments: 12
Patch Set 2 : Second variant, without leaky abstraction and complicated logic of WillRestoreSession() #Patch Set 3 : Fixed opening local file with Chromium on mac while restore session is enabled. File opens in the t… #
Total comments: 1
Patch Set 4 : Opening URLs in session restore now #Patch Set 5 : Removed vlog, that wasn't needed #
Total comments: 10
Patch Set 6 : Simpligied browser creation code, code style Please enter the commit message for your changes. Lin… #Patch Set 7 : Added test, fixed opening URL case when browser is already started. #
Total comments: 15
Patch Set 8 : Fixed opening URLs in various cases. #Patch Set 9 : Code style: disallow copy and assign. #
Total comments: 3
Patch Set 10 : Added tests for opening URL in different cases Chromium state: 1. Not launched. 2. Started, has win… #Patch Set 11 : Added new files for added tests. #
Total comments: 22
Patch Set 12 : Use proper profile in app_controller_mac, code style fixes. #Patch Set 13 : Simplified PRE_test checking #
Total comments: 19
Patch Set 14 : Code style fixes. #
Total comments: 4
Patch Set 15 : Fixed statistics for no tabs at startup case. #
Total comments: 6
Patch Set 16 : After startup with emty tab strip no longer reproducable case - removed it's handling #Patch Set 17 : Added comment #Patch Set 18 : Fixed class name usage. #Messages
Total messages: 52 (14 generated)
No major objections to this approach come to mind, though I'm not really an expert in this area. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:133: std::vector<GURL>* g_startup_urls = nullptr; Why would this need to be global? Can't it be an instance variable? https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool StartupBrowserCreatorImpl::WillRestoreSession( This seems like a bunch of logic that must already exist somewhere. Surely Chrome already knows that it's going to restore a session, so I worry that this will fall out of state with the logic elsewhere.
https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool StartupBrowserCreatorImpl::WillRestoreSession( On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > This seems like a bunch of logic that must already exist somewhere. Surely > Chrome already knows that it's going to restore a session, so I worry that this > will fall out of state with the logic elsewhere. That's probably true and if you can avoid touching startup_browser_creator(_impl) as much as possible that'd be great, it's already *such* a mess..! https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool StartupBrowserCreatorImpl::WillRestoreSession( On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > This seems like a bunch of logic that must already exist somewhere. Surely > Chrome already knows that it's going to restore a session, so I worry that this > will fall out of state with the logic elsewhere. That's probably true and if you can avoid touching startup_browser_creator(_impl) as much as possible that'd be great, it's already *such* a mess..!
On 2017/04/06 16:02:21, gab wrote: > https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool > StartupBrowserCreatorImpl::WillRestoreSession( > On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > > This seems like a bunch of logic that must already exist somewhere. Surely > > Chrome already knows that it's going to restore a session, so I worry that > this > > will fall out of state with the logic elsewhere. > > That's probably true and if you can avoid touching > startup_browser_creator(_impl) as much as possible that'd be great, it's already > *such* a mess..! > > https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool > StartupBrowserCreatorImpl::WillRestoreSession( > On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > > This seems like a bunch of logic that must already exist somewhere. Surely > > Chrome already knows that it's going to restore a session, so I worry that > this > > will fall out of state with the logic elsewhere. > > That's probably true and if you can avoid touching > startup_browser_creator(_impl) as much as possible that'd be great, it's already > *such* a mess..! As a general question, is this Mac-only, or do we see this on other platforms? If the former, can we take inspiration from how other platforms do it? If the latter, can you update the bug?
https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... chrome/browser/app_controller_mac.mm:1271: - (void)openUrls:(const std::vector<GURL>&)urls { I'm not familiar with the startup sequence on the mac. At what point is this called? Before or after session restore has run? https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:430: DCHECK(0); I'm not entirely sure it's valid to assume there is at least one browser. In particular starting up in background mode (or other similar things) may result in no browsers. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:436: tab_strip->AddObserver(this); Make sure you deal with case of this being deleted before TabInsertAt. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:449: VLOG(0) << "begin profiling!"; Remove VLOGs.
https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... chrome/browser/app_controller_mac.mm:1271: - (void)openUrls:(const std::vector<GURL>&)urls { On 2017/04/07 21:52:14, sky wrote: > I'm not familiar with the startup sequence on the mac. At what point is this > called? Before or after session restore has run? This happens before session restore. To be more detailed, this happens when applicationDidFinishLaunching, which happens when session restore starts run loop (but not restored session yet). https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:133: std::vector<GURL>* g_startup_urls = nullptr; On 2017/04/06 15:54:42, Avi (OOO 10-12 April) wrote: > Why would this need to be global? Can't it be an instance variable? This cannot be an instance variable, because different instances of this class are used. For opening URLSs, local StartupBrowserCreatorImpl is used: https://cs.chromium.org/chromium/src/chrome/browser/app_controller_mac.mm?rcl... For restoring session - other instance. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool StartupBrowserCreatorImpl::WillRestoreSession( On 2017/04/06 16:02:21, gab wrote: > On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > > This seems like a bunch of logic that must already exist somewhere. Surely > > Chrome already knows that it's going to restore a session, so I worry that > this > > will fall out of state with the logic elsewhere. > > That's probably true and if you can avoid touching > startup_browser_creator(_impl) as much as possible that'd be great, it's already > *such* a mess..! True, this logic was taken from conditions for calling session restore in this class: https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse... https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse... It explains what I want to change and works as a proof of concept, but I absolutely agree that this code should be significantly improved or rewritten. The problem is that, before changes, app_controller_mac creates browser and shows if necessary. Either browser creating and showing code should be changed or app_controller_mac should know if session will be restored or not (which resulted in a complicated duplicated logic in first iteration) and behave differently according to that - that's what is shown in this CL. Improving this logic looks like the Most Important point here. I'll think on what can be done here, but don't hesitate to give an advise if you have any ideas on this. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/app_controll... chrome/browser/app_controller_mac.mm:1271: - (void)openUrls:(const std::vector<GURL>&)urls { On 2017/04/07 21:52:14, sky wrote: > I'm not familiar with the startup sequence on the mac. At what point is this > called? Before or after session restore has run? This happens before session restore. To be more detailed, this happens when applicationDidFinishLaunching, which happens when session restore starts run loop (but not restored session yet). https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:133: std::vector<GURL>* g_startup_urls = nullptr; On 2017/04/06 15:54:42, Avi (OOO 10-12 April) wrote: > Why would this need to be global? Can't it be an instance variable? This cannot be an instance variable, because different instances of this class are used. For opening URLSs, local StartupBrowserCreatorImpl is used: https://cs.chromium.org/chromium/src/chrome/browser/app_controller_mac.mm?rcl... For restoring session - other instance. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:450: bool StartupBrowserCreatorImpl::WillRestoreSession( On 2017/04/06 16:02:21, gab wrote: > On 2017/04/06 15:54:42, Avi (slow and ooo 10-23 April) wrote: > > This seems like a bunch of logic that must already exist somewhere. Surely > > Chrome already knows that it's going to restore a session, so I worry that > this > > will fall out of state with the logic elsewhere. > > That's probably true and if you can avoid touching > startup_browser_creator(_impl) as much as possible that'd be great, it's already > *such* a mess..! True, this logic was taken from conditions for calling session restore in this class: https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse... https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_browse... It explains what I want to change and works as a proof of concept, but I absolutely agree that this code should be significantly improved or rewritten. The problem is that, before changes, app_controller_mac creates browser and shows if necessary. Either browser creating and showing code should be changed or app_controller_mac should know if session will be restored or not (which resulted in a complicated duplicated logic in first iteration) and behave differently according to that - that's what is shown in this CL. Improving this logic looks like the Most Important point here. I'll think on what can be done here, but don't hesitate to give an advise if you have any ideas on this.
https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:430: DCHECK(0); On 2017/04/07 21:52:14, sky wrote: > I'm not entirely sure it's valid to assume there is at least one browser. In > particular starting up in background mode (or other similar things) may result > in no browsers. Thank you for comments, overlooked this part of code. Have to fix this if the code will not be completely changed/rewritten in the first place. https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:430: DCHECK(0); On 2017/04/07 21:52:14, sky wrote: > I'm not entirely sure it's valid to assume there is at least one browser. In > particular starting up in background mode (or other similar things) may result > in no browsers. Thank you for comments, overlooked this part of code. Have to fix this if the code will not be completely changed/rewritten in the first place.
I removed previous complicated logic of WillRestoreSession() For now, I see 2 issues are left: 1) With startup setting "Open a specific page or set of pages" opening file with chromium turns broken - because opening of deferred urls is not called for this case. Have to add it, doesn't look too complicated. 2) If everything other is fine, need to fix chrome_browser_main_extra_parts_metrics.cc changes issues, kindly pointed out by sky.
Please ignore patch set 2. Have to fix that before it's worth reviewing.
Please take a look at patch set 3. Done it without adding a "copy" of complicated logic like before. Also fixed isuues in chrome_browser_main_extra_parts_metrics.cc.
Instead of using a static vector could you instead detect if we're in session restore and then ask session restore to open the urls? https://codereview.chromium.org/2798143004/diff/40001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/40001/chrome/browser/sessions... chrome/browser/sessions/session_restore.cc:751: VLOG(0) << "RestoreSession"; Is there a reason for this? At best I could see a DVLOG(1), but I don't think that's necessary either.
On 2017/04/17 17:13:34, sky wrote: > Instead of using a static vector could you instead detect if we're in session > restore and then ask session restore to open the urls? Now opening URLs in session restore. > https://codereview.chromium.org/2798143004/diff/40001/chrome/browser/sessions... > File chrome/browser/sessions/session_restore.cc (right): > > https://codereview.chromium.org/2798143004/diff/40001/chrome/browser/sessions... > chrome/browser/sessions/session_restore.cc:751: VLOG(0) << "RestoreSession"; > Is there a reason for this? At best I could see a DVLOG(1), but I don't think > that's necessary either. Removed VLOG.
Please add test coverage as well. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1286: browser = new Browser(Browser::CreateParams([self lastProfile], true)); Why do we need to open a browser here if triggering session restore? https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:361: for_each(browser_list->begin(), browser_list->end(), Using a closure is rather overkill here. Please use a typical for loop. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.cc:865: DCHECK(0) << "Failed to add urls to open for session restore"; NOTREACHED https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.h:104: static void AddURLsToOpen(const Profile* profile, Add description. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.h:105: const std::vector<GURL> urls); const std::vector<GURL>&
https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1286: browser = new Browser(Browser::CreateParams([self lastProfile], true)); On 2017/04/28 17:34:58, sky wrote: > Why do we need to open a browser here if triggering session restore? Initially I was going to change as few as possible. However, I looked at it and found no problems in not opening browser here. It would be good idea to check that it doesn't break tests too - may I ask you to to send email to committers@chromium.org regarding tryjob access for me? https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:361: for_each(browser_list->begin(), browser_list->end(), On 2017/04/28 17:34:58, sky wrote: > Using a closure is rather overkill here. Please use a typical for loop. Done. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.cc:865: DCHECK(0) << "Failed to add urls to open for session restore"; On 2017/04/28 17:34:58, sky wrote: > NOTREACHED Done. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.h:104: static void AddURLsToOpen(const Profile* profile, On 2017/04/28 17:34:58, sky wrote: > Add description. Done. https://codereview.chromium.org/2798143004/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore.h:105: const std::vector<GURL> urls); On 2017/04/28 17:34:59, sky wrote: > const std::vector<GURL>& Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1672: tab_strip->GetWebContentsAt(1)->GetURL().possibly_invalid_spec()); Added test.
Avi should review the app_controller_mac.mm changes https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:361: for (auto* browser : *browser_list) { no {} optional: don't use a local for browser_list, inline it. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore.h:104: // Add |urls| to URLs-t-open list, so that theey will be opened after session URLs-to-open theey->they https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1633: bool BeginsWith(const Container& container, const Container& prefix) { Why are you using a template here? Can't you use substr()? https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1654: }; private: DISALLOW... https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1666: // ui_test_utils::NavigateToURL(browser(),GURL(kSessionURL)); Why is this commented out?
https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1294: // if no browser window exists then create one with no tabs to be filled in While you're here, turn this into a sentence with a capital letter at the beginning and a period at the end. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1295: if (!browser && pref.type != SessionStartupPref::LAST) { I'm not clear on this line. So if there is no browser open, but we are doing LAST, then we pass a nullptr into launch.OpenURLsInBrowser? It's only going to end up creating a browser window itself...
Fixed issues that were kindly pointed out here. Opening URLs logic for different cases made more correct now. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1294: // if no browser window exists then create one with no tabs to be filled in On 2017/05/04 23:47:43, Avi (ping after 24h) wrote: > While you're here, turn this into a sentence with a capital letter at the > beginning and a period at the end. Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1295: if (!browser && pref.type != SessionStartupPref::LAST) { On 2017/05/04 23:47:43, Avi (ping after 24h) wrote: > I'm not clear on this line. So if there is no browser open, but we are doing > LAST, then we pass a nullptr into launch.OpenURLsInBrowser? It's only going to > end up creating a browser window itself... Fixed this logic, so that it now handles 3 open URL cases: 1) Chromium is already started (with browser window and existing tabs). 2) Chromium app is started, but no windows. 3)Chromium is not started when opening URL. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:361: for (auto* browser : *browser_list) { On 2017/05/04 23:40:46, sky wrote: > no {} > optional: don't use a local for browser_list, inline it. Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore.h:104: // Add |urls| to URLs-t-open list, so that theey will be opened after session On 2017/05/04 23:40:46, sky wrote: > URLs-to-open > theey->they Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1633: bool BeginsWith(const Container& container, const Container& prefix) { On 2017/05/04 23:40:46, sky wrote: > Why are you using a template here? Can't you use substr()? Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1654: }; On 2017/05/04 23:40:46, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2798143004/diff/120001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1666: // ui_test_utils::NavigateToURL(browser(),GURL(kSessionURL)); On 2017/05/04 23:40:46, sky wrote: > Why is this commented out? Done.
Ping Avi.
https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1297: Profile* profile = [self lastProfile]; You've tested this with multiple profile setups? https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:859: it != active_session_restorers->end(); ++it) { for (const auto* restorer : active_session_restorers) https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/160001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1674: } You need to test all the branches. In the Mac code you have - not SessionStartupPref::LAST - SessionStartupPref::LAST and in the middle of restoration - SessionStartupPref::LAST, not in the middle of restoration, and there's a browser window - SessionStartupPref::LAST, not in the middle of restoration, and there's no browser window
BTW I don't mean to be a hard reviewer, but touching this code scares me. It's very old, very tricky, and has low test coverage. That's why I'm asking for all the tests, so that we can all be reassured about this working correctly.
On 2017/05/17 16:04:53, Avi (ping after 24h) wrote: > BTW I don't mean to be a hard reviewer, but touching this code scares me. It's > very old, very tricky, and has low test coverage. That's why I'm asking for all > the tests, so that we can all be reassured about this working correctly. I agree that tests are good for this code, so here they are. Please take a look.
Lots of comment typo nits (sorry), and an interesting fact that I learned in app_controller_mac, but otherwise it looks pretty good. (And still scary; good luck!) LGTM with nits. Other reviewers, what do you think? https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1319: browser = new Browser(Browser::CreateParams([self lastProfile], true)); I just learned this, but you need to use [self safeLastProfileForNewWindows] instead, otherwise you'll crash in Guest Mode. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1330: browser = new Browser(Browser::CreateParams([self lastProfile], true)); Same here. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:416: // PRE test should only prepasre session-to-restore. typo: prepare https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:419: std::string pre_ptefix = "PRE_"; typo: prefix https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:420: if (test_name.substr(0, pre_ptefix.length()) != pre_ptefix) { I would have thought there's a better way to test, that the test harness already did this check and set some flag or whatnot. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:458: // This test checks that when AppleEvent on opening URL is recieved during typo: received https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:473: // Open specific set o pages startup setting - expect that set of pages. typo: of pages https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:528: // This test checks that when AppleEvent on opening URL is recieved by already typo: received https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:555: // This test checks that when AppleEvent on opening URL is recieved by typo: received https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:858: for (std::set<SessionRestoreImpl*>::const_iterator it = at least use for (auto it =, perhaps switch to for (auto* session_restorer : active_session_restorers) https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1647: std::string pre_ptefix = "PRE_"; typo: prefix
Description was changed from ========== Initial variant, fix for tab opening code A draft code to fix tab opening at the same time when session is restored. I see that code still needs some re-styling and simlifying, just whan to know if working is this direction is ok or not? When opening URLs in browser, behave differently with session restore enabled - remember those URLs in global variable in startup_browser_ creator_impl (because different instances of it are used to open URLs and to restore session). And open those URLs after session is restored. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 ========== to ========== Fix for tab opening code When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user. This fixes opening URL for cases: 1. When Chromium is not started yet. 2. When Chromium is started, has window(s). 3. When chromium app is started, but has no windows. fort different startup settings: 1. Default startup. 2. Restore session on startup. 3. Open pre-defined set of URLs on startup. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 ==========
https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1319: browser = new Browser(Browser::CreateParams([self lastProfile], true)); On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > I just learned this, but you need to use [self safeLastProfileForNewWindows] > instead, otherwise you'll crash in Guest Mode. Done. Thank you for sharing that! https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1330: browser = new Browser(Browser::CreateParams([self lastProfile], true)); On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > Same here. Done https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:416: // PRE test should only prepasre session-to-restore. On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: prepare Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:419: std::string pre_ptefix = "PRE_"; On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: prefix Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:420: if (test_name.substr(0, pre_ptefix.length()) != pre_ptefix) { On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > I would have thought there's a better way to test, that the test harness already > did this check and set some flag or whatnot. Test launcher itself does detect pre-tests by prefix in name, here is that code: https://chromium.googlesource.com/chromium/src/+/master/content/public/test/t... And doesn't seem to set any flag which cold be used later. So I check that condition the same way as test_launcher.cc does. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:458: // This test checks that when AppleEvent on opening URL is recieved during On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: received Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:473: // Open specific set o pages startup setting - expect that set of pages. On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: of pages Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:528: // This test checks that when AppleEvent on opening URL is recieved by already On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: received Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:555: // This test checks that when AppleEvent on opening URL is recieved by On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: received Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore.cc (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore.cc:858: for (std::set<SessionRestoreImpl*>::const_iterator it = On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > at least use for (auto it =, perhaps switch to for (auto* session_restorer : > active_session_restorers) Done. https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2798143004/diff/200001/chrome/browser/session... chrome/browser/sessions/session_restore_browsertest.cc:1647: std::string pre_ptefix = "PRE_"; On 2017/06/01 19:54:31, Avi (ping after 24h) wrote: > typo: prefix Done.
Ping sky. I would be really glad to know what do you think about the fix and tests in this CL. Typos/nits that Avi kindly pointed out are fixed. Sorry for typos.
This mostly looks good, just minor questions and running new test on all desktop platforms. https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:407: class AppControllerOpenShortcutOnStartupBrowserTest What is mac specific about this test? It seems like it applies to all desktop platforms. https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:522: PRE_OpenShortcutInBrwowserWithWindow) { Looks like you misspelled browser in a bunch of places here. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... File content/public/test/repeated_notification_observer.cc (right): https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.cc:24: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, Can't you call run_loop_.Quit (or QuitWhenIdle()) directly? https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.cc:33: running_ = true; base::AutoReset. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... File content/public/test/repeated_notification_observer.h (right): https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) (see style guide). https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:5: #ifndef CONTENT_PUBLIC_BROWSER_REPEATED_NOTIFICATION_OBSERVER_H_ guard doesn't match file name. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:18: explicit RepeatedNotificationObserver(int type, int count); no explicit https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:20: void Observe(int type, Prefix with where override comes from, e.g. // NotificationObserver: https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:23: void Wait(); Add description.
https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:407: class AppControllerOpenShortcutOnStartupBrowserTest On 2017/06/08 19:17:49, sky wrote: > What is mac specific about this test? It seems like it applies to all desktop > platforms. Implementation (the way URLs/shortcuts are opened) is Mac-specific. This class inherits AppControllerOpenShortcutBrowserTest for opening shortcut, which uses objective-C class TestOpenShortcutOnStartup,that sends Apple Event: https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/app_co... Checking all these cases on all desktop platform looks like a good idea. There are no enabled all-desktop tests for these cases - otherwise they would fail due to bug that I'm fixing here. But they are not needed to provide test cover for changes int this issue - so it looks like It would be better to do that as separate task, not here. https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:522: PRE_OpenShortcutInBrwowserWithWindow) { On 2017/06/08 19:17:49, sky wrote: > Looks like you misspelled browser in a bunch of places here. I'm sorry. fixed those typos. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... File content/public/test/repeated_notification_observer.cc (right): https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.cc:33: running_ = true; On 2017/06/08 19:17:49, sky wrote: > base::AutoReset. Done. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... File content/public/test/repeated_notification_observer.h (right): https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/08 19:17:49, sky wrote: > no (c) (see style guide). Done. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:5: #ifndef CONTENT_PUBLIC_BROWSER_REPEATED_NOTIFICATION_OBSERVER_H_ On 2017/06/08 19:17:49, sky wrote: > guard doesn't match file name. Done. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:18: explicit RepeatedNotificationObserver(int type, int count); On 2017/06/08 19:17:49, sky wrote: > no explicit Done. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:20: void Observe(int type, On 2017/06/08 19:17:49, sky wrote: > Prefix with where override comes from, e.g. > // NotificationObserver: Done. https://codereview.chromium.org/2798143004/diff/240001/content/public/test/re... content/public/test/repeated_notification_observer.h:23: void Wait(); On 2017/06/08 19:17:49, sky wrote: > Add description. Done.
LGTM https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... File chrome/browser/app_controller_mac_browsertest.mm (right): https://codereview.chromium.org/2798143004/diff/240001/chrome/browser/app_con... chrome/browser/app_controller_mac_browsertest.mm:407: class AppControllerOpenShortcutOnStartupBrowserTest On 2017/06/09 13:16:22, eugenebng wrote: > On 2017/06/08 19:17:49, sky wrote: > > What is mac specific about this test? It seems like it applies to all desktop > > platforms. > > Implementation (the way URLs/shortcuts are opened) is Mac-specific. This class > inherits AppControllerOpenShortcutBrowserTest for opening shortcut, which uses > objective-C class TestOpenShortcutOnStartup,that sends Apple Event: > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/app_co... > > Checking all these cases on all desktop platform looks like a good idea. There > are no enabled all-desktop tests for these cases - otherwise they would fail due > to bug that I'm fixing here. > But they are not needed to provide test cover for changes int this issue - so it > looks like It would be better to do that as separate task, not here. I didn't realize the base has obective-c specifics. I agree, keep this class here.
Ping gab.
Description was changed from ========== Fix for tab opening code When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user. This fixes opening URL for cases: 1. When Chromium is not started yet. 2. When Chromium is started, has window(s). 3. When chromium app is started, but has no windows. fort different startup settings: 1. Default startup. 2. Restore session on startup. 3. Open pre-defined set of URLs on startup. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 ========== to ========== Fix for tab opening code When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user. This fixes opening URL for cases: 1. When Chromium is not started yet. 2. When Chromium is started, has window(s). 3. When chromium app is started, but has no windows. for different startup settings: 1. Default startup. 2. Restore session on startup. 3. Open pre-defined set of URLs on startup. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 ==========
https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1310: SessionRestore::AddURLsToOpen(profile, urls); Is this use case specific to Mac? Launching through apps/URLs first is a think on a Linux/Windows/Mac isn't it? Would expect this logic to be in cross-platform code. https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:526: tab_strip->AddObserver(this); This isn't correct as first web contents profiling is based on process start timing. Instead you should notify startup metrics that the signal is no good through startup_metric_utils::SetNonBrowserUIDisplayed().
https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1310: SessionRestore::AddURLsToOpen(profile, urls); On 2017/06/09 16:59:19, gab wrote: > Is this use case specific to Mac? Launching through apps/URLs first is a think > on a Linux/Windows/Mac isn't it? Would expect this logic to be in cross-platform > code. URLs to open come via apple events (see test code that emulates it), that's specific about mac. Because URLs to open come via events, the context (when openUrls is called) is specific too. https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:526: tab_strip->AddObserver(this); On 2017/06/09 16:59:19, gab wrote: > This isn't correct as first web contents profiling is based on process start > timing. Instead you should notify startup metrics that the signal is no good > through startup_metric_utils::SetNonBrowserUIDisplayed(). Done.
lgtm w/ nitsq https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:524: startup_metric_utils::SetNonBrowserUIDisplayed(); Add a comment about when this happens https://codereview.chromium.org/2798143004/diff/280001/content/public/test/re... File content/public/test/repeated_notification_observer.h (right): https://codereview.chromium.org/2798143004/diff/280001/content/public/test/re... content/public/test/repeated_notification_observer.h:25: // Wait unitl |count| events of |type| (both specified in constructor) happen. "until"
https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:524: startup_metric_utils::SetNonBrowserUIDisplayed(); On 2017/06/13 16:29:34, gab wrote: > Add a comment about when this happens It was happening when I started this fix. Now I can't reproduce this odd case manually or in tests, so DCHECK against it now. https://codereview.chromium.org/2798143004/diff/280001/content/public/test/re... File content/public/test/repeated_notification_observer.h (right): https://codereview.chromium.org/2798143004/diff/280001/content/public/test/re... content/public/test/repeated_notification_observer.h:25: // Wait unitl |count| events of |type| (both specified in constructor) happen. On 2017/06/13 16:29:34, gab wrote: > "until" Done.
https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:524: startup_metric_utils::SetNonBrowserUIDisplayed(); On 2017/06/14 11:17:46, eugenebng wrote: > On 2017/06/13 16:29:34, gab wrote: > > Add a comment about when this happens > > It was happening when I started this fix. Now I can't reproduce this odd case > manually or in tests, so DCHECK against it now. Ok but I'd say keep a comment referring to this discussion in case someone hits the DCHECK: https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics...
https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:524: startup_metric_utils::SetNonBrowserUIDisplayed(); On 2017/06/14 17:03:23, gab wrote: > On 2017/06/14 11:17:46, eugenebng wrote: > > On 2017/06/13 16:29:34, gab wrote: > > > Add a comment about when this happens > > > > It was happening when I started this fix. Now I can't reproduce this odd case > > manually or in tests, so DCHECK against it now. > > Ok but I'd say keep a comment referring to this discussion in case someone hits > the DCHECK: > https://codereview.chromium.org/2798143004/diff/280001/chrome/browser/metrics... Added comment.
The CQ bit was checked by eugenebng@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eugenebng@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eugenebng@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, sky@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2798143004/#ps340001 (title: "Fixed class name usage.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1497533651662370,
"parent_rev": "1cd708aa51e84e3aac45da27f4e96c96218ff5f5", "commit_rev":
"7afbc36eeee75f34c8ad3aa2f90cb6fc1f345463"}
Message was sent while issue was closed.
Description was changed from ========== Fix for tab opening code When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user. This fixes opening URL for cases: 1. When Chromium is not started yet. 2. When Chromium is started, has window(s). 3. When chromium app is started, but has no windows. for different startup settings: 1. Default startup. 2. Restore session on startup. 3. Open pre-defined set of URLs on startup. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 ========== to ========== Fix for tab opening code When URL is opened from other program or local file system and session restored at the same time, opened URL is shown in a window behind restore session and not really visible to user. This fixes opening URL for cases: 1. When Chromium is not started yet. 2. When Chromium is started, has window(s). 3. When chromium app is started, but has no windows. for different startup settings: 1. Default startup. 2. Restore session on startup. 3. Open pre-defined set of URLs on startup. R=sky@chromium.org,gab@chromium.org,avi@chromium.org BUG=708873 Review-Url: https://codereview.chromium.org/2798143004 Cr-Commit-Position: refs/heads/master@{#479684} Committed: https://chromium.googlesource.com/chromium/src/+/7afbc36eeee75f34c8ad3aa2f90c... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/7afbc36eeee75f34c8ad3aa2f90c...
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/2964163002/ by dominicc@chromium.org. The reason for reverting is: Reverting this per the comments on crbug.com/735958.
Message was sent while issue was closed.
On 2017/06/30 00:56:43, dominicc (has gone to gerrit) wrote: > A revert of this CL (patchset #18 id:340001) has been created in > https://codereview.chromium.org/2964163002/ by mailto:dominicc@chromium.org. > > The reason for reverting is: Reverting this per the comments on > crbug.com/735958. This was really reverted here: https://chromium-review.googlesource.com/c/567562/ . |
