Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8921008:
Follow-up to bug 102685 (r113862) (Closed)
|
Created:
9 years ago by Tyler Breisacher (Chromium) Modified:
9 years ago CC:
chromium-reviews, estade+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFollow-up to bug 102685 (r113862)
BUG=102685
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114171
Patch Set 1 #Patch Set 2 : Refactoring AddCSSClass into a separate method #
Total comments: 6
Patch Set 3 : Move and rename AddCSSClass #
Total comments: 9
Patch Set 4 : Following suggestions from Evan #
Total comments: 4
Patch Set 5 : Fixing the documentation for CreateSpanWithClass #Messages
Total messages: 16 (0 generated)
New CL for your comments on http://codereview.chromium.org/8822024 since that one is already closed.
http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, This does not need to be on the NTPLoginHandler class, and there does not need to be two functions. http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, Need a more descriptive (non-generic) name for the function.
http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, What would be a better class to put this in? Or should it just be a standalone function? http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, I made two separate functions because "username" at line 196 is a UTF-8 string, but in the other two cases the display_string is a UTF-16 string. Is there a better way to deal with this?
http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, On 2011/12/12 19:15:02, tbreisacher wrote: > What would be a better class to put this in? Or should it just be a standalone > function? Not all functions need to live on an object, and in fact most shouldn't if they don't touch any part of the object. Move the function into the unnamed namespace at the top of the file. http://codereview.chromium.org/8921008/diff/2001/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:221: string16 NTPLoginHandler::AddCSSClass(std::string display_string, On 2011/12/12 19:15:02, tbreisacher wrote: > I made two separate functions because "username" at line 196 is a UTF-8 string, > but in the other two cases the display_string is a UTF-16 string. Is there a > better way to deal with this? UTF8ToUTF16
> Not all functions need to live on an object Of course. I was thinking like a Java programmer. Gotta break that habit.
http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:63: I'm not that happy with this name, other ideas are definitely welcome.
http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:66: std::string css_class) { const ref on the parameters http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:67: return UTF8ToUTF16("<span class='" + css_class + "'>") + both of these should be ASCIITo
Actually, they were ASCIITo before, but then I switched them, for reasons that I'm pretty sure don't apply anymore. I'll switch them back.
http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:63: On 2011/12/12 19:35:05, tbreisacher wrote: > I'm not that happy with this name, other ideas are definitely welcome. CreateSpanWithClass(content, css_class) http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:204: } no curlies
Sorry for sending out a zillion emails. Thank you James for helping me out with the code review system. http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:63: On 2011/12/12 19:48:59, Evan Stade wrote: > On 2011/12/12 19:35:05, tbreisacher wrote: > > I'm not that happy with this name, other ideas are definitely welcome. > > CreateSpanWithClass(content, css_class) Done. http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:66: std::string css_class) { On 2011/12/12 19:35:52, Evan Stade wrote: > const ref on the parameters Done. http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:67: return UTF8ToUTF16("<span class='" + css_class + "'>") + On 2011/12/12 19:35:52, Evan Stade wrote: > both of these should be ASCIITo Done. http://codereview.chromium.org/8921008/diff/8002/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:204: } On 2011/12/12 19:48:59, Evan Stade wrote: > no curlies Done.
LGTM with nits. http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:64: // Put the display_string into a span with the given CSS class. s/Put/Puts/ http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:64: // Put the display_string into a span with the given CSS class. s/display_string/|display_string|/
http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right): http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:64: // Put the display_string into a span with the given CSS class. On 2011/12/12 23:24:41, James Hawkins wrote: > s/Put/Puts/ Done. http://codereview.chromium.org/8921008/diff/8003/chrome/browser/ui/webui/ntp/... chrome/browser/ui/webui/ntp/ntp_login_handler.cc:64: // Put the display_string into a span with the given CSS class. On 2011/12/12 23:24:41, James Hawkins wrote: > s/display_string/|display_string|/ Or, |content| since I renamed it based on estade's suggestion...
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8921008/4004
Change committed as 114171 |
