app_list: Drive app integration.

app_list: Drive app integration.

- DriveAppProvider to map Drive apps to chrome apps or local url apps;
- DriveAppMapping to track the mapped chrome apps;
- DriveServiceBridge to wrap DriveAPIService and DriveAppRegistry to
  provide the user Drive apps info;
- Put feature behind "--enable-drive-apps-in-app-list";

BUG=358791, 345066
TEST=DriveAppProviderTest.*:DriveAppMappingTest.*

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

[xiyuan, 2014-05-29 22:13:10]

Suggested split:
kinaba@:
  c/b/drive/fake_drive_service.*
  c/b/ui/app_list/drive/drive_service_bridge.*

benwells@, for everything else.

Thanks.

[benwells, 2014-05-30 03:30:02]

I have a high level question first: why is c/b/ui/app_list/drive stuff in
c/b/ui/app_list? It doesn't seem to need anything app-listy, could it be
c/b/extensions/drive? Or c/b/drive?

[xiyuan, 2014-05-30 05:55:45]

On 2014/05/30 03:30:02, benwells wrote:
> I have a high level question first: why is c/b/ui/app_list/drive stuff in
> c/b/ui/app_list? It doesn't seem to need anything app-listy, could it be
> c/b/extensions/drive? Or c/b/drive?

I put it there because these files provides integration of Drive to app
launcher. But I see your point that dependency-wise, the files are more closely
coupled with drive and extension system. Now as I think about it, maybe
c/b/extension/drive would be a better place since we are introducing Drive apps
into extension system. What do you guys think?

[kinaba, 2014-05-30 06:20:14]

LGTM for *drive_service* with a nit.

On 2014/05/30 05:55:45, xiyuan wrote:
> On 2014/05/30 03:30:02, benwells wrote:
> > I have a high level question first: why is c/b/ui/app_list/drive stuff in
> > c/b/ui/app_list? It doesn't seem to need anything app-listy, could it be
> > c/b/extensions/drive? Or c/b/drive?
> 
> I put it there because these files provides integration of Drive to app
> launcher. But I see your point that dependency-wise, the files are more
closely
> coupled with drive and extension system. Now as I think about it, maybe
> c/b/extension/drive would be a better place since we are introducing Drive
apps
> into extension system. What do you guys think?

Sounds making sense.

[kinaba, 2014-05-30 06:20:29]

https://codereview.chromium.org/308003005/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/drive/drive_service_bridge.h (right):

https://codereview.chromium.org/308003005/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/drive/drive_service_bridge.h:34: };
nit: DISALLOW_COPY_AND_ASSIGN? (or remove #include base/macros.h)

[xiyuan, 2014-05-30 19:15:30]

Moved DriveAppProvider and friends to c/b/extensions/drive and put under
extensions namespace.

PTAL. Thanks.

https://codereview.chromium.org/308003005/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/drive/drive_service_bridge.h (right):

https://codereview.chromium.org/308003005/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/drive/drive_service_bridge.h:34: };
On 2014/05/30 06:20:29, kinaba wrote:
> nit: DISALLOW_COPY_AND_ASSIGN? (or remove #include base/macros.h)

Removed macros.h since this class intends to be an interface and no need for
DISALLOW_COPY_AND_ASSIGN.

[benwells, 2014-06-02 01:24:06]

+yoz for changes in chrome/browser/extensions to get a core extensions
developer's view on it.

Also +cc kalman who might care about this.

[xiyuan, 2014-06-02 18:10:04]

On 2014/06/02 01:24:06, benwells wrote:
> +yoz for changes in chrome/browser/extensions to get a core extensions
> developer's view on it.
> 
> Also +cc kalman who might care about this.

Background for yoz and kalman:
Master ddoc: go/cros-drive-apps
eng doc: go/drive-apps-in-launcher

The CL contains the implementation based on Ben's idea to provide a mapped app
(corresponding Chrome app or a locally created unsyncable url app) for each of
user's Drive app so that Drive apps is provided on launcher (and other UI
surfaces).

[not at google - send to devlin, 2014-06-02 18:59:30]

I've been commenting on the doc.

Without reading the code: what happens when I uninstall an app locally? does it
also get uninstalled on drive?

I find this a *little* confusing actually, because the app launcher looks like
the app launcher on the Google bar thing. I would expect it to be linked to that
(which can install third party things, somehow, I think) not necessarily to
drive.

Anyhow, in terms of implementation, also have a look at the new remote_install
feature we have, http://crbug.com/365737, it might provide an easier way to do
this.

[xiyuan, 2014-06-02 19:51:42]

On 2014/06/02 18:59:30, kalman wrote:
> I've been commenting on the doc.
> 
> Without reading the code: what happens when I uninstall an app locally? does
it
> also get uninstalled on drive?
> 

Yes, the Drive app will be disconnected (i.e. uninstalled) from the user's
account when the corresponding Chrome app (from CWS or locally created) is
uninstalled.

> I find this a *little* confusing actually, because the app launcher looks like
> the app launcher on the Google bar thing. I would expect it to be linked to
that
> (which can install third party things, somehow, I think) not necessarily to
> drive.
> 

Drive also has its app launcher similar UI surface now. The intention of the CL
is to bring all launcher surfaces more close to each other. That is, if you see
a Drive app in Drive's launcher, you will see it in other launcher too.

> Anyhow, in terms of implementation, also have a look at the new remote_install
> feature we have, http://crbug.com/365737, it might provide an easier way to do
> this.

The remote_install is useful to Chrome apps. However, Drive apps are guaranteed
to have a creation URL but not always have a corresponding Chrome app. We would
need to create a local hosted URL app to handle such case.

[xiyuan, 2014-06-03 17:55:04]

+abodenha

[benwells, 2014-06-03 23:43:27]

On 2014/06/03 17:55:04, xiyuan wrote:
> +abodenha

I think the changes to the core extension system for this are pretty trivial. My
question to yoz / kalman is: should the drive folder be in c/b/extensions? It
could also be in c/b/apps, or maybe even in a new c/b/drive.

[Yoyo Zhou, 2014-06-04 00:02:36]

On 2014/06/03 23:43:27, benwells wrote:
> On 2014/06/03 17:55:04, xiyuan wrote:
> > +abodenha
> 
> I think the changes to the core extension system for this are pretty trivial.
My
> question to yoz / kalman is: should the drive folder be in c/b/extensions? It
> could also be in c/b/apps, or maybe even in a new c/b/drive.

Ah, I didn't know what you were asking me for. I think chrome/browser/apps
(perhaps chrome/browser/apps/drive) would be a better fit.

[benwells, 2014-06-04 00:59:25]

On 2014/06/04 00:02:36, Yoyo Zhou wrote:
> On 2014/06/03 23:43:27, benwells wrote:
> > On 2014/06/03 17:55:04, xiyuan wrote:
> > > +abodenha
> > 
> > I think the changes to the core extension system for this are pretty
trivial.
> My
> > question to yoz / kalman is: should the drive folder be in c/b/extensions?
It
> > could also be in c/b/apps, or maybe even in a new c/b/drive.
> 
> Ah, I didn't know what you were asking me for. I think chrome/browser/apps
> (perhaps chrome/browser/apps/drive) would be a better fit.

Sorry about that, I should have been clearer.

xiyuan: do you want to move the stuff into c/b/apps/drive? Sorry for all the run
around.

[benwells, 2014-06-04 01:02:32]

On 2014/06/04 00:59:25, benwells wrote:
> On 2014/06/04 00:02:36, Yoyo Zhou wrote:
> > On 2014/06/03 23:43:27, benwells wrote:
> > > On 2014/06/03 17:55:04, xiyuan wrote:
> > > > +abodenha
> > > 
> > > I think the changes to the core extension system for this are pretty
> trivial.
> > My
> > > question to yoz / kalman is: should the drive folder be in c/b/extensions?
> It
> > > could also be in c/b/apps, or maybe even in a new c/b/drive.
> > 
> > Ah, I didn't know what you were asking me for. I think chrome/browser/apps
> > (perhaps chrome/browser/apps/drive) would be a better fit.
> 
> Sorry about that, I should have been clearer.
> 
> xiyuan: do you want to move the stuff into c/b/apps/drive? Sorry for all the
run
> around.

Also, can you add a c/b/apps/drive/OWNERS file with whoever you think
appropriate? I don't feel like I should be reviewing that stuff, but I can
rubberstamp this one with an OWNERS file to approve the addition of the folder
into apps. I can also review the remaining changes in app_list and extensions
once the drive stuff has been moved.

[xiyuan, 2014-06-04 18:02:03]

On 2014/06/04 01:02:32, benwells wrote:
> Also, can you add a c/b/apps/drive/OWNERS file with whoever you think
> appropriate? I don't feel like I should be reviewing that stuff, but I can
> rubberstamp this one with an OWNERS file to approve the addition of the folder
> into apps. I can also review the remaining changes in app_list and extensions
> once the drive stuff has been moved.

CL updated to move into c/b/apps and added an OWNERS file.

Ben, I asked you because you work on launcher and have great insights of the
extensions/apps system as well. :)

