drive: Add Profile* as a member of DriveProtocolHandler.

drive: Add Profile* as a member of DriveProtocolHandler.

A Profile* can be passed to the constructor of DriveProtocolHandler and
the handler uses the given Profile* for processing the request.
This will help us adding unittests and introducting support for multiple profiles.
If the handler is constructed without Profile*,  it will use the profile from
ProfileManager::GetDefaultProfile().

BUG=167249
TEST=unittests. try opening a URL of 'drive://' (e.g. HTML file stored in Google Drive) on Chrome OS.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176062

[Haruki Sato, 2013-01-07 04:59:58]

I'd like to start from adding a Profile* member.
This CL keeps the initialization in profile_io_data as is (i.e. not passing any
Profile* to DriveProtocolHandler).
Could you take a look and give me advice if any?

Thanks.

[hashimoto, 2013-01-07 05:18:45]

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:919:
DriveProtocolHandler::DriveProtocolHandler()
How about initializing DriveProtocolHandler with Profile* and stop using
GetDefaultProfile()?
You can use ProfileParams::profile within profile_io_data.cc
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_protocol_handler.h:8: #include
"chrome/browser/profiles/profile.h"
How about replacing this include with a forward declaration "class Profile;"?

[Haruki Sato, 2013-01-07 06:24:42]

Thank you for the review.
I'm not 100% sure if we can pass the Profile* this way so any advice is welcome.
(The comment says something about IsValidProfile.)

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:919:
DriveProtocolHandler::DriveProtocolHandler()
On 2013/01/07 05:18:45, hashimoto wrote:
> How about initializing DriveProtocolHandler with Profile* and stop using
> GetDefaultProfile()?
> You can use ProfileParams::profile within profile_io_data.cc
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...

Done.

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_protocol_handler.h:8: #include
"chrome/browser/profiles/profile.h"
On 2013/01/07 05:18:45, hashimoto wrote:
> How about replacing this include with a forward declaration "class Profile;"?

Ah we need just a pointer to Profile in DriveProtocolHandler.
Done. Thanks!

[hashimoto, 2013-01-07 06:32:32]

https://codereview.chromium.org/11785018/diff/9002/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/11785018/diff/9002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:96: profile ? profile :
ProfileManager::GetDefaultProfile());
It looks like now we can stop using ProfileManager::GetDefaultProfile.

https://codereview.chromium.org/11785018/diff/9002/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:929: return new
DriveURLRequestJob(
nit: No need to have a newline here.

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.h:23: // If |profile_| is
NULL, the profile from ProfileManager::GetDefaultProfile()
Since we don't allow NULL Profile* now, please fix this comment.

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:648: Profile* profile =
reinterpret_cast<Profile*>(profile_params_->profile);
Please call IsValidProfile like chrome_network_delegate.cc
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...

[Haruki Sato, 2013-01-07 08:15:47]

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.h:23: // If |profile_| is
NULL, the profile from ProfileManager::GetDefaultProfile()
On 2013/01/07 06:32:32, hashimoto wrote:
> Since we don't allow NULL Profile* now, please fix this comment.

Done. Thanks.

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/11785018/diff/3004/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:648: Profile* profile =
reinterpret_cast<Profile*>(profile_params_->profile);
On 2013/01/07 06:32:32, hashimoto wrote:
> Please call IsValidProfile like chrome_network_delegate.cc
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...

Done.
Thank you for the advice. As chatted offline. We are adding IsValidProfile check
in DriveProtocolHandler implementation.

[hashimoto, 2013-01-07 08:22:51]

lgtm

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:93: DriveSystemService*
GetSystemService(Profile* profile) {
How about moving the IsValidProfile check to this function to avoid code
duplication?

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.h:10: class Profile;
No need to have this now.

[Haruki Sato, 2013-01-07 08:54:48]

David, Could you take a look as an OWNER of chrome/browser/profiles ?

Thanks.

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:93: DriveSystemService*
GetSystemService(Profile* profile) {
On 2013/01/07 08:22:51, hashimoto wrote:
> How about moving the IsValidProfile check to this function to avoid code
> duplication?

Good catch! Thanks!
Done.

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/1007/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_protocol_handler.h:10: class Profile;
On 2013/01/07 08:22:51, hashimoto wrote:
> No need to have this now.

Done. Thanks.

[Haruki Sato, 2013-01-08 10:32:24]

ping? @davemoore
Could you help me on this?

Thanks.

[DaveMoore, 2013-01-08 19:07:59]

https://codereview.chromium.org/11785018/diff/17001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/17001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_protocol_handler.h:14: explicit
DriveProtocolHandler(void* profile_id);
Why would we take a void* instead of a Profile*

[Haruki Sato, 2013-01-08 23:08:27]

https://codereview.chromium.org/11785018/diff/17001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_protocol_handler.h (right):

https://codereview.chromium.org/11785018/diff/17001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_protocol_handler.h:14: explicit
DriveProtocolHandler(void* profile_id);
On 2013/01/08 19:07:59, DaveMoore wrote:
> Why would we take a void* instead of a Profile*

I just would like to have the same scheme as we already have in
profile_io_data.h and chrome_network_delegate.cc.
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/profiles/pr...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...
Do you think just having Profile* is better?

[DaveMoore, 2013-01-09 17:56:18]

lgtm

[commit-bot: I haz the power, 2013-01-10 00:15:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/11785018/17001

[commit-bot: I haz the power, 2013-01-10 01:17:50]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-10 01:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/11785018/17001

[commit-bot: I haz the power, 2013-01-10 02:42:00]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-10 07:00:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/11785018/29001

[commit-bot: I haz the power, 2013-01-10 14:07:21]

Change committed as 176062