Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832080:
Upstream session_backend_android. (Closed)
|
Created:
8 years, 4 months ago by felipeg Modified:
8 years, 4 months ago CC:
chromium-reviews, marja+watch_chromium.org, gone, thainayu Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUpstream session_backend_android.
BUG=138955
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150342
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 11
Patch Set 3 : Addressing Sky comments. #Patch Set 4 : Addressing Joth's comments #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : Addressed Sky's comment. #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #
Total comments: 1
Patch Set 12 : #
Messages
Total messages: 26 (0 generated)
Adding thakis@ and sky@ to review changes under chrome/ I know you guys are busy, I just need one reviewer, who ever comes first ... Joth@ had worked on these classes downstream. thanks
android trybot says: /usr/local/google/android-ndk-r7/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin//../lib/gcc/arm-linux-androideabi/4.4.3/../../../../arm-linux-androideabi/bin/ld: out/Release/obj.target/unit_tests/chrome/browser/sessions/session_backend_unittest.o: in function SessionBackendTest_BigData_Test::TestBody():chrome/browser/sessions/session_backend_unittest.cc:156:error: undefined reference to 'SessionBackend::kFileReadBufferSize'
Yep, I am fixing that, I will ping you soon. Thanks for the quick reply. On Tue, Jul 31, 2012 at 4:47 PM, <thakis@chromium.org> wrote: > android trybot says: > /usr/local/google/android-ndk-**r7/toolchains/arm-linux-** > androideabi-4.4.3/prebuilt/**linux-x86/bin//../lib/gcc/arm-** > linux-androideabi/4.4.3/../../**../../arm-linux-androideabi/**bin/ld: > out/Release/obj.target/unit_**tests/chrome/browser/sessions/** > session_backend_unittest.o: > in function > SessionBackendTest_BigData_**Test::TestBody():chrome/** > browser/sessions/session_**backend_unittest.cc:156:error: > undefined reference to 'SessionBackend::**kFileReadBufferSize' > > > https://chromiumcodereview.**appspot.com/10832080/<https://chromiumcodereview... >
sky: Since you're in .../session/OWNERS (together with marja, but she's on vacation), I'll leave this to you if that's ok.
https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/session_backend_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/session_backend_android.cc:12: // A better design would be to factor out just those bits of TabRestoreService I don't like this hack either. Please refactor appropriately. https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/session_restore_android.cc:44: static_cast<int>(session_tab.navigations.size() - 1))); This is problematic if navigations is empty https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/tab_restore_service.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/tab_restore_service.cc:456: SessionServiceFactory::GetForProfile(profile()); CAn you instead make GetForPRofile return NULL always on android? https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/tab_restore_service.cc:544: #if !defined(OS_ANDROID) Isn't there a more specific ifdef for extensions? https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/chrome_browser.... chrome/chrome_browser.gypi:2087: 'browser/sessions/session_backend_android.cc', After backend.h
https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... File chrome/browser/sessions/session_backend_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_backend_android.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:26: Profile* profile = static_cast<Profile*>(web_contents->GetBrowserContext()); Remove the FIXME, do this instead: content::BrowserContext* context = web_contents->GetBrowserContext(); Profile* profile = Profile::FromBrowserContext(context). https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:33: content::BrowserContext* context = profile; move this up top of function as per my suggestion above, or just remove |context| and use |profile| throughout. https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:47: &entries); strange indent https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... File chrome/browser/sessions/tab_restore_service.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/tab_restore_service.cc:444: #if !defined(ENABLE_SESSION_SERVICE) what is this ENABLE_SESSION_SERVICE define? Should we be configuring that instead?
https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/session_backend_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/session_backend_android.cc:12: // A better design would be to factor out just those bits of TabRestoreService On 2012/07/31 15:26:09, sky wrote: > I don't like this hack either. Please refactor appropriately. I was blindly upstreaming this code, and don't have much of a background on this issue. I have a bug tracking this where Joth replied. http://code.google.com/p/chromium/issues/detail?id=138955 I will drop the change in this file for now, and ping some people to see if anyone knows how the refactoring should be made. https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/session_restore_android.cc:44: static_cast<int>(session_tab.navigations.size() - 1))); On 2012/07/31 15:26:09, sky wrote: > This is problematic if navigations is empty Good catch. I am surprised that we did not see the DCHECK in void NavigationControllerImpl::Restore() blowing out. https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... File chrome/browser/sessions/tab_restore_service.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/tab_restore_service.cc:456: SessionServiceFactory::GetForProfile(profile()); On 2012/07/31 15:26:09, sky wrote: > CAn you instead make GetForPRofile return NULL always on android? Done. https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/browser/session... chrome/browser/sessions/tab_restore_service.cc:544: #if !defined(OS_ANDROID) On 2012/07/31 15:26:09, sky wrote: > Isn't there a more specific ifdef for extensions? Yaron is taking care of extensions (adding him to the CL too). He said : """ we're not removing extensions wholesale so there's no guard for it """ https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10832080/diff/1/chrome/chrome_browser.... chrome/chrome_browser.gypi:2087: 'browser/sessions/session_backend_android.cc', On 2012/07/31 15:26:09, sky wrote: > After backend.h Done.
https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... File chrome/browser/sessions/session_backend_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_backend_android.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/07/31 17:34:27, joth wrote: > 2012 Done. https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2012/07/31 17:34:27, joth wrote: > 2012 Done. https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:26: Profile* profile = static_cast<Profile*>(web_contents->GetBrowserContext()); On 2012/07/31 17:34:27, joth wrote: > Remove the FIXME, do this instead: > > content::BrowserContext* context = web_contents->GetBrowserContext(); > Profile* profile = Profile::FromBrowserContext(context). > Done. https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:33: content::BrowserContext* context = profile; On 2012/07/31 17:34:27, joth wrote: > move this up top of function as per my suggestion above, or just remove > |context| and use |profile| throughout. Done. https://chromiumcodereview.appspot.com/10832080/diff/7002/chrome/browser/sess... chrome/browser/sessions/session_restore_android.cc:47: &entries); On 2012/07/31 17:34:27, joth wrote: > strange indent Done.
Added Yaron to review the Extensions' IF def issue in tab_restore_service.cc
https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... chrome/browser/sessions/session_restore_android.cc:55: NOTIMPLEMENTED(); This should remain NOTREACHED. We have no plans to do this currently, and there's no flow to get here. https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... File chrome/browser/sessions/tab_restore_service.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... chrome/browser/sessions/tab_restore_service.cc:539: #if !defined(OS_ANDROID) I don't believe this is necessary. We now compile with the shell of the extension system. The "if (extension)" check will just always be false on Android.
https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... chrome/browser/sessions/session_restore_android.cc:55: NOTIMPLEMENTED(); On 2012/08/01 20:45:02, Yaron wrote: > This should remain NOTREACHED. We have no plans to do this currently, and > there's no flow to get here. Done. https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... File chrome/browser/sessions/tab_restore_service.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/10003/chrome/browser/ses... chrome/browser/sessions/tab_restore_service.cc:539: #if !defined(OS_ANDROID) On 2012/08/01 20:45:02, Yaron wrote: > I don't believe this is necessary. We now compile with the shell of the > extension system. The "if (extension)" check will just always be false on > Android. Done.
Elliot, I added you to look at session_service_factory.cc . Is there a better way to go about that? https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... chrome/browser/sessions/session_restore_android.cc:38: selected_index = std::max( I'm wrong here, tab.navigations should never be empty. Instead add a DCHECK to that effect around line 24ish). Also, pull 33-41 into a common function in SessionRestore. Maybe static int NormalizeNavigationIndex. https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... File chrome/browser/sessions/session_service_factory.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... chrome/browser/sessions/session_service_factory.cc:12: #if defined(OS_ANDROID) Ping erg on this. I think there is a better way to go about this rather than ifdefing the two methods.
Adding erg@ to take a look at sky's comment at session_service_factory.cc.
On 2012/08/02 14:49:39, felipeg1 wrote: > Adding erg@ to take a look at sky's comment at session_service_factory.cc. I'm not sure I understand the why here. Is the SessionRestoreFactory used so many places that it's better to just return NULL instead of ifdef each instance?
On Thu, Aug 2, 2012 at 10:15 AM, <erg@chromium.org> wrote: > On 2012/08/02 14:49:39, felipeg1 wrote: >> >> Adding erg@ to take a look at sky's comment at session_service_factory.cc. > > > I'm not sure I understand the why here. Is the SessionRestoreFactory used so > many places that it's better to just return NULL instead of ifdef each > instance? > > https://chromiumcodereview.appspot.com/10832080/ Yes. -Scott
On 2012/08/02 17:25:34, sky wrote: > On Thu, Aug 2, 2012 at 10:15 AM, <mailto:erg@chromium.org> wrote: > > I'm not sure I understand the why here. Is the SessionRestoreFactory used so > > many places that it's better to just return NULL instead of ifdef each > > instance? > > Yes. Then I'm not sure if there's a better way other than creating a different factory implementation that has those two methods in it.
Ok, leave it as is then. Do the changes I mentioned in RestoreForeignSessionTab and I'll approve this.
Thanks a lot Sky. I will be looking into the refactoring of session_backend_android.cc shortly after this CL. I will add you as a reviewer too. https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... File chrome/browser/sessions/session_restore_android.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/15001/chrome/browser/ses... chrome/browser/sessions/session_restore_android.cc:38: selected_index = std::max( On 2012/08/02 14:46:43, sky wrote: > I'm wrong here, tab.navigations should never be empty. Instead add a DCHECK to > that effect around line 24ish). > > Also, pull 33-41 into a common function in SessionRestore. Maybe static int > NormalizeNavigationIndex. Done.
https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... File chrome/browser/sessions/session_types.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... chrome/browser/sessions/session_types.cc:136: selected_index = std::max( Combine all this into a single return, eg: return std::max(0, std::min(current_navigation_index, static_cast<... Don't use this-> here. And move the implementation to the header and name it: normlized_navigation_index()
https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... File chrome/browser/sessions/session_types.cc (right): https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... chrome/browser/sessions/session_types.cc:136: selected_index = std::max( On 2012/08/03 17:21:39, sky wrote: > Combine all this into a single return, eg: > > return std::max(0, > std::min(current_navigation_index, > static_cast<... > > Don't use this-> here. > > And move the implementation to the header and name it: > normlized_navigation_index() Done.
Looks like the implementation is still in the .cc and no need for this->
Also, when you move to header the name should be unix_hacker_style. -Scott On Mon, Aug 6, 2012 at 9:54 AM, <felipeg@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... > File chrome/browser/sessions/session_types.cc (right): > > https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... > chrome/browser/sessions/session_types.cc:136: selected_index = std::max( > On 2012/08/03 17:21:39, sky wrote: >> >> Combine all this into a single return, eg: > > >> return std::max(0, >> std::min(current_navigation_index, >> static_cast<... > > >> Don't use this-> here. > > >> And move the implementation to the header and name it: >> normlized_navigation_index() > > > Done. > > https://chromiumcodereview.appspot.com/10832080/
Ops, Sorry, it should be done now. On 2012/08/06 17:04:16, sky wrote: > Also, when you move to header the name should be unix_hacker_style. > > -Scott > > On Mon, Aug 6, 2012 at 9:54 AM, <mailto:felipeg@chromium.org> wrote: > > > > > https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... > > File chrome/browser/sessions/session_types.cc (right): > > > > > https://chromiumcodereview.appspot.com/10832080/diff/13007/chrome/browser/ses... > > chrome/browser/sessions/session_types.cc:136: selected_index = std::max( > > On 2012/08/03 17:21:39, sky wrote: > >> > >> Combine all this into a single return, eg: > > > > > >> return std::max(0, > >> std::min(current_navigation_index, > >> static_cast<... > > > > > >> Don't use this-> here. > > > > > >> And move the implementation to the header and name it: > >> normlized_navigation_index() > > > > > > Done. > > > > https://chromiumcodereview.appspot.com/10832080/
LGTM with the following change. https://chromiumcodereview.appspot.com/10832080/diff/1016/chrome/browser/sess... File chrome/browser/sessions/session_types.h (right): https://chromiumcodereview.appspot.com/10832080/diff/1016/chrome/browser/sess... chrome/browser/sessions/session_types.h:139: this->current_navigation_index, remove this-> here and 140.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/10832080/6012
Change committed as 150342 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
