Issue 657023008: Add a new field "source" in launchData of chrome.app.runtime.onLaunched() to trace launch source.

Issue 657023008: Add a new field "source" in launchData of chrome.app.runtime.onLaunched() to trace launch source. (Closed)

Created:
6 years, 2 months ago by cylee1

Modified:
6 years, 1 month ago

Reviewers:
benwells, Alexei Svitkine (slow), sky

CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, extensions-reviews_chromium.org, sadrul, estade+watch_chromium.org, tfarina, dbeam+watch-ntp_chromium.org, chromium-apps-reviews_chromium.org, kalyank, ben+ash_chromium.org, pedrosimonetti+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add a new field "source" in launchData of chrome.app.runtime.onLaunched() to trace launch source. BUG=chromium:400619 Committed: https://crrev.com/988a9bb5eebf30245ab1150d94bca22fc1c9c89e Cr-Commit-Position: refs/heads/master@{#302617}

Patch Set 1 #

Total comments: 10

Patch Set 2 : #

Patch Set 3 : #

Total comments: 44

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 4

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Total comments: 2

Patch Set 11 : #

Patch Set 12 : fix typo #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+251 lines, -73 lines)			Patch
M	apps/app_load_service.h	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
M	apps/app_load_service.cc	View	1 2 3	4 chunks	+11 lines, -8 lines	0 comments	Download
M	apps/launcher.h	View	1	3 chunks	+9 lines, -3 lines	0 comments	Download
M	apps/launcher.cc	View	1 2 3 4	7 chunks	+20 lines, -20 lines	0 comments	Download
M	chrome/browser/apps/app_launch_for_metro_restart_win.cc	View	1 2 3 4 5	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc	View	1 2 3	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/app_list/app_list_controller_delegate_impl.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc	View	1 2 3 4 5 6	2 chunks	+5 lines, -1 line	0 comments	Download
M	chrome/browser/ui/chrome_pages.cc	View	1 2 3	1 chunk	+13 lines, -1 line	0 comments	Download
M	chrome/browser/ui/extensions/app_launch_params.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/extensions/app_launch_params.cc	View	1 2 3 4 5 6	3 chunks	+7 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/extensions/application_launch.cc	View	1 2 3	1 chunk	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/webui/ntp/app_launcher_handler.cc	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	extensions/browser/api/app_runtime/app_runtime_api.h	View	1	2 chunks	+3 lines, -1 line	0 comments	Download
M	extensions/browser/api/app_runtime/app_runtime_api.cc	View	1 2 3 4 5 6 7 8 9	8 chunks	+65 lines, -5 lines	0 comments	Download
M	extensions/common/api/app_runtime.idl	View	1 2 3 4	2 chunks	+19 lines, -0 lines	0 comments	Download
M	extensions/common/constants.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+21 lines, -0 lines	0 comments	Download
M	extensions/common/feature_switch.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	extensions/common/feature_switch.cc	View		3 chunks	+21 lines, -23 lines	0 comments	Download
M	extensions/common/switches.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	extensions/common/switches.cc	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	extensions/renderer/resources/app_runtime_custom_bindings.js	View	1 2 3 4 5 6	1 chunk	+4 lines, -1 line	0 comments	Download
M	extensions/shell/browser/shell_extension_system.cc	View	1 2 3 4 5 6	2 chunks	+3 lines, -1 line	0 comments	Download
M	tools/metrics/histograms/histograms.xml	View		2 chunks	+24 lines, -0 lines	0 comments	Download

Messages

Total messages: 32 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

cylee1

On 2014/10/15 18:56:31, cylee1 wrote: > mailto:cylee@chromium.org changed reviewers: > + mailto:benwells@chromium.org Hi Ben, Related ...

6 years, 2 months ago (2014-10-16 11:44:33 UTC) #2

benwells

I've got a question about the API definition, which impacts the .idl file. I've put ...

6 years, 2 months ago (2014-10-16 22:50:20 UTC) #4

benwells

https://codereview.chromium.org/657023008/diff/1/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/657023008/diff/1/apps/app_load_service.cc#newcode103 apps/app_load_service.cc:103: profile_, extension, extensions::SOURCE_RELOAD); This is unclear. Also, the source ...

6 years, 2 months ago (2014-10-21 03:51:58 UTC) #5

cylee1

6 years, 2 months ago (2014-10-21 13:41:17 UTC) #6

benwells

