Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949833005:
Remove incognito advice from Sad Tab if already incognito (Closed)
|
Created:
3 years, 6 months ago by PL Modified:
3 years, 6 months ago Reviewers:
kkhorimoto CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove incognito advice from Sad Tab if already incognito
BUG=735534
TEST=Repeat crashes when not in Incognito mode show the advice to try
Incognito mode. However the same test performed when in Incognito mode
should not offer this advice as one of the bullet points.
Review-Url: https://codereview.chromium.org/2949833005
Cr-Commit-Position: refs/heads/master@{#481763}
Committed: https://chromium.googlesource.com/chromium/src/+/5e1c61bdb08f0704518a64144592ce1a2ee31cf7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review feedback and rebase for clean merge #Patch Set 3 : Protect against a case under unit testing where navigationManager could be nil #Messages
Total messages: 33 (17 generated)
peterlaurens@chromium.org changed reviewers: + kkhorimoto@chromium.org
Hello! This is a change to sad tab where if we're already in Incognito mode, we will not offer the advice to turn it on in the Sad Tab. Thanks!
code lgtm, but have some formatting suggestions. https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:254: nil]; I think the formatting here would be much better if you use the @[] container literal format. https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:257: insertObject:l10n_util::GetNSString(IDS_SAD_TAB_RELOAD_INCOGNITO) Maybe declare the string on a separate line to help formatting a bit?
Thanks! https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:254: nil]; On 2017/06/22 00:33:31, kkhorimoto_ wrote: > I think the formatting here would be much better if you use the @[] container > literal format. I agree, but unfortunately I am not able to make mutable arrays that way, and need to remove the offending item from the array. I could declare two arrays with an if/else... but that seemed to me to be more duplicated code :-(. https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:257: insertObject:l10n_util::GetNSString(IDS_SAD_TAB_RELOAD_INCOGNITO) On 2017/06/22 00:33:31, kkhorimoto_ wrote: > Maybe declare the string on a separate line to help formatting a bit? Done! Thanks!
The CQ bit was checked by peterlaurens@chromium.org
The CQ bit was unchecked by peterlaurens@chromium.org
The CQ bit was checked by peterlaurens@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by peterlaurens@chromium.org
On 2017/06/22 17:33:44, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Simulator failure is due to needing a rebase and manual merge to be applied properly. I'll do that now.
https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:254: nil]; On 2017/06/22 15:15:38, PL wrote: > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > I think the formatting here would be much better if you use the @[] container > > literal format. > > I agree, but unfortunately I am not able to make mutable arrays that way, and > need to remove the offending item from the array. > > I could declare two arrays with an if/else... but that seemed to me to be more > duplicated code :-(. [@[] mutableCopy], or [NSMutableArray arrayWithArray:@[]] would work, although it's doing additional work solely for the sake of formatting. This isn't code that will be executed in a loop or anything, so I don't think the extra work would be an issue. That being said, feel free to leave as is if you want. https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:257: insertObject:l10n_util::GetNSString(IDS_SAD_TAB_RELOAD_INCOGNITO) On 2017/06/22 15:15:38, PL wrote: > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > Maybe declare the string on a separate line to help formatting a bit? > > Done! Thanks! You said Done, but there's no new patch :)
On 2017/06/22 18:20:11, kkhorimoto_ wrote: > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): > > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:254: nil]; > On 2017/06/22 15:15:38, PL wrote: > > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > > I think the formatting here would be much better if you use the @[] > container > > > literal format. > > > > I agree, but unfortunately I am not able to make mutable arrays that way, and > > need to remove the offending item from the array. > > > > I could declare two arrays with an if/else... but that seemed to me to be more > > duplicated code :-(. > > [@[] mutableCopy], or [NSMutableArray arrayWithArray:@[]] would work, although > it's doing additional work solely for the sake of formatting. This isn't code > that will be executed in a loop or anything, so I don't think the extra work > would be an issue. That being said, feel free to leave as is if you want. > > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:257: > insertObject:l10n_util::GetNSString(IDS_SAD_TAB_RELOAD_INCOGNITO) > On 2017/06/22 15:15:38, PL wrote: > > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > > Maybe declare the string on a separate line to help formatting a bit? > > > > Done! Thanks! > > You said Done, but there's no new patch :) It's coming :-)
On 2017/06/22 18:20:55, PL wrote: > On 2017/06/22 18:20:11, kkhorimoto_ wrote: > > > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > > File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): > > > > > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > > ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:254: nil]; > > On 2017/06/22 15:15:38, PL wrote: > > > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > > > I think the formatting here would be much better if you use the @[] > > container > > > > literal format. > > > > > > I agree, but unfortunately I am not able to make mutable arrays that way, > and > > > need to remove the offending item from the array. > > > > > > I could declare two arrays with an if/else... but that seemed to me to be > more > > > duplicated code :-(. > > > > [@[] mutableCopy], or [NSMutableArray arrayWithArray:@[]] would work, although > > it's doing additional work solely for the sake of formatting. This isn't code > > that will be executed in a loop or anything, so I don't think the extra work > > would be an issue. That being said, feel free to leave as is if you want. > > > > > https://codereview.chromium.org/2949833005/diff/1/ios/chrome/browser/ui/sad_t... > > ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:257: > > insertObject:l10n_util::GetNSString(IDS_SAD_TAB_RELOAD_INCOGNITO) > > On 2017/06/22 15:15:38, PL wrote: > > > On 2017/06/22 00:33:31, kkhorimoto_ wrote: > > > > Maybe declare the string on a separate line to help formatting a bit? > > > > > > Done! Thanks! > > > > You said Done, but there's no new patch :) > > It's coming :-) It's here!
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
Except, could you add an EG test verifying the incognito string is handled properly?
On 2017/06/22 18:35:52, kkhorimoto_ wrote: > Except, could you add an EG test verifying the incognito string is handled > properly? Yes! As I'd like to nominate this for m60, can I put up a separate CL for the EG test?
On 2017/06/22 18:37:25, PL wrote: > On 2017/06/22 18:35:52, kkhorimoto_ wrote: > > Except, could you add an EG test verifying the incognito string is handled > > properly? > > Yes! As I'd like to nominate this for m60, can I put up a separate CL for the EG > test? (In m60 we don't have a chrome://crash URL, so it's not possible to get to this crash page on simulator, and only in a flaky way on device :-( ). But we do have chrome://crash now on master so an EG test can only really work there...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2017/06/22 18:47:04, PL wrote: > On 2017/06/22 18:37:25, PL wrote: > > On 2017/06/22 18:35:52, kkhorimoto_ wrote: > > > Except, could you add an EG test verifying the incognito string is handled > > > properly? > > > > Yes! As I'd like to nominate this for m60, can I put up a separate CL for the > EG > > test? > > (In m60 we don't have a chrome://crash URL, so it's not possible to get to this > crash page on simulator, and only in a flaky way on device :-( ). > > But we do have chrome://crash now on master so an EG test can only really work > there... Sure, add the test as a follow-up CL :)
On 2017/06/22 20:23:58, kkhorimoto_ wrote: > On 2017/06/22 18:47:04, PL wrote: > > On 2017/06/22 18:37:25, PL wrote: > > > On 2017/06/22 18:35:52, kkhorimoto_ wrote: > > > > Except, could you add an EG test verifying the incognito string is handled > > > > properly? > > > > > > Yes! As I'd like to nominate this for m60, can I put up a separate CL for > the > > EG > > > test? > > > > (In m60 we don't have a chrome://crash URL, so it's not possible to get to > this > > crash page on simulator, and only in a flaky way on device :-( ). > > > > But we do have chrome://crash now on master so an EG test can only really work > > there... > > Sure, add the test as a follow-up CL :) It's here! https://codereview.chromium.org/2952343002/ (Not sure if there's a formal process to identify a related CL though).
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2949833005/#ps40001 (title: "Protect against a case under unit testing where navigationManager could be nil")
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": 40001, "attempt_start_ts": 1498181131205370,
"parent_rev": "df4550446034859cc1cf083ec826e6351b1dd2e1", "commit_rev":
"5e1c61bdb08f0704518a64144592ce1a2ee31cf7"}
Message was sent while issue was closed.
Description was changed from ========== Remove incognito advice from Sad Tab if already incognito BUG=735534 TEST=Repeat crashes when not in Incognito mode show the advice to try Incognito mode. However the same test performed when in Incognito mode should not offer this advice as one of the bullet points. ========== to ========== Remove incognito advice from Sad Tab if already incognito BUG=735534 TEST=Repeat crashes when not in Incognito mode show the advice to try Incognito mode. However the same test performed when in Incognito mode should not offer this advice as one of the bullet points. Review-Url: https://codereview.chromium.org/2949833005 Cr-Commit-Position: refs/heads/master@{#481763} Committed: https://chromium.googlesource.com/chromium/src/+/5e1c61bdb08f0704518a64144592... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5e1c61bdb08f0704518a64144592... |
