Fix missing headers in libwebp

Fix missing headers in libwebp

Decreased the number of missing header files by 37.

TBR=mathp@chromium.org
BUG=661774
Review-Url: https://codereview.chromium.org/2852003003
Cr-Commit-Position: refs/heads/master@{#468730}
Committed: https://chromium.googlesource.com/chromium/src/+/befe02860a5bdbf38349b76a56b55ae9f03714b7

[wychen, 2017-05-01 05:19:55]

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

[commit-bot: I haz the power, 2017-05-01 05:20:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 05:27:45]

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

[commit-bot: I haz the power, 2017-05-01 05:27:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wychen, 2017-05-01 05:39:36]

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

[commit-bot: I haz the power, 2017-05-01 05:39:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wychen, 2017-05-01 06:07:18]

wychen@chromium.org changed reviewers:
+ fbarchard@chromium.org

[wychen, 2017-05-01 06:07:18]

PTAL

[wychen, 2017-05-01 06:07:54]

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

[wychen, 2017-05-01 06:07:55]

+thakis for the .gn change.

[commit-bot: I haz the power, 2017-05-01 07:27:07]

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

[commit-bot: I haz the power, 2017-05-01 07:27:08]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-01 13:29:41]

Nice!

https://codereview.chromium.org/2852003003/diff/20001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/2852003003/diff/20001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:23: "webp/types.h",
This target feels kind of strange to me: Everythinn except libweb_utils depends
on this, so maybe these should just be in libwebp_utils. But then, the way this
file seems to be organized is that every target only has files from one folder,
so maybe this target should be called libwebp_webp and also include
webp/decode.h, webp/demux.h, webp/encode.h. The latter is probably most
consistent with the existing file if it works.

[fbarchard1, 2017-05-01 18:08:34]

Description was changed from

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

BUG=661774
==========

to

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

BUG=661774
==========

[fbarchard1, 2017-05-01 18:08:34]

fbarchard@google.com changed reviewers:
+ skal@google.com

[fbarchard1, 2017-05-01 18:11:49]

fbarchard@google.com changed reviewers:
+ fbarchard@google.com

[fbarchard1, 2017-05-01 18:11:50]

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:35: "dec/common_dec.h",
If you remove these from the source, a change to them wont cause a rebuild?

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:35: "dec/common_dec.h",
Historically I think we also kept a full source list for project generation..
Visual Studio and such?  Perhaps not an issue going forward

[Nico, 2017-05-01 18:20:47]

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:35: "dec/common_dec.h",
On 2017/05/01 18:11:50, fbarchard1 wrote:
> If you remove these from the source, a change to them wont cause a rebuild?

No, this isn't for build dependency checking, that always works. It's for `gn
check`.

[wychen, 2017-05-01 18:26:06]

https://codereview.chromium.org/2852003003/diff/20001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/2852003003/diff/20001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:23: "webp/types.h",
On 2017/05/01 13:29:41, Nico wrote:
> This target feels kind of strange to me: Everythinn except libweb_utils
depends
> on this, so maybe these should just be in libwebp_utils. But then, the way
this
> file seems to be organized is that every target only has files from one
folder,
> so maybe this target should be called libwebp_webp and also include
> webp/decode.h, webp/demux.h, webp/encode.h. The latter is probably most
> consistent with the existing file if it works.

That was what I did in ps1.

The reason I moved decode/demux/encode.h to separate targets is that
components/image_fetcher/ios/webp_decoder.h includes decode.h, while its deps
only has libwebp:libwebp_dec.

I guess adding libwebp_headers to its deps makes more sense.

[wychen, 2017-05-01 18:30:59]

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

[commit-bot: I haz the power, 2017-05-01 18:32:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wychen, 2017-05-01 18:36:21]

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
File third_party/libwebp/BUILD.gn (right):

https://codereview.chromium.org/2852003003/diff/40001/third_party/libwebp/BUI...
third_party/libwebp/BUILD.gn:35: "dec/common_dec.h",
On 2017/05/01 18:20:47, Nico wrote:
> On 2017/05/01 18:11:50, fbarchard1 wrote:
> > If you remove these from the source, a change to them wont cause a rebuild?
> 
> No, this isn't for build dependency checking, that always works. It's for `gn
> check`.

