Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
610673002:
Expose startNetLog / stopNetLog from HttpUrlRequestFactory. (Closed)
DescriptionExpose startNetLog / stopNetLog from HttpUrlRequestFactory.
BUG=417835
Committed: https://crrev.com/48714b9bfcd0093480f20aac69243b22c97aaa75
Cr-Commit-Position: refs/heads/master@{#297334}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments. #
Total comments: 7
Patch Set 3 : Address review comments. #
Messages
Total messages: 21 (2 generated)
mef@chromium.org changed reviewers: + clm@google.com, ebravo@google.com, mmenke@chromium.org, xunjieli@chromium.org
Per conversation in Cronet meeting I've added startNetLog / stopNetLog methods to HttpUrlRequestFactory, so they could be accessed by embedder without need to get to ChromiumUrlRequestContext.
https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:68: public void startNetLogToFile(String fileName) { Empty functions here look a little weird. Can we make them abstract or throw unimplemented exception?
https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:59: public void startNetLogToFile(String fileName) { Do we need to worry about thread safety?
Thanks, PTAL. https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:59: public void startNetLogToFile(String fileName) { On 2014/09/26 18:23:13, Charles wrote: > Do we need to worry about thread safety? You should not have to as long as context is not destroyed. https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:68: public void startNetLogToFile(String fileName) { On 2014/09/26 18:22:45, xunjieli wrote: > Empty functions here look a little weird. Can we make them abstract or throw > unimplemented exception? Done.
On 2014/09/26 18:33:49, mef wrote: > Thanks, PTAL. > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > File > components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java > (right): > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:59: > public void startNetLogToFile(String fileName) { > On 2014/09/26 18:23:13, Charles wrote: > > Do we need to worry about thread safety? > > You should not have to as long as context is not destroyed. > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > File > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java > (right): > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:68: > public void startNetLogToFile(String fileName) { > On 2014/09/26 18:22:45, xunjieli wrote: > > Empty functions here look a little weird. Can we make them abstract or throw > > unimplemented exception? > > Done. ping.
On 2014/09/29 15:57:53, mef wrote: > On 2014/09/26 18:33:49, mef wrote: > > Thanks, PTAL. > > > > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > > File > > > components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java > > (right): > > > > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > > > components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:59: > > public void startNetLogToFile(String fileName) { > > On 2014/09/26 18:23:13, Charles wrote: > > > Do we need to worry about thread safety? > > > > You should not have to as long as context is not destroyed. > > > > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > > File > > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java > > (right): > > > > > https://codereview.chromium.org/610673002/diff/1/components/cronet/android/ja... > > > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:68: > > public void startNetLogToFile(String fileName) { > > On 2014/09/26 18:22:45, xunjieli wrote: > > > Empty functions here look a little weird. Can we make them abstract or throw > > > unimplemented exception? > > > > Done. > > ping. I have a bit of a backlog of codereviews to drill down on this week. Hope to get caught up in a day or two.
https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:50: public void startNetLogToFile(String fileName) { Should we whine, or return false, or throw an exception or something when this is called? https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. If we're going to document the log level, we should explicitly set it as well (I'm changing the default level of the NetLogLogger in another CL to hide cookies and the like, and am thinking I won't update users of the NetLogLogger, on the assumption if they don't set it, they're fine with the default value).
https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:50: public void startNetLogToFile(String fileName) { On 2014/09/29 18:53:19, mmenke wrote: > Should we whine, or return false, or throw an exception or something when this > is called? No, because doing so would break the abstraction between the java stack and the native stack. A no-op is fine.
On 2014/09/29 19:09:41, Charles wrote: > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java > (right): > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:50: > public void startNetLogToFile(String fileName) { > On 2014/09/29 18:53:19, mmenke wrote: > > Should we whine, or return false, or throw an exception or something when this > > is called? > > No, because doing so would break the abstraction between the java stack and the > native stack. A no-op is fine. Except when a developer tries to get the NetLog and it turns out there isn't one, of course.
On 2014/09/29 19:18:44, mmenke wrote: > On 2014/09/29 19:09:41, Charles wrote: > > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > > File > > > components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java > > (right): > > > > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > > > components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:50: > > public void startNetLogToFile(String fileName) { > > On 2014/09/29 18:53:19, mmenke wrote: > > > Should we whine, or return false, or throw an exception or something when > this > > > is called? > > > > No, because doing so would break the abstraction between the java stack and > the > > native stack. A no-op is fine. > > Except when a developer tries to get the NetLog and it turns out there isn't > one, of course. So let's just create an empty file.
https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. On 2014/09/29 18:53:19, mmenke wrote: > If we're going to document the log level, we should explicitly set it as well > (I'm changing the default level of the NetLogLogger in another CL to hide > cookies and the like, and am thinking I won't update users of the NetLogLogger, > on the assumption if they don't set it, they're fine with the default value). I'm fine with not documenting the log level, although we should mention that it may contain some PII.
https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. On 2014/09/29 21:21:24, mef wrote: > On 2014/09/29 18:53:19, mmenke wrote: > > If we're going to document the log level, we should explicitly set it as well > > (I'm changing the default level of the NetLogLogger in another CL to hide > > cookies and the like, and am thinking I won't update users of the > NetLogLogger, > > on the assumption if they don't set it, they're fine with the default value). > > I'm fine with not documenting the log level, although we should mention that it > may contain some PII. Great, let's do that for now, then. I wonder if it's better to include the cookies by default or not - admittedly, we don't even have an on-disk cookie store hooked up yet, but worth thinking about the typical use case, or if we should just expose the option to the embedder.
On 2014/09/29 21:24:30, mmenke wrote: > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > File > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java > (right): > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: > * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. > On 2014/09/29 21:21:24, mef wrote: > > On 2014/09/29 18:53:19, mmenke wrote: > > > If we're going to document the log level, we should explicitly set it as > well > > > (I'm changing the default level of the NetLogLogger in another CL to hide > > > cookies and the like, and am thinking I won't update users of the > > NetLogLogger, > > > on the assumption if they don't set it, they're fine with the default > value). > > > > I'm fine with not documenting the log level, although we should mention that > it > > may contain some PII. > > Great, let's do that for now, then. > > I wonder if it's better to include the cookies by default or not - admittedly, > we don't even have an on-disk cookie store hooked up yet, but worth thinking > about the typical use case, or if we should just expose the option to the > embedder. When there's an embedder who wants it, we can add it.
On 2014/09/29 21:25:30, Charles wrote: > On 2014/09/29 21:24:30, mmenke wrote: > > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > > File > > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java > > (right): > > > > > https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... > > > components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: > > * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. > > On 2014/09/29 21:21:24, mef wrote: > > > On 2014/09/29 18:53:19, mmenke wrote: > > > > If we're going to document the log level, we should explicitly set it as > > well > > > > (I'm changing the default level of the NetLogLogger in another CL to hide > > > > cookies and the like, and am thinking I won't update users of the > > > NetLogLogger, > > > > on the assumption if they don't set it, they're fine with the default > > value). > > > > > > I'm fine with not documenting the log level, although we should mention that > > it > > > may contain some PII. > > > > Great, let's do that for now, then. > > > > I wonder if it's better to include the cookies by default or not - admittedly, > > we don't even have an on-disk cookie store hooked up yet, but worth thinking > > about the typical use case, or if we should just expose the option to the > > embedder. > > When there's an embedder who wants it, we can add it. Works for me.
Thanks, PTAL! https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlConnectionUrlRequestFactory.java:50: public void startNetLogToFile(String fileName) { On 2014/09/29 19:09:41, Charles wrote: > On 2014/09/29 18:53:19, mmenke wrote: > > Should we whine, or return false, or throw an exception or something when this > > is called? > > No, because doing so would break the abstraction between the java stack and the > native stack. A no-op is fine. Done. Write 'not supported' message into net log file. https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java (right): https://codereview.chromium.org/610673002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:65: * is LOG_ALL_BUT_BYTES. If the file exists it is truncated before starting. On 2014/09/29 21:24:30, mmenke wrote: > On 2014/09/29 21:21:24, mef wrote: > > On 2014/09/29 18:53:19, mmenke wrote: > > > If we're going to document the log level, we should explicitly set it as > well > > > (I'm changing the default level of the NetLogLogger in another CL to hide > > > cookies and the like, and am thinking I won't update users of the > > NetLogLogger, > > > on the assumption if they don't set it, they're fine with the default > value). > > > > I'm fine with not documenting the log level, although we should mention that > it > > may contain some PII. > > Great, let's do that for now, then. > > I wonder if it's better to include the cookies by default or not - admittedly, > we don't even have an on-disk cookie store hooked up yet, but worth thinking > about the typical use case, or if we should just expose the option to the > embedder. Done.
LGTM
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/610673002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as c095948721a7b014c60f9a3ddbb64b65454cb97b
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/48714b9bfcd0093480f20aac69243b22c97aaa75 Cr-Commit-Position: refs/heads/master@{#297334} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
