Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421823003:
[NTP Snippets] Fix UI crash on empty remote sections (Closed)
|
Created:
4 years, 2 months ago by Marc Treib Modified:
4 years, 2 months ago Reviewers:
dgn CC:
chromium-reviews, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP Snippets] Fix UI crash on empty remote sections
BUG=655992
Committed: https://crrev.com/105f0bbe1dda281ddc2076874646b8a21b40897c
Cr-Commit-Position: refs/heads/master@{#425339}
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 14 (7 generated)
treib@chromium.org changed reviewers: + dgn@chromium.org
PTAL! https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java (right): https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:125: Log.wtf(TAG, "Requested description for unsupported category: %d", mCategory); Not quite sure what's best to do here... this will still crash, but only for sections that aren't enabled yet. I guess that's okay for now?
lgtm https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java (right): https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:125: Log.wtf(TAG, "Requested description for unsupported category: %d", mCategory); On 2016/10/14 14:46:22, Marc Treib wrote: > Not quite sure what's best to do here... this will still crash, but only for > sections that aren't enabled yet. I guess that's okay for now? Can they be enabled remotely? In that case it would be better to return something. Or we should make it clear somewhere that all that is not shippable on M55
https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java (right): https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:125: Log.wtf(TAG, "Requested description for unsupported category: %d", mCategory); On 2016/10/14 14:48:29, dgn wrote: > On 2016/10/14 14:46:22, Marc Treib wrote: > > Not quite sure what's best to do here... this will still crash, but only for > > sections that aren't enabled yet. I guess that's okay for now? > > Can they be enabled remotely? In that case it would be better to return > something. Or we should make it clear somewhere that all that is not shippable > on M55 They could in principle be enabled through Finch, but none of them is in a state where that would make sense. It might still be safer to return something, I don't know...
The CQ bit was checked by treib@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...
https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java (right): https://codereview.chromium.org/2421823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java:125: Log.wtf(TAG, "Requested description for unsupported category: %d", mCategory); On 2016/10/14 15:10:44, Marc Treib wrote: > On 2016/10/14 14:48:29, dgn wrote: > > On 2016/10/14 14:46:22, Marc Treib wrote: > > > Not quite sure what's best to do here... this will still crash, but only for > > > sections that aren't enabled yet. I guess that's okay for now? > > > > Can they be enabled remotely? In that case it would be better to return > > something. Or we should make it clear somewhere that all that is not shippable > > on M55 > > They could in principle be enabled through Finch, but none of them is in a state > where that would make sense. > It might still be safer to return something, I don't know... ...but generally I'd prefer crashing, so authors of other sections notice as soon as possible that there's something missing here (rather than getting an easy-to-miss error like an inappropriate string).
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 treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Fix UI crash on empty remote sections BUG=655992 ========== to ========== [NTP Snippets] Fix UI crash on empty remote sections BUG=655992 Committed: https://crrev.com/105f0bbe1dda281ddc2076874646b8a21b40897c Cr-Commit-Position: refs/heads/master@{#425339} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/105f0bbe1dda281ddc2076874646b8a21b40897c Cr-Commit-Position: refs/heads/master@{#425339} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
