Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829326:
Add the webui handler for Android's bookmark section on the NTP. (Closed)
|
Created:
8 years, 4 months ago by Ted C Modified:
8 years, 4 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, newt (away) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd the webui handler for Android's bookmark section on the NTP.
BUG=137011
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152226
Patch Set 1 #
Total comments: 38
Patch Set 2 : Resolve review comments #
Total comments: 8
Patch Set 3 : Resolving review comments. #Patch Set 4 : Fix review comments I missed. #
Total comments: 14
Patch Set 5 : Resolving Review comments. #
Total comments: 8
Patch Set 6 : Review comments round 3 #Patch Set 7 : #
Messages
Total messages: 18 (0 generated)
estade and aruslan for review of webui/ntp code. Will add OWNER for gyp change once the initial round of reviews is over
Looks good to me.
http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:40: return false; \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:42: return false; \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:89: bookmark_model_->RemoveObserver(this); \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:110: } \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:181: return; \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:213: folder == partner_bookmarks_shim_->get_attach_point()) curlies http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:223: hierarchy_entry->SetBoolean("root", true); \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:272: BookmarkModelChanged(); the NTP may not be ready yet. This should not do anything if getBookmarks hasn't yet been called. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:276: BookmarkModelChanged(); ditto (I guess) http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:299: NotifyModelChanged(*(result.get())); you can just do *result however, I don't understand why this is even a scoped_ptr. Just make it a normal object. (also in the next couple functions). http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:313: DCHECK(partner_bookmarks_shim_ != NULL); no need for != http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:314: DCHECK(!partner_bookmarks_shim_->IsPartnerBookmark(node)); DCHECKS go at very beginning. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:344: return; \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:362: return; \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:370: } \n http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:378: if (tab) curlies
Comments addressed, thanks. PTAL http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:40: return false; On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:42: return false; On 2012/08/14 22:04:54, Evan Stade wrote: > \n Is your preference to have blank lines between different conditionals? Just trying to figure out how to be consistent for future reviews. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... This seems to be consistent with that: Blank lines inside a chain of if-else blocks may well help readability. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:89: bookmark_model_->RemoveObserver(this); On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:110: } On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:181: return; On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:213: folder == partner_bookmarks_shim_->get_attach_point()) On 2012/08/14 22:04:54, Evan Stade wrote: > curlies Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:223: hierarchy_entry->SetBoolean("root", true); On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:272: BookmarkModelChanged(); On 2012/08/14 22:04:54, Evan Stade wrote: > the NTP may not be ready yet. This should not do anything if getBookmarks > hasn't yet been called. Added a flag to track whether get has been called and the check in BookmarkModelChanged() http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:299: NotifyModelChanged(*(result.get())); On 2012/08/14 22:04:54, Evan Stade wrote: > you can just do *result > > however, I don't understand why this is even a scoped_ptr. Just make it a normal > object. (also in the next couple functions). Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:313: DCHECK(partner_bookmarks_shim_ != NULL); On 2012/08/14 22:04:54, Evan Stade wrote: > no need for != Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:314: DCHECK(!partner_bookmarks_shim_->IsPartnerBookmark(node)); On 2012/08/14 22:04:54, Evan Stade wrote: > DCHECKS go at very beginning. Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:344: return; On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:362: return; On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:370: } On 2012/08/14 22:04:54, Evan Stade wrote: > \n Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:378: if (tab) On 2012/08/14 22:04:54, Evan Stade wrote: > curlies Done.
http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:42: return false; On 2012/08/14 23:24:28, Ted C wrote: > On 2012/08/14 22:04:54, Evan Stade wrote: > > \n > > Is your preference to have blank lines between different conditionals? Just > trying to figure out how to be consistent for future reviews. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... > > This seems to be consistent with that: > Blank lines inside a chain of if-else blocks may well help readability. really I just want there to be more vertical spacing overall in this file. I asked for newlines in between if statements because otherwise at a glance they look like they could be if/else blocks. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:46: } else { you don't need this else because the if has a return. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:139: if (bookmark_model_) curlies are required if the contents of the conditional are more than one line (even if it's only one expression) http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:169: return "p" + Int64ToString(node->id()); ternary operator here: return (partner ? "p" : "") + Int64ToString(node->id()); http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:324: SetParentInBookmarksResult(*(node->parent()), &result); parens around node->parent() not necessary http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:336: web_ui()->CallJavascriptFunction("ntp.bookmarkChanged", status); what is status used for? why is it needed here but not above http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:371: if (!creating_shortcut_) why is this necessary? http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.h (right): http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.h:63: void HandleShortcutToBookmark(const base::ListValue* args); This purpose function is not obvious. Can you document what it does? it seems to create a shortcut on the home screen for the given bookmark. Seems like it should be called CreateDesktopShortcutForBookmark in that case.
http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:46: } else { On 2012/08/15 00:05:10, Evan Stade wrote: > you don't need this else because the if has a return. Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:139: if (bookmark_model_) On 2012/08/15 00:05:10, Evan Stade wrote: > curlies are required if the contents of the conditional are more than one line > (even if it's only one expression) Done. http://codereview.chromium.org/10829326/diff/1/chrome/browser/ui/webui/ntp/an... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:169: return "p" + Int64ToString(node->id()); On 2012/08/15 00:05:10, Evan Stade wrote: > ternary operator here: > > return (partner ? "p" : "") + Int64ToString(node->id()); Done. http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:324: SetParentInBookmarksResult(*(node->parent()), &result); On 2012/08/15 00:05:10, Evan Stade wrote: > parens around node->parent() not necessary Done. http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:336: web_ui()->CallJavascriptFunction("ntp.bookmarkChanged", status); On 2012/08/15 00:05:10, Evan Stade wrote: > what is status used for? why is it needed here but not above Status allows us to tell the NTP what exact node was affected so it can refresh selectively. In the above case, we don't have the change information, so we just tell the NTP to refresh whatever view it is on. http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:371: if (!creating_shortcut_) On 2012/08/15 00:05:10, Evan Stade wrote: > why is this necessary? I think it was here for legacy reasons. I removed the check and renamed the method to make it explicit when it should be used. http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.h (right): http://codereview.chromium.org/10829326/diff/5001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.h:63: void HandleShortcutToBookmark(const base::ListValue* args); On 2012/08/15 00:05:10, Evan Stade wrote: > This purpose function is not obvious. Can you document what it does? it seems to > create a shortcut on the home screen for the given bookmark. Seems like it > should be called CreateDesktopShortcutForBookmark in that case. Changed the name to: HandleCreateHomeScreenBookmarkShortcut All other javascript call backs in other handlers begin with "Handle". And desktop isn't really an android term, so I used home screen.
https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:130: web_ui()->RegisterMessageCallback("shortcutToBookmark", update this string to match https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:178: + Int64ToString(node->id()); operators go at end of (previous) line https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:215: (const BookmarkNode*) folder->GetChild(i); this cast should not be necessary. Also you are not allowed to use c-style casts but instead should use c++ style (static_cast, for example). https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:231: scoped_ptr<DictionaryValue> hierarchy_entry(new DictionaryValue); bare pointer. https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:276: void BookmarksHandler::SendResult(const scoped_ptr<DictionaryValue>& result) { should take a const DictionaryValue& https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:352: BookmarkNode* node = const_cast<BookmarkNode*>( don't use const cast. https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.h (right): https://chromiumcodereview.appspot.com/10829326/diff/8003/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/bookmarks_handler.h:120: const scoped_ptr<DictionaryValue>& result); Out params should be bare pointers.
Thanks for helping us clean up this cruft. Good to have a new set of eyes finding the things that have slipped in over the lifetime. http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:130: web_ui()->RegisterMessageCallback("shortcutToBookmark", On 2012/08/15 22:24:00, Evan Stade wrote: > update this string to match I'm waiting on this: http://codereview.chromium.org/10831317/#ps2012 I already added a TODO above to update this http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:178: + Int64ToString(node->id()); On 2012/08/15 22:24:00, Evan Stade wrote: > operators go at end of (previous) line Done. http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:215: (const BookmarkNode*) folder->GetChild(i); On 2012/08/15 22:24:00, Evan Stade wrote: > this cast should not be necessary. Also you are not allowed to use c-style casts > but instead should use c++ style (static_cast, for example). Done. http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:231: scoped_ptr<DictionaryValue> hierarchy_entry(new DictionaryValue); On 2012/08/15 22:24:00, Evan Stade wrote: > bare pointer. Done. http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:276: void BookmarksHandler::SendResult(const scoped_ptr<DictionaryValue>& result) { On 2012/08/15 22:24:00, Evan Stade wrote: > should take a const DictionaryValue& Done. http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:352: BookmarkNode* node = const_cast<BookmarkNode*>( On 2012/08/15 22:24:00, Evan Stade wrote: > don't use const cast. Done.
LGTM with outparam nit http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/8003/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:130: web_ui()->RegisterMessageCallback("shortcutToBookmark", On 2012/08/15 22:50:48, Ted C wrote: > On 2012/08/15 22:24:00, Evan Stade wrote: > > update this string to match > > I'm waiting on this: > http://codereview.chromium.org/10831317/#ps2012 > > I already added a TODO above to update this ah i see. Didn't notice the TODO. http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback 2 spaces here too (presubmit should catch this) http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.h (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.h:119: DictionaryValue& result); "Input parameters are usually values or const references, while output and input/output parameters will be non-const pointers." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback On 2012/08/15 22:57:27, Evan Stade wrote: > 2 spaces here too (presubmit should catch this) two spaces after the ; or after the // ?
http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback On 2012/08/16 00:33:04, Ted C wrote: > On 2012/08/15 22:57:27, Evan Stade wrote: > > 2 spaces here too (presubmit should catch this) > > two spaces after the ; or after the // ? seeing that: git grep "; // " --> 5 matches while: git grep "; // " --> a crap ton... I'll go ahead and assume the latter is correct and it is two spaces after the ; http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.h (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.h:119: DictionaryValue& result); On 2012/08/15 22:57:27, Evan Stade wrote: > "Input parameters are usually values or const references, while output and > input/output parameters will be non-const pointers." > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done.
btw http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback On 2012/08/15 22:57:27, Evan Stade wrote: > 2 spaces here too (presubmit should catch this) the presubmit checks for \s{2,} only after {, but otherwise just \s+ - http://code.google.com/searchframe#OAMlx_jo-ck/tools/depot_tools/cpplint.py&t...
lgtm http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback On 2012/08/16 00:47:16, Dan Beam wrote: > On 2012/08/15 22:57:27, Evan Stade wrote: > > 2 spaces here too (presubmit should catch this) > > the presubmit checks for \s{2,} only after {, but otherwise just \s+ - > http://code.google.com/searchframe#OAMlx_jo-ck/tools/depot_tools/cpplint.py&t... Dan, I think you have that backwards.
http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc (right): http://codereview.chromium.org/10829326/diff/12001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/android/bookmarks_handler.cc:152: return; // is handled with a PartnerShimLoaded() callback On 2012/08/16 03:02:29, Evan Stade wrote: > On 2012/08/16 00:47:16, Dan Beam wrote: > > On 2012/08/15 22:57:27, Evan Stade wrote: > > > 2 spaces here too (presubmit should catch this) > > > > the presubmit checks for \s{2,} only after {, but otherwise just \s+ - > > > http://code.google.com/searchframe#OAMlx_jo-ck/tools/depot_tools/cpplint.py&t... > > Dan, I think you have that backwards. Whoops, yeah, you're right.
+thakis for OWNERS of chrome_browser.gypi
gpyi change lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10829326/6
Change committed as 152226 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