Sorry about the delay! https://codereview.chromium.org/657023008/diff/40001/apps/app_load_service.h File apps/app_load_service.h (right): https://codereview.chromium.org/657023008/diff/40001/apps/app_load_service.h#newcode33 apps/app_load_service.h:33: RELOAD, Nit: this should be ...

6 years, 1 month ago (2014-10-28 21:22:48 UTC) #9

cylee1

https://codereview.chromium.org/657023008/diff/40001/apps/app_load_service.h File apps/app_load_service.h (right): https://codereview.chromium.org/657023008/diff/40001/apps/app_load_service.h#newcode33 apps/app_load_service.h:33: RELOAD, On 2014/10/28 21:22:47, benwells wrote: > Nit: this ...

6 years, 1 month ago (2014-10-29 16:40:00 UTC) #10

cylee1

After a git pull some files are integrated with changes by others. So if you ...

6 years, 1 month ago (2014-10-29 17:23:49 UTC) #11

benwells

On 2014/10/29 17:23:49, cylee1 wrote: > After a git pull some files are integrated with ...

6 years, 1 month ago (2014-10-30 05:41:16 UTC) #12

benwells

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extensions/application_launch.cc File chrome/browser/ui/extensions/application_launch.cc (right): https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extensions/application_launch.cc#newcode354 chrome/browser/ui/extensions/application_launch.cc:354: source(extensions::SOURCE_UNKNOWN) { On 2014/10/29 16:39:59, cylee1 wrote: > On ...

6 years, 1 month ago (2014-10-30 06:04:29 UTC) #13

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:354:
source(extensions::SOURCE_UNKNOWN) {
On 2014/10/29 16:39:59, cylee1 wrote:
> On 2014/10/28 21:22:47, benwells wrote:
> > Do you have any idea how many instances of launching this will cause to be
> > unknown? That is, did you try making this a required parameter to the Params
> > constructor and see how many call sites you'd have to update?
> 
> I've checked all calls to OpenApplication
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
> before start implementing. There're 36 occurrences in total for now (including
> from test).
> 
> To be honest  I think it's more practical to make "source' an optional
> parameters - starts from big entries and add minor entries in an on-demand
> manner. For one, there are app-specific sources (e.g., calls from
> wallpaper_manager_util.cc and first_run.cc, where only wallpaper app and first
> run app will ever care about the source.) For the other, the calls to to
> OpenApplication is likely to grow in the future so hard to keep track of every
> entry points.

I am OK with starting like this, but I think you've missed some important call
sites. Before releasing this I'd like this parameter to be mandatory. It might
mean changing UNKNOWN to UNTRACED or something.

