Media Galleries Partial Deprecation: Remove scan functionality.

Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912
Committed: https://crrev.com/d3f29202a41db90b9657b39158777b58ef81bfdb
Cr-Commit-Position: refs/heads/master@{#378624}

[tommycli, 2016-02-11 23:01:33]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-11 23:04:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695563002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695563002/60001

[commit-bot: I haz the power, 2016-02-11 23:28:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-11 23:28:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tommycli, 2016-02-12 00:09:51]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 00:10:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695563002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695563002/80001

[tommycli, 2016-02-12 00:16:38]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove scan functionality.

BUG=542912
==========

to

==========
Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912
==========

[commit-bot: I haz the power, 2016-02-12 01:28:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 01:28:34]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-02-12 01:45:41]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2016-02-12 01:45:42]

thestig: PTAL, monster patch for removing the scan functionality. Most of it is
pure deletion.

There may be some remnants that we can discover with scythe afterwards, but I
think I took out everything.

I used the list of patches on this bug:
https://code.google.com/p/chromium/issues/detail?id=161119

I didn't remove most of the scanning preferences, since we still have to support
people with those keys.

[Lei Zhang, 2016-02-12 02:11:25]

Have you tested this scenarios?
1) There exists a profile with an extension installed that has the scan
permission.
2) Run Chromium with this CL applied on that profile.

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:326: //
TODO(tommycli): Remove these CHECKs after fixing https://crbug.com/467627.
Oh hey, what's this? :)

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_histograms.h (right):

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_histograms.h:25:
DELETED_START_MEDIA_SCAN,
Mark these as obsolete in tools/metrics/histograms/histograms.xml as well.

[tommycli, 2016-02-16 20:16:39]

thestig: Thanks!

I followed your recommendation to manually test with an extension. Actually it
crashed!

So... the 'nocompile' flag in the IDL still generates the JS schema for the
function, it's just assumed you'll implement it with a custom binding.

I added a 'nodefine' flag in the IDL parser which takes it one step further and
just does not generate anything.

