Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
734243003:
Adding the chrome://copresence page (Closed)
DescriptionAdding the chrome://copresence page. Initially this will just have debug info, but soon we need to add some settings too.
BUG=420889
Committed: https://crrev.com/92c8e4e90f364f4db499b0edc8d9d491fdaa064a
Cr-Commit-Position: refs/heads/master@{#309353}
Patch Set 1 #
Total comments: 27
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Fixing BUILD.gn #
Total comments: 2
Patch Set 6 : BUILD.gn tweak #
Total comments: 21
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 44
Patch Set 10 : Addressing review comments #Patch Set 11 : Fixing compile errors #
Total comments: 4
Patch Set 12 : #Patch Set 13 : #
Total comments: 1
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Messages
Total messages: 52 (10 generated)
ckehoe@chromium.org changed reviewers: + rkc@chromium.org, xiyuan@chromium.org
This depends on https://codereview.chromium.org/764673003. Would a browsertest or unittest make more sense?
https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.css:6: background-color: white; nit: not necessary, default bg color is white. https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:13: </head> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:27: <td i18n-content="duration"> </tr> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:28: <tbody> </tdoby> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:40: <td i18n-content="token_send_time"> </tr> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:41: <tbody> </tbody> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:53: <td i18n-content="token_receive_time"> </tr> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:54: <tbody> </tbody> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:64: <script src="chrome://resources/js/i18n_template2.js"></script> </body> </html> https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.js:24: row.cells.item(0).innerText = directive.type; nit: .innerText -> .textContent, here an other places. Think .textContent is more preferrable when it can be used. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui.cc (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui.cc:5: nit: nuke one empty line https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui.h (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui.h:16: DISALLOW_COPY_AND_ASSIGN(CopresenceUI); nit: #include "base/macros.h" https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.h:43: void PopulateState(const base::ListValue* args); nit: PopulateState -> HandlePopulateState as in most of the webui handlers. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.h:47: DISALLOW_COPY_AND_ASSIGN(CopresenceUIHandler); nit: #include "base/macros.h"
https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:40: return "Broadcast"; Any reason we aren't just sticking with "Transmit/Receive"? https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:150: const std::vector<Directive>& active_directives) { Why does this parameter exist? You always call this on state_->active_directives(), which this function has access to also. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:182: TokenSent(token_entry.second); nit: Making calls to JS is expensive. Populate an array with the sent and received tokens and send them off to JS in one call.
ckehoe@chromium.org changed reviewers: + sky@chromium.org
+sky for reviews of: chrome/browser/browser_resources.grd chrome/browser/ui/BUILD.gn https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.css:6: background-color: white; On 2014/12/05 18:56:55, xiyuan wrote: > nit: not necessary, default bg color is white. Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.html:13: On 2014/12/05 18:56:55, xiyuan wrote: > </head> The Google HTML/CSS style guide recommends omitting optional tags: https://engdoc.corp.google.com/eng/doc/htmlcssguide.xml?showone=Optional_Tags... For a summary of the closing tag rules, see: http://stackoverflow.com/questions/3008593/html-include-or-exclude-optional-c... https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/resources/cop... chrome/browser/resources/copresence.js:24: row.cells.item(0).innerText = directive.type; On 2014/12/05 18:56:55, xiyuan wrote: > nit: .innerText -> .textContent, here an other places. > > Think .textContent is more preferrable when it can be used. Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui.cc (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui.cc:5: On 2014/12/05 18:56:55, xiyuan wrote: > nit: nuke one empty line Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui.h (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui.h:16: DISALLOW_COPY_AND_ASSIGN(CopresenceUI); On 2014/12/05 18:56:55, xiyuan wrote: > nit: #include "base/macros.h" Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:40: return "Broadcast"; On 2014/12/05 19:46:36, Rahul Chaturvedi wrote: > Any reason we aren't just sticking with "Transmit/Receive"? Broadcast and scan is the latest terminology used by the server protos. Although "receive" does seem better than "scan". Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:150: const std::vector<Directive>& active_directives) { On 2014/12/05 19:46:36, Rahul Chaturvedi wrote: > Why does this parameter exist? You always call this on > state_->active_directives(), which this function has access to also. Seemed more natural, but it's true that it's redundant. Fixed. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.h:43: void PopulateState(const base::ListValue* args); On 2014/12/05 18:56:56, xiyuan wrote: > nit: PopulateState -> HandlePopulateState as in most of the webui handlers. Done. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.h:47: DISALLOW_COPY_AND_ASSIGN(CopresenceUIHandler); On 2014/12/05 18:56:56, xiyuan wrote: > nit: #include "base/macros.h" Done.
lgtm % (comment and xiyuan's lgtm) https://codereview.chromium.org/734243003/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/copresence_ui_handler.cc:40: return "Send"; We should either stick with Broadcast/Scan or Transmit/Receive - lets not mix and match.
xiyuan@chromium.org changed reviewers: + dbeam@chromium.org
I would like get Dan's thoughts on adopting the "Omit optional tags" rule in chromium html. https://engdoc.corp.google.com/eng/doc/htmlcssguide.xml?showone=Optional_Tags... LGTM once Dan approves that.
Xiyuan, I made some small HTML/CSS changes. Let me know if there are any problems. https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/1/chrome/browser/ui/webui/copr... chrome/browser/ui/webui/copresence_ui_handler.cc:182: TokenSent(token_entry.second); On 2014/12/05 19:46:36, Rahul Chaturvedi wrote: > nit: Making calls to JS is expensive. Populate an array with the sent and > received tokens and send them off to JS in one call. Added a TODO. To make a difference, this requires reworking the JS to handle batches intelligently, instead of the obvious n^2 solution.
On 2014/12/05 23:02:37, Charlie wrote: > Xiyuan, I made some small HTML/CSS changes. Let me know if there are any > problems. HTML/CSS changes SLGTM. But wait for Dan to weigh in on omitting optional tags.
https://codereview.chromium.org/734243003/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/734243003/diff/60001/chrome/browser/ui/BUILD.... chrome/browser/ui/BUILD.gn:412: if (!is_ios && !is_android) { Move to block on 128.
https://codereview.chromium.org/734243003/diff/60001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/734243003/diff/60001/chrome/browser/ui/BUILD.... chrome/browser/ui/BUILD.gn:412: if (!is_ios && !is_android) { On 2014/12/05 23:51:51, sky wrote: > Move to block on 128. Done.
2 out of the 3 style guide[1] maintainers agree: omitting "rootier" end tags (e.g. <html>, <head>, <body>) looks funky while omitting "leafier" (e.g. <p>, <li>) eng tags is more acceptable. [1] http://www.chromium.org/developers/web-development-style-guide
On 2014/12/06 01:39:18, Dan Beam wrote: > 2 out of the 3 style guide[1] maintainers agree: > omitting "rootier" end tags (e.g. <html>, <head>, <body>) looks funky while > omitting "leafier" (e.g. <p>, <li>) eng tags is more acceptable. > > [1] http://www.chromium.org/developers/web-development-style-guide If I also omit the starting html, head, and body tags (see the most recent rev of copresence.html), is that better, or worse? Let me clarify that I'm not trying to argue this issue. I'd just like the style guide to reflect the team's actual standards. If we do want to require these tags, can we add a point to the chromium guide listed above indicating that?
LGTM https://codereview.chromium.org/734243003/diff/80001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/734243003/diff/80001/chrome/browser/ui/BUILD.... chrome/browser/ui/BUILD.gn:132: deps += [ "//device/bluetooth", "//components/copresence" ] I believe we wrap to multiple lines instead of one line in this case. See 31-33 as an example.
https://codereview.chromium.org/734243003/diff/80001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/734243003/diff/80001/chrome/browser/ui/BUILD.... chrome/browser/ui/BUILD.gn:132: deps += [ "//device/bluetooth", "//components/copresence" ] On 2014/12/06 21:19:49, sky wrote: > I believe we wrap to multiple lines instead of one line in this case. See 31-33 > as an example. Done.
On 2014/12/06 01:51:15, Charlie wrote: > On 2014/12/06 01:39:18, Dan Beam wrote: > > 2 out of the 3 style guide[1] maintainers agree: > > omitting "rootier" end tags (e.g. <html>, <head>, <body>) looks funky while > > omitting "leafier" (e.g. <p>, <li>) eng tags is more acceptable. > > > > [1] http://www.chromium.org/developers/web-development-style-guide > > If I also omit the starting html, head, and body tags (see the most recent rev > of copresence.html), is that better, or worse? We say worse. > > Let me clarify that I'm not trying to argue this issue. I'd just like the style > guide to reflect the team's actual standards. If we do want to require these > tags, can we add a point to the chromium guide listed above indicating that? Is this a new addition to the Google HTML/CSS style guide? This issue hasn't come up in the 3.5 years I've been working on webui code. The styleguide also says: """If you’re editing code, take a few minutes to look at the code around you and determine its style.""" Every other webui .html page I've seen has an <html>, <body>, etc.
https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.css:7: font-family: Helvetica,Arial,sans-serif; you should include chrome_shared.css instead https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.css:64: table td.spacer { td:last-of-type instead? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:2: if you don't have an <html> tag, where does [dir] get set for RTL pages? will this page be translated? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:9: <script src="chrome://copresence/strings.js"></script> nit: can these be at the bottom of the page? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:18: <table class="directive-table" id="directives-table"> nit: what's the advantage to using a <table> over flex box here? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:31: <table class="token-table" id="sent-tokens-table"> nit: arguably don't put "-table" in the class, but I guess if it's presentational and not tied to the <table> element it's not so bad https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:57: <!-- TODO(ckehoe): Add server calls --> nit: end comments with . https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} directives can you make this type stricter (e.g. {{type: string, medium: string, duration: number}})? maybe even create a @typedef? we're just starting to compile our JS in chromium and would be useful to have this info. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:70: * @param {Object} token nit: can you @typedef tokens as well? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:79: token.statuses.replace('confirmed', '(Confirmed)'); i18n?
On 2014/12/08 22:28:55, Dan Beam wrote: > On 2014/12/06 01:51:15, Charlie wrote: > > On 2014/12/06 01:39:18, Dan Beam wrote: > > > 2 out of the 3 style guide[1] maintainers agree: > > > omitting "rootier" end tags (e.g. <html>, <head>, <body>) looks funky while > > > omitting "leafier" (e.g. <p>, <li>) eng tags is more acceptable. > > > > > > [1] http://www.chromium.org/developers/web-development-style-guide > > > > If I also omit the starting html, head, and body tags (see the most recent rev > > of copresence.html), is that better, or worse? > > We say worse. > > > > > Let me clarify that I'm not trying to argue this issue. I'd just like the > style > > guide to reflect the team's actual standards. If we do want to require these > > tags, can we add a point to the chromium guide listed above indicating that? > > Is this a new addition to the Google HTML/CSS style guide? This issue hasn't > come up in the 3.5 years I've been working on webui code. The styleguide also > says: > > """If you’re editing code, take a few minutes to look at the code around you > and determine its style.""" > > Every other webui .html page I've seen has an <html>, <body>, etc. Sure, I wouldn't expect to pull the html and body tags out of a webui page I was just editing. But this is a completely new one. The point about optional tags has been around at least since March 2012 when the style guide was migrated from a wiki page: https://cs.corp.google.com/#piper///depot/eng/doc/htmlcssguide.xml&rcl=281867... However, it continues to be optional. So I've put them back in. Will address your other comments next.
https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.css:7: font-family: Helvetica,Arial,sans-serif; On 2014/12/08 22:41:59, Dan Beam wrote: > you should include chrome_shared.css instead Done. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.css:64: table td.spacer { On 2014/12/08 22:41:59, Dan Beam wrote: > td:last-of-type instead? I'm not sure this will work. The HTML includes only table header rows, and the JS then populates data rows as the data becomes available. Data rows don't have spacers because the scroll bar is there instead. Also, I like that the table cells are explicitly labeled as spacers this way. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:2: On 2014/12/08 22:41:59, Dan Beam wrote: > if you don't have an <html> tag, where does [dir] get set for RTL pages? will > this page be translated? It doesn't. But I put it back. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:9: <script src="chrome://copresence/strings.js"></script> On 2014/12/08 22:41:59, Dan Beam wrote: > nit: can these be at the bottom of the page? Done. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:18: <table class="directive-table" id="directives-table"> On 2014/12/08 22:41:59, Dan Beam wrote: > nit: what's the advantage to using a <table> over flex box here? Only that I already know how to use tables :-) Would the columns get lined up in the same way? How about the scrolling behavior? It's important that extra rows be contained and remain accessible via scrolling. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:31: <table class="token-table" id="sent-tokens-table"> On 2014/12/08 22:41:59, Dan Beam wrote: > nit: arguably don't put "-table" in the class, but I guess if it's > presentational and not tied to the <table> element it's not so bad Done. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.html:57: <!-- TODO(ckehoe): Add server calls --> On 2014/12/08 22:41:59, Dan Beam wrote: > nit: end comments with . Done.
On 2014/12/08 23:10:46, Charlie wrote: > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > File chrome/browser/resources/copresence.css (right): > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.css:7: font-family: > Helvetica,Arial,sans-serif; > On 2014/12/08 22:41:59, Dan Beam wrote: > > you should include chrome_shared.css instead > > Done. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.css:64: table td.spacer { > On 2014/12/08 22:41:59, Dan Beam wrote: > > td:last-of-type instead? > > I'm not sure this will work. The HTML includes only table header rows, and the > JS then populates data rows as the data becomes available. Data rows don't have > spacers because the scroll bar is there instead. > > Also, I like that the table cells are explicitly labeled as spacers this way. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > File chrome/browser/resources/copresence.html (right): > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.html:2: > On 2014/12/08 22:41:59, Dan Beam wrote: > > if you don't have an <html> tag, where does [dir] get set for RTL pages? will > > this page be translated? > > It doesn't. But I put it back. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.html:9: <script > src="chrome://copresence/strings.js"></script> > On 2014/12/08 22:41:59, Dan Beam wrote: > > nit: can these be at the bottom of the page? > > Done. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.html:18: <table class="directive-table" > id="directives-table"> > On 2014/12/08 22:41:59, Dan Beam wrote: > > nit: what's the advantage to using a <table> over flex box here? > > Only that I already know how to use tables :-) Would the columns get lined up in > the same way? How about the scrolling behavior? It's important that extra rows > be contained and remain accessible via scrolling. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.html:31: <table class="token-table" > id="sent-tokens-table"> > On 2014/12/08 22:41:59, Dan Beam wrote: > > nit: arguably don't put "-table" in the class, but I guess if it's > > presentational and not tied to the <table> element it's not so bad > > Done. > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.html:57: <!-- TODO(ckehoe): Add server calls > --> > On 2014/12/08 22:41:59, Dan Beam wrote: > > nit: end comments with . > > Done. Oops, didn't see your JS comments. Hang on...
https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} directives On 2014/12/08 22:41:59, Dan Beam wrote: > can you make this type stricter (e.g. {{type: string, medium: string, duration: > number}})? maybe even create a @typedef? we're just starting to compile our JS > in chromium and would be useful to have this info. Done. Can I run closure for just this file and have it check that I did it right? Also, is there a way to provide descriptions of the individual fields? https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:70: * @param {Object} token On 2014/12/08 22:41:59, Dan Beam wrote: > nit: can you @typedef tokens as well? Done. https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:79: token.statuses.replace('confirmed', '(Confirmed)'); On 2014/12/08 22:41:59, Dan Beam wrote: > i18n? i18n of our debug UI is currently a non-goal. More display strings are listed verbatim in copresence_ui_handler.cc. I added an i18n TODO there.
https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} directives On 2014/12/08 23:37:07, Charlie wrote: > On 2014/12/08 22:41:59, Dan Beam wrote: > > can you make this type stricter (e.g. {{type: string, medium: string, > duration: > > number}})? maybe even create a @typedef? we're just starting to compile our > JS > > in chromium and would be useful to have this info. > > Done. Can I run closure for just this file and have it check that I did it > right? Also, is there a way to provide descriptions of the individual fields? create a chrome/browser/resources/compiled_resources.gyp like others[1] and run: # run from src/, may require sudo apt-get install gyp gyp --depth . chrome/browser/resources/compiled_resources.gyp # directory may differ depending on your GYP config? ninja -C out/Default if you'd like your code to be typechecked continuously (on every commit) add your file to third_party/closure_compiler/compiled_resources.gyp. [1] https://code.google.com/p/chromium/codesearch#search/&q=compiled_resources.gy...
On 2014/12/08 23:52:56, Dan Beam wrote: > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > File chrome/browser/resources/copresence.js (right): > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} directives > On 2014/12/08 23:37:07, Charlie wrote: > > On 2014/12/08 22:41:59, Dan Beam wrote: > > > can you make this type stricter (e.g. {{type: string, medium: string, > > duration: > > > number}})? maybe even create a @typedef? we're just starting to compile > our > > JS > > > in chromium and would be useful to have this info. > > > > Done. Can I run closure for just this file and have it check that I did it > > right? Also, is there a way to provide descriptions of the individual fields? > > create a chrome/browser/resources/compiled_resources.gyp like others[1] and run: > > # run from src/, may require sudo apt-get install gyp > gyp --depth . chrome/browser/resources/compiled_resources.gyp > > # directory may differ depending on your GYP config? > ninja -C out/Default > > if you'd like your code to be typechecked continuously (on every commit) add > your file to third_party/closure_compiler/compiled_resources.gyp. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=compiled_resources.gy... Cool, but that digs up more errors than I'm prepared to deal with right now :-)
On 2014/12/09 00:28:15, Charlie wrote: > On 2014/12/08 23:52:56, Dan Beam wrote: > > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > > File chrome/browser/resources/copresence.js (right): > > > > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > > chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} directives > > On 2014/12/08 23:37:07, Charlie wrote: > > > On 2014/12/08 22:41:59, Dan Beam wrote: > > > > can you make this type stricter (e.g. {{type: string, medium: string, > > > duration: > > > > number}})? maybe even create a @typedef? we're just starting to compile > > our > > > JS > > > > in chromium and would be useful to have this info. > > > > > > Done. Can I run closure for just this file and have it check that I did it > > > right? Also, is there a way to provide descriptions of the individual > fields? > > > > create a chrome/browser/resources/compiled_resources.gyp like others[1] and > run: > > > > # run from src/, may require sudo apt-get install gyp > > gyp --depth . chrome/browser/resources/compiled_resources.gyp > > > > # directory may differ depending on your GYP config? > > ninja -C out/Default > > > > if you'd like your code to be typechecked continuously (on every commit) add > > your file to third_party/closure_compiler/compiled_resources.gyp. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#search/&q=compiled_resources.gy... > > Cool, but that digs up more errors than I'm prepared to deal with right now :-) that's why I waited for you to try it first ;)
On 2014/12/09 00:37:57, Dan Beam wrote: > On 2014/12/09 00:28:15, Charlie wrote: > > On 2014/12/08 23:52:56, Dan Beam wrote: > > > > > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > > > File chrome/browser/resources/copresence.js (right): > > > > > > > > > https://codereview.chromium.org/734243003/diff/100001/chrome/browser/resource... > > > chrome/browser/resources/copresence.js:7: * @param {Array.<Object>} > directives > > > On 2014/12/08 23:37:07, Charlie wrote: > > > > On 2014/12/08 22:41:59, Dan Beam wrote: > > > > > can you make this type stricter (e.g. {{type: string, medium: string, > > > > duration: > > > > > number}})? maybe even create a @typedef? we're just starting to > compile > > > our > > > > JS > > > > > in chromium and would be useful to have this info. > > > > > > > > Done. Can I run closure for just this file and have it check that I did it > > > > right? Also, is there a way to provide descriptions of the individual > > fields? > > > > > > create a chrome/browser/resources/compiled_resources.gyp like others[1] and > > run: > > > > > > # run from src/, may require sudo apt-get install gyp > > > gyp --depth . chrome/browser/resources/compiled_resources.gyp > > > > > > # directory may differ depending on your GYP config? > > > ninja -C out/Default > > > > > > if you'd like your code to be typechecked continuously (on every commit) add > > > your file to third_party/closure_compiler/compiled_resources.gyp. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=compiled_resources.gy... > > > > Cool, but that digs up more errors than I'm prepared to deal with right now > :-) > > that's why I waited for you to try it first ;) So LG from your end? I think xiyuan wanted your approval.
https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.css:6: font-size: 10pt; nit: please match the rest of webui unless there's a really really darn good reason not to (i.e. delete this rule) https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.html:1: <!DOCTYPE HTML> nit: lowercase https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.html:70: <script src="chrome://resources/js/i18n_template2.js"></script> nit: put just before </body> https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.js:8: * type:string, nit: type: string ^ \s https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.js:28: * @param {Array.<Directive>} directives nit: Array<Directive> https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:456: #else nit: put somewhere in here https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui.cc:21: WebUIDataSource* data_source = nit: you could make ownership more explicit with: scoped_ptr<WebUIDataSource> data_source( WebUIDataSource::Create(chrome::kChromeUICopresenceHost)); ... same code ... return data_source.release(); and then I also don't think you'd need the comment on L19 https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui.cc:58: DCHECK(web_ui); eh, we typically don't do DCHECK(ptr); ptr->DoSomething(); because just ptr->DoSomething(); explodes sufficiently. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:32: // TODO(ckehoe): s/Sent/Transmitted/g\ nit: did you mean to end this with .? https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:42: if (directive_type == copresence::TRANSMIT) { should this be a switch so you get notified when new things are added to copresence::TokenInstructionType? https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:44: } else { no else after return (everywhere in this file) https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:45: DCHECK(directive_type == copresence::RECEIVE); nit: DCHECK_EQ(copresence::RECEIVE, directive_type); https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:54: DCHECK(medium == copresence::AUDIO_AUDIBLE_DTMF); same nits https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:62: return base::StringPrintf("%ld milliseconds", milliseconds); can you use this instead? https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/time_... https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:119: DCHECK(web_ui && web_ui->GetWebContents()); this gives less information than: DCHECK(web_ui); DCHECK(web_ui->GetWebContents()); additionally, if you say "Safely retrieve" you may consider: if (!web_ui || !web_ui->GetWebContents()) { NOTREACHED(); return nullptr; } https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:127: \n\n -> \n https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:143: \n\n -> \n https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:156: DictionaryValue* js_directive = new DictionaryValue; nit: scoped_ptr<> + release() https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:182: DCHECK(args->empty()) << "populateCopresenceState() doesn't take arguments"; remove message https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:8: #include <vector> unused? https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:27: // TODO(ckehoe): Write tests. why not just write the tests?
Synced to head. (Well, yesterday's head.) Sorry for the churn. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.css:6: font-size: 10pt; On 2014/12/09 05:33:38, Dan Beam wrote: > nit: please match the rest of webui unless there's a really really darn good > reason not to (i.e. delete this rule) Done. This was a holdover from not using chrome_shared.css. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.html (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.html:1: <!DOCTYPE HTML> On 2014/12/09 05:33:38, Dan Beam wrote: > nit: lowercase Done. The example in the Chromium style guide is uppercase though. Can you update it? http://www.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.html:70: <script src="chrome://resources/js/i18n_template2.js"></script> On 2014/12/09 05:33:38, Dan Beam wrote: > nit: put just before </body> Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... File chrome/browser/resources/copresence.js (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.js:8: * type:string, On 2014/12/09 05:33:38, Dan Beam wrote: > nit: type: string > ^ \s Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... chrome/browser/resources/copresence.js:28: * @param {Array.<Directive>} directives On 2014/12/09 05:33:38, Dan Beam wrote: > nit: Array<Directive> Oh. I thought jsdoc required the dot? http://usejsdoc.org/tags-type.html https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:456: #else On 2014/12/09 05:33:38, Dan Beam wrote: > nit: put somewhere in here Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui.cc:21: WebUIDataSource* data_source = On 2014/12/09 05:33:38, Dan Beam wrote: > nit: you could make ownership more explicit with: > > scoped_ptr<WebUIDataSource> data_source( > WebUIDataSource::Create(chrome::kChromeUICopresenceHost)); > ... same code ... > return data_source.release(); > > and then I also don't think you'd need the comment on L19 Sure. I like that better too. But the other webui code seems to just use bare pointers. Sometimes it's tricky to know when to be consistent :-) https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui.cc:58: DCHECK(web_ui); On 2014/12/09 05:33:38, Dan Beam wrote: > eh, we typically don't do > > DCHECK(ptr); > ptr->DoSomething(); > > because just > > ptr->DoSomething(); > > explodes sufficiently. Yes, but then you have to rerun through the debugger to get the actual line number. Instead, the copresence code uses these kinds of checks a lot. That said, if this pointer is null, it's probably someone else's fault. Deleted. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:32: // TODO(ckehoe): s/Sent/Transmitted/g\ On 2014/12/09 05:33:38, Dan Beam wrote: > nit: did you mean to end this with .? Just went ahead and did the TODO. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:42: if (directive_type == copresence::TRANSMIT) { On 2014/12/09 05:33:38, Dan Beam wrote: > should this be a switch so you get notified when new things are added to > copresence::TokenInstructionType? That isn't supposed to happen. But sure. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:44: } else { On 2014/12/09 05:33:38, Dan Beam wrote: > no else after return (everywhere in this file) Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:45: DCHECK(directive_type == copresence::RECEIVE); On 2014/12/09 05:33:38, Dan Beam wrote: > nit: DCHECK_EQ(copresence::RECEIVE, directive_type); Deleted. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:54: DCHECK(medium == copresence::AUDIO_AUDIBLE_DTMF); On 2014/12/09 05:33:38, Dan Beam wrote: > same nits Fixed. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:62: return base::StringPrintf("%ld milliseconds", milliseconds); On 2014/12/09 05:33:38, Dan Beam wrote: > can you use this instead? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/time_... Yes, much better. Thanks! https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:119: DCHECK(web_ui && web_ui->GetWebContents()); On 2014/12/09 05:33:38, Dan Beam wrote: > this gives less information than: > > DCHECK(web_ui); > DCHECK(web_ui->GetWebContents()); > > additionally, if you say "Safely retrieve" you may consider: > > if (!web_ui || !web_ui->GetWebContents()) { > NOTREACHED(); > return nullptr; > } Done with a clarifying comment. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:127: On 2014/12/09 05:33:38, Dan Beam wrote: > \n\n -> \n I separate file sections, like the public and private functions, with double newlines. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:143: On 2014/12/09 05:33:38, Dan Beam wrote: > \n\n -> \n Acknowledged. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:156: DictionaryValue* js_directive = new DictionaryValue; On 2014/12/09 05:33:38, Dan Beam wrote: > nit: scoped_ptr<> + release() Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:182: DCHECK(args->empty()) << "populateCopresenceState() doesn't take arguments"; On 2014/12/09 05:33:38, Dan Beam wrote: > remove message Done. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:8: #include <vector> On 2014/12/09 05:33:39, Dan Beam wrote: > unused? Yep. Nixed. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:27: // TODO(ckehoe): Write tests. On 2014/12/09 05:33:39, Dan Beam wrote: > why not just write the tests? Haha. You haven't worked with my manager :-) This is a debug UI for an as yet internal-only Chrome service. Breakages are both more likely to be noticed and much less problematic than for a typical Chrome feature. I do like well-tested code, and I wish I had the time to write tests for this right now. If the browsertest infrastructure was easier to use, or at least more familiar to me, I'd do it in this CL. But I think leaving tests as a TODO is the best decision for our team right now.
On 2014/12/17 23:39:56, Charlie wrote: > Synced to head. (Well, yesterday's head.) Sorry for the churn. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > File chrome/browser/resources/copresence.css (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > chrome/browser/resources/copresence.css:6: font-size: 10pt; > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: please match the rest of webui unless there's a really really darn good > > reason not to (i.e. delete this rule) > > Done. This was a holdover from not using chrome_shared.css. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > File chrome/browser/resources/copresence.html (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > chrome/browser/resources/copresence.html:1: <!DOCTYPE HTML> > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: lowercase > > Done. The example in the Chromium style guide is uppercase though. Can you > update it? > > http://www.chromium.org/developers/web-development-style-guide > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > chrome/browser/resources/copresence.html:70: <script > src="chrome://resources/js/i18n_template2.js"></script> > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: put just before </body> > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > File chrome/browser/resources/copresence.js (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > chrome/browser/resources/copresence.js:8: * type:string, > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: type: string > > ^ \s > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/resource... > chrome/browser/resources/copresence.js:28: * @param {Array.<Directive>} > directives > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: Array<Directive> > > Oh. I thought jsdoc required the dot? > > http://usejsdoc.org/tags-type.html > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:456: #else > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: put somewhere in here > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/copresence_ui.cc (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui.cc:21: WebUIDataSource* data_source = > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: you could make ownership more explicit with: > > > > scoped_ptr<WebUIDataSource> data_source( > > WebUIDataSource::Create(chrome::kChromeUICopresenceHost)); > > ... same code ... > > return data_source.release(); > > > > and then I also don't think you'd need the comment on L19 > > Sure. I like that better too. But the other webui code seems to just use bare > pointers. Sometimes it's tricky to know when to be consistent :-) > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui.cc:58: DCHECK(web_ui); > On 2014/12/09 05:33:38, Dan Beam wrote: > > eh, we typically don't do > > > > DCHECK(ptr); > > ptr->DoSomething(); > > > > because just > > > > ptr->DoSomething(); > > > > explodes sufficiently. > > Yes, but then you have to rerun through the debugger to get the actual line > number. Instead, the copresence code uses these kinds of checks a lot. > > That said, if this pointer is null, it's probably someone else's fault. Deleted. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/copresence_ui_handler.cc (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:32: // TODO(ckehoe): > s/Sent/Transmitted/g\ > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: did you mean to end this with .? > > Just went ahead and did the TODO. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:42: if (directive_type == > copresence::TRANSMIT) { > On 2014/12/09 05:33:38, Dan Beam wrote: > > should this be a switch so you get notified when new things are added to > > copresence::TokenInstructionType? > > That isn't supposed to happen. But sure. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:44: } else { > On 2014/12/09 05:33:38, Dan Beam wrote: > > no else after return (everywhere in this file) > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:45: DCHECK(directive_type == > copresence::RECEIVE); > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: DCHECK_EQ(copresence::RECEIVE, directive_type); > > Deleted. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:54: DCHECK(medium == > copresence::AUDIO_AUDIBLE_DTMF); > On 2014/12/09 05:33:38, Dan Beam wrote: > > same nits > > Fixed. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:62: return > base::StringPrintf("%ld milliseconds", milliseconds); > On 2014/12/09 05:33:38, Dan Beam wrote: > > can you use this instead? > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/time_... > > Yes, much better. Thanks! > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:119: DCHECK(web_ui && > web_ui->GetWebContents()); > On 2014/12/09 05:33:38, Dan Beam wrote: > > this gives less information than: > > > > DCHECK(web_ui); > > DCHECK(web_ui->GetWebContents()); > > > > additionally, if you say "Safely retrieve" you may consider: > > > > if (!web_ui || !web_ui->GetWebContents()) { > > NOTREACHED(); > > return nullptr; > > } > > Done with a clarifying comment. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:127: > On 2014/12/09 05:33:38, Dan Beam wrote: > > \n\n -> \n > > I separate file sections, like the public and private functions, with double > newlines. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:143: > On 2014/12/09 05:33:38, Dan Beam wrote: > > \n\n -> \n > > Acknowledged. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:156: DictionaryValue* > js_directive = new DictionaryValue; > On 2014/12/09 05:33:38, Dan Beam wrote: > > nit: scoped_ptr<> + release() > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.cc:182: DCHECK(args->empty()) << > "populateCopresenceState() doesn't take arguments"; > On 2014/12/09 05:33:38, Dan Beam wrote: > > remove message > > Done. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/copresence_ui_handler.h (right): > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.h:8: #include <vector> > On 2014/12/09 05:33:39, Dan Beam wrote: > > unused? > > Yep. Nixed. > > https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... > chrome/browser/ui/webui/copresence_ui_handler.h:27: // TODO(ckehoe): Write > tests. > On 2014/12/09 05:33:39, Dan Beam wrote: > > why not just write the tests? > > Haha. You haven't worked with my manager :-) > > This is a debug UI for an as yet internal-only Chrome service. Breakages are > both more likely to be noticed and much less problematic than for a typical > Chrome feature. > > I do like well-tested code, and I wish I had the time to write tests for this > right now. If the browsertest infrastructure was easier to use, or at least more > familiar to me, I'd do it in this CL. But I think leaving tests as a TODO is the > best decision for our team right now. Ping? If there's nothing else, I'll go ahead and submit later today. Would prefer not to leave this dangling over the holidays.
LGTM
https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:27: // TODO(ckehoe): Write tests. On 2014/12/17 23:39:56, Charlie wrote: > On 2014/12/09 05:33:39, Dan Beam wrote: > > why not just write the tests? > > Haha. You haven't worked with my manager :-) > > This is a debug UI for an as yet internal-only Chrome service. Breakages are > both more likely to be noticed and much less problematic than for a typical > Chrome feature. > > I do like well-tested code, and I wish I had the time to write tests for this > right now. If the browsertest infrastructure was easier to use, or at least more > familiar to me, I'd do it in this CL. But I think leaving tests as a TODO is the > best decision for our team right now. i'd just do this todo or remove it as it's a waste of bits otherwise (as I'm not confident it'll ever get done). https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:47: } instead of swith (...) { default: return "Unknown"; } can you do switch (...) { } NOTREACHED(); return "Unkown"; https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:58: default: NOTREACHED();
dbeam@chromium.org changed reviewers: + adlr@chromium.org
dbeam@chromium.org changed reviewers: - adlr@chromium.org
+adlr -- why wouldn't this code benefit from having tests like the rest of chrome? https://codereview.chromium.org/734243003/diff/240001/chrome/browser/resource... File chrome/browser/resources/copresence.css (right): https://codereview.chromium.org/734243003/diff/240001/chrome/browser/resource... chrome/browser/resources/copresence.css:38: text-transform: capitalize; this probably doesn't internationalize well
I think you got the wrong manager. Mine is +Andrew Bunner <abunner@google.com>. On Fri Dec 19 2014 at 4:15:41 PM <dbeam@chromium.org> wrote: > +adlr -- why wouldn't this code benefit from having tests like the rest of > chrome? > > > https://codereview.chromium.org/734243003/diff/240001/ > chrome/browser/resources/copresence.css > File chrome/browser/resources/copresence.css (right): > > https://codereview.chromium.org/734243003/diff/240001/ > chrome/browser/resources/copresence.css#newcode38 > chrome/browser/resources/copresence.css:38 > <https://codereview.chromium.org/734243003/diff/240001/chrome/browser/resource...>: > text-transform: capitalize; > this probably doesn't internationalize well > > https://codereview.chromium.org/734243003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
whoops, -adlr, +abunner
Andrew and I are planning to be out the next two weeks for the holidays. Submitting before I leave as most issues seem resolved. https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.h (right): https://codereview.chromium.org/734243003/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.h:27: // TODO(ckehoe): Write tests. On 2014/12/20 00:10:29, Dan Beam wrote: > On 2014/12/17 23:39:56, Charlie wrote: > > On 2014/12/09 05:33:39, Dan Beam wrote: > > > why not just write the tests? > > > > Haha. You haven't worked with my manager :-) > > > > This is a debug UI for an as yet internal-only Chrome service. Breakages are > > both more likely to be noticed and much less problematic than for a typical > > Chrome feature. > > > > I do like well-tested code, and I wish I had the time to write tests for this > > right now. If the browsertest infrastructure was easier to use, or at least > more > > familiar to me, I'd do it in this CL. But I think leaving tests as a TODO is > the > > best decision for our team right now. > > i'd just do this todo or remove it as it's a waste of bits otherwise (as I'm not > confident it'll ever get done). Not only do I plan to do it, but I can tell you that it'll probably be in the M43 timeframe. Filed a bug. M42 will be really busy for us, since we're planning to launch two new APIs. https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:47: } On 2014/12/20 00:10:29, Dan Beam wrote: > instead of > > swith (...) { > default: > return "Unknown"; > } > > can you do > > switch (...) { > } > > NOTREACHED(); > return "Unkown"; Done. https://codereview.chromium.org/734243003/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:58: default: On 2014/12/20 00:10:29, Dan Beam wrote: > NOTREACHED(); Done.
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734243003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/734243003/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:62: } I meant remove the default: and add a NOTREACHED() return "Unknown" after the switch. putting a default means that adding new values will never break this code (which we want to happen).
https://codereview.chromium.org/734243003/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/copresence_ui_handler.cc (right): https://codereview.chromium.org/734243003/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/copresence_ui_handler.cc:62: } On 2014/12/20 03:44:20, Dan Beam wrote: > I meant remove the default: and add a NOTREACHED() return "Unknown" after the > switch. putting a default means that adding new values will never break this > code (which we want to happen). I'm not following. I think the code behaves the same whether NOTREACHED() is in a default case, or after the switch with no default. Either way, unhandled values will hit NOTREACHED(). In this particular case, there are actually other TokenMediums that we don't handle, because we don't support them. However we tell the server that we don't support them, so it shouldn't send them to us.
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734243003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/734243003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/92c8e4e90f364f4db499b0edc8d9d491fdaa064a Cr-Commit-Position: refs/heads/master@{#309353} |
