Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
891493003:
[Android] Upstreamed Stars UMA from iOS. (Closed)
|
Created:
5 years, 10 months ago by Kibeom Kim (inactive) Modified:
5 years, 10 months ago CC:
chromium-reviews, noyau+watch_chromium.org, Ian Wen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Upstreamed Stars UMA from iOS.
There are some UMA strings, enum constants in iOS repository
that can be shared with Android, so moved to components.
BUG=452254
Committed: https://crrev.com/f0a2c44b48f239ea4e6c4d0571be846a5fc524fa
Cr-Commit-Position: refs/heads/master@{#314707}
Patch Set 1 #
Total comments: 4
Patch Set 2 : assigned enum numbers #Patch Set 3 : updated GN #
Total comments: 4
Patch Set 4 : addressed GN nits and fixed gyp #
Total comments: 4
Patch Set 5 : more GN fixes #Patch Set 6 : gn #
Messages
Total messages: 34 (14 generated)
kkimlabs@chromium.org changed reviewers: + lpromero@chromium.org, newt@chromium.org, noyau@chromium.org
PTAL
https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... File components/enhanced_bookmarks/enhanced_bookmark_utils.h (right): https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... components/enhanced_bookmarks/enhanced_bookmark_utils.h:27: ALL_ITEMS, As those values have significance outside of the code, please assign them numbers ALL_ITEMs = 0; https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... components/enhanced_bookmarks/enhanced_bookmark_utils.h:34: }; Since this is used externally, I'd put a = 0 for the first item. I think we are now missing a TOP_LEVEL entry in there, for the three top level folders.
lgtm https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... File components/enhanced_bookmarks/enhanced_bookmark_utils.h (right): https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... components/enhanced_bookmarks/enhanced_bookmark_utils.h:34: }; On 2015/02/04 12:53:40, noyau wrote: > Since this is used externally, I'd put a = 0 for the first item. > > I think we are now missing a TOP_LEVEL entry in there, for the three top level > folders. They are considered as FOLDER.
New patchsets have been uploaded after l-g-t-m from lpromero@chromium.org
sky@: PTAL chrome/chrome.gyp (one line change) https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... File components/enhanced_bookmarks/enhanced_bookmark_utils.h (right): https://codereview.chromium.org/891493003/diff/1/components/enhanced_bookmark... components/enhanced_bookmarks/enhanced_bookmark_utils.h:27: ALL_ITEMS, On 2015/02/04 12:53:40, noyau wrote: > As those values have significance outside of the code, please assign them > numbers > > ALL_ITEMs = 0; Done.
kkimlabs@chromium.org changed reviewers: + sky@chromium.org
LGTM
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891493003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Don't you need to update some GN files?
yeah just noticed that, working on..
New patchsets have been uploaded after l-g-t-m from sky@chromium.org
The CQ bit was checked by kkimlabs@chromium.org
The CQ bit was unchecked by kkimlabs@chromium.org
kkimlabs@chromium.org changed reviewers: + cjhopman@chromium.org
PTAL Chris or Newton, could you take a look at GN files?
lgtm https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... components/enhanced_bookmarks.gypi:67: # GN: //components/enhanced_bookmarks: nit: s/marks:/marks:enhanced_bookmarks_launch_location_java https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... components/enhanced_bookmarks/BUILD.gn:58: # GYP: //components/enhanced_bookmarks.enhanced_bookmarks_launch_location_java nit: s/marks.enhanced/marks.gypi:enhanced/
New patchsets have been uploaded after l-g-t-m from cjhopman@chromium.org
https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... components/enhanced_bookmarks.gypi:67: # GN: //components/enhanced_bookmarks: On 2015/02/04 22:00:04, cjhopman wrote: > nit: s/marks:/marks:enhanced_bookmarks_launch_location_java Done. https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... File components/enhanced_bookmarks/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/40001/components/enhanced_book... components/enhanced_bookmarks/BUILD.gn:58: # GYP: //components/enhanced_bookmarks.enhanced_bookmarks_launch_location_java On 2015/02/04 22:00:04, cjhopman wrote: > nit: s/marks.enhanced/marks.gypi:enhanced/ Done.
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891493003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
https://codereview.chromium.org/891493003/diff/60001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/60001/chrome/android/BUILD.gn#... chrome/android/BUILD.gn:85: "//components/enhanced_bookmarks:enhanced_bookmarks_launch_location_java", sry, this should go in srcjar_deps https://codereview.chromium.org/891493003/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/60001/components/enhanced_book... components/enhanced_bookmarks/BUILD.gn:59: java_cpp_enum("enhanced_bookmarks_launch_location_java") { This target actually generates a srcjar, not a full java library. It should then end in _javagen or _srcjar (it looks like _srcjar is currently used just a little bit more).
https://codereview.chromium.org/891493003/diff/60001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/60001/chrome/android/BUILD.gn#... chrome/android/BUILD.gn:85: "//components/enhanced_bookmarks:enhanced_bookmarks_launch_location_java", On 2015/02/04 22:25:01, cjhopman wrote: > sry, this should go in srcjar_deps Done. https://codereview.chromium.org/891493003/diff/60001/components/enhanced_book... File components/enhanced_bookmarks/BUILD.gn (right): https://codereview.chromium.org/891493003/diff/60001/components/enhanced_book... components/enhanced_bookmarks/BUILD.gn:59: java_cpp_enum("enhanced_bookmarks_launch_location_java") { On 2015/02/04 22:25:01, cjhopman wrote: > This target actually generates a srcjar, not a full java library. It should then > end in _javagen or _srcjar (it looks like _srcjar is currently used just a > little bit more). Done.
The CQ bit was checked by kkimlabs@chromium.org
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891493003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f0a2c44b48f239ea4e6c4d0571be846a5fc524fa Cr-Commit-Position: refs/heads/master@{#314707} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