Now these [nodefine] functions do not appear on the chrome.mediaGalleries app.
I've tested using the MG Sample app, and now clicking Scan just seems to do
nothing. (I don't see any console errors either).

Thanks!

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:326: //
TODO(tommycli): Remove these CHECKs after fixing https://crbug.com/467627.
On 2016/02/12 02:11:25, Lei Zhang wrote:
> Oh hey, what's this? :)

Done. I made a separate CL here: https://codereview.chromium.org/1699223002/

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_histograms.h (right):

https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_histograms.h:25:
DELETED_START_MEDIA_SCAN,
On 2016/02/12 02:11:25, Lei Zhang wrote:
> Mark these as obsolete in tools/metrics/histograms/histograms.xml as well.

Done.

[tommycli, 2016-02-16 20:17:09]

On 2016/02/16 20:16:39, tommycli wrote:
> thestig: Thanks!
> 
> I followed your recommendation to manually test with an extension. Actually it
> crashed!
> 
> So... the 'nocompile' flag in the IDL still generates the JS schema for the
> function, it's just assumed you'll implement it with a custom binding.
> 
> I added a 'nodefine' flag in the IDL parser which takes it one step further
and
> just does not generate anything.
> 
> Now these [nodefine] functions do not appear on the chrome.mediaGalleries app.
> I've tested using the MG Sample app, and now clicking Scan just seems to do
> nothing. (I don't see any console errors either).
> 
> Thanks!
> 
>
https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
> File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
> (right):
> 
>
https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/extensio...
> chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:326: //
> TODO(tommycli): Remove these CHECKs after fixing https://crbug.com/467627.
> On 2016/02/12 02:11:25, Lei Zhang wrote:
> > Oh hey, what's this? :)
> 
> Done. I made a separate CL here: https://codereview.chromium.org/1699223002/
> 
>
https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
> File chrome/browser/media_galleries/media_galleries_histograms.h (right):
> 
>
https://codereview.chromium.org/1695563002/diff/80001/chrome/browser/media_ga...
> chrome/browser/media_galleries/media_galleries_histograms.h:25:
> DELETED_START_MEDIA_SCAN,
> On 2016/02/12 02:11:25, Lei Zhang wrote:
> > Mark these as obsolete in tools/metrics/histograms/histograms.xml as well.
> 
> Done.

The 'nodefine' flag can be seen in the changes to tools/*. I can pull in devlin
to review 'nodefine' if you prefer.

[tommycli, 2016-02-29 18:19:15]

tommycli@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- thestig@chromium.org

[tommycli, 2016-02-29 18:19:16]

rdevlin.cronin: PTAL (since thestig is OOO)

[Devlin, 2016-02-29 20:37:24]

Woo!

Note: I mostly skimmed the massive deletion here, trusting you and our tests to
verify that we didn't take out too much.  Let me know if you prefer me to do a
more thorough review.

LGTM.

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/media_galleries.idl:36: [nodefine, inline_doc] enum
ScanProgressType {
Have you checked locally that this does the right thing both on the docserver
and in compilation?

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:1973: const char kMediaGalleriesLastScanTime[] =
"media_galleries.last_scan_time";
Do we need to clean this up in users' pref files?

[Devlin, 2016-02-29 20:38:16]

https://codereview.chromium.org/1695563002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema_test.py (right):

https://codereview.chromium.org/1695563002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema_test.py:129: def testNoDefine(self):
It might also be nice to test add a test that these aren't present in the c++,
etc.

[tommycli, 2016-02-29 21:53:31]

devlin: Thanks, see below.

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/media_galleries.idl:36: [nodefine, inline_doc] enum
ScanProgressType {
On 2016/02/29 20:37:24, Devlin wrote:
> Have you checked locally that this does the right thing both on the docserver
> and in compilation?

Yes, I have checked manually via server2/preview.py. chrome-apps-doc.appspot.com
is unfortunately broken (500 error).

One more question: Didn't we used to be able to access the beta/dev/trunk
channel docs via https://developer.chrome.com/beta/apps/mediaGalleries ? That
doesn't seem to work anymore. I'm trying to verify the Deprecated Since Chrome
XX is correct, since the version number is not determined when you run server2
locally.

I have also checked that these structs are omitted from the generated
media_galleries.h file.

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:1973: const char kMediaGalleriesLastScanTime[] =
"media_galleries.last_scan_time";
On 2016/02/29 20:37:24, Devlin wrote:
> Do we need to clean this up in users' pref files?

When I discussed this with thestig, we thought that it would be okay if this key
dangled around user profiles.

When the profile is saved, do you know if unknown keys are written to disk
again? Or discarded?

https://codereview.chromium.org/1695563002/diff/100001/tools/json_schema_comp...
File tools/json_schema_compiler/idl_schema_test.py (right):

https://codereview.chromium.org/1695563002/diff/100001/tools/json_schema_comp...
tools/json_schema_compiler/idl_schema_test.py:129: def testNoDefine(self):
On 2016/02/29 20:38:16, Devlin wrote:
> It might also be nice to test add a test that these aren't present in the c++,
> etc.

I added a test in the latest patchset in the same vein as your existing
nocompile test.

This should verify that it doesn't generate C++, and that it's never defined in
JS-land.

[tommycli, 2016-02-29 21:54:02]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 21:54:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695563002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695563002/140001

[Devlin, 2016-02-29 22:10:44]

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/media_galleries.idl:36: [nodefine, inline_doc] enum
ScanProgressType {
On 2016/02/29 21:53:30, tommycli wrote:
> One more question: Didn't we used to be able to access the beta/dev/trunk
> channel docs via https://developer.chrome.com/beta/apps/mediaGalleries ? That
> doesn't seem to work anymore. I'm trying to verify the Deprecated Since Chrome
> XX is correct, since the version number is not determined when you run server2
> locally.

I don't think we ever had beta/dev/trunk docs.  We have APIs that are only
available in certain channels, but only one version of the documentation.  The
server constructs the "deprecated since chrome XX" by looking at the commit
history, IIRC.

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:1973: const char kMediaGalleriesLastScanTime[] =
"media_galleries.last_scan_time";
On 2016/02/29 21:53:30, tommycli wrote:
> On 2016/02/29 20:37:24, Devlin wrote:
> > Do we need to clean this up in users' pref files?
> 
> When I discussed this with thestig, we thought that it would be okay if this
key
> dangled around user profiles.
> 
> When the profile is saved, do you know if unknown keys are written to disk
> again? Or discarded?

Unknown keys are still written to disk last I checked (a few months back, so
maybe things have changed?).  I think this is somewhat by design - maybe we
didn't want to unintentially clear out everyone's prefs if we accidentally
unregistered one?

Easy enough to test locally. :)

[tommycli, 2016-02-29 22:49:04]

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/extensio...
chrome/common/extensions/api/media_galleries.idl:36: [nodefine, inline_doc] enum
ScanProgressType {
On 2016/02/29 22:10:44, Devlin wrote:
> On 2016/02/29 21:53:30, tommycli wrote:
> > One more question: Didn't we used to be able to access the beta/dev/trunk
> > channel docs via https://developer.chrome.com/beta/apps/mediaGalleries ?
That
> > doesn't seem to work anymore. I'm trying to verify the Deprecated Since
Chrome
> > XX is correct, since the version number is not determined when you run
server2
> > locally.
> 
> I don't think we ever had beta/dev/trunk docs.  We have APIs that are only
> available in certain channels, but only one version of the documentation.  The
> server constructs the "deprecated since chrome XX" by looking at the commit
> history, IIRC.

Yes, reading the code that looks correct.

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1695563002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:1973: const char kMediaGalleriesLastScanTime[] =
"media_galleries.last_scan_time";
On 2016/02/29 22:10:44, Devlin wrote:
> On 2016/02/29 21:53:30, tommycli wrote:
> > On 2016/02/29 20:37:24, Devlin wrote:
> > > Do we need to clean this up in users' pref files?
> > 
> > When I discussed this with thestig, we thought that it would be okay if this
> key
> > dangled around user profiles.
> > 
> > When the profile is saved, do you know if unknown keys are written to disk
> > again? Or discarded?
> 
> Unknown keys are still written to disk last I checked (a few months back, so
> maybe things have changed?).  I think this is somewhat by design - maybe we
> didn't want to unintentially clear out everyone's prefs if we accidentally
> unregistered one?
> 
> Easy enough to test locally. :)

SGTM. I'm okay with leaving this dangling since the # of MG users who have ever
scanned is basically non-existent.

[tommycli, 2016-02-29 23:05:49]

tommycli@chromium.org changed reviewers:
+ thakis@chromium.org

[tommycli, 2016-02-29 23:05:50]

thakis: PTAL epic deletion. Thanks!

[Nico, 2016-02-29 23:41:12]

lgtm

Is there some process for removing extension apis? do you have to announce this
on some extension-dev mailing list or similar? (like blink has their "intent to
remove" threads...)

[tommycli, 2016-02-29 23:50:05]

On 2016/02/29 23:41:12, Nico wrote:
> lgtm
> 
> Is there some process for removing extension apis? do you have to announce
this
> on some extension-dev mailing list or similar? (like blink has their "intent
to
> remove" threads...)

Hi Nico, it's been publicly announced here.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-apps/2-ANdbxevpc

I have reached out directly to the affected extension developers. I also have a
design doc I can send you if you want. Just let me know.

[Nico, 2016-02-29 23:51:52]

Cool, sounds good, thanks.

On Mon, Feb 29, 2016 at 6:50 PM, <tommycli@chromium.org> wrote:

> On 2016/02/29 23:41:12, Nico wrote:
> > lgtm
> >
> > Is there some process for removing extension apis? do you have to
> announce
> this
> > on some extension-dev mailing list or similar? (like blink has their
> "intent
> to
> > remove" threads...)
>
> Hi Nico, it's been publicly announced here.
>
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-apps/2-ANdbxevpc
>
> I have reached out directly to the affected extension developers. I also
> have a
> design doc I can send you if you want. Just let me know.
>
> https://codereview.chromium.org/1695563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommycli, 2016-03-01 01:03:30]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2016-03-01 01:03:30]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-03-01 01:03:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1695563002/#ps140001
(title: "add idl apitest")

[commit-bot: I haz the power, 2016-03-01 01:04:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695563002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695563002/140001

[commit-bot: I haz the power, 2016-03-01 01:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 01:28:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommycli, 2016-03-01 16:36:39]

tommycli@chromium.org changed reviewers:
+ isherman@chromium.org

[tommycli, 2016-03-01 16:36:40]

isherman: PTAL
    extensions/browser/extension_function_histogram_value.h
    tools/metrics/histograms/histograms.xml

Thanks!

[Ilya Sherman, 2016-03-01 22:26:28]

histograms.xml and extensions/browser/extension_function_histogram_value.h lgtm
% a comment:

https://codereview.chromium.org/1695563002/diff/140001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/1695563002/diff/140001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:726:
DELETED_MEDIAGALLERIES_ADDSCANRESULTS,
Please run the script to update histograms.xml to match.

[tommycli, 2016-03-01 22:41:30]

isherman: thanks!

https://codereview.chromium.org/1695563002/diff/140001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/1695563002/diff/140001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:726:
DELETED_MEDIAGALLERIES_ADDSCANRESULTS,
On 2016/03/01 22:26:28, Ilya Sherman wrote:
> Please run the script to update histograms.xml to match.

Done.

[tommycli, 2016-03-01 22:41:41]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-03-01 22:41:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, thakis@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1695563002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-03-01 22:42:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695563002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695563002/160001

[commit-bot: I haz the power, 2016-03-02 00:23:20]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912
==========

to

==========
Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912
==========

[commit-bot: I haz the power, 2016-03-02 00:23:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-02 00:24:33]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912
==========

to

==========
Media Galleries Partial Deprecation: Remove scan functionality.

Removes ~4100 sloc, and 50k off the binary size.

BUG=542912

Committed: https://crrev.com/d3f29202a41db90b9657b39158777b58ef81bfdb
Cr-Commit-Position: refs/heads/master@{#378624}
==========

[commit-bot: I haz the power, 2016-03-02 00:24:35]

Patchset 9 (id:??) landed as
https://crrev.com/d3f29202a41db90b9657b39158777b58ef81bfdb
Cr-Commit-Position: refs/heads/master@{#378624}