Before this change, if we have a CL only modifying, say,
webp/format_constants.h, and send it to CQ, CQ would wrongly skip many targets
because GN doesn't know about the true dependency, right? This is the main
motivation to fix the missing headers in GN.

"gn check" is more fine-grained, and further check the per-target relationship.
However, it wouldn't work without having these headers in GN in the first place.

[commit-bot: I haz the power, 2017-05-01 18:45:09]

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

[commit-bot: I haz the power, 2017-05-01 18:45:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Nico, 2017-05-01 20:02:14]

On Mon, May 1, 2017 at 2:26 PM, <wychen@chromium.org> wrote:

>
> https://codereview.chromium.org/2852003003/diff/20001/
> third_party/libwebp/BUILD.gn
> File third_party/libwebp/BUILD.gn (right):
>
> https://codereview.chromium.org/2852003003/diff/20001/
> third_party/libwebp/BUILD.gn#newcode23
> third_party/libwebp/BUILD.gn:23: "webp/types.h",
> On 2017/05/01 13:29:41, Nico wrote:
> > This target feels kind of strange to me: Everythinn except
> libweb_utils depends
> > on this, so maybe these should just be in libwebp_utils. But then, the
> way this
> > file seems to be organized is that every target only has files from
> one folder,
> > so maybe this target should be called libwebp_webp and also include
> > webp/decode.h, webp/demux.h, webp/encode.h. The latter is probably
> most
> > consistent with the existing file if it works.
>
> That was what I did in ps1.
>
> The reason I moved decode/demux/encode.h to separate targets is that
> components/image_fetcher/ios/webp_decoder.h includes decode.h, while its
> deps only has libwebp:libwebp_dec.
>

Sorry, I don't understand -- what's the difference between making
image_fetcher depend on libwebp_webp and libwebp_headers?


>
> I guess adding libwebp_headers to its deps makes more sense.
>
> https://codereview.chromium.org/2852003003/
>

-- 
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.

[wychen, 2017-05-01 20:09:19]

On 2017/05/01 20:02:14, Nico wrote:
> On Mon, May 1, 2017 at 2:26 PM, <mailto:wychen@chromium.org> wrote:
> > That was what I did in ps1.
> >
> > The reason I moved decode/demux/encode.h to separate targets is that
> > components/image_fetcher/ios/webp_decoder.h includes decode.h, while its
> > deps only has libwebp:libwebp_dec.
> >
> 
> Sorry, I don't understand -- what's the difference between making
> image_fetcher depend on libwebp_webp and libwebp_headers?

I'm not sure I understand your question.

libwebp_webp is the same as libwebp_headers in ps1, right? They both contain all
headers under webp/. Do you want me to rename libwebp_headers to libwebp_webp?

Before this change, image_fetcher depends on libwebp_dec, but it includes
webp/decode.h, which is not part of libwebp_dec.
In ps2, I moved webp/decode.h from libwebp_headers to libwebp_dec so that we
don't have to change image_fetcher, and it kind of makes sense, too.
To preserve the original structure, I moved these headers back in ps4, and
modify image_fetcher instead.

[Nico, 2017-05-01 20:28:19]

My question is "what is wrong with patch set 1 (except that libwebp_headers
should be called libwebp_webp)"? I like patch set 4, but I think
s/libwebp_headers/libwebp_webp/ would be more consistent with the other
targets in that build.gn file.

On Mon, May 1, 2017 at 4:09 PM, <wychen@chromium.org> wrote:

> On 2017/05/01 20:02:14, Nico wrote:
> > On Mon, May 1, 2017 at 2:26 PM, <mailto:wychen@chromium.org> wrote:
> > > That was what I did in ps1.
> > >
> > > The reason I moved decode/demux/encode.h to separate targets is that
> > > components/image_fetcher/ios/webp_decoder.h includes decode.h, while
> its
> > > deps only has libwebp:libwebp_dec.
> > >
> >
> > Sorry, I don't understand -- what's the difference between making
> > image_fetcher depend on libwebp_webp and libwebp_headers?
>
> I'm not sure I understand your question.
>
> libwebp_webp is the same as libwebp_headers in ps1, right? They both
> contain all
> headers under webp/. Do you want me to rename libwebp_headers to
> libwebp_webp?
>
> Before this change, image_fetcher depends on libwebp_dec, but it includes
> webp/decode.h, which is not part of libwebp_dec.
> In ps2, I moved webp/decode.h from libwebp_headers to libwebp_dec so that
> we
> don't have to change image_fetcher, and it kind of makes sense, too.
> To preserve the original structure, I moved these headers back in ps4, and
> modify image_fetcher instead.
>
>
> https://codereview.chromium.org/2852003003/
>

-- 
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.

[wychen, 2017-05-01 21:04:20]

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

[commit-bot: I haz the power, 2017-05-01 21:04:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wychen, 2017-05-01 21:05:23]

On 2017/05/01 20:28:19, Nico wrote:
> My question is "what is wrong with patch set 1 (except that libwebp_headers
> should be called libwebp_webp)"? I like patch set 4, but I think
> s/libwebp_headers/libwebp_webp/ would be more consistent with the other
> targets in that build.gn file.

I see. ps1 failed the build because image_fetcher depends on libwebp_dec only,
not libwebp_headers, but it includes webp/decode.h.

It's renamed.

[Nico, 2017-05-01 21:08:03]

lgtm

[commit-bot: I haz the power, 2017-05-01 23:05:22]

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

[commit-bot: I haz the power, 2017-05-01 23:05:22]

Dry run: This issue passed the CQ dry run.

[wychen, 2017-05-01 23:13:05]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2017-05-01 23:13:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 23:22:59]

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

[commit-bot: I haz the power, 2017-05-01 23:22:59]

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

[wychen, 2017-05-01 23:36:09]

wychen@chromium.org changed reviewers:
+ markusheintz@chromium.org

[wychen, 2017-05-01 23:36:10]

markusheintz@chromium.org: Please review changes in image fetcher.

[fbarchard1, 2017-05-02 00:15:52]

lgtm

[wychen, 2017-05-02 00:30:13]

The CQ bit was checked by wychen@chromium.org

[wychen, 2017-05-02 00:30:24]

The CQ bit was unchecked by wychen@chromium.org

[wychen, 2017-05-02 18:23:53]

wychen@chromium.org changed reviewers:
- markusheintz@chromium.org

[wychen, 2017-05-02 18:25:51]

wychen@chromium.org changed reviewers:
+ mathp@chromium.org

[wychen, 2017-05-02 18:25:51]

mathp@chromium.org: Please review changes in image fetcher. Thanks!

[Nico, 2017-05-02 18:52:28]

I think it's ok if you TBR this image_fetcher change.

[wychen, 2017-05-02 18:53:24]

Description was changed from

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

BUG=661774
==========

to

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

TBR=mathp@chromium.org
BUG=661774
==========

[wychen, 2017-05-02 18:53:37]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2017-05-02 18:54:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 19:03:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493751217191780,
"parent_rev": "ccb33e88ae90c672f98cd24ac631bc92581a199f", "commit_rev":
"befe02860a5bdbf38349b76a56b55ae9f03714b7"}

[commit-bot: I haz the power, 2017-05-02 19:04:25]

Description was changed from

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

TBR=mathp@chromium.org
BUG=661774
==========

to

==========
Fix missing headers in libwebp

Decreased the number of missing header files by 37.

TBR=mathp@chromium.org
BUG=661774

Review-Url: https://codereview.chromium.org/2852003003
Cr-Commit-Position: refs/heads/master@{#468730}
Committed:
https://chromium.googlesource.com/chromium/src/+/befe02860a5bdbf38349b76a56b5...
==========

[commit-bot: I haz the power, 2017-05-02 19:04:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/befe02860a5bdbf38349b76a56b5...