Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
36473002:
Foreign session pages now load into current tab. (Closed)
|
Created:
7 years, 2 months ago by apiccion Modified:
7 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPages from other devices open in current tab.
* Opening a foreign session or recently closed now takes over the tab
(nuking its navigation history)
* Opening a chrome-to-mobile page now loads in the current tab.
BUG=257102
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234684
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Newt #
Total comments: 8
Patch Set 4 : Newt nits. #Patch Set 5 : moar owners #
Total comments: 7
Patch Set 6 : Yaron #
Total comments: 8
Patch Set 7 : Yaron #Patch Set 8 : Ted #Patch Set 9 : Ted #
Total comments: 4
Patch Set 10 : #
Total comments: 4
Patch Set 11 : sky #
Messages
Total messages: 38 (0 generated)
Work in progress.
This CL is ready for review.
https://codereview.chromium.org/36473002/diff/40001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:200: ForeignSessionTab foreignTab) { could you add windowDisposition here as another argument? we'll use that to determine whether the tab should be opened in the current tab or a new tab. See: WindowOpenDisposition.java (it's autogenerated... in the out directory). Or you could just use a boolean openInCurrentTab. Then, you can delete the ForeignSessionHelper::OpenForeignSessionTabOld() and handle both cases in ForeignSessionHelper::OpenForeignSessionTab() by passing in the disposition/boolean to that method. https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:252: jobject obj, don't need this method https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:313: if (tab->navigations.empty()) { you can just replace the rest of this method with a call to SessionRestore::RestoreForeignSessionTab(), right? https://codereview.chromium.org/36473002/diff/40001/chrome/browser/sessions/s... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/40001/chrome/browser/sessions/s... chrome/browser/sessions/session_restore_android.cc:43: DCHECK(current_tab); Check the value of disposition here and either open in a new tab or in the current tab, depending on its value.
https://codereview.chromium.org/36473002/diff/40001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/40001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:200: ForeignSessionTab foreignTab) { Added windowDisposition. Cannot remove *Old() call due to dependency on web_contents. On 2013/11/01 23:41:32, newt wrote: > could you add windowDisposition here as another argument? we'll use that to > determine whether the tab should be opened in the current tab or a new tab. See: > WindowOpenDisposition.java (it's autogenerated... in the out directory). Or you > could just use a boolean openInCurrentTab. > > Then, you can delete the ForeignSessionHelper::OpenForeignSessionTabOld() and > handle both cases in ForeignSessionHelper::OpenForeignSessionTab() by passing in > the disposition/boolean to that method. https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:252: jobject obj, On 2013/11/01 23:41:32, newt wrote: > don't need this method Done. https://codereview.chromium.org/36473002/diff/40001/chrome/browser/android/fo... chrome/browser/android/foreign_session_helper.cc:313: if (tab->navigations.empty()) { On 2013/11/01 23:41:32, newt wrote: > you can just replace the rest of this method with a call to > SessionRestore::RestoreForeignSessionTab(), right? Done. https://codereview.chromium.org/36473002/diff/40001/chrome/browser/sessions/s... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/40001/chrome/browser/sessions/s... chrome/browser/sessions/session_restore_android.cc:43: DCHECK(current_tab); On 2013/11/01 23:41:32, newt wrote: > Check the value of disposition here and either open in a new tab or in the > current tab, depending on its value. Done.
lgtm with an impressive number of nits... sorry! https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:181: * TODO (apiccion): Remvoe this method once downstream CL Lands. nit: no space between "TODO" and "(" tip: you can use http://crbug.com/257102 to be terse https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:194: * @param tab Tab to load the session into. This is some pretty crazy indentation in the javadoc :) You just need a single space between the variable name and the description. That keeps things from moving around every time a param is added, renamed, or removed. https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.cc:305: const SessionTab* tab; call this session_tab or foreign_tab? It's confusing having j_tab, tab (not at all related to j_tab), tab_id, and tab_android. I think using session_tab and session_tab_id would make things clearer. https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.h (right): https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.h:42: jint j_disposition); typically don't use j_ for primitive types (since they don't need conversion)
https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:181: * TODO (apiccion): Remvoe this method once downstream CL Lands. On 2013/11/02 02:04:42, newt wrote: > nit: no space between "TODO" and "(" > tip: you can use http://crbug.com/257102 to be terse Done. https://codereview.chromium.org/36473002/diff/120001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:194: * @param tab Tab to load the session into. On 2013/11/02 02:04:42, newt wrote: > This is some pretty crazy indentation in the javadoc :) You just need a single > space between the variable name and the description. That keeps things from > moving around every time a param is added, renamed, or removed. Done. https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.cc:305: const SessionTab* tab; On 2013/11/02 02:04:42, newt wrote: > call this session_tab or foreign_tab? It's confusing having j_tab, tab (not at > all related to j_tab), tab_id, and tab_android. I think using session_tab and > session_tab_id would make things clearer. Done. And changed the names throughout the file for consistency. https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.h (right): https://codereview.chromium.org/36473002/diff/120001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.h:42: jint j_disposition); On 2013/11/02 02:04:42, newt wrote: > typically don't use j_ for primitive types (since they don't need conversion) Done.
Seeking owners LGTM. @nyquist: chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java @sky: chrome/browser/sessions/session_restore_android.cc
I'm not a good reviewer for the android side. I don't think Marja is either. Please fine a suitable owner and add them to OWNERS (with per file permissions).
@newt, do you want own this file?
On 2013/11/04 16:58:01, apiccion wrote: > @newt, do you want own this file? sure, I'm pretty familiar with it now. I'd add yfriedman and another reviewer or two.
Added newt, yfriedman and tedchoc as OWNERS to chrome/browser/sessions/session_restore_android*
lgtm with comments https://codereview.chromium.org/36473002/diff/270001/chrome/browser/android/r... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/36473002/diff/270001/chrome/browser/android/r... chrome/browser/android/recently_closed_tabs_bridge.cc:116: CURRENT_TAB); Mention this change in the CL description. https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... File chrome/browser/sessions/OWNERS (right): https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... chrome/browser/sessions/OWNERS:8: per-file session_restore_android*=yfriedman@chromium.org As an FYI, both yfriedman and tedchoc already have OWNERS of this file through *android* in chrome/OWNERS. But I guess it is fine to clarify here as well.
https://codereview.chromium.org/36473002/diff/270001/chrome/browser/android/r... File chrome/browser/android/recently_closed_tabs_bridge.cc (right): https://codereview.chromium.org/36473002/diff/270001/chrome/browser/android/r... chrome/browser/android/recently_closed_tabs_bridge.cc:116: CURRENT_TAB); On 2013/11/04 18:36:08, nyquist wrote: > Mention this change in the CL description. Done.
https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:45: current_tab->SwapTabContents(web_contents, new_web_contents); looks like you're leaking the old web_contents. From the comments of CoreTabHelperDelegate you need to take care of ownership. https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:47: DCHECK(disposition == NEW_FOREGROUND_TAB); DCHECK_EQ
https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:45: current_tab->SwapTabContents(web_contents, new_web_contents); On 2013/11/04 18:48:37, Yaron wrote: > looks like you're leaking the old web_contents. From the comments of > CoreTabHelperDelegate you need to take care of ownership. Deleted the object here. Added a comment in .h file explaining Android ownership. https://codereview.chromium.org/36473002/diff/270001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:47: DCHECK(disposition == NEW_FOREGROUND_TAB); On 2013/11/04 18:48:37, Yaron wrote: > DCHECK_EQ Done.
https://codereview.chromium.org/36473002/diff/370001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/370001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:203: foreignTab.id, windowOpenDisposition); +4 indent https://codereview.chromium.org/36473002/diff/370001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.cc:310: session_tab_id, &session_tab)) { I think the second param needs to go on it's own line. https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... File chrome/browser/sessions/OWNERS (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... chrome/browser/sessions/OWNERS:9: per-file session_restore_android*=tedchoc@chromium.org I don't think newt or me belong in here. I would vote for yfriedman and felipeg@chromium.org though https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:50: delete web_contents; should this delete only go in the CURRENT_TAB statement?
https://codereview.chromium.org/36473002/diff/370001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java (right): https://codereview.chromium.org/36473002/diff/370001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/ForeignSessionHelper.java:203: foreignTab.id, windowOpenDisposition); On 2013/11/04 19:46:28, Ted C wrote: > +4 indent Done. https://codereview.chromium.org/36473002/diff/370001/chrome/browser/android/f... File chrome/browser/android/foreign_session_helper.cc (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/android/f... chrome/browser/android/foreign_session_helper.cc:310: session_tab_id, &session_tab)) { On 2013/11/04 19:46:28, Ted C wrote: > I think the second param needs to go on it's own line. Done. https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... File chrome/browser/sessions/OWNERS (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... chrome/browser/sessions/OWNERS:9: per-file session_restore_android*=tedchoc@chromium.org On 2013/11/04 19:46:28, Ted C wrote: > I don't think newt or me belong in here. I would vote for yfriedman and > mailto:felipeg@chromium.org though Done. https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/370001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:50: delete web_contents; On 2013/11/04 19:46:28, Ted C wrote: > should this delete only go in the CURRENT_TAB statement? Done.
https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:46: delete web_contents; I think this might be more complicated than just deleting the web_contents. In prerender_manager.cc, there's a fair bit of logic to delete the old web contents: if (old_web_contents->NeedToFireBeforeUnload()) { // Schedule the delete to occur after the tab has run its unload handlers. on_close_web_contents_deleters_.push_back( new OnCloseWebContentsDeleter(this, old_web_contents)); old_web_contents->GetRenderViewHost()-> FirePageBeforeUnload(false); } else { // No unload handler to run, so delete asap. ScheduleDeleteOldWebContents(old_web_contents, NULL); }
lgtm https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... File chrome/browser/sessions/OWNERS (right): https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... chrome/browser/sessions/OWNERS:7: per-file session_restore_android*=newt@chromium.org I think you misread Ted's comments.. I should probably be here.
https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore_android.cc (right): https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... chrome/browser/sessions/session_restore_android.cc:46: delete web_contents; On 2013/11/04 21:18:18, newt wrote: > I think this might be more complicated than just deleting the web_contents. In > prerender_manager.cc, there's a fair bit of logic to delete the old web > contents: > > if (old_web_contents->NeedToFireBeforeUnload()) { > // Schedule the delete to occur after the tab has run its unload handlers. > on_close_web_contents_deleters_.push_back( > new OnCloseWebContentsDeleter(this, old_web_contents)); > old_web_contents->GetRenderViewHost()-> > FirePageBeforeUnload(false); > } else { > // No unload handler to run, so delete asap. > ScheduleDeleteOldWebContents(old_web_contents, NULL); > } Ya, I scanned that too. I didn't look very hard at it but I thought it was specific to prerendering and fine to omit. Certainly one of the issues is "before unload" handling which I thought is unsupported on Android. I think the rest is scaffolding for prerendering but I might be mistaken.
Waiting for OK from Ted. https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... File chrome/browser/sessions/OWNERS (right): https://codereview.chromium.org/36473002/diff/460001/chrome/browser/sessions/... chrome/browser/sessions/OWNERS:7: per-file session_restore_android*=newt@chromium.org On 2013/11/04 21:19:59, Yaron wrote: > I think you misread Ted's comments.. I should probably be here. > Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/36473002/530001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
@sky, added new owners seeking your review for:
chrome/browser/sessions/OWNERS
chrome/browser/sessions/session_restore.h
https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... chrome/browser/sessions/session_restore.h:68: // to the restored tab. The Android implementation of this method takes I don't like having a different ownership model for android. That makes it very easy to do the wrong thing. Can't we have the caller deal with Android specifics?
https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... chrome/browser/sessions/session_restore.h:68: // to the restored tab. The Android implementation of this method takes On 2013/11/06 21:19:42, sky wrote: > I don't like having a different ownership model for android. That makes it very > easy to do the wrong thing. Can't we have the caller deal with Android > specifics? The interaction with source_web_contents depends on the disposition parameter. When opened in the current-tab: web_contents gets swapped out with a new one (the old one needs to be deleted). But when opened in a new tab, we still need to keep the old content. If the caller were responsible for cleanup, we'd still need to update this comment to indicate under which conditions. We could create a separate android header. However, I think the simplest solution lets android callers to know they should never reference source_web_contents after calling this method.
https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... chrome/browser/sessions/session_restore.h:68: // to the restored tab. The Android implementation of this method takes On 2013/11/06 21:37:03, apiccion wrote: > On 2013/11/06 21:19:42, sky wrote: > > I don't like having a different ownership model for android. That makes it > very > > easy to do the wrong thing. Can't we have the caller deal with Android > > specifics? > > The interaction with source_web_contents depends on the disposition parameter. > When opened in the current-tab: web_contents gets swapped out with a new one > (the old one needs to be deleted). But when opened in a new tab, we still need > to keep the old content. > > If the caller were responsible for cleanup, we'd still need to update this > comment to indicate under which conditions. > > We could create a separate android header. However, I think the simplest > solution lets android callers to know they should never reference > source_web_contents after calling this method. Actually, the non-android implementation does the same. How about a comment of: 'If |disposition| is CURRENT_TAB, |source_web_contents| may be destroyed.' I say may as this invokes CloseWebContentsAt(), which may not destroy. One more question though. You're directly deleting the WebContents, what about unload handlers? Are they not supported on android?
https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... File chrome/browser/sessions/session_restore.h (right): https://codereview.chromium.org/36473002/diff/530001/chrome/browser/sessions/... chrome/browser/sessions/session_restore.h:68: // to the restored tab. The Android implementation of this method takes On 2013/11/07 01:12:24, sky wrote: > On 2013/11/06 21:37:03, apiccion wrote: > > On 2013/11/06 21:19:42, sky wrote: > > > I don't like having a different ownership model for android. That makes it > > very > > > easy to do the wrong thing. Can't we have the caller deal with Android > > > specifics? > > > > The interaction with source_web_contents depends on the disposition parameter. > > When opened in the current-tab: web_contents gets swapped out with a new one > > (the old one needs to be deleted). But when opened in a new tab, we still need > > to keep the old content. > > > > If the caller were responsible for cleanup, we'd still need to update this > > comment to indicate under which conditions. > > > > We could create a separate android header. However, I think the simplest > > solution lets android callers to know they should never reference > > source_web_contents after calling this method. > > Actually, the non-android implementation does the same. How about a comment of: > 'If |disposition| is CURRENT_TAB, |source_web_contents| may be destroyed.' > > I say may as this invokes CloseWebContentsAt(), which may not destroy. One more > question though. You're directly deleting the WebContents, what about unload > handlers? Are they not supported on android? Done. Regarding unload handlers: Please see Yaron's comment. (I believe they're unsupported)
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/36473002/800001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/36473002/800001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/36473002/800001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/36473002/800001
Message was sent while issue was closed.
Change committed as 234684 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
