Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491253003:
Enable enhanced bookmarks by default on iOS. (Closed)
|
Created:
5 years ago by stkhapugin Modified:
5 years ago CC:
chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable enhanced bookmarks by default on iOS.
The config creation is tracked as crbug.com/564710.
R=lpromero@chromium.org, sdefresne@chromium.org
BUG=564694
Committed: https://crrev.com/0c5985cc6426c5bdf94dbcd3370109f1348a7d54
Cr-Commit-Position: refs/heads/master@{#363469}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated comment. #Patch Set 3 : Missing include. #Messages
Total messages: 19 (9 generated)
Description was changed from ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org BUG=564694 ========== to ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org, sdefresne@chromium.org BUG=564694 ==========
stkhapugin@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL
lgtm https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... ios/chrome/browser/experimental_flags.mm:48: // kEnhancedBookmarksExperiment flag could have values "", "1" and "0". "" - This doesn't wrap nicely. Add a new line after "0". https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... ios/chrome/browser/experimental_flags.mm:51: // The default behavior is Opt In. I wouldn’t add this comment here. The following lines are for checking OptIn and OptOut. Default is dealt with later. (The comment would get stale easily if not close to the code it comments.)
not lgtm, this should be done upstream
On 2015/12/02 at 17:45:19, sdefresne wrote: > not lgtm, this should be done upstream Sorry, lgtm, I thought (incorrectly) this was downstream.
Thanks for review! https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... File ios/chrome/browser/experimental_flags.mm (right): https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... ios/chrome/browser/experimental_flags.mm:48: // kEnhancedBookmarksExperiment flag could have values "", "1" and "0". "" - On 2015/12/02 17:37:08, lpromero wrote: > This doesn't wrap nicely. Add a new line after "0". Done. https://codereview.chromium.org/1491253003/diff/1/ios/chrome/browser/experime... ios/chrome/browser/experimental_flags.mm:51: // The default behavior is Opt In. On 2015/12/02 17:37:08, lpromero wrote: > I wouldn’t add this comment here. The following lines are for checking OptIn and > OptOut. Default is dealt with later. (The comment would get stale easily if not > close to the code it comments.) Done.
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/1491253003/#ps20001 (title: "Updated comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491253003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491253003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/1491253003/#ps40001 (title: "Missing include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491253003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491253003/40001
Message was sent while issue was closed.
Description was changed from ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org, sdefresne@chromium.org BUG=564694 ========== to ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org, sdefresne@chromium.org BUG=564694 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org, sdefresne@chromium.org BUG=564694 ========== to ========== Enable enhanced bookmarks by default on iOS. The config creation is tracked as crbug.com/564710. R=lpromero@chromium.org, sdefresne@chromium.org BUG=564694 Committed: https://crrev.com/0c5985cc6426c5bdf94dbcd3370109f1348a7d54 Cr-Commit-Position: refs/heads/master@{#363469} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0c5985cc6426c5bdf94dbcd3370109f1348a7d54 Cr-Commit-Position: refs/heads/master@{#363469} |
