Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
55423004:
Enable Google Drive offline mode automatically on first run. (Closed)
|
Created:
7 years, 1 month ago by Tim Song Modified:
7 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd flow to enable Google Drive offline mode automatically on first run.
How this works is that we create a WebContents in the background to a Drive
endpoint that will take care of all the offline initialization for the app.
We succeed if a background page is opened; otherwise, we fail if the page does
not load or after a timeout.
BUG=307302
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233501
Patch Set 1 #
Total comments: 26
Patch Set 2 : review fixes #Patch Set 3 : rename flag #Patch Set 4 : Renamed to DriveFirstRunController #
Total comments: 19
Patch Set 5 : Fixes #
Total comments: 6
Patch Set 6 : rename opt-in names #Patch Set 7 : rebase #
Messages
Total messages: 22 (0 generated)
Please take a look, Achuith.
First round. I'll take a closer look in the afternoon. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... File chrome/browser/chromeos/first_run/drive_opt_in_controller.cc (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:53: // mode. If successful, a background page will be opened to do sync the user's "do" unnecessary https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:61: ~DriveOptInWebContentsManager(); virtual https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:85: content::RenderViewHost* render_view_host); OVERRIDE https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:125: GURL url(kDriveOfflineOptInUrl); const? https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:184: // content::WebContentsDelegate overrides: don't think you need this https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:192: content::SessionStorageNamespace* session_storage_namespace) OVERRIDE { don't think you need OVERRIDE here https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:234: // content::NotificationObserver overrides: don't think you need this https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:240: std::string app_id = UTF16ToUTF8( const? https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:243: if (app_id == kDriveHostedAppId) { don't need {} https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... File chrome/browser/chromeos/first_run/drive_opt_in_controller.h (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.h:23: virtual ~DriveOptInController(); Should this method be virtual? https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.h:28: friend class DriveOptInWebContentsManager; Could we get rid of the friend relationship here, move DriveOptInWebContentsManager to the anonymous namespace and just pass in a callback for DriveOptInController::OnOptInDone(bool success)? https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:303: // TODO(tengs): This should be refactored together with the first run UI. Can we file a bug for this? https://codereview.chromium.org/55423004/diff/1/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/55423004/diff/1/chromeos/chromeos_switches.cc... chromeos/chromeos_switches.cc:85: // Enables automatically opting into Google Drive offline mode on first run. I think we have a bunch of memes about auto-opt-in (it's a bit of an oxymoron, right?). Why not just call this kEnableDriveOffline?
https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... File chrome/browser/chromeos/first_run/drive_opt_in_controller.cc (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:53: // mode. If successful, a background page will be opened to do sync the user's On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > "do" unnecessary Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:61: ~DriveOptInWebContentsManager(); On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > virtual Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:85: content::RenderViewHost* render_view_host); On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > OVERRIDE Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:125: GURL url(kDriveOfflineOptInUrl); On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > const? Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:184: // content::WebContentsDelegate overrides: On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > don't think you need this Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:192: content::SessionStorageNamespace* session_storage_namespace) OVERRIDE { On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > don't think you need OVERRIDE here Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:234: // content::NotificationObserver overrides: On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > don't think you need this Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:240: std::string app_id = UTF16ToUTF8( On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > const? Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.cc:243: if (app_id == kDriveHostedAppId) { On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > don't need {} Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... File chrome/browser/chromeos/first_run/drive_opt_in_controller.h (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.h:23: virtual ~DriveOptInController(); On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > Should this method be virtual? Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/first... chrome/browser/chromeos/first_run/drive_opt_in_controller.h:28: friend class DriveOptInWebContentsManager; On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > Could we get rid of the friend relationship here, move > DriveOptInWebContentsManager to the anonymous namespace and just pass in a > callback for DriveOptInController::OnOptInDone(bool success)? Done. https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/login... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/55423004/diff/1/chrome/browser/chromeos/login... chrome/browser/chromeos/login/login_display_host_impl.cc:303: // TODO(tengs): This should be refactored together with the first run UI. On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > Can we file a bug for this? I'll file the bug when this patch lands. https://codereview.chromium.org/55423004/diff/1/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/55423004/diff/1/chromeos/chromeos_switches.cc... chromeos/chromeos_switches.cc:85: // Enables automatically opting into Google Drive offline mode on first run. On 2013/11/01 19:25:15, achuith.bhandarkar wrote: > I think we have a bunch of memes about auto-opt-in (it's a bit of an oxymoron, > right?). Why not just call this kEnableDriveOffline? Done.
I renamed the class to DriveFirstRunController to avoid the potential memetastic consequences. Please take another look.
https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:60: typedef base::Callback<void(bool)> DoneCallback; CompletionCallback? https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:67: void StartLoad(); Please add a function commment for all the functions in this class as well (not including the overrides) https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:124: registrar_.Add(this, chrome::NOTIFICATION_BACKGROUND_CONTENTS_OPENED, Maybe assert that done_callback is not null? https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:153: base::MessageLoop::current()->PostTask( Why do we post instead of running the callback directly? Can you add a comment explaining this? https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:156: base::Unretained(this), Is this safe (using base::Unretained)? If so, can you add a comment explaining why? https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:271: LOG(ERROR) << "Attempting to opt-in but not logged in a regular user."; Attempting to enable offline access to drive but not logged in as a regular user https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:279: LOG(WARNING) << "Drive app not is not installed."; unnecessary "not" https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.h (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.h:24: void EnableOfflineMode(); Please add a short comment for each function in this class explaining what it does or noting that it's used as a callback, etc. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_display_host_impl.cc:302: #if defined(GOOGLE_CHROME_BUILD) Why only for official builds?
https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:60: typedef base::Callback<void(bool)> DoneCallback; On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > CompletionCallback? Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:67: void StartLoad(); On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Please add a function commment for all the functions in this class as well (not > including the overrides) Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:124: registrar_.Add(this, chrome::NOTIFICATION_BACKGROUND_CONTENTS_OPENED, On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Maybe assert that done_callback is not null? Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:153: base::MessageLoop::current()->PostTask( On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Why do we post instead of running the callback directly? Can you add a comment > explaining this? Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:156: base::Unretained(this), On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Is this safe (using base::Unretained)? If so, can you add a comment explaining > why? Done. Replaced with WeakPtr. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:271: LOG(ERROR) << "Attempting to opt-in but not logged in a regular user."; On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Attempting to enable offline access to drive but not logged in as a regular user Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:279: LOG(WARNING) << "Drive app not is not installed."; On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > unnecessary "not" Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.h (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.h:24: void EnableOfflineMode(); On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Please add a short comment for each function in this class explaining what it > does or noting that it's used as a callback, etc. Done. https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_display_host_impl.cc:302: #if defined(GOOGLE_CHROME_BUILD) On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > Why only for official builds? We won't have the Drive app installed by default in non official builds.
https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/login_display_host_impl.cc (right): https://codereview.chromium.org/55423004/diff/130001/chrome/browser/chromeos/... chrome/browser/chromeos/login/login_display_host_impl.cc:302: #if defined(GOOGLE_CHROME_BUILD) On 2013/11/05 01:18:06, Tim Song wrote: > On 2013/11/04 20:33:24, achuith.bhandarkar wrote: > > Why only for official builds? > > We won't have the Drive app installed by default in non official builds. The code handles this case though right? It fails gracefully when it doesn't find the drive app, no?
https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right): https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:64: CompletionCallback completion_callback); const CompletionCallback& https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:77: void OnOptInDone(bool success); OnWebContentsLoaded? https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.h (right): https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.h:30: void OnOptInDone(bool success); OnOfflineInit? OnOfflineEnabled?
Changed patch description to remove opt-in references as well. https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right): https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:64: CompletionCallback completion_callback); On 2013/11/05 01:30:47, achuith.bhandarkar wrote: > const CompletionCallback& Done. https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.cc:77: void OnOptInDone(bool success); On 2013/11/05 01:30:47, achuith.bhandarkar wrote: > OnWebContentsLoaded? Ah sorry, that comment is a bit inaccurate. Renamed to OnOfflineInit. https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... File chrome/browser/chromeos/first_run/drive_first_run_controller.h (right): https://codereview.chromium.org/55423004/diff/210001/chrome/browser/chromeos/... chrome/browser/chromeos/first_run/drive_first_run_controller.h:30: void OnOptInDone(bool success); On 2013/11/05 01:30:47, achuith.bhandarkar wrote: > OnOfflineInit? OnOfflineEnabled? Done.
lgtm. Thanks for taking care of the review comments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/55423004/290001
Failed to apply patch for chromeos/chromeos_switches.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file chromeos/chromeos_switches.h Hunk #1 FAILED at 41. 1 out of 1 hunk FAILED -- saving rejects to file chromeos/chromeos_switches.h.rej Patch: chromeos/chromeos_switches.h Index: chromeos/chromeos_switches.h diff --git a/chromeos/chromeos_switches.h b/chromeos/chromeos_switches.h index 49071cc1e857c3722b61c67d01568b683191bb9a..20bd01da51798c04539dabfbf4b28b0c7eb518dd 100644 --- a/chromeos/chromeos_switches.h +++ b/chromeos/chromeos_switches.h @@ -41,6 +41,7 @@ CHROMEOS_EXPORT extern const char kEchoExtensionPath[]; CHROMEOS_EXPORT extern const char kEnableBackgroundLoader[]; CHROMEOS_EXPORT extern const char kEnableCarrierSwitching[]; CHROMEOS_EXPORT extern const char kEnableChromeCaptivePortalDetector[]; +CHROMEOS_EXPORT extern const char kEnableDriveOfflineFirstRun[]; CHROMEOS_EXPORT extern const char kEnableIMEModeIndicator[]; CHROMEOS_EXPORT extern const char kEnableKioskMode[]; CHROMEOS_EXPORT extern const char kEnableRequestTabletSite[];
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/55423004/380001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/55423004/380001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/55423004/380001
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=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tengs@chromium.org/55423004/380001
Message was sent while issue was closed.
Change committed as 233501
Message was sent while issue was closed.
On 2013/11/07 06:18:14, I haz the power (commit-bot) wrote: > Change committed as 233501 Hi Tim, why didn't you added me as reviewer here? I'm owner of "chrome/browser/chromeos/first_run". This directory is supposed to be used for new first-run UI only. In addition as I know it's possible to run code from extension right after extension install (for example "Tips & Tricks" opens its window after install). I think its better to move this code to Drive extension's private API as long as it's related to Drive extension, and run this code on extension install.
Message was sent while issue was closed.
Hi Pavel, I was looking for a good place to put these files, and "chrome/browser/chromeos/first_run" seemed like the best choice. Basically I wanted to avoid a confusing namespace situation like between "chrome/browser/chromeos/app_mode/" and "chrome/browser/chromeos/kiosk_mode". My apologies, I should have consulted you about putting the files here, but I think it's the right place to put it. Because the Drive app is a hosted app that is also widely used on other platforms (https://chrome.google.com/webstore/detail/google-drive/apdfllckaahabafndbhiea...), we need this bootstrapping code. The "Tips & Tricks" app is a v2 app and can use event pages and the chrome.runtime.onInstalled API to efficiently execute only once on first run. However, we can't use event pages for hosted apps, and adding a normal background page to the Drive app would be too costly as it will always be executed on browser startup. Additionally, my reasoning that this code shouldn't belong to "chrome/browser/chromeos/drive" is that that although ostensibly related, it is a completely different code path for the files app. I think "first_run" better describes what this code does, and I think there is an opportunity to refactor the first run code in login_display_host_impl.cc so we don't have duplicate code for starting stuff for new users. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
