Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591893002:
[Refactor Xcode Objects] Allow extra attributes for native targets. (Closed)
Description[Refactor Xcode Objects] Allow extra attributes for native targets.
This CL refactors xcode objects to allow specifying extra attributes for native
targets, which enables configuring the build settings.
BUG=614818
Committed: https://crrev.com/67e8c60e4b189e0fe6ca3e3699d0b6bed00a4e12
Cr-Commit-Position: refs/heads/master@{#440184}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed feedback #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 ========== to ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 ==========
liaoyuke@chromium.org changed reviewers: + justincohen@chromium.org, sdefresne@chromium.org
Hi Sylvain, Justin, Please take a look. Thank you very much!
lgtm https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc File tools/gn/xcode_object.cc (right): https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc#ne... tools/gn/xcode_object.cc:683: PBXAttributes attributes; You can use copy construction here instead: PBXAttributes attributes = extra_attributes; attributes["CODE_SIGNING_REQUIRED"] = "NO"; ... https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h File tools/gn/xcode_object.h (right): https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h#new... tools/gn/xcode_object.h:288: // NOTE: |extra_attributes| values may be overridden within this function. I understand what you mean by this comment, but only after reading the code. I've tried to think of better comments (things like "Attributes from |extra_attributes| may be ignored.") but nothing that meant the code clearer to understand. In fact, I find it easier to understand without the comment (as I then think "oh, they are extra attributes, so I can only set things that are not already set inside the function). WDYT about just removing this comment?
PTAL. Thank you very much! https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc File tools/gn/xcode_object.cc (right): https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc#ne... tools/gn/xcode_object.cc:683: PBXAttributes attributes; On 2016/12/21 09:39:12, sdefresne ooo till 2nd Jan 17 wrote: > You can use copy construction here instead: > > PBXAttributes attributes = extra_attributes; > attributes["CODE_SIGNING_REQUIRED"] = "NO"; > ... > Done. https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h File tools/gn/xcode_object.h (right): https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h#new... tools/gn/xcode_object.h:288: // NOTE: |extra_attributes| values may be overridden within this function. On 2016/12/21 09:39:12, sdefresne ooo till 2nd Jan 17 wrote: > I understand what you mean by this comment, but only after reading the code. > I've tried to think of better comments (things like "Attributes from > |extra_attributes| may be ignored.") but nothing that meant the code clearer to > understand. In fact, I find it easier to understand without the comment (as I > then think "oh, they are extra attributes, so I can only set things that are not > already set inside the function). > > WDYT about just removing this comment? Make sense. The "extra" in the name should explain everything.
On 2016/12/21 16:59:26, liaoyuke wrote: > PTAL. > > Thank you very much! > > https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc > File tools/gn/xcode_object.cc (right): > > https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.cc#ne... > tools/gn/xcode_object.cc:683: PBXAttributes attributes; > On 2016/12/21 09:39:12, sdefresne ooo till 2nd Jan 17 wrote: > > You can use copy construction here instead: > > > > PBXAttributes attributes = extra_attributes; > > attributes["CODE_SIGNING_REQUIRED"] = "NO"; > > ... > > > > Done. > > https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h > File tools/gn/xcode_object.h (right): > > https://codereview.chromium.org/2591893002/diff/1/tools/gn/xcode_object.h#new... > tools/gn/xcode_object.h:288: // NOTE: |extra_attributes| values may be > overridden within this function. > On 2016/12/21 09:39:12, sdefresne ooo till 2nd Jan 17 wrote: > > I understand what you mean by this comment, but only after reading the code. > > I've tried to think of better comments (things like "Attributes from > > |extra_attributes| may be ignored.") but nothing that meant the code clearer > to > > understand. In fact, I find it easier to understand without the comment (as I > > then think "oh, they are extra attributes, so I can only set things that are > not > > already set inside the function). > > > > WDYT about just removing this comment? > > Make sense. The "extra" in the name should explain everything. Sorry, didn't realize you already lgtmed :)
lgtm
liaoyuke@chromium.org changed reviewers: + dpranke@chromium.org
Thank you for reviewing! Hello Dirk, Do you mind taking a look for owner's approval? Thanks, Yuke
rs- lgtm.
Thank you! On Wed, Dec 21, 2016 at 11:24 AM <dpranke@chromium.org> wrote: > rs- lgtm. > > https://codereview.chromium.org/2591893002/ > -- 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.
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2591893002/#ps20001 (title: "Addressed feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482348441632360,
"parent_rev": "d7f68746c5b907fd9cd167f50b3e7347d0862093", "commit_rev":
"694bb1edbdf065ea4c52f1b161f2585376342adc"}
Message was sent while issue was closed.
Description was changed from ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 ========== to ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 Review-Url: https://codereview.chromium.org/2591893002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 Review-Url: https://codereview.chromium.org/2591893002 ========== to ========== [Refactor Xcode Objects] Allow extra attributes for native targets. This CL refactors xcode objects to allow specifying extra attributes for native targets, which enables configuring the build settings. BUG=614818 Committed: https://crrev.com/67e8c60e4b189e0fe6ca3e3699d0b6bed00a4e12 Cr-Commit-Position: refs/heads/master@{#440184} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/67e8c60e4b189e0fe6ca3e3699d0b6bed00a4e12 Cr-Commit-Position: refs/heads/master@{#440184} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
