Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
160973002:
Two chagnes to cast socket: (Closed)
|
Created:
6 years, 10 months ago by Munjal (Google) Modified:
6 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFew changes to cast socket:
1. Log IP address of the receiver in every message to help with debugging
2. Remove dependency on cast_channel_api.pb.h from cast_socket.h to avoid
flaky build issue
3. Log lower priority messages at level 2
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251219
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 16 (0 generated)
nit: Please fix typo in CL description, and end sentences with "." - thanks!
CL description should also mention that this moves more detailed logging to VLOG(2). https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (left): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:700: // networks Did this need re-wrapping? https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:74: url_log_str_("[URL: " + url.spec() + "] "), nit: Is "URL:" redundant here? https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:627: current_message_->Clear(); Rather than clearing & re-using the CastMessage, why not .reset() it here, and create a new one on each read? https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.h (left): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:19: #include "chrome/browser/extensions/api/cast_channel/cast_channel.pb.h" How did this make the build flaky? Something including this header that did not itself depend on the API protos? https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:265: // String used in log messages for url. Suggest renaming this to |log_prefix_| - you could then do without a description, but if you prefer to keep one I'd suggest "String containing the peer's URL, to prefix to all log messages".
https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:74: url_log_str_("[URL: " + url.spec() + "] "), This is really only for the purposes of logging and doesn't have to do with the core functionality of the class. WDYT of defining a macro like VLOG_WITH_URL instead? https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.h (left): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:19: #include "chrome/browser/extensions/api/cast_channel/cast_channel.pb.h" On 2014/02/12 21:13:16, Wez wrote: > How did this make the build flaky? Something including this header that did not > itself depend on the API protos? The files generated from the IDL included the .pb.h, but didn't depend on the pb target in GYP. If the API target is built before the pb target the build breaks.
https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:74: url_log_str_("[URL: " + url.spec() + "] "), On 2014/02/12 21:45:02, mark a. foltz wrote: > This is really only for the purposes of logging and doesn't have to do with the > core functionality of the class. WDYT of defining a macro like VLOG_WITH_URL > instead? +1
On 2014/02/12 21:05:34, Wez wrote: > nit: Please fix typo in CL description, and end sentences with "." - thanks! Done
https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (left): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:700: // networks On 2014/02/12 21:13:16, Wez wrote: > Did this need re-wrapping? Yes, I think it was 81 without wrapping. https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:74: url_log_str_("[URL: " + url.spec() + "] "), On 2014/02/12 21:52:14, Wez wrote: > On 2014/02/12 21:45:02, mark a. foltz wrote: > > This is really only for the purposes of logging and doesn't have to do with > the > > core functionality of the class. WDYT of defining a macro like VLOG_WITH_URL > > instead? > > +1 Done. Started doing that but thought I cannot do << syntax in that case. But that is not the case - macro's text replacement semantics come to the rescue :). https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:74: url_log_str_("[URL: " + url.spec() + "] "), On 2014/02/12 21:13:16, Wez wrote: > nit: Is "URL:" redundant here? Done. https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.cc:627: current_message_->Clear(); On 2014/02/12 21:13:16, Wez wrote: > Rather than clearing & re-using the CastMessage, why not .reset() it here, and > create a new one on each read? So that we don't create many objects on the heap even though we only need one at a time. Clear gets the object in the exact same state as creating a new one. https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.h (left): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:19: #include "chrome/browser/extensions/api/cast_channel/cast_channel.pb.h" On 2014/02/12 21:45:02, mark a. foltz wrote: > On 2014/02/12 21:13:16, Wez wrote: > > How did this make the build flaky? Something including this header that did > not > > itself depend on the API protos? > > The files generated from the IDL included the .pb.h, but didn't depend on the pb > target in GYP. If the API target is built before the pb target the build > breaks. It is complicated and hence took me a while to figure out. So what happens is that generated_api.cc includes all files under extensions/api that contain EXTENSION_FUNCTION macro defintion. The file generated_api.cc itself is generated by a python script. So generated_api.cc includes cast_channel_api.h, which includes cast_socket.h, which includes cast_channel.pb.h. But cast_channel.pb.h is sometimes not yet generated from proto buffer before generated_api.cc is compiled. There are otehr approaches to fix this but this appears to be the cleanest. https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:265: // String used in log messages for url. On 2014/02/12 21:13:16, Wez wrote: > Suggest renaming this to |log_prefix_| - you could then do without a > description, but if you prefer to keep one I'd suggest "String containing the > peer's URL, to prefix to all log messages". Done. https://codereview.chromium.org/160973002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_socket.h:265: // String used in log messages for url. On 2014/02/12 21:13:16, Wez wrote: > Suggest renaming this to |log_prefix_| - you could then do without a > description, but if you prefer to keep one I'd suggest "String containing the > peer's URL, to prefix to all log messages". Done.
lgtm https://codereview.chromium.org/160973002/diff/20003/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/20003/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:32: // Assumes member name url_. Anyway compile error will occur if that changes. Please #undef the macro after the last usage (even if it is scoped to the .cc) https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
https://codereview.chromium.org/160973002/diff/20003/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/20003/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:32: // Assumes member name url_. Anyway compile error will occur if that changes. On 2014/02/12 23:05:51, mark a. foltz wrote: > Please #undef the macro after the last usage (even if it is scoped to the .cc) > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done.
I'm not able to get patch-sets 6 or 7 to come up in codereview - can you try re-uploading? On 12 February 2014 15:25, <munjal@chromium.org> wrote: > > https://codereview.chromium.org/160973002/diff/20003/ > chrome/browser/extensions/api/cast_channel/cast_socket.cc > File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/160973002/diff/20003/ > chrome/browser/extensions/api/cast_channel/cast_socket.cc#newcode32 > chrome/browser/extensions/api/cast_channel/cast_socket.cc:32: // Assumes > member name url_. Anyway compile error will occur if that changes. > On 2014/02/12 23:05:51, mark a. foltz wrote: > >> Please #undef the macro after the last usage (even if it is scoped to >> > the .cc) > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/ > styleguide.shtml?cl=head#Preprocessor_Macros > > Done. > > https://codereview.chromium.org/160973002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah, at least 4 to 5 patch sets are because I had to reupload. Anyway, re-uploaded and tested that patch set 8 comes up fine.
lgtm https://codereview.chromium.org/160973002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:32: // Assumes member name url_. Anyway compile error will occur if that changes. nit: "Anyway ... changes." is not a sentence. Suggest replacing this comment with "Assumes a |url_| string is available in the current scope."
https://codereview.chromium.org/160973002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/160973002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/api/cast_channel/cast_socket.cc:32: // Assumes member name url_. Anyway compile error will occur if that changes. On 2014/02/13 21:46:08, Wez wrote: > nit: "Anyway ... changes." is not a sentence. > > Suggest replacing this comment with "Assumes a |url_| string is available in the > current scope." Done.
The CQ bit was checked by munjal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/160973002/370001
Message was sent while issue was closed.
Change committed as 251219 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
