Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
557733002:
[Android WebView] Use NetLog for providing full request headers to DevTools (Closed)
|
Created:
6 years, 3 months ago by mnaganov (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Android WebView] Use NetLog for providing full request headers to DevTools
AwURLRequestContextGetter now owns an instance of net::NetLog.
BUG=277075
R=boliu@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/a41817cd3386444c5b20ec8beda42e32f803cd55
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved NetLog ownership to AwURLRequestContextGetter #
Total comments: 2
Patch Set 3 : Comments addressed #
Total comments: 6
Patch Set 4 : Comments addressed #Patch Set 5 : Moved NetLog creation to AwURLRequestContextGetter constructor #
Total comments: 2
Patch Set 6 : Added comment #
Messages
Total messages: 27 (7 generated)
mnaganov@chromium.org changed reviewers: + benm@chromium.org, boliu@chromium.org
Please see my comments on b/17352664
https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... android_webview/browser/aw_browser_context.cc:233: return GetDefaultStoragePartition(this)->GetURLRequestContext(); I was looking to see if we could have the BrowserContext (or even the URLRequestContextGetter) own the NetLog instead of passing the pointer through constructors and noticed this ... I wonder if this should this be a return url_request_context_getter_? If so, then I think we would be able to have the URLRequestContextGetter own the netlog, and have the content client fish it out from there via browsercontext. WDYT?
Seems like a good idea. At least, until we only have a single profile in WebView. https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... android_webview/browser/aw_browser_context.cc:233: return GetDefaultStoragePartition(this)->GetURLRequestContext(); On 2014/09/09 11:11:27, benm wrote: > I was looking to see if we could have the BrowserContext (or even the > URLRequestContextGetter) own the NetLog instead of passing the pointer through > constructors and noticed this ... I wonder if this should this be a return > url_request_context_getter_? If so, then I think we would be able to have the > URLRequestContextGetter own the netlog, and have the content client fish it out > from there via browsercontext. > > WDYT? This seems to be a pattern in Chrome of creating an instance of URLRequestContextGetter via BrowserContext::CreateRequestContext, then sharing it with StoragePartitionImpl, and then always retrieving via StoragePartition::GetURLRequestContext. I guess, this is made for supporting several StoragePartitions inside the browser (for incognito, etc), but as WebView only has single profile, this doesn't seem to be relevant for us. Thus, the URLRequestContextGetter instance returned from AwBrowserContext::GetRequestContext is indeed always the same one stored in url_request_context_getter_. But just for the sake of uniformity, we should leave this code as is. Anyway, because net::URLRequestContextGetter interface doesn't provide access to a NetLog instance, we can't use AwBrowserContext::GetRequestContex in AwContentBrowserClient, and instead I have added a specific getter for obtaining AwURLRequestContextGetter.
On 2014/09/09 13:02:28, mnaganov (cr) wrote: > Seems like a good idea. At least, until we only have a single profile in > WebView. > > https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... > File android_webview/browser/aw_browser_context.cc (right): > > https://codereview.chromium.org/557733002/diff/1/android_webview/browser/aw_b... > android_webview/browser/aw_browser_context.cc:233: return > GetDefaultStoragePartition(this)->GetURLRequestContext(); > On 2014/09/09 11:11:27, benm wrote: > > I was looking to see if we could have the BrowserContext (or even the > > URLRequestContextGetter) own the NetLog instead of passing the pointer through > > constructors and noticed this ... I wonder if this should this be a return > > url_request_context_getter_? If so, then I think we would be able to have the > > URLRequestContextGetter own the netlog, and have the content client fish it > out > > from there via browsercontext. > > > > WDYT? > > This seems to be a pattern in Chrome of creating an instance of > URLRequestContextGetter via BrowserContext::CreateRequestContext, then sharing > it with StoragePartitionImpl, and then always retrieving via > StoragePartition::GetURLRequestContext. I guess, this is made for supporting > several StoragePartitions inside the browser (for incognito, etc), but as > WebView only has single profile, this doesn't seem to be relevant for us. > > Thus, the URLRequestContextGetter instance returned from > AwBrowserContext::GetRequestContext is indeed always the same one stored in > url_request_context_getter_. But just for the sake of uniformity, we should > leave this code as is. > > Anyway, because net::URLRequestContextGetter interface doesn't provide access to > a NetLog instance, we can't use AwBrowserContext::GetRequestContex in > AwContentBrowserClient, and instead I have added a specific getter for obtaining > AwURLRequestContextGetter. thanks for the explanation, sounds good.
https://codereview.chromium.org/557733002/diff/20001/android_webview/browser/... File android_webview/browser/aw_content_browser_client.h (right): https://codereview.chromium.org/557733002/diff/20001/android_webview/browser/... android_webview/browser/aw_content_browser_client.h:16: } needed?
https://codereview.chromium.org/557733002/diff/20001/android_webview/browser/... File android_webview/browser/aw_content_browser_client.h (right): https://codereview.chromium.org/557733002/diff/20001/android_webview/browser/... android_webview/browser/aw_content_browser_client.h:16: } On 2014/09/09 14:18:07, benm wrote: > needed? Removed, thanks!
https://codereview.chromium.org/557733002/diff/40001/android_webview/android_... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/android_... android_webview/android_webview.gyp:137: '../net/net.gyp:net', Just wondering...how did all the other stuff from //net link without this? https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:283: return net_log_.get(); DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); if (!net_log_) InitializeURLRequestContext() This is called on the IO thread, right? https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:43: void InitializeOnNetworkThread(); Can you delete this line?
https://codereview.chromium.org/557733002/diff/40001/android_webview/android_... File android_webview/android_webview.gyp (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/android_... android_webview/android_webview.gyp:137: '../net/net.gyp:net', On 2014/09/09 15:41:39, boliu wrote: > Just wondering...how did all the other stuff from //net link without this? Target 'webview_native' depends on 'net/net.gyp:net', but without adding this dependency here I was missing some generated file and compilation was failing. Although, now I have removed back this line, nuked 'out', re-generated ninja files, and everything still builds. So I will remove this line unless I encounter any build issues on bots. https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:283: return net_log_.get(); On 2014/09/09 15:41:40, boliu wrote: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); > if (!net_log_) > InitializeURLRequestContext() > > This is called on the IO thread, right? Correct, this runs on the IO thread. Although, at least DevTools protects itself from dealing with a NULL NetLog pointer, initializing still looks like a good idea, thanks! https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/557733002/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:43: void InitializeOnNetworkThread(); On 2014/09/09 15:41:40, boliu wrote: > Can you delete this line? Done.
lgtm
The CQ bit was checked by mnaganov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/557733002/50007
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Failing android tests due to threading issue. Good thing I asked for the DCHECK
:)
[FATAL:aw_url_request_context_getter.cc(283)] Check failed:
BrowserThread::CurrentlyOn(BrowserThread::IO).
Stack Trace:
RELADDR FUNCTION
FILE:LINE
0037a023 logging::LogMessage::~LogMessage()+42
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../base/logging.cc:548
002816bd android_webview::AwURLRequestContextGetter::GetNetLog()+56
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/net/aw_url_request_context_getter.cc:283
00966b63
content::BrowserContext::GetDownloadManager(content::BrowserContext*)+118
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/browser_context.cc:115
009a16dd content::(anonymous
namespace)::StartOnUIThread(scoped_ptr<content::DownloadCreateInfo,
base::DefaultDeleter<content::DownloadCreateInfo> >,
content::DownloadResourceHandler::DownloadTabInfo*,
scoped_ptr<content::ByteStreamReader,
base::DefaultDeleter<content::ByteStreamReader> >, base::Callback<void
(content::DownloadItem*, content::DownloadInterruptReason)> const&)+76
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/download/download_resource_handler.cc:66
<snip>
NetLog is thread-safe, so it seems natural for people to request it from random threads. I have moved its creation to the constructor of AwURLRequestContextGetter, so we don't need to call InitializeURLRequestContext from the getter. On 2014/09/10 01:40:57, boliu wrote: > Failing android tests due to threading issue. Good thing I asked for the DCHECK > :) > > [FATAL:aw_url_request_context_getter.cc(283)] Check failed: > BrowserThread::CurrentlyOn(BrowserThread::IO). > > Stack Trace: > RELADDR FUNCTION > > > > > > > > > > > > > > > > > > > FILE:LINE > 0037a023 logging::LogMessage::~LogMessage()+42 > > > > > > > > > > > > > > > > > > > > /b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../base/logging.cc:548 > 002816bd android_webview::AwURLRequestContextGetter::GetNetLog()+56 > > > > > > > > > > > > > > > > > > > > /b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/net/aw_url_request_context_getter.cc:283 > 00966b63 > content::BrowserContext::GetDownloadManager(content::BrowserContext*)+118 > > > > > > > > > > > > > > > > > > > /b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/browser_context.cc:115 > 009a16dd content::(anonymous > namespace)::StartOnUIThread(scoped_ptr<content::DownloadCreateInfo, > base::DefaultDeleter<content::DownloadCreateInfo> >, > content::DownloadResourceHandler::DownloadTabInfo*, > scoped_ptr<content::ByteStreamReader, > base::DefaultDeleter<content::ByteStreamReader> >, base::Callback<void > (content::DownloadItem*, content::DownloadInterruptReason)> const&)+76 > > > > > > > > > > > > > > > > /b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/download/download_resource_handler.cc:66 > > <snip>
The CQ bit was checked by mnaganov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/557733002/70001
https://codereview.chromium.org/557733002/diff/70001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/557733002/diff/70001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:50: net::NetLog* GetNetLog(); Add a comment this can be called on the UI as well as IO thread.
The CQ bit was unchecked by mnaganov@chromium.org
https://codereview.chromium.org/557733002/diff/70001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/557733002/diff/70001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:50: net::NetLog* GetNetLog(); On 2014/09/10 14:57:24, boliu wrote: > Add a comment this can be called on the UI as well as IO thread. Done.
The CQ bit was checked by mnaganov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/557733002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (None)
Patchset 6 (id:??) landed as https://crrev.com/a41817cd3386444c5b20ec8beda42e32f803cd55 Cr-Commit-Position: refs/heads/master@{#294359}
Message was sent while issue was closed.
Committed patchset #6 (id:90001) manually as a41817c (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
