Media Galleries Partial Deprecation: Remove image metadata & libexif.

Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs and security issues.

BUG=542912, 577272
Committed: https://crrev.com/f3890c75101ff9b07f48aed992285cc45bf5b4d5
Cr-Commit-Position: refs/heads/master@{#380306}

[tommycli, 2016-03-08 21:36:30]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is huge. It's 200k and has bugs and security issues.

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

to

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux Chrome
distributions. It has bugs and security issues.

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

[tommycli, 2016-03-08 21:36:58]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux Chrome
distributions. It has bugs and security issues.

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

to

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux Chrome
distributions. It has bugs and security issues.

BUG=542912,577272
==========

[tommycli, 2016-03-09 00:13:29]

tommycli@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, thakis@chromium.org

[tommycli, 2016-03-09 00:13:30]

rdevlin.cronin, thakis: PTAL, removes libexif from Chrome

[Nico, 2016-03-09 02:50:05]

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

[Nico, 2016-03-09 02:50:06]

niiiiice lgtm. i assume this has been communicated to extension authors and has
low usage.

sbc, please double-check the sysroot changes.

you'll probably need to update chrome/installer/linux/debian/expected_deps_ia32
/ chrome/installer/linux/debian/expected_deps_x64 too.

https://codereview.chromium.org/1773233002/diff/100001/chrome/utility/media_g...
File chrome/utility/media_galleries/image_metadata_extractor.cc (left):

https://codereview.chromium.org/1773233002/diff/100001/chrome/utility/media_g...
chrome/utility/media_galleries/image_metadata_extractor.cc:416:
g_exif_lib.Get().ExtractInt(data, EXIF_TAG_ORIENTATION, &orientation);
(fwiw if people need just the orientation tag, we extract that ourselves in
blink, it's not too hard)

[Lei Zhang, 2016-03-09 03:00:24]

On 2016/03/09 02:50:06, Nico wrote:
> niiiiice lgtm. i assume this has been communicated to extension authors and
has
> low usage.
> 
> sbc, please double-check the sysroot changes.

sbc is OOO longer than me. Looks ok, use git grep and make sure there aren't any
libexif references left.

> you'll probably need to update
chrome/installer/linux/debian/expected_deps_ia32
> / chrome/installer/linux/debian/expected_deps_x64 too.
> 

Yep, like those.

Also, chrome/installer/linux/sysroot_scripts/packagelist.wheezy.mipsel probably
should be deleted?

[Nico, 2016-03-09 03:03:35]

On Tue, Mar 8, 2016 at 7:00 PM, <thestig@chromium.org> wrote:

> On 2016/03/09 02:50:06, Nico wrote:
> > niiiiice lgtm. i assume this has been communicated to extension authors
> and
> has
> > low usage.
> >
> > sbc, please double-check the sysroot changes.
>
> sbc is OOO longer than me.
>

Heh, I thought you're busy for some reason ;-)


> Looks ok, use git grep and make sure there aren't any
> libexif references left.
>
> > you'll probably need to update
> chrome/installer/linux/debian/expected_deps_ia32
> > / chrome/installer/linux/debian/expected_deps_x64 too.
> >
>
> Yep, like those.
>
> Also, chrome/installer/linux/sysroot_scripts/packagelist.wheezy.mipsel
> probably
> should be deleted?
>
> https://codereview.chromium.org/1773233002/
>

-- 
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-09 20:27:06]

On 2016/03/09 02:50:06, Nico wrote:
> niiiiice lgtm. i assume this has been communicated to extension authors and
has
> low usage.

Hello. Yes, this has been communicated. All the getMetadata users were Video and
Audio players, so we concluded there were no extensions who used the EXIF data.

> 
> sbc, please double-check the sysroot changes.

sbc is OOO until Sept., so we'll have to find another person who knows it?

> 
> you'll probably need to update
chrome/installer/linux/debian/expected_deps_ia32
> / chrome/installer/linux/debian/expected_deps_x64 too.

Surprisingly this was not even in these files. But it probably should have been,
since the Linux version of Chrome relies on the system libexif...

> 
>
https://codereview.chromium.org/1773233002/diff/100001/chrome/utility/media_g...
> File chrome/utility/media_galleries/image_metadata_extractor.cc (left):
> 
>
https://codereview.chromium.org/1773233002/diff/100001/chrome/utility/media_g...
> chrome/utility/media_galleries/image_metadata_extractor.cc:416:
> g_exif_lib.Get().ExtractInt(data, EXIF_TAG_ORIENTATION, &orientation);
> (fwiw if people need just the orientation tag, we extract that ourselves in
> blink, it's not too hard)

[Devlin, 2016-03-09 21:13:15]

lgtm.  One small nitty nit - your description reads:
"Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux Chrome
distributions. It has bugs and security issues."

Please rephrase it so it's clear that libexif adds 200kb, has bugs, etc, rather
than *removing it* does that. :)  (Not that I think it's likely that anyone
would misconstrue it, but, hey.)

[tommycli, 2016-03-09 21:14:59]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux Chrome
distributions. It has bugs and security issues.

BUG=542912,577272
==========

to

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs
and security issues.

BUG=542912,577272
==========

[tommycli, 2016-03-09 21:16:55]

On 2016/03/09 21:13:15, Devlin wrote:
> lgtm.  One small nitty nit - your description reads:
> "Removing libexif is huge. It adds a 200KB DLL to Windows, Mac, and Linux
Chrome
> distributions. It has bugs and security issues."
> 
> Please rephrase it so it's clear that libexif adds 200kb, has bugs, etc,
rather
> than *removing it* does that. :)  (Not that I think it's likely that anyone
> would misconstrue it, but, hey.)

