Dan Burkert has posted comments on this change. Change subject: Integrate Mustache templates to webserver ......................................................................
Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/7448/9/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: Line 479: static const char* const kMainTemplate = R"( How come you decided to keep this template inline instead of putting into the www/ directory? I don't have much an opinion on inlining vs. not, but seems like we should keep it consistent. In fact it may make things simpler to always inline? I know we discussed this previously, but the templates in www/ seem simple enough that inlining wouldn't be too bad, especially with these fancy raw string initializers. http://gerrit.cloudera.org:8080/#/c/7448/8/src/kudu/server/webserver.h File src/kudu/server/webserver.h: Line 63: const PathHandlerCallback& callback, reindent the param lists here and below. http://gerrit.cloudera.org:8080/#/c/7448/9/src/kudu/server/webserver.h File src/kudu/server/webserver.h: Line 67: const PrerenderedPathHandlerCallback& callback, re-indent here and above http://gerrit.cloudera.org:8080/#/c/7448/9/src/kudu/util/easy_json.cc File src/kudu/util/easy_json.cc: Line 187: } I think this needs a final 'else { LOG(FATAL) << "Unknown initializer type" }'; it's possible to case an arbitrary int to a C++ enum. http://gerrit.cloudera.org:8080/#/c/7448/9/src/kudu/util/easy_json.h File src/kudu/util/easy_json.h: Line 120: // Returns a reference to the child object. This method and below don't return a reference, perhaps just 'Returns the child object.'? Line 161: std::string ToString(bool html = false) const; I think it'd be better to keep ToString without a param, since that matches the style of the rest of the codebase. Instead you could add 'ToStringHtml', or just inline the <pre> tag generation into the caller, since it looks like it's only used once currently. -- To view, visit http://gerrit.cloudera.org:8080/7448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8cf8181b2b2904a57c9b24ce73316b92a4f6700f Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sam Okrent <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
