Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197713002:
Comment out an identifier for for an obsolete infobar, and notate (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionComment out an identifier for for an obsolete infobar, and notate
two that seem unused as iOS only.
BUG=none
Committed: https://crrev.com/2d9512bfd5875286cdaa3532b0bffc4c67a0963e
Cr-Commit-Position: refs/heads/master@{#408740}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 17 (6 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, // Used on iOS. I would have preferred to change the names to ..._IOS but that would break iOS and then someone would revert this patch.
LGTM https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, // Used on iOS. On 2016/07/29 17:10:39, Evan Stade wrote: > I would have preferred to change the names to ..._IOS but that would break iOS > and then someone would revert this patch. I thought iOS was fully upstreamed? They still have private code using these? Can we just ask dfalcantara to make the naming change here or else comment these out if they're dead?
estade@chromium.org changed reviewers: + dfalcantara@chromium.org
+cc dfalcantara
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, // Used on iOS. On 2016/07/29 19:10:05, Peter Kasting wrote: > On 2016/07/29 17:10:39, Evan Stade wrote: > > I would have preferred to change the names to ..._IOS but that would break iOS > > and then someone would revert this patch. > > I thought iOS was fully upstreamed? They still have private code using these? > > Can we just ask dfalcantara to make the naming change here or else comment these > out if they're dead? I don't do iOS development and don't know who to ask nowadays :/
https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, // Used on iOS. On 2016/07/29 20:00:57, dfalcantara wrote: > On 2016/07/29 19:10:05, Peter Kasting wrote: > > On 2016/07/29 17:10:39, Evan Stade wrote: > > > I would have preferred to change the names to ..._IOS but that would break > iOS > > > and then someone would revert this patch. > > > > I thought iOS was fully upstreamed? They still have private code using these? > > > > Can we just ask dfalcantara to make the naming change here or else comment > these > > out if they're dead? > > I don't do iOS development and don't know who to ask nowadays :/ (Mainly because I don't know if I even have access to the source.)
pkasting@chromium.org changed reviewers: + jif@chromium.org
jif: Since you reverted the last patch, maybe you can answer. Can iOS upstream its use of the relevant infobar delegate enum values, or at the very least land patches to rename them _IOS in place of the comment Evan is adding?
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Comment out an identifier for for an obsolete infobar, and notate two that seem unused as iOS only. BUG=none ========== to ========== Comment out an identifier for for an obsolete infobar, and notate two that seem unused as iOS only. BUG=none Committed: https://crrev.com/2d9512bfd5875286cdaa3532b0bffc4c67a0963e Cr-Commit-Position: refs/heads/master@{#408740} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2d9512bfd5875286cdaa3532b0bffc4c67a0963e Cr-Commit-Position: refs/heads/master@{#408740}
Message was sent while issue was closed.
jif@google.com changed reviewers: + jif@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, // Used on iOS. Chrome for iOS eng here. I realize that it's super annoying for a random engineer to revert a CL just because their downstream usage gets broken. I sincerely apologize for that. > I thought iOS was fully upstreamed? Chrome for iOS used to be a fork of Chrome. This has recently stopped being the case; maybe this is what you were thinking of? Unfortunately, Chrome iOS is still mostly in a private repo (whose code is mirrored in Google3 btw) in which we "roll" chromium code. We have several people working fulltime on upstreaming all the iOS code. rohitrao@ or sdefresne@ can talk to you about the various challenges we are facing (this includes rewriting the integration tests and stop using some iOS libraries shared across Google Apps). > I would have preferred to change the names to ..._IOS but that would break iOS > and then someone would revert this patch. Again, sorry about that. I tried to not revert the patch by defining the removed constants downstream, but it was deemed "too ugly". Renaming the names to ..._IOS is not a problem. It would indeed break iOS, but it would be an easy fix for us. I'm writing a CL now.
Message was sent while issue was closed.
On 2016/08/01 12:08:06, jif-google wrote: > https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... > File components/infobars/core/infobar_delegate.h (right): > > https://codereview.chromium.org/2197713002/diff/1/components/infobars/core/in... > components/infobars/core/infobar_delegate.h:129: UPGRADE_INFOBAR_DELEGATE = 59, > // Used on iOS. > Chrome for iOS eng here. > I realize that it's super annoying for a random engineer to revert a CL just > because their downstream usage gets broken. I sincerely apologize for that. > > > I thought iOS was fully upstreamed? > Chrome for iOS used to be a fork of Chrome. This has recently stopped being the > case; maybe this is what you were thinking of? > > Unfortunately, Chrome iOS is still mostly in a private repo (whose code is > mirrored in Google3 btw) in which we "roll" chromium code. > > We have several people working fulltime on upstreaming all the iOS code. > rohitrao@ or sdefresne@ can talk to you about the various challenges we are > facing (this includes rewriting the integration tests and stop using some iOS > libraries shared across Google Apps). > > > I would have preferred to change the names to ..._IOS but that would break iOS > > and then someone would revert this patch. > Again, sorry about that. > I tried to not revert the patch by defining the removed constants downstream, > but it was deemed "too ugly". > Renaming the names to ..._IOS is not a problem. It would indeed break iOS, but > it would be an easy fix for us. I'm writing a CL now. thanks. The biggest problem with leaving them here is the potential for repeated future breakage as there's no test coverage. That seems more important than aesthetic appeal, but it's kind of a bike shed so I'll leave it alone. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