[xiyuan, 2014-06-04 18:04:44]

+tapted

Trent, could your help reviewing c/b/apps/drive/*? Thanks.

cc calamity, stevenjb since I enlisted you guys as c/b/apps/drive/OWNERS and you
are welcome to review the CL too. :p

[benwells, 2014-06-04 22:39:58]

On 2014/06/04 18:04:44, xiyuan wrote:
> +tapted
> 
> Trent, could your help reviewing c/b/apps/drive/*? Thanks.
> 
> cc calamity, stevenjb since I enlisted you guys as c/b/apps/drive/OWNERS and
you
> are welcome to review the CL too. :p

Sorry I miscommunicated. I didn't mean I didn't want to review, more that you
shouldn't feel obliged to get me to review it. I don't mind looking at all. I'll
assume the drive API level stuff is alright though.

Trent, feel free to look as well if y'all want.

[benwells, 2014-06-05 01:24:38]

Some initial comments and questions.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:179: // Drive apps goes with
the user's account and url apps will be created when
Nit: goes -> go

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.h (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:20: namespace extensions {
This and the other files in c/b/apps shouldn't be in the extensions namespace.
They shouldn't be in any namespace.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.h (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:60: // Migrate launcher model
data of |old_chrome_app_id| to |new_chrome_app_id|.
Can you explain under which circumstances this can happen?

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:66: // to keep chrome app over
URL app and new app over existing app.
I don't understand this comment, specifically the bits about keeping one over
another. Is new_app the chrome / url app?

Maybe also adding when this happens would make it easier to understand.

[not at google - send to devlin, 2014-06-05 16:54:04]

drive-by of crx_installer.cc

thanks for putting this behind a flag!

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:233: ? Extension::FROM_BOOKMARK
what does this mean?

[xiyuan, 2014-06-05 17:06:19]

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:233: ? Extension::FROM_BOOKMARK
On 2014/06/05 16:54:05, kalman wrote:
> what does this mean?

This is to keep the current behavior, where Extension::FROM_BOOKMARK  is the
hardcoded flags of the URL app created from InstallWebApp and existing callers
of InstallWebApp does not specify any creation flags.

Would you prefer to update the existing callers instead? There are a couple of
them, I think.

[not at google - send to devlin, 2014-06-05 17:17:59]

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:233: ? Extension::FROM_BOOKMARK
On 2014/06/05 17:06:19, xiyuan wrote:
> On 2014/06/05 16:54:05, kalman wrote:
> > what does this mean?
> 
> This is to keep the current behavior, where Extension::FROM_BOOKMARK  is the
> hardcoded flags of the URL app created from InstallWebApp and existing callers
> of InstallWebApp does not specify any creation flags.
> 
> Would you prefer to update the existing callers instead? There are a couple of
> them, I think.

Yes I would prefer that. DCHECK if you wish.

[xiyuan, 2014-06-05 17:48:32]

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:179: // Drive apps goes with
the user's account and url apps will be created when
On 2014/06/05 01:24:39, benwells wrote:
> Nit: goes -> go

Done.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.h (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:20: namespace extensions {
On 2014/06/05 01:24:39, benwells wrote:
> This and the other files in c/b/apps shouldn't be in the extensions namespace.
> They shouldn't be in any namespace.

Done.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.h (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:60: // Migrate launcher model
data of |old_chrome_app_id| to |new_chrome_app_id|.
On 2014/06/05 01:24:39, benwells wrote:
> Can you explain under which circumstances this can happen?

Updated the comment to explain when this is called.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:66: // to keep chrome app over
URL app and new app over existing app.
On 2014/06/05 01:24:39, benwells wrote:
> I don't understand this comment, specifically the bits about keeping one over
> another. Is new_app the chrome / url app?
> 
> Maybe also adding when this happens would make it easier to understand.

Update the comments to be more specific.

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:233: ? Extension::FROM_BOOKMARK
On 2014/06/05 17:17:58, kalman wrote:
> On 2014/06/05 17:06:19, xiyuan wrote:
> > On 2014/06/05 16:54:05, kalman wrote:
> > > what does this mean?
> > 
> > This is to keep the current behavior, where Extension::FROM_BOOKMARK  is the
> > hardcoded flags of the URL app created from InstallWebApp and existing
callers
> > of InstallWebApp does not specify any creation flags.
> > 
> > Would you prefer to update the existing callers instead? There are a couple
of
> > them, I think.
> 
> Yes I would prefer that. DCHECK if you wish.

Done. Two callers: BookmarkAppHelper and AppLauncherHandler updated.

[not at google - send to devlin, 2014-06-05 17:50:45]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:232: DCHECK(creation_flags_ &
Extension::FROM_BOOKMARK);
could you briefly explain why this check is made? the function name
"InstallWebApp" does not adequately imply the validity of this DCHECK without
some sort of explanation.

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:245: int creation_flags,
this shouldn't be necessary, it can be read from creation_flags_.

[xiyuan, 2014-06-05 18:06:54]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:232: DCHECK(creation_flags_ &
Extension::FROM_BOOKMARK);
On 2014/06/05 17:50:45, kalman wrote:
> could you briefly explain why this check is made? the function name
> "InstallWebApp" does not adequately imply the validity of this DCHECK without
> some sort of explanation.

This is inherited from the current ConvertWebAppToExtension behavior.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I put a DCHECK so that I don't break any expectations. This may not be
necessary.

Added a comment "The app created should always be marked as a bookmark app." to
explain the DCHECK. But I am open to removing it all together if you feel that
is the right thing.

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:245: int creation_flags,
On 2014/06/05 17:50:45, kalman wrote:
> this shouldn't be necessary, it can be read from creation_flags_.

I am slightly prefer to have an arg here because it avoid potential read/write
races since this function runs on a different thread (even though it is very
unlikely to happen). If we can access members, then |install_directory| is also
not necessarily to be an argument.

[not at google - send to devlin, 2014-06-05 18:11:54]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:232: DCHECK(creation_flags_ &
Extension::FROM_BOOKMARK);
On 2014/06/05 18:06:53, xiyuan wrote:
> On 2014/06/05 17:50:45, kalman wrote:
> > could you briefly explain why this check is made? the function name
> > "InstallWebApp" does not adequately imply the validity of this DCHECK
without
> > some sort of explanation.
> 
> This is inherited from the current ConvertWebAppToExtension behavior.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> I put a DCHECK so that I don't break any expectations. This may not be
> necessary.
> 
> Added a comment "The app created should always be marked as a bookmark app."
to
> explain the DCHECK. But I am open to removing it all together if you feel that
> is the right thing.

Yeah, I think that a comment like "Web apps can only ever be converted to
bookmark apps".

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:245: int creation_flags,
On 2014/06/05 18:06:53, xiyuan wrote:
> On 2014/06/05 17:50:45, kalman wrote:
> > this shouldn't be necessary, it can be read from creation_flags_.
> 
> I am slightly prefer to have an arg here because it avoid potential read/write
> races since this function runs on a different thread (even though it is very
> unlikely to happen). If we can access members, then |install_directory| is
also
> not necessarily to be an argument.

I see your point. It looks like a bunch of this accessing local variables
happens anyway, like in OnUnpackSuccess. Perhaps mark it (and install
directory?) as const for some peace of mind.

[xiyuan, 2014-06-05 18:27:36]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:232: DCHECK(creation_flags_ &
Extension::FROM_BOOKMARK);
On 2014/06/05 18:11:54, kalman wrote:
> On 2014/06/05 18:06:53, xiyuan wrote:
> > On 2014/06/05 17:50:45, kalman wrote:
> > > could you briefly explain why this check is made? the function name
> > > "InstallWebApp" does not adequately imply the validity of this DCHECK
> without
> > > some sort of explanation.
> > 
> > This is inherited from the current ConvertWebAppToExtension behavior.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > 
> > I put a DCHECK so that I don't break any expectations. This may not be
> > necessary.
> > 
> > Added a comment "The app created should always be marked as a bookmark app."
> to
> > explain the DCHECK. But I am open to removing it all together if you feel
that
> > is the right thing.
> 
> Yeah, I think that a comment like "Web apps can only ever be converted to
> bookmark apps".

Done.

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:245: int creation_flags,
On 2014/06/05 18:11:54, kalman wrote:
> I see your point. It looks like a bunch of this accessing local variables
> happens anyway, like in OnUnpackSuccess. Perhaps mark it (and install
> directory?) as const for some peace of mind.

Right. I might be over-thinking it. :p

- Removed creation_flags and install_direction from arguments use members;
- |creation_flags_| could not be const because it has a setter;
- |install_directory_| is changed to be const;

[not at google - send to devlin, 2014-06-05 18:31:45]

happy with crx_installer, don't wait for my lg.

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:245: int creation_flags,
On 2014/06/05 18:27:35, xiyuan wrote:
> On 2014/06/05 18:11:54, kalman wrote:
> > I see your point. It looks like a bunch of this accessing local variables
> > happens anyway, like in OnUnpackSuccess. Perhaps mark it (and install
> > directory?) as const for some peace of mind.
> 
> Right. I might be over-thinking it. :p
> 
> - Removed creation_flags and install_direction from arguments use members;
> - |creation_flags_| could not be const because it has a setter;
> - |install_directory_| is changed to be const;

ah.

[stevenjb, 2014-06-05 20:15:16]

Mostly just looking at app_list_syncable_service, but that part lgtm.

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
WAS_INSTALLED_BY_DEFAULT might trigger other behaviors, maybe we should add an
explicit IS_SYNCABLE flag?

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:57: FactorySet
factories;
nit: 'dependent_factories'

[xiyuan, 2014-06-05 20:36:14]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
On 2014/06/05 20:15:16, stevenjb wrote:
> WAS_INSTALLED_BY_DEFAULT might trigger other behaviors, maybe we should add an
> explicit IS_SYNCABLE flag?

The converted app is sort of a default (in a sense that it comes as part of the
user's account). I checked where WAS_INSTALLED_BY_DEFAULT is used (not many of
places) and seems to be safe to use.

But I'll leave this to the judgement of extensions OWNERs (benwells@, yoz@,
kalman@) and suggestions of what to use.

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:57: FactorySet
factories;
On 2014/06/05 20:15:16, stevenjb wrote:
> nit: 'dependent_factories'

Done in the next patch.

[Yoyo Zhou, 2014-06-05 20:45:33]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
On 2014/06/05 20:36:14, xiyuan wrote:
> On 2014/06/05 20:15:16, stevenjb wrote:
> > WAS_INSTALLED_BY_DEFAULT might trigger other behaviors, maybe we should add
an
> > explicit IS_SYNCABLE flag?
> 
> The converted app is sort of a default (in a sense that it comes as part of
the
> user's account). I checked where WAS_INSTALLED_BY_DEFAULT is used (not many of
> places) and seems to be safe to use.
> 
> But I'll leave this to the judgement of extensions OWNERs (benwells@, yoz@,
> kalman@) and suggestions of what to use.

This isn't really what we mean by default apps; those come with the chrome
installation, rather than with a drive login. One of these would be better:

- if we could determine from the extension that it was a drive app, and
therefore not to sync it (see several existing cases in GetSyncType in
sync_helper.cc)
- if there was a new creation flag like SYNC_NOT_ALLOWED

[not at google - send to devlin, 2014-06-05 20:47:47]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
On 2014/06/05 20:36:14, xiyuan wrote:
> On 2014/06/05 20:15:16, stevenjb wrote:
> > WAS_INSTALLED_BY_DEFAULT might trigger other behaviors, maybe we should add
an
> > explicit IS_SYNCABLE flag?
> 
> The converted app is sort of a default (in a sense that it comes as part of
the
> user's account). I checked where WAS_INSTALLED_BY_DEFAULT is used (not many of
> places) and seems to be safe to use.
> 
> But I'll leave this to the judgement of extensions OWNERs (benwells@, yoz@,
> kalman@) and suggestions of what to use.

I agree with Steven, even if this works at the moment it's absolutely not
"installed by default" and it's things like that have made sync bugs creep in in
the past.

Attaching this stuff to "creation flags" isn't really right anyway. Things like
it being a bookmark is a property of the environment not a property of the
extension itself, if that makes sense (basically: it should be stored in Prefs
not on the Extension object, if it needs to be).

If you don't want to sync it, how about a set_do_not_sync(..) function on the
CrxInstaller.

[xiyuan, 2014-06-05 21:17:06]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
Okay.

- Add a ExtensionPrefs backed boolean property do_not_sync() to Extension;
- The property is initiated in CrxInstaller when creating the URL app;
- GetSyncType checks this property and returns SYNC_TYPE_NONE to disable sync
for this item. 

How does this sound?

[Yoyo Zhou, 2014-06-05 21:18:20]

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/308003005/diff/160001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.cc:182:
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
On 2014/06/05 21:17:06, xiyuan wrote:
> Okay.
> 
> - Add a ExtensionPrefs backed boolean property do_not_sync() to Extension;
> - The property is initiated in CrxInstaller when creating the URL app;
> - GetSyncType checks this property and returns SYNC_TYPE_NONE to disable sync
> for this item. 
> 
> How does this sound?

Minor detail: you can check this in IsSyncable instead of GetSyncType. But
sounds fine to me.

[not at google - send to devlin, 2014-06-05 22:22:54]

sounds good, thanks.

[xiyuan, 2014-06-05 23:31:45]

CL updated to use an ExtensionPrefs backed boolean prop for disable sync for URL
apps as discussed.

I changed to use "is_syncable" instead of "do_not_sync" because it is more
intuitive to associate that with true/false boolean value with whether sync is
allowed or not.

[tapted, 2014-06-05 23:56:42]

On 2014/06/04 22:39:58, benwells wrote:
> On 2014/06/04 18:04:44, xiyuan wrote:
> > +tapted
> > 
> > Trent, could your help reviewing c/b/apps/drive/*? Thanks.
> > 
> > cc calamity, stevenjb since I enlisted you guys as c/b/apps/drive/OWNERS and
> you
> > are welcome to review the CL too. :p
> 
> Sorry I miscommunicated. I didn't mean I didn't want to review, more that you
> shouldn't feel obliged to get me to review it. I don't mind looking at all.
I'll
> assume the drive API level stuff is alright though.
> 
> Trent, feel free to look as well if y'all want.

I had a look through c/b/apps/drive in patches 7/8, and drilled down in a few
places. That's some quality coding :). Nothing stood out to me. My only thought
was that base::DictionaryValue could probably benefit from a new layer of
abstraction with some built-in type safety and helper functions. But that's a
yak for another day.

lgtm.

[benwells, 2014-06-06 00:31:05]

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.h (right):

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:20: class AppListModel;
This dependency is not good. My questions about MigrateModelSettings were for me
to understand why this is needed.

To remove the dependency, we could introduce some sort of delegate that the app
list model listens to.

But I'm wondering if we need to (see below).

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:64: void
MigrateModelSettings(const std::string& drive_app_id,
Has this behavior come from the UX team? I couldn't find it described in either
of the design docs.

I think it will be confusing as described. If a user installs an app it should
be treated like any other new app, and be added to the top level in the folder.

This also means there is no need for the dependency back onto app list.

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:68: // Updates drive app id to
chrome app id mapping. If the drive app has
Nit: has -> was.

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:74: // preferred and |new_app|
will auto uninstalled. Otherwise, if |new_app| is
Again I'm not sure of this behaviour. I think the only way it can happen is if
the user manually adds a URL app. In that case we shouldn't auto uninstall it.

[not at google - send to devlin, 2014-06-06 01:26:30]

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:735:
mutable_extension->set_is_syncable(is_syncable_);
this means that every extension will end up with a redundant "is_syncable" pref,
right?

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:207: // synced. Default is true.
pet peeve is optional boolean values whose default value is true. can you phrase
this as a negation?

also: prefs go somewhere else these days, I can't remember... yoz@ ?

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:2182: new
base::FundamentalValue(extension->is_syncable()));
only set it if its value is non-default

[xiyuan, 2014-06-06 01:32:01]

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.h (right):

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:64: void
MigrateModelSettings(const std::string& drive_app_id,
On 2014/06/06 00:31:04, benwells wrote:
> Has this behavior come from the UX team? I couldn't find it described in
either
> of the design docs.
> 
> I think it will be confusing as described. If a user installs an app it should
> be treated like any other new app, and be added to the top level in the
folder.
> 
> This also means there is no need for the dependency back onto app list.

The behavior is not defined by UX. I guess I put it this way is partly because
my first proposal is to have typed items and changing the item type dynamically.
I stuck with that impression and feel that we should keep the model settings
when that needs to happen.

I agree this could be confusing. I will remove this and get rid of AppListModel
dependency. We can revisit this when UX requests it.

https://codereview.chromium.org/308003005/diff/180001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.h:74: // preferred and |new_app|
will auto uninstalled. Otherwise, if |new_app| is
On 2014/06/06 00:31:04, benwells wrote:
> Again I'm not sure of this behaviour. I think the only way it can happen is if
> the user manually adds a URL app. In that case we shouldn't auto uninstall it.

Agree. The CL does too much. I will change it to keep |new_app| in this case.

[Yoyo Zhou, 2014-06-06 01:39:27]

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:735:
mutable_extension->set_is_syncable(is_syncable_);
On 2014/06/06 01:26:29, kalman wrote:
> this means that every extension will end up with a redundant "is_syncable"
pref,
> right?

Yeah, that's why I was suggesting the negation. Only in a few cases would we
need to set is_not_syncable.

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:207: // synced. Default is true.
On 2014/06/06 01:26:29, kalman wrote:
> pet peeve is optional boolean values whose default value is true. can you
phrase
> this as a negation?
> 
> also: prefs go somewhere else these days, I can't remember... yoz@ ?

You mean this const char? It's fine to go here if it's only used in this file.

[xiyuan, 2014-06-06 04:12:37]

CL updated.

- Extension pref changed from "is_syncable" to "do_not_sync" and only write when
it is not the default; Still using is_syncable in Extension and ExtensionPrefs
interface since it is easier to think without negation;
- AppListModel deps removed and simplified mapping update code;

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/308003005/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.cc:735:
mutable_extension->set_is_syncable(is_syncable_);
On 2014/06/06 01:39:27, Yoyo Zhou wrote:
> On 2014/06/06 01:26:29, kalman wrote:
> > this means that every extension will end up with a redundant "is_syncable"
> pref,
> > right?
> 
> Yeah, that's why I was suggesting the negation. Only in a few cases would we
> need to set is_not_syncable.

The prefs writing code is changed to only write the pref when it is not the
default.

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:207: // synced. Default is true.
On 2014/06/06 01:26:29, kalman wrote:
> pet peeve is optional boolean values whose default value is true. can you
phrase
> this as a negation?

Changed the pref to "do_not_sync".

https://codereview.chromium.org/308003005/diff/200001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:2182: new
base::FundamentalValue(extension->is_syncable()));
On 2014/06/06 01:26:29, kalman wrote:
> only set it if its value is non-default

Done.

[not at google - send to devlin, 2014-06-06 19:15:05]

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
extensions/common/extension.h:334: void set_is_syncable(bool is_syncable) {
is_syncable_ = is_syncable; }
why did you need to add this here? it should be *only* a property of prefs.

[xiyuan, 2014-06-06 19:24:30]

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
extensions/common/extension.h:334: void set_is_syncable(bool is_syncable) {
is_syncable_ = is_syncable; }
On 2014/06/06 19:15:05, kalman wrote:
> why did you need to add this here? it should be *only* a property of prefs.

sync_helper:: IsSyncable and friends make their decisions from Extension only.
They don't have access to ExtensionPrefs.

[not at google - send to devlin, 2014-06-06 19:37:17]

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:1750: bool
ExtensionPrefs::IsSyncable(const std::string& extension_id) const {
i'd mildly prefer this to reflect the actual preference being stored (DoNotSync)
but not a big deal.

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:2135: bool is_ephemeral,
should be passing do_not_sync in here

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
extensions/common/extension.h:334: void set_is_syncable(bool is_syncable) {
is_syncable_ = is_syncable; }
On 2014/06/06 19:24:30, xiyuan wrote:
> On 2014/06/06 19:15:05, kalman wrote:
> > why did you need to add this here? it should be *only* a property of prefs.
> 
> sync_helper:: IsSyncable and friends make their decisions from Extension only.
> They don't have access to ExtensionPrefs.

I see. That's... wrong. Sync helper should query prefs, and it doesn't belong in
chrome/common.

[xiyuan, 2014-06-09 20:59:59]

Split the do_not_sync changes into
https://codereview.chromium.org/323843003/

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:1750: bool
ExtensionPrefs::IsSyncable(const std::string& extension_id) const {
On 2014/06/06 19:37:16, kalman wrote:
> i'd mildly prefer this to reflect the actual preference being stored
(DoNotSync)
> but not a big deal.

Done.

https://codereview.chromium.org/308003005/diff/220001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:2135: bool is_ephemeral,
On 2014/06/06 19:37:16, kalman wrote:
> should be passing do_not_sync in here

Done.

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/308003005/diff/220001/extensions/common/exten...
extensions/common/extension.h:334: void set_is_syncable(bool is_syncable) {
is_syncable_ = is_syncable; }
On 2014/06/06 19:37:16, kalman wrote:
> On 2014/06/06 19:24:30, xiyuan wrote:
> > On 2014/06/06 19:15:05, kalman wrote:
> > > why did you need to add this here? it should be *only* a property of
prefs.
> > 
> > sync_helper:: IsSyncable and friends make their decisions from Extension
only.
> > They don't have access to ExtensionPrefs.
> 
> I see. That's... wrong. Sync helper should query prefs, and it doesn't belong
in
> chrome/common.

This is removed. The info is now passed via OnExtensionInstalled.

And we have a ShouldSyncApp function that looks at both Extension and Profile,
added the pref checking logic there instead of in sync_helper.

[benwells, 2014-06-11 00:15:21]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
This from_bookmark doesn't differentiate between user created bookmark apps and
generated drive bookmark apps. I think that might be OK as only generated ones
will be mapped to drive apps. Is that right? It feels a bit fragile if correct.

If that is correct, can you add a comment explaining? Longer term I think we
need to rethink from_bookmark.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:134: return drive_app.app_name
== url_app->name() &&
What if there is a matching app but it is not a URL app. I.e. it was installed
manually and has app id matching the drive app's product_id?

Edit: OK I can see now this is not called in that case. I think a better name
would be IsGeneratedDriveAppUpToDate or something similar. What do you think?

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:154: if (!(*it)->IsStarted() &&
(*it)->app_info().app_id == drive_app.app_id) {
What will happen if there is a converter in progress?

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:168: void
DriveAppProvider::RemoveDriveApp(const std::string& drive_app_id) {
Nit: I think a better name would be ProcessRemovedDriveApp, as this doesn't go
an uninstall a drive app which the current name suggests.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:186: void
DriveAppProvider::OnDriveAppRegistryUpdated() {
This is the only place that drive_apps_ is initialized. Is it guaranteed to be
called at startup, after this object is created?

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:211:
base::MessageLoop::current()->PostTask(
Why is this deferred? Could you add a comment explaining?

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider_browsertest.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider_browsertest.cc:156:
WaitForPendingDriveAppConverters();
Why is this necessary here? Shouldn't there be no converters running because
there was already an app installed?

[xiyuan, 2014-06-11 04:52:34]

I will update and upload CL tomorrow.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/11 00:15:20, benwells wrote:
> This from_bookmark doesn't differentiate between user created bookmark apps
and
> generated drive bookmark apps. I think that might be OK as only generated ones
> will be mapped to drive apps. Is that right? It feels a bit fragile if
correct.
> 
> If that is correct, can you add a comment explaining? Longer term I think we
> need to rethink from_bookmark.

You are correct that the code could uninstall user created URL app. The case
should be rare but it's possible. If we want to be 100% sure, we could add a
boolean flag in the mappings to indicate whether the URL app is generated or not
and only uninstall the generated ones.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:134: return drive_app.app_name
== url_app->name() &&
On 2014/06/11 00:15:20, benwells wrote:
> What if there is a matching app but it is not a URL app. I.e. it was installed
> manually and has app id matching the drive app's product_id?
> 
> Edit: OK I can see now this is not called in that case. I think a better name
> would be IsGeneratedDriveAppUpToDate or something similar. What do you think?

Sounds good. Will rename.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:154: if (!(*it)->IsStarted() &&
(*it)->app_info().app_id == drive_app.app_id) {
On 2014/06/11 00:15:20, benwells wrote:
> What will happen if there is a converter in progress?

The "while" loop here is just an optimization. The converter could create the
same URL app multiple times (covered in InstallTwice test case). But it could
not reliably abort running process.  So when it happens, we waste some resources
to create the URL app twice.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:168: void
DriveAppProvider::RemoveDriveApp(const std::string& drive_app_id) {
On 2014/06/11 00:15:21, benwells wrote:
> Nit: I think a better name would be ProcessRemovedDriveApp, as this doesn't go
> an uninstall a drive app which the current name suggests.

Will do.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:186: void
DriveAppProvider::OnDriveAppRegistryUpdated() {
On 2014/06/11 00:15:20, benwells wrote:
> This is the only place that drive_apps_ is initialized. Is it guaranteed to be
> called at startup, after this object is created?

It actually ties to the ProfileOAuth2TokenService's token status. If the profile
is signed in, DriveServiceBridgeImpl::Initialize will have
drive_service_->CanSendRequest() returns true and call
drive_app_registry_->Update() to trigger this. Otherwise,
DriveServiceBridgeImpl::OnReadyToSendRequests will be called when profile is
signed in and token is available and we call drive_app_registry_->Update() then
to trigger this.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:211:
base::MessageLoop::current()->PostTask(
On 2014/06/11 00:15:21, benwells wrote:
> Why is this deferred? Could you add a comment explaining?

Will add a comment. This is because we touch extensions system during the
processing, doing so in an observer callback might have problem that it might
have unexpected incomplete status.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider_browsertest.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider_browsertest.cc:156:
WaitForPendingDriveAppConverters();
On 2014/06/11 00:15:21, benwells wrote:
> Why is this necessary here? Shouldn't there be no converters running because
> there was already an app installed?

There should not be any. But it would be good to have it in case there is a bug
and a converter runs.

[xiyuan, 2014-06-11 21:36:43]

CL updated. PTAL. Thanks.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:134: return drive_app.app_name
== url_app->name() &&
On 2014/06/11 04:52:34, xiyuan wrote:
> On 2014/06/11 00:15:20, benwells wrote:
> > What if there is a matching app but it is not a URL app. I.e. it was
installed
> > manually and has app id matching the drive app's product_id?
> > 
> > Edit: OK I can see now this is not called in that case. I think a better
name
> > would be IsGeneratedDriveAppUpToDate or something similar. What do you
think?
> 
> Sounds good. Will rename.

Renamed to IsMappedUrlAppUpToDate because we should do the check for user
installed url app as well.

[benwells, 2014-06-12 00:16:02]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/11 04:52:34, xiyuan wrote:
> On 2014/06/11 00:15:20, benwells wrote:
> > This from_bookmark doesn't differentiate between user created bookmark apps
> and
> > generated drive bookmark apps. I think that might be OK as only generated
ones
> > will be mapped to drive apps. Is that right? It feels a bit fragile if
> correct.
> > 
> > If that is correct, can you add a comment explaining? Longer term I think we
> > need to rethink from_bookmark.
> 
> You are correct that the code could uninstall user created URL app. The case
> should be rare but it's possible. If we want to be 100% sure, we could add a
> boolean flag in the mappings to indicate whether the URL app is generated or
not
> and only uninstall the generated ones.

Actually I didn't think it was possible, and I don't understand what
circumstances it could happen in.

From what I can see things can get in the drive mapping in two ways:
- they are manually installed by the user and have an id that matches the drive
app's product id
- they were auto generated by a pending_converter_ thing

Is there some way that a user created bookmark app can get into the drive
mapping? Can you explain how?

https://codereview.chromium.org/308003005/diff/280001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/280001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:215: // Bail if the |extension|
is installed from a converter. The post install
Why was this added in this patch set? Is there a bug it fixed?

(Just asking because I think I had grasped the previous logic and it seemed fine
when I did).

[xiyuan, 2014-06-12 00:53:20]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/12 00:16:02, benwells wrote:
> Actually I didn't think it was possible, and I don't understand what
> circumstances it could happen in.
> 
> From what I can see things can get in the drive mapping in two ways:
> - they are manually installed by the user and have an id that matches the
drive
> app's product id
> - they were auto generated by a pending_converter_ thing
> 
> Is there some way that a user created bookmark app can get into the drive
> mapping? Can you explain how?

The id of the URL apps are not random and derived from the URL. If user has
manually created an URL app that matches a Drive app's create url, the URL app
will then be in the mapping via the pending_converter_ way. I have added
UserInstallBefore/AfterDriveApp test to cover the cases.

https://codereview.chromium.org/308003005/diff/280001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/280001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:215: // Bail if the |extension|
is installed from a converter. The post install
On 2014/06/12 00:16:02, benwells wrote:
> Why was this added in this patch set? Is there a bug it fixed?
> 
> (Just asking because I think I had grasped the previous logic and it seemed
fine
> when I did).

This is because the added code to have a generated flag in the mapping. We need
to differentiate user install and converter generated app here to properly flip
the generated flag for user installed URL app.

[benwells, 2014-06-12 06:29:00]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/12 00:53:19, xiyuan wrote:
> On 2014/06/12 00:16:02, benwells wrote:
> > Actually I didn't think it was possible, and I don't understand what
> > circumstances it could happen in.
> > 
> > From what I can see things can get in the drive mapping in two ways:
> > - they are manually installed by the user and have an id that matches the
> drive
> > app's product id
> > - they were auto generated by a pending_converter_ thing
> > 
> > Is there some way that a user created bookmark app can get into the drive
> > mapping? Can you explain how?
> 
> The id of the URL apps are not random and derived from the URL. If user has
> manually created an URL app that matches a Drive app's create url, the URL app
> will then be in the mapping via the pending_converter_ way. I have added
> UserInstallBefore/AfterDriveApp test to cover the cases.

Sorry I still don't get it (I'm probably being dumb). The only way something can
get into the drive map from a pending_converter_ is if it is scheduled via
OnDriveAppRegistryUpdated. Something the user creates won't go through
pending_conveters_ (will it??) so it won't ever get in the drive app -> chrome
app map, so it can't get deleted.

So I think your tests UserInstallBefore/AfterDriveApp should have passed before
the last change.

Maybe the problem is that the user could create one, then if this system tries
to create another that matches exactly, it won't actually succeed but the drive
mapping will be added anyway?

The reason I am asking is that I think patch set 9 was easier to understand. I
did think the logic was a bit fragile, but I think that problem is more with the
from_bookmark flag itself which is a bit of a hack. If you can go back to patch
set 9 and not have problems I think you should.

Sorry for slowing you down on this. I tried to patch in your change to
understand better but it wouldn't compile for me (or patch, but I forced it in).

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider_browsertest.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider_browsertest.cc:156:
WaitForPendingDriveAppConverters();
On 2014/06/11 04:52:34, xiyuan wrote:
> On 2014/06/11 00:15:21, benwells wrote:
> > Why is this necessary here? Shouldn't there be no converters running because
> > there was already an app installed?
> 
> There should not be any. But it would be good to have it in case there is a
bug
> and a converter runs.

OK, could we check instead that there are no pending converters in this case?

[xiyuan, 2014-06-12 19:30:21]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/12 06:28:59, benwells wrote:
> Sorry I still don't get it (I'm probably being dumb). The only way something
can
> get into the drive map from a pending_converter_ is if it is scheduled via
> OnDriveAppRegistryUpdated. Something the user creates won't go through
> pending_conveters_ (will it??) so it won't ever get in the drive app -> chrome
> app map, so it can't get deleted.
> 

This is where we differ. Suppose user installs an URL app that has exactly the
same URL as a Drive app's create URL. User installed app does not go through
converters but the Drive app meta would cause a converter be created to create
the URL app (with the same URL thus same id). This would cause the URL app's id
to get into the mapping. from_bookmark(), as you pointed out earlier would not
tell the difference. Thus, when the Drive app is removed (or switched to a
different URL), the user installed URL app will be auto uninstalled too. So
UserInstallBefore/AfterDriveApp would fail in PS 9.

This is an edge case that PS 9 does not handle and it is good that you pointed
out. PS 10 tracks the "generated" fact explicitly and we no longer uninstalls a
user installed URL app.

> So I think your tests UserInstallBefore/AfterDriveApp should have passed
before
> the last change.
> 

The tests would fail with PS 9.

> Maybe the problem is that the user could create one, then if this system tries
> to create another that matches exactly, it won't actually succeed but the
drive
> mapping will be added anyway?
> 

Yes, the mapping will be added as long as the Drive app meta exists. Creating an
app with exactly matched URL succeeds and CrxInstaller treats it as an update.

> The reason I am asking is that I think patch set 9 was easier to understand. I
> did think the logic was a bit fragile, but I think that problem is more with
the
> from_bookmark flag itself which is a bit of a hack. If you can go back to
patch
> set 9 and not have problems I think you should.
> 
> Sorry for slowing you down on this. I tried to patch in your change to
> understand better but it wouldn't compile for me (or patch, but I forced it
in).

To try the CL, you also need to patch https://codereview.chromium.org/323843003/
that I split out.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider_browsertest.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider_browsertest.cc:156:
WaitForPendingDriveAppConverters();
On 2014/06/12 06:28:59, benwells wrote:
> On 2014/06/11 04:52:34, xiyuan wrote:
> > On 2014/06/11 00:15:21, benwells wrote:
> > > Why is this necessary here? Shouldn't there be no converters running
because
> > > there was already an app installed?
> > 
> > There should not be any. But it would be good to have it in case there is a
> bug
> > and a converter runs.
> 
> OK, could we check instead that there are no pending converters in this case?

Done. Updated here an elsewhere it makes sense.

[benwells, 2014-06-13 02:03:01]

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:80: if (existing_app &&
existing_app->from_bookmark()) {
On 2014/06/12 19:30:20, xiyuan wrote:
> On 2014/06/12 06:28:59, benwells wrote:
> > Sorry I still don't get it (I'm probably being dumb). The only way something
> can
> > get into the drive map from a pending_converter_ is if it is scheduled via
> > OnDriveAppRegistryUpdated. Something the user creates won't go through
> > pending_conveters_ (will it??) so it won't ever get in the drive app ->
chrome
> > app map, so it can't get deleted.
> > 
> 
> This is where we differ. Suppose user installs an URL app that has exactly the
> same URL as a Drive app's create URL. User installed app does not go through
> converters but the Drive app meta would cause a converter be created to create
> the URL app (with the same URL thus same id). This would cause the URL app's
id
> to get into the mapping. from_bookmark(), as you pointed out earlier would not
> tell the difference. Thus, when the Drive app is removed (or switched to a
> different URL), the user installed URL app will be auto uninstalled too. So
> UserInstallBefore/AfterDriveApp would fail in PS 9.
> 
> This is an edge case that PS 9 does not handle and it is good that you pointed
> out. PS 10 tracks the "generated" fact explicitly and we no longer uninstalls
a
> user installed URL app.
> 
> > So I think your tests UserInstallBefore/AfterDriveApp should have passed
> before
> > the last change.
> > 
> 
> The tests would fail with PS 9.
> 
> > Maybe the problem is that the user could create one, then if this system
tries
> > to create another that matches exactly, it won't actually succeed but the
> drive
> > mapping will be added anyway?
> > 
> 
> Yes, the mapping will be added as long as the Drive app meta exists. Creating
an
> app with exactly matched URL succeeds and CrxInstaller treats it as an update.
> 
> > The reason I am asking is that I think patch set 9 was easier to understand.
I
> > did think the logic was a bit fragile, but I think that problem is more with
> the
> > from_bookmark flag itself which is a bit of a hack. If you can go back to
> patch
> > set 9 and not have problems I think you should.
> > 
> > Sorry for slowing you down on this. I tried to patch in your change to
> > understand better but it wouldn't compile for me (or patch, but I forced it
> in).
> 
> To try the CL, you also need to patch
https://codereview.chromium.org/323843003/
> that I split out.

Thanks for the detailed explanation, I get it now :)

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:154: if (!(*it)->IsStarted() &&
(*it)->app_info().app_id == drive_app.app_id) {
On 2014/06/11 04:52:34, xiyuan wrote:
> On 2014/06/11 00:15:20, benwells wrote:
> > What will happen if there is a converter in progress?
> 
> The "while" loop here is just an optimization. The converter could create the
> same URL app multiple times (covered in InstallTwice test case). But it could
> not reliably abort running process.  So when it happens, we waste some
resources
> to create the URL app twice.

If two of these do happen now, the second one will be marked as not generated,
which will uninstall the first. I think. Is that a problem?

https://codereview.chromium.org/308003005/diff/300001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/300001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:223: // tagged as generated.
I think I understand why this is needed but it feels pretty hacky. Can you think
of a better way to do this?

Maybe in this case you don't need to continue, just update the mapping and
return?

[xiyuan, 2014-06-13 21:01:15]

CL update.

PS 12 contains changes for comments in PS11.
PS 13 is the after rebase.

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/220001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:154: if (!(*it)->IsStarted() &&
(*it)->app_info().app_id == drive_app.app_id) {
On 2014/06/13 02:03:01, benwells wrote:
> If two of these do happen now, the second one will be marked as not generated,
> which will uninstall the first. I think. Is that a problem?

The current code does not change the "generated" flag if the mapped url app id
is not changed. So the flag sticks.

I gave it a second look and feels the code is tricky. So I have added
|was_generated| flag check in OnLocalAppConverted to make this explicit
(generated flag should be kept no matter how many convertions it goes through)
and not depending on UpdateMappingAndExtensionSystem impl. Also added a
KeepGeneratedFlagBetweenUpdates to ensure it would not break later.

https://codereview.chromium.org/308003005/diff/300001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_provider.cc (right):

https://codereview.chromium.org/308003005/diff/300001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_provider.cc:223: // tagged as generated.
On 2014/06/13 02:03:01, benwells wrote:
> I think I understand why this is needed but it feels pretty hacky. Can you
think
> of a better way to do this?
> 
> Maybe in this case you don't need to continue, just update the mapping and
> return?

We definitely don't need to continue. "return" added.
Also fixed a bug in line 218 where we did not detect user install properly.

I thought of moving this to OnFinishCrxInstall but then realized that
OnExtensionInstalled is moved to ExtensionRegistryObserver and we probably want
to watch two sources. So keeping the code here.

[benwells, 2014-06-16 03:45:45]

lgtm. Thanks for bearing with me.

As discussed offline a more detailed behaviour description document is needed
here so we can discuss the UI with PMs and UX.

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.h (left):

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:39: const drive::DriveAppInfo&
app_info() const { return app_info_; }
Nit: make this and associated field drive_app_info. I had to look it up when
reviewing the other changes to know whether it was drive or extension info.

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:40: const extensions::Extension*
app() const { return app_; }
Nit: make this and associated field extension()

[xiyuan, 2014-06-17 17:57:57]

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
File chrome/browser/apps/drive/drive_app_converter.h (left):

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:39: const drive::DriveAppInfo&
app_info() const { return app_info_; }
On 2014/06/16 03:45:45, benwells wrote:
> Nit: make this and associated field drive_app_info. I had to look it up when
> reviewing the other changes to know whether it was drive or extension info.

Done.

https://codereview.chromium.org/308003005/diff/320001/chrome/browser/apps/dri...
chrome/browser/apps/drive/drive_app_converter.h:40: const extensions::Extension*
app() const { return app_; }
On 2014/06/16 03:45:45, benwells wrote:
> Nit: make this and associated field extension()

Done.

[xiyuan, 2014-06-17 18:00:12]

bauerb@, could you help with an owner's stamp on
chrome/browser/prefs/browser_prefs.cc ?

Thanks.

[not at google - send to devlin, 2014-06-17 21:15:19]

why are there all those changes relating to bookmark apps in the extensions
code? I thought this was a separate feature?

https://codereview.chromium.org/308003005/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/308003005/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.h:200: void set_install_flag(int flag,
bool val) {
in the other CL I lgtm'd without looking at the last patchset. this isn't really
what I meant, I was imagining:

public:
void set_install_immediately(bool val) {
  set_install_flag(kInstallFlagInstallImmediately, val);
}
void set_is_ephemeral(bool val) {
  set_install_flag(kInstallFlagIsEphemeral, val);
}
void set_do_not_sync(bool val) {
  set_install_flag(kInstallFlagDoNotSync, val);
}

private:
void set_install_flag(int flag, bool val) {
  ...
}

[xiyuan, 2014-06-17 21:20:53]

On 2014/06/17 21:15:19, kalman wrote:
> why are there all those changes relating to bookmark apps in the extensions
> code? I thought this was a separate feature?

The changes in bookmark apps code is because convert_web_app.h is changed to
take |creation_flags| instead of defaulting to FROM_BOOKMARK. This part actually
is no longer needed since we have a different do-not-sync impl. Will revert.

[xiyuan, 2014-06-17 21:48:03]

Reverted ConvertWebAppToExtension and changes in bookmark apps files.

https://codereview.chromium.org/308003005/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/crx_installer.h (right):

https://codereview.chromium.org/308003005/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/crx_installer.h:200: void set_install_flag(int flag,
bool val) {
On 2014/06/17 21:15:19, kalman wrote:
> in the other CL I lgtm'd without looking at the last patchset. this isn't
really
> what I meant, I was imagining:
> 
> public:
> void set_install_immediately(bool val) {
>   set_install_flag(kInstallFlagInstallImmediately, val);
> }
> void set_is_ephemeral(bool val) {
>   set_install_flag(kInstallFlagIsEphemeral, val);
> }
> void set_do_not_sync(bool val) {
>   set_install_flag(kInstallFlagDoNotSync, val);
> }
> 
> private:
> void set_install_flag(int flag, bool val) {
>   ...
> }

Done.

[Bernhard Bauer, 2014-06-18 08:38:48]

c/b/p LGTM

[xiyuan, 2014-06-18 23:22:42]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-06-18 23:26:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/308003005/400001

[commit-bot: I haz the power, 2014-06-19 04:10:19]

Change committed as 278265