Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088443003:
Shortcut ctrl+shift+T added on android. (Closed)
|
Created:
4 years, 6 months ago by xingliu Modified:
4 years, 5 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Ted C, David Trainor- moved to gerrit Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShortcut ctrl+shift+T added on android.
This CL first check if we can undo the recent closure in java by using
TabModelImpl.
If nothing we can do with undo snackbar logic, then use
recently_closed_tabs_bridge in native code to retrive data from
tab_restore_service.
BUG=602559
https://bugs.chromium.org/p/chromium/issues/detail?id=602559#c7
Committed: https://crrev.com/6fbb24a66cb8bb459fed7a4db23428620edb9048
Cr-Commit-Position: refs/heads/master@{#405624}
Patch Set 1 #Patch Set 2 : Shortcut ctrl+shift+T added on android. #Patch Set 3 : Shortcut ctrl+shift+T added on android, similar to desktop chrome. #Patch Set 4 : fix issues based on feedback. #
Total comments: 7
Patch Set 5 : Do not use content platform data, still need to fix the tab model selection hack. #Patch Set 6 : set window id for android into TabRestoreService::Tab.browser_id, to select the correct android tab… #
Total comments: 1
Patch Set 7 : use entries() to access tab restore data. change const to non const #Patch Set 8 : not to modify session restore component, need to figure out a better way to find correct window dur… #Patch Set 9 : Wire android tab model with LiveTabContext, so when access tab restore service, we don't hurt its c… #
Total comments: 11
Patch Set 10 : fixes based on code review feedbacks. #Patch Set 11 : add incognito check in native code, based on code review feedback. #
Total comments: 40
Patch Set 12 : fixes based on review feedback. #
Total comments: 65
Patch Set 13 : Specify sdk version on test case to make try bot happy. #Patch Set 14 : Fixes based on review feedback. #Patch Set 15 : remove commented code, my bad. #
Total comments: 6
Patch Set 16 : Improve the quality of multi-window test case, also refactor some test code. #Patch Set 17 : Refactor some code to android_live_tab_context from recently_closed_tabs_bridge #
Total comments: 7
Patch Set 18 : Refactor more code to android_tab_context, based on new review feedback, now it looks much cleaner. #Patch Set 19 : Removed useless comment in code. #Patch Set 20 : Removed unused includes in recently_closed_tab_bridge.cc #
Total comments: 40
Patch Set 21 : One more test case on Multiwindow, fixes for review comments. #Patch Set 22 : Change the tab back to restore in background by default. Add special code to fix the 'only tab' iss… #Patch Set 23 : Fix test code for changing back the selection behavior. #
Total comments: 15
Patch Set 24 : UMA added correctly, nit changes. #
Total comments: 4
Patch Set 25 : Rebase master, and minor changes. #
Total comments: 3
Patch Set 26 : Nit fixes based on review. #
Total comments: 5
Patch Set 27 : nit fixes based on review. #Messages
Total messages: 151 (68 generated)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2088443003/20001
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 xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2088443003/40001
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org
nyquist@chromium.org: Please review changes in dtrainor@chromium.org: Please review changes in
xingliu@chromium.org changed reviewers: + shaktisahu@chromium.org
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 xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2088443003/60001
xingliu@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Hi, Please review changes in components/sessions/content/content_platform_specific_tab_data.h components/sessions/content/content_platform_specific_tab_data.cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... File components/sessions/content/content_platform_specific_tab_data.h (right): https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:35: return window_id_; Can you elaborate on why you need to add this here? In general I would prefer to keep this class at just the parts needed by session restore. AFAICT the window_id_ isn't generally needed by components/session. I you need to associated arbitrary data with WebContents, use SupportsUserData. https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:38: void SetWindowId(SessionID::id_type id) { set_window_id https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:49: SessionID::id_type window_id_; This needs to be initialized in the member initialize list.
https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... File components/sessions/content/content_platform_specific_tab_data.h (right): https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:35: return window_id_; On 2016/06/24 15:54:22, sky wrote: > Can you elaborate on why you need to add this here? In general I would prefer to > keep this class at just the parts needed by session restore. AFAICT the > window_id_ isn't generally needed by components/session. I you need to > associated arbitrary data with WebContents, use SupportsUserData. Thank you for the feedback, The window_id here is used by android chrome to write the window id into TabRestoreService::Tab. So during restoring, it can find the correct window. TabRestoreService::Tab.browser_id is not used by android since LiveTabContext is bypassed. The place where Tab.browser_id is bypassed on android is TabRestoreServiceHelper::PopulateTab line 383. The reason to read window id from TabRestoreService::Tab is that we're trying to restore a tab when no parent tab context is given, so it can't find the window id or browser id from WebContents in any existing tab. https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:38: void SetWindowId(SessionID::id_type id) { On 2016/06/24 15:54:22, sky wrote: > set_window_id Will be fixed shortly. https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:49: SessionID::id_type window_id_; On 2016/06/24 15:54:22, sky wrote: > This needs to be initialized in the member initialize list. Will be fixed shortly.
Hi, just update comments in content_platform_specific_tab_data.h to elaborate reason to write data here, also propose another way to do this, please feel free to add comment. https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... File components/sessions/content/content_platform_specific_tab_data.h (right): https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... components/sessions/content/content_platform_specific_tab_data.h:35: return window_id_; On 2016/06/24 15:54:22, sky wrote: > Can you elaborate on why you need to add this here? In general I would prefer to > keep this class at just the parts needed by session restore. AFAICT the > window_id_ isn't generally needed by components/session. I you need to > associated arbitrary data with WebContents, use SupportsUserData. Or I can add function to access TabRestoreService::Tab entry from external class to write the window id into TabRestoreService::Tab.browser_id, if this makes more sense.
On 2016/06/24 17:52:30, xingliu wrote: > Hi, just update comments in content_platform_specific_tab_data.h to elaborate > reason to write data here, also propose another way to do this, please feel free > to add comment. > > https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... > File components/sessions/content/content_platform_specific_tab_data.h (right): > > https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... > components/sessions/content/content_platform_specific_tab_data.h:35: return > window_id_; > On 2016/06/24 15:54:22, sky wrote: > > Can you elaborate on why you need to add this here? In general I would prefer > to > > keep this class at just the parts needed by session restore. AFAICT the > > window_id_ isn't generally needed by components/session. I you need to > > associated arbitrary data with WebContents, use SupportsUserData. > > Or I can add function to access TabRestoreService::Tab entry from external class > to write the window id into TabRestoreService::Tab.browser_id, if this makes > more sense. It looks like you removed the changes from components, so I'm happy. I don't know enough about the android side to comment on the changes there.
The CQ bit was checked by xingliu@chromium.org 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...
On 2016/06/24 20:02:44, sky wrote: > On 2016/06/24 17:52:30, xingliu wrote: > > Hi, just update comments in content_platform_specific_tab_data.h to elaborate > > reason to write data here, also propose another way to do this, please feel > free > > to add comment. > > > > > https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... > > File components/sessions/content/content_platform_specific_tab_data.h (right): > > > > > https://codereview.chromium.org/2088443003/diff/60001/components/sessions/con... > > components/sessions/content/content_platform_specific_tab_data.h:35: return > > window_id_; > > On 2016/06/24 15:54:22, sky wrote: > > > Can you elaborate on why you need to add this here? In general I would > prefer > > to > > > keep this class at just the parts needed by session restore. AFAICT the > > > window_id_ isn't generally needed by components/session. I you need to > > > associated arbitrary data with WebContents, use SupportsUserData. > > > > Or I can add function to access TabRestoreService::Tab entry from external > class > > to write the window id into TabRestoreService::Tab.browser_id, if this makes > > more sense. > > It looks like you removed the changes from components, so I'm happy. I don't > know enough about the android side to comment on the changes there. Thank you for the great review, in order to fix an issue on the android site, I add a function to access most recent Entry in tab restore service, please help to take a look if this change make sense. The purpose is to write the android window id into Tab.browser_id(previously not used by android).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... File components/sessions/core/persistent_tab_restore_service.h (right): https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... components/sessions/core/persistent_tab_restore_service.h:38: const Entries& entries() const override; Why can't you use entries()?
On 2016/06/24 22:25:47, sky wrote: > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > File components/sessions/core/persistent_tab_restore_service.h (right): > > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > components/sessions/core/persistent_tab_restore_service.h:38: const Entries& > entries() const override; > Why can't you use entries()? I see that's const method but I'm trying to modify only the first entry in the list, if it's ok, I can make entries() non-const off course.
On 2016/06/24 22:25:47, sky wrote: > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > File components/sessions/core/persistent_tab_restore_service.h (right): > > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > components/sessions/core/persistent_tab_restore_service.h:38: const Entries& > entries() const override; > Why can't you use entries()? I see that's const method but I'm trying to modify only the first entry in the list, if it's ok, I can make entries() non-const off course.
On 2016/06/24 23:22:18, xingliu wrote: > On 2016/06/24 22:25:47, sky wrote: > > > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > > File components/sessions/core/persistent_tab_restore_service.h (right): > > > > > https://codereview.chromium.org/2088443003/diff/100001/components/sessions/co... > > components/sessions/core/persistent_tab_restore_service.h:38: const Entries& > > entries() const override; > > Why can't you use entries()? > > I see that's const method but I'm trying to modify only the first entry in the > list, if it's ok, I can make entries() non-const off course. It seems like you need this to modify Tab::browser_id. Tab::browser_id is populated from LiveTabContext. If you need custom logic can't you create your own LiveTabContext? The reason the entries are not exposed as mutable is that so that random of parts of the code can't alter entries and impact other places trying to restore or use the entries. The right place to change the browser_id is at the time of creation of the Entry.
xingliu@chromium.org changed reviewers: + twellington@chromium.org - shaktisahu@chromium.org
Just remove any code in session restore component, so there is still an issue where it's awkward to find the correct window id during session restore.
Can't you provide your own LiveTabContext? On Mon, Jun 27, 2016 at 10:54 AM, <xingliu@chromium.org> wrote: > Just remove any code in session restore component, so there is still an > issue > where it's awkward to find the correct window id during session restore. > > https://codereview.chromium.org/2088443003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/27 20:36:34, sky wrote: > Can't you provide your own LiveTabContext? > > On Mon, Jun 27, 2016 at 10:54 AM, <mailto:xingliu@chromium.org> wrote: > > Just remove any code in session restore component, so there is still an > > issue > > where it's awkward to find the correct window id during session restore. > > > > https://codereview.chromium.org/2088443003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I think android don't use LiveTabContext, in chrome_tab_restore_service_client.cc line 54.
I would investigate wiring it up. On Mon, Jun 27, 2016 at 2:30 PM, <xingliu@chromium.org> wrote: > On 2016/06/27 20:36:34, sky wrote: >> Can't you provide your own LiveTabContext? >> >> On Mon, Jun 27, 2016 at 10:54 AM, <mailto:xingliu@chromium.org> wrote: >> > Just remove any code in session restore component, so there is still an >> > issue >> > where it's awkward to find the correct window id during session restore. >> > >> > https://codereview.chromium.org/2088443003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I think android don't use LiveTabContext, in > chrome_tab_restore_service_client.cc line 54. > > https://codereview.chromium.org/2088443003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/27 21:57:34, sky wrote: > I would investigate wiring it up. > > On Mon, Jun 27, 2016 at 2:30 PM, <mailto:xingliu@chromium.org> wrote: > > On 2016/06/27 20:36:34, sky wrote: > >> Can't you provide your own LiveTabContext? > >> > >> On Mon, Jun 27, 2016 at 10:54 AM, <mailto:xingliu@chromium.org> wrote: > >> > Just remove any code in session restore component, so there is still an > >> > issue > >> > where it's awkward to find the correct window id during session restore. > >> > > >> > https://codereview.chromium.org/2088443003/ > >> > >> -- > >> You received this message because you are subscribed to the Google Groups > >> "Chromium-reviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send an > > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > I think android don't use LiveTabContext, in > > chrome_tab_restore_service_client.cc line 54. > > > > https://codereview.chromium.org/2088443003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Hi, thank you for the comments, I'll implement an android version of LiveTabContext, should I put it in another CL or in this CL?
It's own cl SGTM On Mon, Jun 27, 2016 at 5:12 PM, <xingliu@chromium.org> wrote: > On 2016/06/27 21:57:34, sky wrote: >> I would investigate wiring it up. >> >> On Mon, Jun 27, 2016 at 2:30 PM, <mailto:xingliu@chromium.org> wrote: >> > On 2016/06/27 20:36:34, sky wrote: >> >> Can't you provide your own LiveTabContext? >> >> >> >> On Mon, Jun 27, 2016 at 10:54 AM, <mailto:xingliu@chromium.org> wrote: >> >> > Just remove any code in session restore component, so there is still >> >> > an >> >> > issue >> >> > where it's awkward to find the correct window id during session >> >> > restore. >> >> > >> >> > https://codereview.chromium.org/2088443003/ >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "Chromium-reviews" group. >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> > >> > I think android don't use LiveTabContext, in >> > chrome_tab_restore_service_client.cc line 54. >> > >> > https://codereview.chromium.org/2088443003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Hi, thank you for the comments, I'll implement an android version of > LiveTabContext, should I put it in another CL or in this CL? > > > https://codereview.chromium.org/2088443003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by xingliu@chromium.org 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...
xingliu@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi, please help to review. This cl implements the short cut for restore, also wires LiveTabContext with TabModel, to access tab restore service.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not a good reviewer of the android files. Is there a specific file you wanted me to look at? https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:27: return ""; std::string() https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:72: sessions::LiveTabContext* AndroidLiveTabContext::FindContextForWebContents( // static https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.h (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) (see style guide). https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:26: : tab_model_(tab_model) { Don't inline constructor/destructor (see style guide).
No, but thank you for the review, I just wired up the LiveTabContext with some android class, it makes sense and don't break the tab restore service code elegance. Thank you. On 2016/06/28 19:13:37, sky wrote: > I'm not a good reviewer of the android files. Is there a specific file you > wanted me to look at? > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > chrome/browser/ui/android/tab_model/android_live_tab_context.cc:27: return ""; > std::string() > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > chrome/browser/ui/android/tab_model/android_live_tab_context.cc:72: > sessions::LiveTabContext* AndroidLiveTabContext::FindContextForWebContents( > // static > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > File chrome/browser/ui/android/tab_model/android_live_tab_context.h (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > chrome/browser/ui/android/tab_model/android_live_tab_context.h:1: // Copyright > (c) 2016 The Chromium Authors. All rights reserved. > nit: no (c) (see style guide). > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... > chrome/browser/ui/android/tab_model/android_live_tab_context.h:26: : > tab_model_(tab_model) { > Don't inline constructor/destructor (see style guide).
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) anymore https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:37: return nullptr; This "seems" like something we could add right now. BrowserLiveTabContext: return sessions::ContentLiveTab::GetForWebContents( browser_->tab_strip_model()->GetActiveWebContents()); Our version: return sessions::ContentLiveTab::GetForWebContents( tab_model_->GetActiveWebContents()); Same general comment as above https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:41: return false; Add a comment that Android does not support the concept of pinned tabs. https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:69: void AndroidLiveTabContext::CloseTab() { We should add NOTIMPLEMENTED() in places like this so we can better track it
https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:148: using RestoreTab = const sessions::TabRestoreService::Tab; The benefit of implementing AndroidLiveTabContext is so that we can use TabRestoreService::RestoreMostRecentEntry instead of writing our own logic here. RestoreMostRecentEntry calls into TabRestoreService::RestoreTab, which will call into LiveTabContext::AddRestoredTab(). I think we should be able to just hook in there if there's any extra work not already done by the TabRestoreService itself. https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... chrome/browser/sessions/chrome_tab_restore_service_client.cc:57: // implemented in java. We should double check that this can't get called (and cause weird issues or crashes) once we start using TabRestoreServiceHelper. We'll probably want to safe-guard this with NOTREACHED(); If we do need to implement it, we can look at ways to create the LiveTabContext on the fly (it looks like we just need to get the TabModel somehow).
I think we basically only use data in tab restore service instead of directly call key logic(like RestoreEntryById), this needs some refactor so probably in separate CL. On 2016/06/28 20:47:09, Theresa Wellington wrote: > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... > File chrome/browser/android/recently_closed_tabs_bridge.cc (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... > chrome/browser/android/recently_closed_tabs_bridge.cc:148: using RestoreTab = > const sessions::TabRestoreService::Tab; > The benefit of implementing AndroidLiveTabContext is so that we can use > TabRestoreService::RestoreMostRecentEntry instead of writing our own logic here. > > RestoreMostRecentEntry calls into TabRestoreService::RestoreTab, which will call > into LiveTabContext::AddRestoredTab(). I think we should be able to just hook in > there if there's any extra work not already done by the TabRestoreService > itself. > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... > File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... > chrome/browser/sessions/chrome_tab_restore_service_client.cc:57: // implemented > in java. > We should double check that this can't get called (and cause weird issues or > crashes) once we start using TabRestoreServiceHelper. We'll probably want to > safe-guard this with NOTREACHED(); > > If we do need to implement it, we can look at ways to create the LiveTabContext > on the fly (it looks like we just need to get the TabModel somehow).
I think we basically only use data in tab restore service instead of directly call key logic(like RestoreEntryById), this needs some refactor so probably in separate CL. On 2016/06/28 20:47:09, Theresa Wellington wrote: > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... > File chrome/browser/android/recently_closed_tabs_bridge.cc (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... > chrome/browser/android/recently_closed_tabs_bridge.cc:148: using RestoreTab = > const sessions::TabRestoreService::Tab; > The benefit of implementing AndroidLiveTabContext is so that we can use > TabRestoreService::RestoreMostRecentEntry instead of writing our own logic here. > > RestoreMostRecentEntry calls into TabRestoreService::RestoreTab, which will call > into LiveTabContext::AddRestoredTab(). I think we should be able to just hook in > there if there's any extra work not already done by the TabRestoreService > itself. > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... > File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): > > https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/session... > chrome/browser/sessions/chrome_tab_restore_service_client.cc:57: // implemented > in java. > We should double check that this can't get called (and cause weird issues or > crashes) once we start using TabRestoreServiceHelper. We'll probably want to > safe-guard this with NOTREACHED(); > > If we do need to implement it, we can look at ways to create the LiveTabContext > on the fly (it looks like we just need to get the TabModel somehow).
The CQ bit was checked by xingliu@chromium.org 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.
I played around with this on my Nexus 9 with multi-window and overall it seems to be working great! One thing that was unexpected was that it looks like tabs re-opened while the undo bar is showing are opened in the background and tabs re-opened while it's not showing are opened in the foreground. I think all tabs opened with ctrl+shift+t should either open in the foreground (like desktop) or background (like undo bar). I'll finish reviewing the native code in the morning. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:58: <item type="id" name="open_recent_closed_tab" /> nit: open_recently_closed_tab https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:128: activity.onMenuOrKeyboardAction(R.id.open_recent_closed_tab, false); Let's have this just call into the activity and the activity itself can choose whether to or not to handle the keyboard input based on if the current model is incognito. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:49: public void openRecentClosedTab() { nit: openRecentlyClosedTab() This also needs a JavaDoc. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:151: * Open recent closed tab. I think we could flesh this out more, e.g. Opens the most recently closed tab, bringing the tab back into this model. If no tabs are currently in the rewound list, the native... https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:153: public void openRecentClosedTab(); nit: s/recent/recently https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:727: TabLaunchType.FROM_LONGPRESS_FOREGROUND); I suspect that changing this affects other places that were originally relying on this method to open things in the background. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:751: // recover tab from rewind list in java, same as the undo snackbar. nit: use proper sentence capitalization/punctuation for comments e.g. "First try to recover the tab from the rewound list." http://source.android.com/source/code-style.html#use-javadoc-standard-comments https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:760: // if no pending closure in java, open it from tab restore service in native code. nit: "If there are no pending closures in Java, ..." https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:338: private void openRecentClosedTabOnUiThread(final TabModelSelector selector) { nit: openRecentlyClosedTabOnUiThread https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1438: * Test open recently closed tab which uses rewinding list in java. nit: Test opening recently closed tabs using the rewound list in Java. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1444: public void testOpenRecentClosedTab() throws InterruptedException { nit: testOpenRecentlyClosedTab() https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1449: createTabOnUiThread(tabCreator, TEST_URL); This test doesn't rely on native, so I think we should just create a normal tab rather than one with TEST_URL. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1463: // helper class to wait for fully loaded page. nit: Helper class that notifies when a page load is finished. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1464: private class TestTabObserver extends EmptyTabObserver { Let's put this by the rest of the private methods https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1479: mCallbackHelper.notifyCalled(); I think only the callback should be notified from this method. The closure should be done in the test itself. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1501: // reason to do this is we need to wait the page to be fully loaded, then it has navigation nit: I would drop the "reason to do this is" and just tart with "We need to wait for the page to be fully loaded so that it has navigation history and native code..." https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1524: assertTrue(TabModelUtils.getCurrentTab(model) == newTab); We should call checkState here too. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1525: } Can we add a test that makes sure tabs are restored in the correct window? MultiWindowUtilsTest.java shows an example of creating multiple activities for testing. https://codereview.chromium.org/2088443003/diff/200001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:21: nit: remove extra blank line https://codereview.chromium.org/2088443003/diff/200001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:148: using RestoreTab = const sessions::TabRestoreService::Tab; put this using line up with the rest. https://codereview.chromium.org/2088443003/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:89: std::unique_ptr<AndroidLiveTabContext> live_tab_context_; Let's explain what we're using the LiveTabContext for here. It's not obvious without tracing through the code.
xingliu@chromium.org changed reviewers: - dfalcantara@chromium.org, sky@chromium.org, tedchoc@chromium.org
https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/160001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:148: using RestoreTab = const sessions::TabRestoreService::Tab; On 2016/06/28 20:47:09, Theresa Wellington wrote: > The benefit of implementing AndroidLiveTabContext is so that we can use > TabRestoreService::RestoreMostRecentEntry instead of writing our own logic here. > > RestoreMostRecentEntry calls into TabRestoreService::RestoreTab, which will call > into LiveTabContext::AddRestoredTab(). I think we should be able to just hook in > there if there's any extra work not already done by the TabRestoreService > itself. This probably need another CL, since it needs some refactoring. Currently I think we only use data in tab restore service instead of actual logic like TabRestoreServiceHelper::RestoreEntryById. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:58: <item type="id" name="open_recent_closed_tab" /> On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: open_recently_closed_tab Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/KeyboardShortcuts.java:128: activity.onMenuOrKeyboardAction(R.id.open_recent_closed_tab, false); On 2016/06/29 23:53:29, Theresa Wellington wrote: > Let's have this just call into the activity and the activity itself can choose > whether to or not to handle the keyboard input based on if the current model is > incognito. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:49: public void openRecentClosedTab() { On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: openRecentlyClosedTab() > > This also needs a JavaDoc. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:727: TabLaunchType.FROM_LONGPRESS_FOREGROUND); On 2016/06/29 23:53:29, Theresa Wellington wrote: > I suspect that changing this affects other places that were originally relying > on this method to open things in the background. Used to investigate this a little bit, this change make the restore tab to the foreground when using tab restore service code in c++. In java, it seems we have other class manage the tab ordering(and I guess foreground and ordering are totally manged in java because they are android UI). And this enum seems totally serve for other purpose. I agree to put a TODO here to figure out if this break anything. However, the worst case scenario I can think of is, this stuff might make restored pages running javascripts in the background. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:751: // recover tab from rewind list in java, same as the undo snackbar. On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: use proper sentence capitalization/punctuation for comments e.g. "First try > to recover the tab from the rewound list." > > http://source.android.com/source/code-style.html#use-javadoc-standard-comments Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:760: // if no pending closure in java, open it from tab restore service in native code. On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: "If there are no pending closures in Java, ..." Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:338: private void openRecentClosedTabOnUiThread(final TabModelSelector selector) { On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: openRecentlyClosedTabOnUiThread Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1438: * Test open recently closed tab which uses rewinding list in java. On 2016/06/29 23:53:29, Theresa Wellington wrote: > nit: Test opening recently closed tabs using the rewound list in Java. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1444: public void testOpenRecentClosedTab() throws InterruptedException { On 2016/06/29 23:53:30, Theresa Wellington wrote: > nit: testOpenRecentlyClosedTab() Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1449: createTabOnUiThread(tabCreator, TEST_URL); On 2016/06/29 23:53:30, Theresa Wellington wrote: > This test doesn't rely on native, so I think we should just create a normal tab > rather than one with TEST_URL. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1463: // helper class to wait for fully loaded page. On 2016/06/29 23:53:30, Theresa Wellington wrote: > nit: Helper class that notifies when a page load is finished. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1464: private class TestTabObserver extends EmptyTabObserver { On 2016/06/29 23:53:30, Theresa Wellington wrote: > Let's put this by the rest of the private methods Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1479: mCallbackHelper.notifyCalled(); On 2016/06/29 23:53:30, Theresa Wellington wrote: > I think only the callback should be notified from this method. The closure > should be done in the test itself. Make sense. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1479: mCallbackHelper.notifyCalled(); On 2016/06/29 23:53:30, Theresa Wellington wrote: > I think only the callback should be notified from this method. The closure > should be done in the test itself. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1501: // reason to do this is we need to wait the page to be fully loaded, then it has navigation On 2016/06/29 23:53:30, Theresa Wellington wrote: > nit: I would drop the "reason to do this is" and just tart with "We need to wait > for the page to be fully loaded so that it has navigation history and native > code..." Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1524: assertTrue(TabModelUtils.getCurrentTab(model) == newTab); On 2016/06/29 23:53:30, Theresa Wellington wrote: > We should call checkState here too. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1525: } On 2016/06/29 23:53:30, Theresa Wellington wrote: > Can we add a test that makes sure tabs are restored in the correct window? > > MultiWindowUtilsTest.java shows an example of creating multiple activities for > testing. Done. https://codereview.chromium.org/2088443003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1525: } On 2016/06/29 23:53:30, Theresa Wellington wrote: > Can we add a test that makes sure tabs are restored in the correct window? > > MultiWindowUtilsTest.java shows an example of creating multiple activities for > testing. Implemented another test case in the patch. But hit exception when try to open tab in second window.
xingliu@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by xingliu@chromium.org 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by xingliu@chromium.org 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:727: TabLaunchType.FROM_LONGPRESS_FOREGROUND); On 2016/06/30 05:43:55, xingliu wrote: > On 2016/06/29 23:53:29, Theresa Wellington wrote: > > I suspect that changing this affects other places that were originally relying > > on this method to open things in the background. > > Used to investigate this a little bit, this change make the restore tab to the > foreground when using tab restore service code in c++. In java, it seems we have > other class manage the tab ordering(and I guess foreground and ordering are > totally manged in java because they are android UI). And this enum seems totally > serve for other purpose. > > I agree to put a TODO here to figure out if this break anything. > > However, the worst case scenario I can think of is, this stuff might make > restored pages running javascripts in the background. If you go to "Recent tabs", long press on a recently closed tab or a tab from another device and select "Open in new tab" previously this opened in the background but with this patch it opens in the foreground. For phones in particular, I think we want to retain the open in background behavior so that this use case doesn't break: 1. Go to recents 2. Open a bunch of things in new tabs in the background 3. Look through newly created tabs. This method could take a new parameter indicating whether the tab should open in the foreground or background. I think we should make sure UX agrees that ctrl+shift+t tabs should restore in the foreground before adding the plumbing. I can introduce you to rolfe@ this afternoon or tomorrow if we don't hear back soon :) https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1002: public void onDidNavigateMainFrame(Tab tab, String url, String baseUrl, Keeping rebases in a separate git cl upload from your changes makes reviews easier. Ideally the patchset uploads would look like this: ... Patchset 11: add incognito check.. Patchset 12: fixes based on review feedback Patchset 13: rebase Or, if you have to rebase before continuing local work: ... Patchset 11: add incognito check.. Patchset 13: rebase Patchset 12: fixes based on review feedback https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:49: * Opens the most recently closed tab. Read data from tab restore service in native code, and nit: move this down by the other openRecentlyClosedTab() Also, maybe we should rename this openMostRecentlyClosedTab() so that it's a bit more obvious how the two methods differ. We should also make the comment more similar to the comment for the existing openRecentlyClosedTab(), e,g, "Opens the most recently closed tab in a new tab." https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:152: * If no tabs are currently in the rewound list, restore the tab from tab restore service. Sorry for the conflicting review comments - we should probably just drop the second sentence since the implementation details (using the rewound list vs tab restore service) are up to the implementing class. The interface shouldn't dictate the implementation. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:154: public void openRecentlyClosedTab(); To match the RecentlyClosedBridge, let's make this openMostRecentlyClosedTab(); https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:750: * If no tabs are currently in the rewound list, restore the tab from tab restore service. Generally we don't add JavaDocs for overridden methods. The inline methods below explain what the code is doing. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:763: // then try to restore the tab from tab restore service in native code. nit: "..from the native tab restore service." https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:339: private void openRecentlyClosedTabOnUiThread(final TabModelSelector selector) { nit: openMostRecentlyClosedTabOnUiThread? Sorry I changed my mind on the naming. I know it's annoying but hopefully easy to change. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:349: private class TestTabObserver extends EmptyTabObserver { findbugs says that this should be a static inner class. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:361: private ChromeTabbedActivity2 createSecondChromeTabbedActivity() throws InterruptedException { This is just a copy-paste from MultiWindowUtilsTest.java, right? I think it'd be better to make the method in that class be public static (and accept an Activity param) and have this test class call into it. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1488: * Test open recently closed tab using the rewound list in java. nit: "Test opening recently closed tabs using the rewound list in Java." and then I would drop the second sentence since this doesn't use the native restore service. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1514: * Test open recently closed tab which uses native code from tab restore service. nit: "Test opening recently closed tabs using the native tab restore service." and drop the second sentence. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1527: // Create new tab and attach observer to listen to loaded event. nit: "... to listen for page load finished events." https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1542: fail(); Let's add an error message to the failure to help make this easier to tell why/where the failure occurred in the future if, for example, this test gets disabled because it failed on a bot, then the code changes so the line #s change. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1556: assertTrue(TabModelUtils.getCurrentTab(model) == newTab); This line is covered by checkState() https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1562: * Test open recently closed tab when we have multiple window. nit: "Test opening recently closed tabs when we have multiple windows." And remove the second sentence. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1569: public void testOpenRecentlyClosedTabMultiWindow() throws InterruptedException { Thank you for adding this! https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1572: // first window context. nit: s/first/First https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1579: // second window context. nit: s/second/Second https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1583: // Expected to one tab in first window and 0 tabs on second window. nit: "Assert the first window has 1 tab and the second window has 0 tabs." https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1584: assertEquals(model.getCount(), 1); nit: include an error message and put the expected value before the actual value: assertEquals("Unexpected number of tabs in ChromeTabbedActivity", 1, model.getCount()) https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1585: assertEquals(0, secondModel.getCount()); nit: use an error message like "Unexpected number of tabs in ChromeTabbedActivity2" https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1588: // currently hit exception while do stuff in second window. What exception are you seeing? We may need to limit this test to API level 24+. I can help you trouble shoot. This test would be more useful if we did something like: 1. close tab1 in window 1 2. close tab2 in window 2 3. close tab3 in window 1 4. close tab4 in window 2 5. undo, assert tab4 restored in window 2 6. undo, assert tab3 restored in window 3 .... It'd also be cool if we do some of the undo tab closures using ctrl+shirt+t for a more end-to-end integration test. We can use the framework's Instrumentation#sendKeys() https://developer.android.com/reference/android/test/InstrumentationTestCase.... https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:133: // Open most recently closed tab, don't need parent tab. nit: I don't think this comment is necessary. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:147: using RestoreTab = const sessions::TabRestoreService::Tab; reminder to move this up to the top of the file. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:151: // if no tab model, fall back to first non-incognito model. nit: "If no tab model matches the window_id, fallback to the first non-incognito model." https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:165: // remove last tab restore record. nit: capitalize Remove (same for other code comments in this file) Maybe this should be "Remove most recent tab restore record."? It's a little bit confusing to read "last" then see entries.front() below. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:167: tab_restore_service_->RemoveTabEntryById(entries.front()->id)); use restore_tab->id here instead of entries.front()->id? https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:178: content::WebContents* web_contents = content::WebContents::Create( nit: make this a new block? e.g. // Restore navigation history. session::SessionTab session_tab; ... // Prepare WebContents. content::WebContents* web_contents... https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.h:39: nit: remove this line and the one after the method to match the style for the other *RecentlyClosedTab methods. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/tab_android.cc:101: const content::WebContents* web_contents) { Does this still need to be const or is this left over from earlier patchsets? https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:21: // Implementation of LiveTabContext which used by android. nit: is used by Android. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:22: // Serve as adapter to LiveTabContext. nit: I think this line can be removed. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:56: static LiveTabContext* FindContextForWebContents(const nit: put const on next line with content::WebContents* contents); https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:89: // Used when we access the tab restore service and write in the correct nit: "... service to write the correct window id..." https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model_list.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model_list.h:38: const content::WebContents* web_contents); Does this still need to be const?
The CQ bit was checked by xingliu@chromium.org 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by xingliu@chromium.org 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 checked by xingliu@chromium.org 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.
Fix things based on Theresa's review. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:49: * Opens the most recently closed tab. Read data from tab restore service in native code, and On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: move this down by the other openRecentlyClosedTab() > > Also, maybe we should rename this openMostRecentlyClosedTab() so that it's a bit > more obvious how the two methods differ. We should also make the comment more > similar to the comment for the existing openRecentlyClosedTab(), e,g, "Opens the > most recently closed tab in a new tab." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:152: * If no tabs are currently in the rewound list, restore the tab from tab restore service. On 2016/06/30 20:50:24, Theresa Wellington wrote: > Sorry for the conflicting review comments - we should probably just drop the > second sentence since the implementation details (using the rewound list vs tab > restore service) are up to the implementing class. The interface shouldn't > dictate the implementation. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:154: public void openRecentlyClosedTab(); On 2016/06/30 20:50:24, Theresa Wellington wrote: > To match the RecentlyClosedBridge, let's make this openMostRecentlyClosedTab(); Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:750: * If no tabs are currently in the rewound list, restore the tab from tab restore service. On 2016/06/30 20:50:24, Theresa Wellington wrote: > Generally we don't add JavaDocs for overridden methods. The inline methods below > explain what the code is doing. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:763: // then try to restore the tab from tab restore service in native code. On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: "..from the native tab restore service." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:339: private void openRecentlyClosedTabOnUiThread(final TabModelSelector selector) { On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: openMostRecentlyClosedTabOnUiThread? > > Sorry I changed my mind on the naming. I know it's annoying but hopefully easy > to change. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:349: private class TestTabObserver extends EmptyTabObserver { On 2016/06/30 20:50:24, Theresa Wellington wrote: > findbugs says that this should be a static inner class. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:361: private ChromeTabbedActivity2 createSecondChromeTabbedActivity() throws InterruptedException { On 2016/06/30 20:50:24, Theresa Wellington wrote: > This is just a copy-paste from MultiWindowUtilsTest.java, right? I think it'd be > better to make the method in that class be public static (and accept an Activity > param) and have this test class call into it. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1488: * Test open recently closed tab using the rewound list in java. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: "Test opening recently closed tabs using the rewound list in Java." and > then I would drop the second sentence since this doesn't use the native restore > service. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1527: // Create new tab and attach observer to listen to loaded event. On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: "... to listen for page load finished events." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1542: fail(); On 2016/06/30 20:50:25, Theresa Wellington wrote: > Let's add an error message to the failure to help make this easier to tell > why/where the failure occurred in the future if, for example, this test gets > disabled because it failed on a bot, then the code changes so the line #s > change. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1556: assertTrue(TabModelUtils.getCurrentTab(model) == newTab); On 2016/06/30 20:50:25, Theresa Wellington wrote: > This line is covered by checkState() Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1572: // first window context. On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: s/first/First Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1579: // second window context. On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: s/second/Second Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1583: // Expected to one tab in first window and 0 tabs on second window. On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: "Assert the first window has 1 tab and the second window has 0 tabs." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1584: assertEquals(model.getCount(), 1); On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: include an error message and put the expected value before the actual > value: > > assertEquals("Unexpected number of tabs in ChromeTabbedActivity", 1, > model.getCount()) Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1585: assertEquals(0, secondModel.getCount()); On 2016/06/30 20:50:24, Theresa Wellington wrote: > nit: use an error message like "Unexpected number of tabs in > ChromeTabbedActivity2" Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:133: // Open most recently closed tab, don't need parent tab. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: I don't think this comment is necessary. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:147: using RestoreTab = const sessions::TabRestoreService::Tab; On 2016/06/30 20:50:25, Theresa Wellington wrote: > reminder to move this up to the top of the file. This is different from using Someclass, I remember some other code do aliasing in functions, do we have coding convention on this? https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:151: // if no tab model, fall back to first non-incognito model. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: "If no tab model matches the window_id, fallback to the first non-incognito > model." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:165: // remove last tab restore record. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: capitalize Remove (same for other code comments in this file) > > Maybe this should be "Remove most recent tab restore record."? It's a little bit > confusing to read "last" then see entries.front() below. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:167: tab_restore_service_->RemoveTabEntryById(entries.front()->id)); On 2016/06/30 20:50:25, Theresa Wellington wrote: > use restore_tab->id here instead of entries.front()->id? Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:178: content::WebContents* web_contents = content::WebContents::Create( On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: make this a new block? e.g. > > // Restore navigation history. > session::SessionTab session_tab; > ... > > // Prepare WebContents. > content::WebContents* web_contents... Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.h:39: On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: remove this line and the one after the method to match the style for the > other *RecentlyClosedTab methods. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/android... chrome/browser/android/tab_android.cc:101: const content::WebContents* web_contents) { On 2016/06/30 20:50:25, Theresa Wellington wrote: > Does this still need to be const or is this left over from earlier patchsets? Yes. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:21: // Implementation of LiveTabContext which used by android. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: is used by Android. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:22: // Serve as adapter to LiveTabContext. On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: I think this line can be removed. Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.h:56: static LiveTabContext* FindContextForWebContents(const On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: put const on next line with content::WebContents* contents); Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:89: // Used when we access the tab restore service and write in the correct On 2016/06/30 20:50:25, Theresa Wellington wrote: > nit: "... service to write the correct window id..." Done. https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model_list.h (right): https://codereview.chromium.org/2088443003/diff/220001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model_list.h:38: const content::WebContents* web_contents); On 2016/06/30 20:50:25, Theresa Wellington wrote: > Does this still need to be const? Done.
The CQ bit was checked by xingliu@chromium.org 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 checked by xingliu@chromium.org 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...
I was playing around with the patch today and found a way that we can use TabRestoreService::RestoreMostRecentEntry :) I'm in next week (not sure if you are) - let's sync next time we're both in the office and I'll show you my prototype code and we can discuss. https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:133: jboolean RecentlyClosedTabsBridge::OpenMostRecentlyClosedTab( I did some testing, and we can change this to use the TabRestoreService, which is ideal because then we're diverging from the other platforms less. Instead of most of the logic below, we could just call tab_restore_service_->RestoreMostRecentEntry(tab_model->GetLiveTabContext()); https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:148: SessionID::id_type window_id = restore_tab->browser_id; In my prototyping, I hadn't come up with a different way to figure out the window id in order to get the TabModel/LiveTabContext, but it'd be nice if there were a more elegant way to do this. I'll think on it. https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/session... File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/session... chrome/browser/sessions/chrome_tab_restore_service_client.cc:59: return nullptr; This needs to be implemented to use TabRestoreService::RestoreMostRecentEntry: return AndroidLiveTabContext::FindContextWithID(desired_id); https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:69: return nullptr; This would also need to be implemented. It would basically be the second half of the method in recently_closed_tabs_bridge.cc. We can use tab_model_ to get the profile and sessions::ContentLiveTab::GetForWebContents() to create the LiveTab at the end. https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:98: We would need a new method here, sessions::LiveTabContext* AndroidLiveTabContext::FindContextWithID(SessionID::id_type desired_id). A simple implementation would be TabModelList::FindModelWithId(). It'd be nice if we could find a way to move the fallback logic currently in recently_closed_tabs_bridge.cc to return the first non-incognito model if a model matching the id isn't found to this method.
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 xingliu@chromium.org 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...
xingliu@chromium.org changed reviewers: - dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org, sky@chromium.org
To Theresa: Please help to review new patch, refactor code to use RestoreMostRecentEntry, also improve multi-window test case. https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/280001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:98: On 2016/07/02 01:00:37, Theresa Wellington wrote: > We would need a new method here, > sessions::LiveTabContext* > AndroidLiveTabContext::FindContextWithID(SessionID::id_type desired_id). > > A simple implementation would be TabModelList::FindModelWithId(). It'd be nice > if we could find a way to move the fallback logic currently in > recently_closed_tabs_bridge.cc to return the first non-incognito model if a > model matching the id isn't found to this method. Updated the CL, refactor the code to use RestoreMostRecentEntry, also improve test case quality for multi-window scenario(from earlier comment in patch 16). Didn't find a more elegant way to pick the correct window id, too.
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org, sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:156: if (!tab_model || tab_model->IsOffTheRecord()) { I think that rather than finding the correct tab model here, we can just use the first non-incognito tab model here (quickly tested this in my prototype) because TabRestoreServiceHelper::RestoreTab calls TabRestoreService::FindLiveTabContextWithID (which we can use to find the correct LiveTabContext to restore into based on the browser id). I only tested this briefly, so there could be problems that I'm not considering. https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:177: tab_model->CreateTab(NULL, content_tab->web_contents(), -1); This can move to AndroidLiveTabContext::AddRestoredTab() https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:119: TabModel* model = TabModelList::FindTabModelWithId(desired_id); This gets called from tab_restore_service_helper (402). We should include the logic in android_live_tab_context.cc for finding the fallback tab_model here. If we don't and the desired_id matches a window that's been closed, we will crash because tab_restore_service_helper (413) calls CreateLiveTabContext() (which we don't implement).
The CQ bit was checked by xingliu@chromium.org 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 xingliu@chromium.org 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...
Refactor more code in recently_closed_tabs_bridge.cc. Much cleaner now. https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:156: if (!tab_model || tab_model->IsOffTheRecord()) { On 2016/07/06 18:51:05, Theresa Wellington wrote: > I think that rather than finding the correct tab model here, we can just use the > first non-incognito tab model here (quickly tested this in my prototype) because > TabRestoreServiceHelper::RestoreTab calls > TabRestoreService::FindLiveTabContextWithID (which we can use to find the > correct LiveTabContext to restore into based on the browser id). > > I only tested this briefly, so there could be problems that I'm not considering. Smart.. Thanks for making this code much cleaner! https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:177: tab_model->CreateTab(NULL, content_tab->web_contents(), -1); On 2016/07/06 18:51:06, Theresa Wellington wrote: > This can move to AndroidLiveTabContext::AddRestoredTab() Done. https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:119: TabModel* model = TabModelList::FindTabModelWithId(desired_id); On 2016/07/06 18:51:06, Theresa Wellington wrote: > This gets called from tab_restore_service_helper (402). We should include the > logic in android_live_tab_context.cc for finding the fallback tab_model here. If > we don't and the desired_id matches a window that's been closed, we will crash > because tab_restore_service_helper (413) calls CreateLiveTabContext() (which we > don't implement). Done.
The CQ bit was checked by xingliu@chromium.org 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...
This is looking good over all, mostly nits and an open UX question. https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/320001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:156: if (!tab_model || tab_model->IsOffTheRecord()) { On 2016/07/06 22:35:41, xingliu wrote: > On 2016/07/06 18:51:05, Theresa Wellington wrote: > > I think that rather than finding the correct tab model here, we can just use > the > > first non-incognito tab model here (quickly tested this in my prototype) > because > > TabRestoreServiceHelper::RestoreTab calls > > TabRestoreService::FindLiveTabContextWithID (which we can use to find the > > correct LiveTabContext to restore into based on the browser id). > > > > I only tested this briefly, so there could be problems that I'm not > considering. > > Smart.. > Thanks for making this code much cleaner! Thanks for reworking it! https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:109: * Opens the most recently closed tab. Read data from tab restore service in native code, and nit: "Opens the most recently closed tab in a new tab by reading data from the native tab restore service."? https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:726: // TODO(xingliu): figure out if TabLaunchType.FROM_LONGPRESS_FOREGROUND affects We need to figure this out before the CL lands. I pinged UX, will try to get a decision around this resolved soon. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:760: mRecentlyClosedBridge.openRecentlyClosedTab(); Should we record any UMA for this? Maybe just a user action, e.g. AndroidTabCloseUndo.FromKeyboard or something like that? We currently record a histogram for undoing via the snackbar in UndoBarController.java https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:368: private static class TestTabObserver extends EmptyTabObserver { nit: Maybe this should be called TabLoadedObserver or something like that so it's scoped to the one thing we're notifying the CallbackHelper of? https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1471: * Test opening recently closed tabs using the rewound list in Java nit: add a period after "Java" https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1489: // Ensure tab recovery, ordering, and reuse of {@link Tab} objects in java. nit: capitalize Java https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1504: // We need to listen for page load finished events, then it has navigation nit: maybe rephrase this as something like "Native can only successfully recover the tab after a page load has finished and it has navigation history." https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1524: * Test opening recently closed tab when we have multiple window. nit: s/window/windows https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1530: public void testOpenRecentlyClosedTabMultiWindow() throws InterruptedException { Thank you for adding this test. Can we do one more in here where the first or second activity is killed before undoing the tab closures to cover the case where a tab restores into a window other than it's original? https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1551: assertEquals("Unexpecetd number of tabs in first window.", 1, firstModel.getCount()); s/Unexpecetd/Unexpected https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:138: tab_restore_service_->RestoreMostRecentEntry(nullptr); Please add a comment here explaining that we're passing null here because the LiveTabContext will be determined later by a call to AndroidLiveTabContext::FindLiveTabContextWithID https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:139: return true; Should we check that TabRestoreService::RestoreMostRecentEntry returned a LiveTab before returning true? https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/session... File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/session... chrome/browser/sessions/chrome_tab_restore_service_client.cc:56: // Android does not initialize LiveTabContext here, and tab persistence is nit: maybe something like "Android does not support creating a LiveTabContext here." https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:92: // Currently do nothing. nit: "Currently does nothing." https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:110: sessions::LiveTabContext* AndroidLiveTabContext::FindContextForWebContents( Is there ever a case where this would need the fallback logic in FindContextWithId as well? Or is the tab model for the window guaranteed to exist when this is called? https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:112: TabAndroid* tab_android = TabAndroid::FromWebContents(contents); The comment above this method in tab_android.h says that it can return null, so we should check for null before using tab_android to get the window id https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:120: sessions::LiveTabContext* AndroidLiveTabContext::FindContextWithID( I think unit tests for the native side would be good too. That can be a follow-up CL handled in a separate CR bug with a TODO note in code. We have some unit tests for our android tab_model/ already that we may be able to draw on for examples: https://cs.chromium.org/chromium/src/chrome/browser/ui/android/tab_model/?sq=... https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:125: // if we can't find the correct model, fall back to first non-incognito model. nit: capitalize If, "... fallback to the first..." https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:89: // Used when we access the tab restore service and write the correct nit: "Used to restore closed tabs through the TabRestoreService." since it's being used for more than the window id now?
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 xingliu@chromium.org 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 xingliu@chromium.org 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.
Fixes based on code review. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java:109: * Opens the most recently closed tab. Read data from tab restore service in native code, and On 2016/07/07 19:00:12, Theresa Wellington wrote: > nit: "Opens the most recently closed tab in a new tab by reading data from the > native tab restore service."? Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:726: // TODO(xingliu): figure out if TabLaunchType.FROM_LONGPRESS_FOREGROUND affects On 2016/07/07 19:00:13, Theresa Wellington wrote: > We need to figure this out before the CL lands. I pinged UX, will try to get a > decision around this resolved soon. change it back to background, and add code to fix 'the only tab' problem. So it don't modify current UI behavior. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java:760: mRecentlyClosedBridge.openRecentlyClosedTab(); On 2016/07/07 19:00:12, Theresa Wellington wrote: > Should we record any UMA for this? Maybe just a user action, e.g. > AndroidTabCloseUndo.FromKeyboard or something like that? > > We currently record a histogram for undoing via the snackbar in > UndoBarController.java Done in ChromeTabbedActivity.onMenuOrKeyboardAction() https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:368: private static class TestTabObserver extends EmptyTabObserver { On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: Maybe this should be called TabLoadedObserver or something like that so > it's scoped to the one thing we're notifying the CallbackHelper of? Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1471: * Test opening recently closed tabs using the rewound list in Java On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: add a period after "Java" Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1489: // Ensure tab recovery, ordering, and reuse of {@link Tab} objects in java. On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: capitalize Java Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1504: // We need to listen for page load finished events, then it has navigation On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: maybe rephrase this as something like "Native can only successfully recover > the tab after a page load has finished and it has navigation history." Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1524: * Test opening recently closed tab when we have multiple window. On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: s/window/windows Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1530: public void testOpenRecentlyClosedTabMultiWindow() throws InterruptedException { On 2016/07/07 19:00:13, Theresa Wellington wrote: > Thank you for adding this test. Can we do one more in here where the first or > second activity is killed before undoing the tab closures to cover the case > where a tab restores into a window other than it's original? Sure, also did manual testing, fallback logic works. https://codereview.chromium.org/2088443003/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1551: assertEquals("Unexpecetd number of tabs in first window.", 1, firstModel.getCount()); On 2016/07/07 19:00:13, Theresa Wellington wrote: > s/Unexpecetd/Unexpected Done.My bad.. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:138: tab_restore_service_->RestoreMostRecentEntry(nullptr); On 2016/07/07 19:00:13, Theresa Wellington wrote: > Please add a comment here explaining that we're passing null here because the > LiveTabContext will be determined later by a call to > AndroidLiveTabContext::FindLiveTabContextWithID Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:139: return true; On 2016/07/07 19:00:13, Theresa Wellington wrote: > Should we check that TabRestoreService::RestoreMostRecentEntry returned a > LiveTab before returning true? Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/session... File chrome/browser/sessions/chrome_tab_restore_service_client.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/session... chrome/browser/sessions/chrome_tab_restore_service_client.cc:56: // Android does not initialize LiveTabContext here, and tab persistence is On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: maybe something like "Android does not support creating a LiveTabContext > here." Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:92: // Currently do nothing. On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: "Currently does nothing." Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:110: sessions::LiveTabContext* AndroidLiveTabContext::FindContextForWebContents( On 2016/07/07 19:00:14, Theresa Wellington wrote: > Is there ever a case where this would need the fallback logic in > FindContextWithId as well? Or is the tab model for the window guaranteed to > exist when this is called? I think this is used in the flow that write the browser id into tab restore service entry, the closing tab must have a tab model window id associated with it. The fallback logic is used when we want to read the browser_id out of restore service. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:112: TabAndroid* tab_android = TabAndroid::FromWebContents(contents); On 2016/07/07 19:00:13, Theresa Wellington wrote: > The comment above this method in tab_android.h says that it can return null, so > we should check for null before using tab_android to get the window id Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:120: sessions::LiveTabContext* AndroidLiveTabContext::FindContextWithID( On 2016/07/07 19:00:13, Theresa Wellington wrote: > I think unit tests for the native side would be good too. That can be a > follow-up CL handled in a separate CR bug with a TODO note in code. > > We have some unit tests for our android tab_model/ already that we may be able > to draw on for examples: > https://cs.chromium.org/chromium/src/chrome/browser/ui/android/tab_model/?sq=... Added TODO. But current java unittest will call native function then it actually covered most of the function here. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:125: // if we can't find the correct model, fall back to first non-incognito model. On 2016/07/07 19:00:13, Theresa Wellington wrote: > nit: capitalize If, "... fallback to the first..." Done. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:89: // Used when we access the tab restore service and write the correct On 2016/07/07 19:00:14, Theresa Wellington wrote: > nit: "Used to restore closed tabs through the TabRestoreService." since it's > being used for more than the window id now? Done.
This is getting really close, all nits. https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:110: sessions::LiveTabContext* AndroidLiveTabContext::FindContextForWebContents( On 2016/07/08 21:26:48, xingliu wrote: > On 2016/07/07 19:00:14, Theresa Wellington wrote: > > Is there ever a case where this would need the fallback logic in > > FindContextWithId as well? Or is the tab model for the window guaranteed to > > exist when this is called? > > I think this is used in the flow that write the browser id into tab restore > service entry, the closing tab must have a tab model window id associated with > it. > > The fallback logic is used when we want to read the browser_id out of restore > service. > Can we add a comment to that effect so it's easier to reason about wh this behaves differently than FindContextWithId when looking at this file in the future? https://codereview.chromium.org/2088443003/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:120: sessions::LiveTabContext* AndroidLiveTabContext::FindContextWithID( On 2016/07/08 21:26:48, xingliu wrote: > On 2016/07/07 19:00:13, Theresa Wellington wrote: > > I think unit tests for the native side would be good too. That can be a > > follow-up CL handled in a separate CR bug with a TODO note in code. > > > > We have some unit tests for our android tab_model/ already that we may be able > > to draw on for examples: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/android/tab_model/?sq=... > > Added TODO. But current java unittest will call native function then it actually > covered most of the function here. Thanks :) It's still (generally) good to do unit tests (here, that's C++ tests) and integration tests (Java tests). https://codereview.chromium.org/2088443003/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1083: RecordUserAction.record("MobileOpenClosedTab"); nit: Let's use AndroidTabCloseUndo.Keyboard to mirror AndroidTabCloseUndo.Toast (used for the undo bar) https://codereview.chromium.org/2088443003/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:151: * Opens the most recently closed tab, bringing the tab back into this model. nit: since this can technically bring it back into a different tab model, we should reword this as "... the tab back into its original model or this model if the original model no longer exists." https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:110: } catch (InterruptedException e) { Should we fail if this is interrupted too? Java supports chaining catch reasons - e.g. ... } catch (TimeoutException | InterruptedException e) { ... https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:364: ((TabModelSelectorImpl) selector).getCurrentModel().openMostRecentlyClosedTab(); This shouldn't need to be cast - selector.getCurrentModel() should work. https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1529: assertEquals(1, model.getCount()); nit: let's call checkState() here and ensure that the rewound list is empty. https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1542: * Draft: nit: remove this line -- is the draft comment finished? https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1572: final TabModelSelector secondSelector = secondActivity.getTabModelSelector(); nit: condense this with the line below since secondSelector isn't used anywhere else? Same for firstSelector & variables in testOpenRecentlyClosedTabMultiWindowFallback() https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1601: Tab[] firstWindowTabs = new Tab[]{firstModel.getTabAt(0), firstModel.getTabAt(1)}; Since getTatAt(0) isn't changing, I think it would be a little bit better to store this variable before adding/removing tabs and reuse the same object when checking state here. https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1641: assertEquals("Window 2 should has 2 tab.", 2, secondModel.getCount()); nit: s/has/have s/tab/tabs https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1652: } catch (InterruptedException e) { Same comment here about failing on both exceptions. https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:139: // Passing nullptr here because LiveTabContext will be found in nit: "... will be determined later by a call to AndroidLiveTabContext::..." https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:20: void AndroidLiveTabContext::ShowBrowserWindow() { nit: Let's add NotReached() here so we crashes if this method is called.
The CQ bit was checked by xingliu@chromium.org 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...
Add Uma entry in actions.xml, and a couple of other minor fixes based on review. https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1542: * Draft: On 2016/07/12 00:06:16, Theresa Wellington wrote: > nit: remove this line -- is the draft comment finished? Comment is done, gonna remove 'Draft' since it brings confusion. https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:20: void AndroidLiveTabContext::ShowBrowserWindow() { On 2016/07/12 00:06:16, Theresa Wellington wrote: > nit: Let's add NotReached() here so we crashes if this method is called. This will be called by android. Since we wired in to that part of the code in tab_restore_service_helper.cc. Didn't implement this because that function is like show certain window, where android doesn't quite have this feature.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % nits https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:20: void AndroidLiveTabContext::ShowBrowserWindow() { On 2016/07/12 03:57:18, xingliu wrote: > On 2016/07/12 00:06:16, Theresa Wellington wrote: > > nit: Let's add NotReached() here so we crashes if this method is called. > > This will be called by android. Since we wired in to that part of the code in > tab_restore_service_helper.cc. > > Didn't implement this because that function is like show certain window, where > android doesn't quite have this feature. Ah, okay. Then instead of saying "Not supported by Android." let's say something like: void AndroidLiveTabContext::ShowBrowserWindow() { // Intentionally empty. This method is called during restoration of closed tabs, but is not expected to do anything on Android. } https://codereview.chromium.org/2088443003/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:1600: Tab firstModelTab = firstModel.getTabAt(0); nit: move this above line 1582, before any tabs are closed/reopened. Can we also do this for testOpenRecentlyClosedTabNative() and testOpenRecentlyClosedTabMultiWindowFallback()? https://codereview.chromium.org/2088443003/diff/460001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/460001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:140: // a call to AndroidLiveTabContext. s/AndroidLiveTabContext/AndroidLiveTabContext::FindLiveTabContextWithID https://codereview.chromium.org/2088443003/diff/460001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2088443003/diff/460001/chrome/browser/android... chrome/browser/android/tab_android.cc:669: const float device_scale_factor = Please upload rebases as a separate patchset (upload a patchset for only the rebase, then a patchset with your changes). https://codereview.chromium.org/2088443003/diff/460001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2088443003/diff/460001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9339: <description>Undo tab closure by keyboard shortcut ctrl+shift+T.</description> nit: add something like "Android only." at the end to indicate that this is only logged on that platform.
xingliu@chromium.org changed reviewers: + asvitkine@chromium.org
To asvitkine: Please help to review, tools/metrics/actions/actions.xml.
The CQ bit was checked by xingliu@chromium.org 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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2088443003/diff/480001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/480001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:140: // a call to AndroidLiveTabContext::FindLiveTabContextWithID. Nit: Could you mention that this is done in ChromeTabRestoreServiceClient ? https://codereview.chromium.org/2088443003/diff/480001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.cc:144: return restored_tab.empty() ? false : true; Optional nit: How about !restored_tab.empty()? https://codereview.chromium.org/2088443003/diff/480001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/480001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:87: tab_model_->CreateTab(NULL, web_contents, -1); Nit: 'nullptr'
The CQ bit was checked by xingliu@chromium.org 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.
xingliu@chromium.org changed reviewers: + marja@chromium.org
To marja: Please help review chrome/browser/sessions/chrome_tab_restore_service_client.cc
lgtm % nits. https://codereview.chromium.org/2088443003/diff/500001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java (right): https://codereview.chromium.org/2088443003/diff/500001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/UndoTabModelTest.java:39: private static final String TEST_URL_0 = UrlUtils.encodeHtmlDataUri("<html>test_url_0.</html>"); . https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/android... File chrome/browser/android/recently_closed_tabs_bridge.h (right): https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/android... chrome/browser/android/recently_closed_tabs_bridge.h:16: class TabModel; Do you need this? https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/android_live_tab_context.cc (right): https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/android_live_tab_context.cc:130: TabModel* model = *it; Could we tweak the variable name so it's not the same as the outer scope? Just to make it a bit clearer. https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model.h (right): https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model.h:28: namespace sessiosn { sessiosn -> sessions? Do we need this at all with the include above? https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... File chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc (right): https://codereview.chromium.org/2088443003/diff/500001/chrome/browser/ui/andr... chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc:90: (parent ? parent->GetJavaObject().obj() : NULL), nullptr? Or does that cause issues with the JNI method?
chrome/browser/sessions/chrome_tab_restore_service_client.cc rubberstamp lgtm, assuming you've done the comments from other reviewers (I didn't verify).
The CQ bit was checked by xingliu@chromium.org 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, asvitkine@chromium.org, nyquist@chromium.org, dtrainor@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2088443003/#ps520001 (title: "nit fixes based on review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Shortcut ctrl+shift+T added on android. This CL first check if we can undo the recent closure in java by using TabModelImpl. If nothing we can do with undo snackbar logic, then use recently_closed_tabs_bridge in native code to retrive data from tab_restore_service. BUG=602559 https://bugs.chromium.org/p/chromium/issues/detail?id=602559#c7 ========== to ========== Shortcut ctrl+shift+T added on android. This CL first check if we can undo the recent closure in java by using TabModelImpl. If nothing we can do with undo snackbar logic, then use recently_closed_tabs_bridge in native code to retrive data from tab_restore_service. BUG=602559 https://bugs.chromium.org/p/chromium/issues/detail?id=602559#c7 Committed: https://crrev.com/6fbb24a66cb8bb459fed7a4db23428620edb9048 Cr-Commit-Position: refs/heads/master@{#405624} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/6fbb24a66cb8bb459fed7a4db23428620edb9048 Cr-Commit-Position: refs/heads/master@{#405624} |