https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
extensions/browser/api/app_runtime/app_runtime_api.cc:65: case
extensions::SOURCE_APP_LAUNCHER:
On 2014/10/29 16:39:59, cylee1 wrote:
> On 2014/10/28 21:22:48, benwells wrote:
> > Can you do this with an array lookup instead?
> 
> Could you give me an example? Do you mean defining something like
> mapping = [extensions::SOURCE_APP_LAUNCHER, extensions::SOURCE_NEW_PAGE, ...]
> and match it by mapping[source] ?
> If so it's vulnerable to enum deletion/reordering/insertion in the middle, and
> requires enum->int type conversion and boundary checking ("default:" case
> here"), which I think make no much difference to a direct type conversion as
> static_cast<app_runtime::LaunchSource>(source) since both way need to sync the
> two enums manually.
> This way is ugly but safe and I see others doing the same thing. Solutions
> welcome.
> 

OK fair enough.

https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
extensions/browser/api/app_runtime/app_runtime_api.cc:142:
base::DictionaryValue* launch_data = new base::DictionaryValue();
On 2014/10/29 16:39:59, cylee1 wrote:
> On 2014/10/28 21:22:47, benwells wrote:
> > Why change this?
> 
> Because the ownership is meant to be transferred when calling
> DispatchOnLaunchedEventImpl (just realized I don't need to call Pass() at the
> caller according to
> http://www.chromium.org/developers/smart-pointer-guidelines), so IMHO it's not
> necessary to use scoped_ptr here. Is there any convention that we should
always
> use smart pointer when possible?
> 
> It's especially true for the variable "items", which was declared as a
scope_ptr
> and then items.release() is called soon after. In the case I see no need to
use
> smart pointer ?

If you want to change that please do it in another patch, it makes your actual
change harder to see.

https://codereview.chromium.org/657023008/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/app_runtime_custom_bindings.js (right):

https://codereview.chromium.org/657023008/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/app_runtime_custom_bindings.js:52: delete
launchData.items;
On 2014/10/29 16:40:00, cylee1 wrote:
> On 2014/10/28 21:22:48, benwells wrote:
> > Why is this changing?
> 
> The behavior is not changed - if "items" is empty, the object passed to
> onLaunched event doesn't contains this key at all - in the original
> implementation.
> 
> In the original implementation, the actual object being delivered to
> onLaunched() event is a totally new object  created at line 50 (in the old
> version) so only contains some hard-coded fields in this function. As a
result,
> the new field "source" is not copied to this object. I think it should be the
> opposite way - all fields other than "items" in the raw object should be
passed
> to onLaunched event by default.

I think I see what you're trying to do. I'd prefer you to use the previous
method. What your doing here I think is dangerous as it will be easier for
implementation details / things we want hidden to leak out to extensions.

I'm not sure though ... I need to think about it. Either way, I'd prefer  to
separate that change out. I don't think it is related to the change you're
making. E.g. if there was a problem with passing the object through I'd prefer
that to be a separate, revertable-by-itself change.

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:47: BrowserContext*
context) {
Please add source in here and do the metrics reporting here. BTW I don't think
the UMA reporting should be gated on the feature.

cylee1

6 years, 1 month ago (2014-10-30 13:03:44 UTC) #14

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:354:
source(extensions::SOURCE_UNKNOWN) {
On 2014/10/30 06:04:28, benwells wrote:
> On 2014/10/29 16:39:59, cylee1 wrote:
> > On 2014/10/28 21:22:47, benwells wrote:
> > > Do you have any idea how many instances of launching this will cause to be
> > > unknown? That is, did you try making this a required parameter to the
Params
> > > constructor and see how many call sites you'd have to update?
> > 
> > I've checked all calls to OpenApplication
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
> > before start implementing. There're 36 occurrences in total for now
(including
> > from test).
> > 
> > To be honest  I think it's more practical to make "source' an optional
> > parameters - starts from big entries and add minor entries in an on-demand
> > manner. For one, there are app-specific sources (e.g., calls from
> > wallpaper_manager_util.cc and first_run.cc, where only wallpaper app and
first
> > run app will ever care about the source.) For the other, the calls to to
> > OpenApplication is likely to grow in the future so hard to keep track of
every
> > entry points.
> 
> I am OK with starting like this, but I think you've missed some important call
> sites. Before releasing this I'd like this parameter to be mandatory. It might
> mean changing UNKNOWN to UNTRACED or something.

In my proposal document I've listed a bunch of sources I can identify from
sources:
https://docs.google.com/a/chromium.org/document/d/1ozJ7ojy9bblBzivtBaGcNjTJJk...
So far I've implemented those "first tier" ones. Since I'm not very familiar
with the whole chrome apps code base, could tell me if you remember which
important sources I'm missing? 
If possible, I'd like to only include those most important sources in this CL,
and make up other sources in following CLs. Since this CL is getting larger I
feel it's getting hard to manage all changes in one CL. This feature is
protected by a flag so it should be safe.
Changed UNKNOWN to UNTRACKED

https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
extensions/browser/api/app_runtime/app_runtime_api.cc:142:
base::DictionaryValue* launch_data = new base::DictionaryValue();
On 2014/10/30 06:04:28, benwells wrote:
> On 2014/10/29 16:39:59, cylee1 wrote:
> > On 2014/10/28 21:22:47, benwells wrote:
> > > Why change this?
> > 
> > Because the ownership is meant to be transferred when calling
> > DispatchOnLaunchedEventImpl (just realized I don't need to call Pass() at
the
> > caller according to
> > http://www.chromium.org/developers/smart-pointer-guidelines), so IMHO it's
not
> > necessary to use scoped_ptr here. Is there any convention that we should
> always
> > use smart pointer when possible?
> > 
> > It's especially true for the variable "items", which was declared as a
> scope_ptr
> > and then items.release() is called soon after. In the case I see no need to
> use
> > smart pointer ?
> 
> If you want to change that please do it in another patch, it makes your actual
> change harder to see.

Done.

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:47: BrowserContext*
context) {
On 2014/10/30 06:04:28, benwells wrote:
> Please add source in here and do the metrics reporting here. BTW I don't think
> the UMA reporting should be gated on the feature.

Tried, but since source is already translated into a string when passed in this
function, I'll need to parse it back. Otherwise it needs some refactoring, which
you might want to avoid.

benwells

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/app_runtime/app_runtime_api.cc File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/app_runtime/app_runtime_api.cc#newcode47 extensions/browser/api/app_runtime/app_runtime_api.cc:47: BrowserContext* context) { On 2014/10/30 13:03:43, cylee1 wrote: > ...

6 years, 1 month ago (2014-10-30 22:30:55 UTC) #15

benwells

On 2014/10/30 13:03:44, cylee1 wrote: > https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extensions/application_launch.cc > File chrome/browser/ui/extensions/application_launch.cc (right): > > https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extensions/application_launch.cc#newcode354 > ...

6 years, 1 month ago (2014-10-30 22:32:06 UTC) #16

On 2014/10/30 13:03:44, cylee1 wrote:
>
https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
> File chrome/browser/ui/extensions/application_launch.cc (right):
> 
>
https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
> chrome/browser/ui/extensions/application_launch.cc:354:
> source(extensions::SOURCE_UNKNOWN) {
> On 2014/10/30 06:04:28, benwells wrote:
> > On 2014/10/29 16:39:59, cylee1 wrote:
> > > On 2014/10/28 21:22:47, benwells wrote:
> > > > Do you have any idea how many instances of launching this will cause to
be
> > > > unknown? That is, did you try making this a required parameter to the
> Params
> > > > constructor and see how many call sites you'd have to update?
> > > 
> > > I've checked all calls to OpenApplication
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
> > > before start implementing. There're 36 occurrences in total for now
> (including
> > > from test).
> > > 
> > > To be honest  I think it's more practical to make "source' an optional
> > > parameters - starts from big entries and add minor entries in an on-demand
> > > manner. For one, there are app-specific sources (e.g., calls from
> > > wallpaper_manager_util.cc and first_run.cc, where only wallpaper app and
> first
> > > run app will ever care about the source.) For the other, the calls to to
> > > OpenApplication is likely to grow in the future so hard to keep track of
> every
> > > entry points.
> > 
> > I am OK with starting like this, but I think you've missed some important
call
> > sites. Before releasing this I'd like this parameter to be mandatory. It
might
> > mean changing UNKNOWN to UNTRACED or something.
> 
> In my proposal document I've listed a bunch of sources I can identify from
> sources:
>
https://docs.google.com/a/chromium.org/document/d/1ozJ7ojy9bblBzivtBaGcNjTJJk...
> So far I've implemented those "first tier" ones. Since I'm not very familiar
> with the whole chrome apps code base, could tell me if you remember which
> important sources I'm missing? 
> If possible, I'd like to only include those most important sources in this CL,
> and make up other sources in following CLs. Since this CL is getting larger I
> feel it's getting hard to manage all changes in one CL. This feature is
> protected by a flag so it should be safe.
> Changed UNKNOWN to UNTRACKED

I mustn't have been clear - I am perfectly OK with the CL not changing this. By
'before releasing' I meant before removing the feature flag.

> 
>
https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
> File extensions/browser/api/app_runtime/app_runtime_api.cc (right):
> 
>
https://codereview.chromium.org/657023008/diff/40001/extensions/browser/api/a...
> extensions/browser/api/app_runtime/app_runtime_api.cc:142:
> base::DictionaryValue* launch_data = new base::DictionaryValue();
> On 2014/10/30 06:04:28, benwells wrote:
> > On 2014/10/29 16:39:59, cylee1 wrote:
> > > On 2014/10/28 21:22:47, benwells wrote:
> > > > Why change this?
> > > 
> > > Because the ownership is meant to be transferred when calling
> > > DispatchOnLaunchedEventImpl (just realized I don't need to call Pass() at
> the
> > > caller according to
> > > http://www.chromium.org/developers/smart-pointer-guidelines), so IMHO it's
> not
> > > necessary to use scoped_ptr here. Is there any convention that we should
> > always
> > > use smart pointer when possible?
> > > 
> > > It's especially true for the variable "items", which was declared as a
> > scope_ptr
> > > and then items.release() is called soon after. In the case I see no need
to
> > use
> > > smart pointer ?
> > 
> > If you want to change that please do it in another patch, it makes your
actual
> > change harder to see.
> 
> Done.
> 
>
https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
> File extensions/browser/api/app_runtime/app_runtime_api.cc (right):
> 
>
https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
> extensions/browser/api/app_runtime/app_runtime_api.cc:47: BrowserContext*
> context) {
> On 2014/10/30 06:04:28, benwells wrote:
> > Please add source in here and do the metrics reporting here. BTW I don't
think
> > the UMA reporting should be gated on the feature.
> 
> Tried, but since source is already translated into a string when passed in
this
> function, I'll need to parse it back. Otherwise it needs some refactoring,
which
> you might want to avoid.

cylee1

6 years, 1 month ago (2014-10-31 06:37:53 UTC) #17

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/657023008/diff/40001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:354:
source(extensions::SOURCE_UNKNOWN) {
On 2014/10/30 13:03:43, cylee1 wrote:
> On 2014/10/30 06:04:28, benwells wrote:
> > On 2014/10/29 16:39:59, cylee1 wrote:
> > > On 2014/10/28 21:22:47, benwells wrote:
> > > > Do you have any idea how many instances of launching this will cause to
be
> > > > unknown? That is, did you try making this a required parameter to the
> Params
> > > > constructor and see how many call sites you'd have to update?
> > > 
> > > I've checked all calls to OpenApplication
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
> > > before start implementing. There're 36 occurrences in total for now
> (including
> > > from test).
> > > 
> > > To be honest  I think it's more practical to make "source' an optional
> > > parameters - starts from big entries and add minor entries in an on-demand
> > > manner. For one, there are app-specific sources (e.g., calls from
> > > wallpaper_manager_util.cc and first_run.cc, where only wallpaper app and
> first
> > > run app will ever care about the source.) For the other, the calls to to
> > > OpenApplication is likely to grow in the future so hard to keep track of
> every
> > > entry points.
> > 
> > I am OK with starting like this, but I think you've missed some important
call
> > sites. Before releasing this I'd like this parameter to be mandatory. It
might
> > mean changing UNKNOWN to UNTRACED or something.
> 
> In my proposal document I've listed a bunch of sources I can identify from
> sources:
>
https://docs.google.com/a/chromium.org/document/d/1ozJ7ojy9bblBzivtBaGcNjTJJk...
> So far I've implemented those "first tier" ones. Since I'm not very familiar
> with the whole chrome apps code base, could tell me if you remember which
> important sources I'm missing? 
> If possible, I'd like to only include those most important sources in this CL,
> and make up other sources in following CLs. Since this CL is getting larger I
> feel it's getting hard to manage all changes in one CL. This feature is
> protected by a flag so it should be safe.
> Changed UNKNOWN to UNTRACKED

I got what you mean, but I think it's fine to change UNKNOWN to UNTRACKED now if
I need to do it sooner or later.
Plus I also considered to use untracked instead of unknown earlier.

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/657023008/diff/100001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:47: BrowserContext*
context) {
On 2014/10/30 22:30:55, benwells wrote:
> On 2014/10/30 13:03:43, cylee1 wrote:
> > On 2014/10/30 06:04:28, benwells wrote:
> > > Please add source in here and do the metrics reporting here. BTW I don't
> think
> > > the UMA reporting should be gated on the feature.
> > 
> > Tried, but since source is already translated into a string when passed in
> this
> > function, I'll need to parse it back. Otherwise it needs some refactoring,
> which
> > you might want to avoid.
> 
> I meant to pass in the source as an app_runtime::LaunchSource parameter. As it
> is only called in three places in this file I don't think it is a problem.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657023008/180001

6 years, 1 month ago (2014-11-03 19:23:58 UTC) #20

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/21827)

6 years, 1 month ago (2014-11-03 19:29:37 UTC) #22

cylee1

Hi Scott, Alexei, This is a CL for tracing where a chrome app is launched ...

6 years, 1 month ago (2014-11-03 19:36:58 UTC) #24

Alexei Svitkine (slow)

lgtm % a comment https://codereview.chromium.org/657023008/diff/180001/extensions/common/constants.h File extensions/common/constants.h (right): https://codereview.chromium.org/657023008/diff/180001/extensions/common/constants.h#newcode111 extensions/common/constants.h:111: // Enumeration of possible app ...

6 years, 1 month ago (2014-11-03 22:03:43 UTC) #26

cylee1

Done. Thanks all. https://codereview.chromium.org/657023008/diff/180001/extensions/common/constants.h File extensions/common/constants.h (right): https://codereview.chromium.org/657023008/diff/180001/extensions/common/constants.h#newcode111 extensions/common/constants.h:111: // Enumeration of possible app launch ...

6 years, 1 month ago (2014-11-04 13:40:23 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/657023008/220001

6 years, 1 month ago (2014-11-04 13:43:26 UTC) #29

commit-bot: I haz the power

6 years, 1 month ago (2014-11-04 16:40:05 UTC) #31

Message was sent while issue was closed.

Patchset 12 (id:??) landed as
https://crrev.com/988a9bb5eebf30245ab1150d94bca22fc1c9c89e
Cr-Commit-Position: refs/heads/master@{#302617}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages