Editing sources for //net.

Editing iOS sources for //net.

Check that the list of files build with gn and gyp are the same.

BUG=BUG=459705
Committed: https://crrev.com/51b4b098b6e52a587f3ab3a7bdfa31c4b2d9ed92
Cr-Commit-Position: refs/heads/master@{#342592}

[sherouk, 2015-08-05 13:13:13]

sherouk@google.com changed reviewers:
+ dpranke@chromium.org, sdefresne@chromium.org

[sherouk, 2015-08-05 13:13:29]

Can you review please ?

[sdefresne, 2015-08-05 13:31:11]

https://codereview.chromium.org/1272193002/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1272193002/diff/1/net/BUILD.gn#newcode1293
net/BUILD.gn:1293: executable("quic_client") {
I think we don't want this target at all on iOS. I'd suggest putting it behind
!is_ios check, i.e.

if (!is_ios) {
  executable("quic_client") {
    sources = [
      "tools/quic/quic_simple_client_bin.cc",
    ]
    deps = [
      ":net",
      ":simple_quic_tools",
      "//base",
      "//url",
    ]
  }
}

https://codereview.chromium.org/1272193002/diff/1/net/BUILD.gn#newcode1307
net/BUILD.gn:1307: executable("quic_server") {
Same remark as for "quic_client", put the whole target behind !is_ios check.

[sherouk, 2015-08-05 13:58:13]

Patchset #3 (id:40001) has been deleted

[sherouk, 2015-08-05 14:01:50]

Can you review please?

[sdefresne, 2015-08-05 14:53:00]

https://codereview.chromium.org/1272193002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1272193002/diff/60001/net/BUILD.gn#newcode1305
net/BUILD.gn:1305: }
Can you put both targets in the same block?

if (!is_ios) {
  executable("quic_client") {
    # content omitted for keeping the comment brief
  }

  executable("quic_server") {
    # content omitted for keeping the comment brief
  }
}

[sherouk, 2015-08-05 15:09:40]

Can you review please?

[Dirk Pranke, 2015-08-05 15:29:33]

dpranke@chromium.org changed reviewers:
+ sky@chromium.org

[Dirk Pranke, 2015-08-05 15:29:33]

Also, you should either split your CL so that the //ui/base change is in a new
CL, or update the description to make it clearer that you're changing files in
//net and //ui/base.

Scott, do you know anything about this //ui/base/ime target that seems to only
half exist?

https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn#newcode13
ui/base/BUILD.gn:13: build_ime = is_ios
this doesn't seem right. The GYP file has !is_ios. 

Using this flag also causes us to put a dependency on //ui/base/ime, which
doesn't actually exist. In the GYP file, this gets pulled in via a typo, so I
think this is actually a no-op.

Maybe these files aren't actually needed?

[sdefresne, 2015-08-05 15:30:07]

sdefresne@chromium.org changed reviewers:
- sky@chromium.org

[sdefresne, 2015-08-05 15:30:07]

https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn
File ui/base/BUILD.gn (left):

https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn#oldcode13
ui/base/BUILD.gn:13: build_ime = !is_ios
This should not be included in this CL. Did you forget to create a new branch
when working on //ui/base?
Please revert.

[sherouk, 2015-08-05 16:24:23]

Patchset #5 (id:100001) has been deleted

[sherouk, 2015-08-05 16:26:13]

The changes to //ui/base are reverted.

[sdefresne, 2015-08-05 16:27:13]

Changes to //net/BUILD.gn lgtm

[sky, 2015-08-05 17:26:11]

On Wed, Aug 5, 2015 at 8:29 AM,  <dpranke@chromium.org> wrote:
> Also, you should either split your CL so that the //ui/base change is in a
> new
> CL, or update the description to make it clearer that you're changing files
> in
> //net and //ui/base.
>
> Scott, do you know anything about this //ui/base/ime target that seems to
> only
> half exist?

Sorry, I'm not familiar with it.

  -Scott
>
>
> https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn
> File ui/base/BUILD.gn (right):
>
>
https://codereview.chromium.org/1272193002/diff/80001/ui/base/BUILD.gn#newcode13
> ui/base/BUILD.gn:13: build_ime = is_ios
> this doesn't seem right. The GYP file has !is_ios.
>
> Using this flag also causes us to put a dependency on //ui/base/ime,
> which doesn't actually exist. In the GYP file, this gets pulled in via a
> typo, so I think this is actually a no-op.
>
> Maybe these files aren't actually needed?
>
> https://codereview.chromium.org/1272193002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2015-08-05 18:37:22]

last patch lgtm. You probably still need a //net OWNER's approval.

[sdefresne, 2015-08-06 17:40:23]

sdefresne@chromium.org changed reviewers:
+ ellyjones@chromium.org
- sky@chromium.org

[sdefresne, 2015-08-06 17:40:24]

ellyjones: can you review as //net OWNERS?

[Elly Fong-Jones, 2015-08-07 14:41:57]

lgtm

[sherouk, 2015-08-10 08:32:07]

The CQ bit was checked by sherouk@google.com

[commit-bot: I haz the power, 2015-08-10 08:32:30]

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

[commit-bot: I haz the power, 2015-08-10 09:00:48]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-08-10 09:01:22]

Patchset 5 (id:??) landed as
https://crrev.com/51b4b098b6e52a587f3ab3a7bdfa31c4b2d9ed92
Cr-Commit-Position: refs/heads/master@{#342592}