Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678343004:
[Suggestions category] Warn more about removing categories. (Closed)
Description[Suggestions category] Warn more about removing categories.
This CL adds a more verbose DLOG warning if a local category is removed.
Furthermore, a big comment warns you in advance when you add a new local
category.
BUG=682184
Review-Url: https://codereview.chromium.org/2678343004
Cr-Commit-Position: refs/heads/master@{#448958}
Committed: https://chromium.googlesource.com/chromium/src/+/2d864d4d394055b7e20606037f7d4465d3928bd2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments #
Total comments: 6
Patch Set 3 : Comments #2 #
Messages
Total messages: 17 (7 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org, vitaliii@chromium.org
Marc, Vitalii, PTAL!
https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.cc (right): https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.cc:28: "you switch out of a branch with a new local category."; I have multiple concerns regarding this message: 1) It does not look like a sentence: "Not a valid ID: 13 Note that local categories..." 2) Too much attention to category removal. There may be other reasons for this DCHECK to fail, also shuffling categories is bad as well. 3) The last sentence seems very difficult to understand. I think it is enough to point the reader into right direction (KnownCategories) and have more elaborate message there. Taking all of these into account, I would go for: "id << is not a valid category ID. This may have been caused by removal of a local KnownCategory." https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:41: // Otherwise, Chrome will crash and you would need to reinstall it to fix it. > This happens easily if you switch out of a > local branch that contains a new category. Jumping directly to "switching out of a local branch" seems premature. This is just one example of "removing a category implicitly", I would like to point this out explicitly instead of an example. Also "switching out of a branch" does not cause this by itself, you also need to install the app, so this is very indirect way of saying what is going on. > If you cannot avoid it, consider forcing ConstantCategoryRanker > via chrome://flags. Otherwise, Chrome will crash and you would > need to reinstall it to fix it. I do not like giving advice on how to avoid this. There may be other places where this information is persisted, so this comment is likely to become outdated (and misleading). Also this is not Android-only code, so I am not sure whether "Chrome will crash" is always true. Overall, I would go for: " Existing categories are persisted and they must never be removed. This may happen implicitly, e.g. when an older version without some local category is installed. " As for development, you can make a parent branch with your new category and make all other branches depend on it. Or you probably could temporarily add your category as remote.
+1 to Vitalii's comments. I have nothing to add :)
Thanks! https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.cc (right): https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.cc:28: "you switch out of a branch with a new local category."; On 2017/02/08 08:40:35, vitaliii wrote: > I have multiple concerns regarding this message: > 1) It does not look like a sentence: > "Not a valid ID: 13 Note that local categories..." > > 2) Too much attention to category removal. There may be other reasons for this > DCHECK to fail, also shuffling categories is bad as well. > > 3) The last sentence seems very difficult to understand. I think it is enough to > point the reader into right direction (KnownCategories) and have more elaborate > message there. > > Taking all of these into account, I would go for: > > "id << is not a valid category ID. This may have been caused by removal of a > local KnownCategory." Done. https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:41: // Otherwise, Chrome will crash and you would need to reinstall it to fix it. On 2017/02/08 08:40:35, vitaliii wrote: > > This happens easily if you switch out of a > > local branch that contains a new category. > > Jumping directly to "switching out of a local branch" seems premature. This is > just one example of "removing a category implicitly", I would like to point this > out explicitly instead of an example. Also "switching out of a branch" does not > cause this by itself, you also need to install the app, so this is very indirect > way of saying what is going on. > > > If you cannot avoid it, consider forcing ConstantCategoryRanker > > via chrome://flags. Otherwise, Chrome will crash and you would > > need to reinstall it to fix it. > > I do not like giving advice on how to avoid this. There may be other places > where this information is persisted, so this comment is likely to become > outdated (and misleading). Also this is not Android-only code, so I am not sure > whether "Chrome will crash" is always true. > > Overall, I would go for: > > " Existing categories are persisted and they must never be removed. This may > happen implicitly, e.g. when an older version without some local category is > installed. " > > As for development, you can make a parent branch with your new category and make > all other branches depend on it. Or you probably could temporarily add your > category as remote. Done.
The CQ bit was checked by jkrcal@chromium.org
The CQ bit was unchecked by jkrcal@chromium.org
On 2017/02/08 10:12:06, jkrcal wrote: > Thanks! > > https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... > File components/ntp_snippets/category.cc (right): > > https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... > components/ntp_snippets/category.cc:28: "you switch out of a branch with a new > local category."; > On 2017/02/08 08:40:35, vitaliii wrote: > > I have multiple concerns regarding this message: > > 1) It does not look like a sentence: > > "Not a valid ID: 13 Note that local categories..." > > > > 2) Too much attention to category removal. There may be other reasons for this > > DCHECK to fail, also shuffling categories is bad as well. > > > > 3) The last sentence seems very difficult to understand. I think it is enough > to > > point the reader into right direction (KnownCategories) and have more > elaborate > > message there. > > > > Taking all of these into account, I would go for: > > > > "id << is not a valid category ID. This may have been caused by removal of a > > local KnownCategory." > > Done. > > https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... > File components/ntp_snippets/category.h (right): > > https://codereview.chromium.org/2678343004/diff/1/components/ntp_snippets/cat... > components/ntp_snippets/category.h:41: // Otherwise, Chrome will crash and you > would need to reinstall it to fix it. > On 2017/02/08 08:40:35, vitaliii wrote: > > > This happens easily if you switch out of a > > > local branch that contains a new category. > > > > Jumping directly to "switching out of a local branch" seems premature. This is > > just one example of "removing a category implicitly", I would like to point > this > > out explicitly instead of an example. Also "switching out of a branch" does > not > > cause this by itself, you also need to install the app, so this is very > indirect > > way of saying what is going on. > > > > > If you cannot avoid it, consider forcing ConstantCategoryRanker > > > via chrome://flags. Otherwise, Chrome will crash and you would > > > need to reinstall it to fix it. > > > > I do not like giving advice on how to avoid this. There may be other places > > where this information is persisted, so this comment is likely to become > > outdated (and misleading). Also this is not Android-only code, so I am not > sure > > whether "Chrome will crash" is always true. > > > > Overall, I would go for: > > > > " Existing categories are persisted and they must never be removed. This may > > happen implicitly, e.g. when an older version without some local category is > > installed. " > > > > As for development, you can make a parent branch with your new category and > make > > all other branches depend on it. Or you probably could temporarily add your > > category as remote. > > Done. Please take a look again.
LGTM with nits. Thank you, it looks much better now! https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.cc (right): https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.cc:25: DCHECK(IsValidIDValue(id)) << id nit: remove newline after id? There is no newline if you just write '... << id << " is not a valid ... KnownCategory.";' and autoformat it. https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.cc:26: << "is not a valid category ID. This may " nit: s/"is/" is (missing space) https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.h:39: // happen implicitly, e.g.when an older version without some local category is nit: s/e.g.when/e.g. when (missing space)
Thanks! https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.cc (right): https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.cc:25: DCHECK(IsValidIDValue(id)) << id On 2017/02/08 10:18:25, vitaliii wrote: > nit: remove newline after id? > There is no newline if you just write '... << id << " is not a valid ... > KnownCategory.";' and autoformat it. Done. The previous version was formatted by clang-format. There is some weirdness in the behaviour, though :| https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.cc:26: << "is not a valid category ID. This may " On 2017/02/08 10:18:25, vitaliii wrote: > nit: s/"is/" is > (missing space) Sorry, done. https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2678343004/diff/20001/components/ntp_snippets... components/ntp_snippets/category.h:39: // happen implicitly, e.g.when an older version without some local category is On 2017/02/08 10:18:25, vitaliii wrote: > nit: s/e.g.when/e.g. when > (missing space) Done.
lgtm
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vitaliii@chromium.org Link to the patchset: https://codereview.chromium.org/2678343004/#ps40001 (title: "Comments #2")
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": 1486551330048980,
"parent_rev": "7e5c61fd4a063db154f623e75bc71bdcc4bf65fc", "commit_rev":
"2d864d4d394055b7e20606037f7d4465d3928bd2"}
Message was sent while issue was closed.
Description was changed from ========== [Suggestions category] Warn more about removing categories. This CL adds a more verbose DLOG warning if a local category is removed. Furthermore, a big comment warns you in advance when you add a new local category. BUG=682184 ========== to ========== [Suggestions category] Warn more about removing categories. This CL adds a more verbose DLOG warning if a local category is removed. Furthermore, a big comment warns you in advance when you add a new local category. BUG=682184 Review-Url: https://codereview.chromium.org/2678343004 Cr-Commit-Position: refs/heads/master@{#448958} Committed: https://chromium.googlesource.com/chromium/src/+/2d864d4d394055b7e20606037f7d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2d864d4d394055b7e20606037f7d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