Done.

I'll interpret thestig's review of the build/linux as replacing sbc's.

Let's see if this breaks any bots. Thanks!

[tommycli, 2016-03-09 21:17:20]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-03-09 21:17:56]

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

[commit-bot: I haz the power, 2016-03-09 21:30:26]

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

[commit-bot: I haz the power, 2016-03-09 21:30:27]

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-09 22:33:54]

tommycli@chromium.org changed reviewers:
+ phajdan.jr@chromium.org
- sbc@chromium.org

[tommycli, 2016-03-09 22:33:55]

phajdan.jr: May I have an OWNER review on these files?

    chrome/installer/linux/sysroot_scripts/packagelist.wheezy.mipsel
    testing/chromoting/chromoting_browser_tests.isolate
    testing/libfuzzer/fuzzers/BUILD.gn
    testing/libfuzzer/fuzzers/libexif_parser_fuzzer.cc

[Paweł Hajdan Jr., 2016-03-10 00:24:39]

LGTM

[tommycli, 2016-03-10 00:40:59]

On 2016/03/10 00:24:39, Paweł Hajdan Jr. wrote:
> LGTM

Thank you all. I will send this to CQ again and pray it doesn't break bots (I
worry it will).

[tommycli, 2016-03-10 00:41:11]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-03-10 00:41:57]

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

[commit-bot: I haz the power, 2016-03-10 02:13:45]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs
and security issues.

BUG=542912,577272
==========

to

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs
and security issues.

BUG=542912,577272
==========

[commit-bot: I haz the power, 2016-03-10 02:13:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-10 02:15:53]

Description was changed from

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs
and security issues.

BUG=542912,577272
==========

to

==========
Media Galleries Partial Deprecation: Remove image metadata & libexif.

Removing libexif is a great thing.

It will remove a ~200KB DLL from Windows, Mac, and ChromeOS. It will remove bugs
and security issues.

BUG=542912,577272

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

[commit-bot: I haz the power, 2016-03-10 02:15:55]

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

[erikchen, 2016-03-11 23:00:41]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1791623002/ by erikchen@chromium.org.

The reason for reverting is: Causes build errors on official waterfall.

https://bugs.chromium.org/p/chromium/issues/detail?id=593580.