Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
845973005:
[New Tab Page] Change the mechanism to intercept online NTP errors (Closed)
|
Created:
5 years, 11 months ago by Mathieu Modified:
5 years, 10 months ago CC:
chromium-reviews, David Black, dhollowa+watch_chromium.org, donnd+watch_chromium.org, dougw+watch_chromium.org, Jered, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[New Tab Page] Change the mechanism to intercept online NTP errors
No longer rely on DidFailProvisionalLoad, solving other issues.
Includes a rename of URLRequestMockHTTPJob::AddUrlHandler
BUG=373455
TEST=NewTabPageInterceptorTest
Committed: https://crrev.com/6f2b16735659425f6a0167754051df62b0b4431b
Cr-Commit-Position: refs/heads/master@{#314004}
Patch Set 1 : Initial #Patch Set 2 : working #Patch Set 3 : remove file #
Total comments: 37
Patch Set 4 : template url service observer #
Total comments: 7
Patch Set 5 : Addressed comments #Patch Set 6 : test comment fix #
Total comments: 22
Patch Set 7 : moved impl to cc #
Total comments: 4
Patch Set 8 : nits #Patch Set 9 : moved code #Patch Set 10 : test fix #
Total comments: 2
Patch Set 11 : moved to profile_io_data #Messages
Total messages: 50 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
mathp@chromium.org changed reviewers: + kmadhusu@chromium.org, mmenke@chromium.org
Hello Matt and Kausalya, Please have an initial look!
Some nits. Want to do a full review tomorrow. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/browsing_... File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/browsing_... chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:33: BrowserThread::GetBlockingPool()); Optional: While you're here, could break the dependency on how mock handlers are installed by using SetUrlRequestMocksEnabled(true) here instead. This file will still need to depend on URLRequestMockHTTPJob, though. Still think it's an improvement, though. Could do it for PolicyBrowserTest as well, but not for the content/ tests. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } Can't the service just sign up as an InstantServiceObserver instead? Makes the new service more modular. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:59: (request->url() == GURL(chrome::kChromeSearchLocalNtpUrl))) { optional: The second comparison left me scratching my head. Suggest second comparison may be a little clearer as: new_tab_url_ == GURL(chrome::kChromeSearchLocalNtpUrl) With maybe a comment. Alternatively, think making these the last checks instead of the first, putting it right next to where we make the URLRequestRedirectJob, would work as well. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:63: if (request->status().status() == net::URLRequestStatus::CANCELED) { Should also check for status() == net::URLRequestStatus::FAILED && error() == ERR_ABORTED here. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:69: request->GetResponseCode() >= 400) { optional: Early return is generally preferred. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:71: base::Bind(&RecordNTPLoadFailure)); Histograms are threadsafe, so no reason to post a task here. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor.h (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:17: // Destroys the interceptor. nit: Don't think these two comments add anything. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:20: base::WeakPtr<NewTabPageInterceptor> GetWeakPtr(); Making globally accessible weak pointers is strongly recommended against. Admittedly, the scope of the URLRequestInterceptor is pretty limited, but I'd still like to hide this method. Maybe move this class into the service file's CC file, making this a private class? Could make it private and make the service a friend, but not a huge fan of that, if we can avoid it. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:36: DISALLOW_COPY_AND_ASSIGN(NewTabPageInterceptor); Should include base/macros.h for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:45: void ChangeDefaultSearchProvider(const std::string& newtab_path) { Optional: Don't think we get anything out of making all these protected instead of public. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:65: base::FilePath path_; Should make these private, per Google style guidelines, and make setters/getters as necessary https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:24: content::BrowserThread::IO, FROM_HERE, Should include base/location.h for FROM_HERE. Believe browser_thread currently includes it, but shouldn't depend on that. https://codereview.chromium.org/845973005/diff/90001/net/test/url_request/url... File net/test/url_request/url_request_mock_http_job.cc (right): https://codereview.chromium.org/845973005/diff/90001/net/test/url_request/url... net/test/url_request/url_request_mock_http_job.cc:180: GURL URLRequestMockHTTPJob::GetMockUrlForScheme(const base::FilePath& path, nit: Instead of making this a static member of URLRequestMockHTTPJob, can just move this into the anonymous namespace up top. At least in net/, this is the preferred pattern, to keep stuff out of the headers.
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/15 22:35:30, mmenke wrote: > Can't the service just sign up as an InstantServiceObserver instead? Makes the > new service more modular. I tried for a long time to make that work. It turns out that if the Interceptor depends on the InstantService, the latter seems to be constructed too early in the Chrome lifecycle. The whole thing crashes and burns with a thread_checker later on, while it shouldn't because we're on UI thread here, same as when InstantService usually gets created. Anyhow, I've spoken to various Chrome engineers and this way is a solution to that. I know it's not 100% satisfactory but it's not totally out of place here either.
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/15 22:42:09, Mathieu Perreault wrote: > On 2015/01/15 22:35:30, mmenke wrote: > > Can't the service just sign up as an InstantServiceObserver instead? Makes > the > > new service more modular. > > I tried for a long time to make that work. It turns out that if the Interceptor > depends on the InstantService, the latter seems to be constructed too early in > the Chrome lifecycle. The whole thing crashes and burns with a thread_checker > later on, while it shouldn't because we're on UI thread here, same as when > InstantService usually gets created. Anyhow, I've spoken to various Chrome > engineers and this way is a solution to that. I know it's not 100% satisfactory > but it's not totally out of place here either. Did you have all those "DCHECK_CURRENTLY_ON(content::BrowserThread::UI);" in the NewTabPageInterceptorServiceFactory when you ran into the issue? Any explanation I can think of for that problem should trigger those. Should at least move the GetNewTabPageURL into the new service - no reason for the InstantService to what the service actually needs to know.
On 2015/01/15 23:22:21, mmenke wrote: > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > File chrome/browser/search/instant_service.cc (right): > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > chrome/browser/search/instant_service.cc:432: } > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > On 2015/01/15 22:35:30, mmenke wrote: > > > Can't the service just sign up as an InstantServiceObserver instead? Makes > > the > > > new service more modular. > > > > I tried for a long time to make that work. It turns out that if the > Interceptor > > depends on the InstantService, the latter seems to be constructed too early in > > the Chrome lifecycle. The whole thing crashes and burns with a thread_checker > > later on, while it shouldn't because we're on UI thread here, same as when > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > engineers and this way is a solution to that. I know it's not 100% > satisfactory > > but it's not totally out of place here either. > > Did you have all those "DCHECK_CURRENTLY_ON(content::BrowserThread::UI);" in the > NewTabPageInterceptorServiceFactory when you ran into the issue? Any > explanation I can think of for that problem should trigger those. > > Should at least move the GetNewTabPageURL into the new service - no reason for > the InstantService to what the service actually needs to know. It was this exact code and I logged the thread Ids in both the crashing and no crashing scenarios, they were the same.
Thanks for picking up this CL. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/15 22:42:09, Mathieu Perreault wrote: > On 2015/01/15 22:35:30, mmenke wrote: > > Can't the service just sign up as an InstantServiceObserver instead? Makes > the > > new service more modular. > > I tried for a long time to make that work. It turns out that if the Interceptor > depends on the InstantService, the latter seems to be constructed too early in > the Chrome lifecycle. The whole thing crashes and burns with a thread_checker > later on, while it shouldn't because we're on UI thread here, same as when > InstantService usually gets created. Anyhow, I've spoken to various Chrome > engineers and this way is a solution to that. I know it's not 100% satisfactory > but it's not totally out of place here either. (Just curious) Did you get a chance to find the culprit CL that is causing this to crash now? When I wrote the initial code it was working fine.
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/16 00:13:12, kmadhusu wrote: > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > On 2015/01/15 22:35:30, mmenke wrote: > > > Can't the service just sign up as an InstantServiceObserver instead? Makes > > the > > > new service more modular. > > > > I tried for a long time to make that work. It turns out that if the > Interceptor > > depends on the InstantService, the latter seems to be constructed too early in > > the Chrome lifecycle. The whole thing crashes and burns with a thread_checker > > later on, while it shouldn't because we're on UI thread here, same as when > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > engineers and this way is a solution to that. I know it's not 100% > satisfactory > > but it's not totally out of place here either. > > (Just curious) Did you get a chance to find the culprit CL that is causing this > to crash now? When I wrote the initial code it was working fine. Currently, the InstantService is created once profile creation is complete, when we create the BrowserInstantController. If the new service calls into it on construction, we'd now be creating the InstantService before profile creation completes, before we even have a URLRequestContext. I assume that somewhere down the line, that results in calling SetUserData on the ResourceContext on the IO thread, before we've passed the ResourceContext from the IO thread over to the UI thread...Which causes a DCHECK when we continue setting up the ResourceContext on the UI thread. So...How could we fix this? On option would be to create the interceptor on the IO thread, and then have it set up the service, instead of the other way around, though that's still a little ugly. Open to other ideas. Currently, having the new service "depend" on a service that has to be created *AFTER* the new service seems really weird.
On 2015/01/16 17:54:04, mmenke wrote: > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > File chrome/browser/search/instant_service.cc (right): > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > chrome/browser/search/instant_service.cc:432: } > On 2015/01/16 00:13:12, kmadhusu wrote: > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > Can't the service just sign up as an InstantServiceObserver instead? > Makes > > > the > > > > new service more modular. > > > > > > I tried for a long time to make that work. It turns out that if the > > Interceptor > > > depends on the InstantService, the latter seems to be constructed too early > in > > > the Chrome lifecycle. The whole thing crashes and burns with a > thread_checker > > > later on, while it shouldn't because we're on UI thread here, same as when > > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > > engineers and this way is a solution to that. I know it's not 100% > > satisfactory > > > but it's not totally out of place here either. > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > this > > to crash now? When I wrote the initial code it was working fine. > > Currently, the InstantService is created once profile creation is complete, when > we create the BrowserInstantController. > > If the new service calls into it on construction, we'd now be creating the > InstantService before profile creation completes, before we even have a > URLRequestContext. > > I assume that somewhere down the line, that results in calling SetUserData on > the ResourceContext on the IO thread, before we've passed the ResourceContext > from the IO thread over to the UI thread...Which causes a DCHECK when we > continue setting up the ResourceContext on the UI thread. > > So...How could we fix this? On option would be to create the interceptor on the > IO thread, and then have it set up the service, instead of the other way around, > though that's still a little ugly. Open to other ideas. > > Currently, having the new service "depend" on a service that has to be created > *AFTER* the new service seems really weird. kmadhusu@: see below. Thanks for the analysis, it makes sense. Agreed it's not ideal. My rationalization for why this was OK was that the InstantService dependency is not really needed for initialization, but rather just when the default search engine provider changes. This means the NewTabPageInterceptor is functional without the InstantService and we're never in a "bad" state. I would like kmadhusu@ to comment on initializing the NewTabPageInterceptor on the IO thread. IIRC she mentioned that it was not trivial.
On 2015/01/16 18:04:27, Mathieu Perreault wrote: > On 2015/01/16 17:54:04, mmenke wrote: > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > File chrome/browser/search/instant_service.cc (right): > > > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > chrome/browser/search/instant_service.cc:432: } > > On 2015/01/16 00:13:12, kmadhusu wrote: > > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > > Can't the service just sign up as an InstantServiceObserver instead? > > Makes > > > > the > > > > > new service more modular. > > > > > > > > I tried for a long time to make that work. It turns out that if the > > > Interceptor > > > > depends on the InstantService, the latter seems to be constructed too > early > > in > > > > the Chrome lifecycle. The whole thing crashes and burns with a > > thread_checker > > > > later on, while it shouldn't because we're on UI thread here, same as when > > > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > > > engineers and this way is a solution to that. I know it's not 100% > > > satisfactory > > > > but it's not totally out of place here either. > > > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > > this > > > to crash now? When I wrote the initial code it was working fine. > > > > Currently, the InstantService is created once profile creation is complete, > when > > we create the BrowserInstantController. > > > > If the new service calls into it on construction, we'd now be creating the > > InstantService before profile creation completes, before we even have a > > URLRequestContext. > > > > I assume that somewhere down the line, that results in calling SetUserData on > > the ResourceContext on the IO thread, before we've passed the ResourceContext > > from the IO thread over to the UI thread...Which causes a DCHECK when we > > continue setting up the ResourceContext on the UI thread. > > > > So...How could we fix this? On option would be to create the interceptor on > the > > IO thread, and then have it set up the service, instead of the other way > around, > > though that's still a little ugly. Open to other ideas. > > > > Currently, having the new service "depend" on a service that has to be created > > *AFTER* the new service seems really weird. > > kmadhusu@: see below. > > Thanks for the analysis, it makes sense. > > Agreed it's not ideal. My rationalization for why this was OK was that the > InstantService dependency is not really needed for initialization, but rather > just when the default search engine provider changes. This means the > NewTabPageInterceptor is functional without the InstantService and we're never > in a "bad" state. > > I would like kmadhusu@ to comment on initializing the NewTabPageInterceptor on > the IO thread. IIRC she mentioned that it was not trivial. I'll spend some time thinking about when I can find the time, but with the current architecture, it's actually the InstantServiceFactory that should be depending on the NTP factory, not the other way around.
On 2015/01/16 18:13:44, mmenke wrote: > On 2015/01/16 18:04:27, Mathieu Perreault wrote: > > On 2015/01/16 17:54:04, mmenke wrote: > > > > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > > File chrome/browser/search/instant_service.cc (right): > > > > > > > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > > chrome/browser/search/instant_service.cc:432: } > > > On 2015/01/16 00:13:12, kmadhusu wrote: > > > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > > > Can't the service just sign up as an InstantServiceObserver instead? > > > Makes > > > > > the > > > > > > new service more modular. > > > > > > > > > > I tried for a long time to make that work. It turns out that if the > > > > Interceptor > > > > > depends on the InstantService, the latter seems to be constructed too > > early > > > in > > > > > the Chrome lifecycle. The whole thing crashes and burns with a > > > thread_checker > > > > > later on, while it shouldn't because we're on UI thread here, same as > when > > > > > InstantService usually gets created. Anyhow, I've spoken to various > Chrome > > > > > engineers and this way is a solution to that. I know it's not 100% > > > > satisfactory > > > > > but it's not totally out of place here either. > > > > > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > > > this > > > > to crash now? When I wrote the initial code it was working fine. > > > > > > Currently, the InstantService is created once profile creation is complete, > > when > > > we create the BrowserInstantController. > > > > > > If the new service calls into it on construction, we'd now be creating the > > > InstantService before profile creation completes, before we even have a > > > URLRequestContext. > > > > > > I assume that somewhere down the line, that results in calling SetUserData > on > > > the ResourceContext on the IO thread, before we've passed the > ResourceContext > > > from the IO thread over to the UI thread...Which causes a DCHECK when we > > > continue setting up the ResourceContext on the UI thread. > > > > > > So...How could we fix this? On option would be to create the interceptor on > > the > > > IO thread, and then have it set up the service, instead of the other way > > around, > > > though that's still a little ugly. Open to other ideas. > > > > > > Currently, having the new service "depend" on a service that has to be > created > > > *AFTER* the new service seems really weird. > > > > kmadhusu@: see below. > > > > Thanks for the analysis, it makes sense. > > > > Agreed it's not ideal. My rationalization for why this was OK was that the > > InstantService dependency is not really needed for initialization, but rather > > just when the default search engine provider changes. This means the > > NewTabPageInterceptor is functional without the InstantService and we're never > > in a "bad" state. > > > > I would like kmadhusu@ to comment on initializing the NewTabPageInterceptor on > > the IO thread. IIRC she mentioned that it was not trivial. > > I'll spend some time thinking about when I can find the time, but with the > current architecture, it's actually the InstantServiceFactory that should be > depending on the NTP factory, not the other way around. Thanks, I'll have a look Monday at reversing this dependency.
On 2015/01/16 18:04:27, Mathieu Perreault wrote: > On 2015/01/16 17:54:04, mmenke wrote: > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > File chrome/browser/search/instant_service.cc (right): > > > > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... > > chrome/browser/search/instant_service.cc:432: } > > On 2015/01/16 00:13:12, kmadhusu wrote: > > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > > Can't the service just sign up as an InstantServiceObserver instead? > > Makes > > > > the > > > > > new service more modular. > > > > > > > > I tried for a long time to make that work. It turns out that if the > > > Interceptor > > > > depends on the InstantService, the latter seems to be constructed too > early > > in > > > > the Chrome lifecycle. The whole thing crashes and burns with a > > thread_checker > > > > later on, while it shouldn't because we're on UI thread here, same as when > > > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > > > engineers and this way is a solution to that. I know it's not 100% > > > satisfactory > > > > but it's not totally out of place here either. > > > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > > this > > > to crash now? When I wrote the initial code it was working fine. > > > > Currently, the InstantService is created once profile creation is complete, > when > > we create the BrowserInstantController. > > > > If the new service calls into it on construction, we'd now be creating the > > InstantService before profile creation completes, before we even have a > > URLRequestContext. > > > > I assume that somewhere down the line, that results in calling SetUserData on > > the ResourceContext on the IO thread, before we've passed the ResourceContext > > from the IO thread over to the UI thread...Which causes a DCHECK when we > > continue setting up the ResourceContext on the UI thread. > > > > So...How could we fix this? On option would be to create the interceptor on > the > > IO thread, and then have it set up the service, instead of the other way > around, > > though that's still a little ugly. Open to other ideas. > > > > Currently, having the new service "depend" on a service that has to be created > > *AFTER* the new service seems really weird. > > kmadhusu@: see below. > > Thanks for the analysis, it makes sense. > > Agreed it's not ideal. My rationalization for why this was OK was that the > InstantService dependency is not really needed for initialization, but rather > just when the default search engine provider changes. This means the > NewTabPageInterceptor is functional without the InstantService and we're never > in a "bad" state. > > I would like kmadhusu@ to comment on initializing the NewTabPageInterceptor on > the IO thread. IIRC she mentioned that it was not trivial. I am all in to instantiate NewTabPageInterceptor in the IO thread. As of now, NewTabPageInterceptor constructor takes the NTP url as the argument. We need to call chrome::GetNewTabPageURL(Profile*) for that. This function will in turn access TemplateURLService. If I remember correct, TemplateURLService cannot be called on the IO thread. That's why I constructed the interceptor object on the UI thread.
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:989: request_interceptors.push_back(service->CreateInterceptor().release()); Should we really make one of these for incognito? https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/16 17:54:04, mmenke wrote: > On 2015/01/16 00:13:12, kmadhusu wrote: > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > Can't the service just sign up as an InstantServiceObserver instead? > Makes > > > the > > > > new service more modular. > > > > > > I tried for a long time to make that work. It turns out that if the > > Interceptor > > > depends on the InstantService, the latter seems to be constructed too early > in > > > the Chrome lifecycle. The whole thing crashes and burns with a > thread_checker > > > later on, while it shouldn't because we're on UI thread here, same as when > > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > > engineers and this way is a solution to that. I know it's not 100% > > satisfactory > > > but it's not totally out of place here either. > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > this > > to crash now? When I wrote the initial code it was working fine. > > Currently, the InstantService is created once profile creation is complete, when > we create the BrowserInstantController. > > If the new service calls into it on construction, we'd now be creating the > InstantService before profile creation completes, before we even have a > URLRequestContext. > > I assume that somewhere down the line, that results in calling SetUserData on > the ResourceContext on the IO thread, before we've passed the ResourceContext > from the IO thread over to the UI thread...Which causes a DCHECK when we > continue setting up the ResourceContext on the UI thread. > > So...How could we fix this? On option would be to create the interceptor on the > IO thread, and then have it set up the service, instead of the other way around, > though that's still a little ugly. Open to other ideas. > > Currently, having the new service "depend" on a service that has to be created > *AFTER* the new service seems really weird. Shouldn't we just be watching the TemplateURLService, like the InstantService is?
Have a look. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:28: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); kmadhusu@: Do you think it's worth replicating the logic in InstantService::OnTemplateURLServiceChanged() here? The code is much simpler here, but not sure how often it'll trigger for nothing. What do you think?
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/search/in... chrome/browser/search/instant_service.cc:432: } On 2015/01/16 22:04:16, mmenke wrote: > On 2015/01/16 17:54:04, mmenke wrote: > > On 2015/01/16 00:13:12, kmadhusu wrote: > > > On 2015/01/15 22:42:09, Mathieu Perreault wrote: > > > > On 2015/01/15 22:35:30, mmenke wrote: > > > > > Can't the service just sign up as an InstantServiceObserver instead? > > Makes > > > > the > > > > > new service more modular. > > > > > > > > I tried for a long time to make that work. It turns out that if the > > > Interceptor > > > > depends on the InstantService, the latter seems to be constructed too > early > > in > > > > the Chrome lifecycle. The whole thing crashes and burns with a > > thread_checker > > > > later on, while it shouldn't because we're on UI thread here, same as when > > > > InstantService usually gets created. Anyhow, I've spoken to various Chrome > > > > engineers and this way is a solution to that. I know it's not 100% > > > satisfactory > > > > but it's not totally out of place here either. > > > > > > (Just curious) Did you get a chance to find the culprit CL that is causing > > this > > > to crash now? When I wrote the initial code it was working fine. > > > > Currently, the InstantService is created once profile creation is complete, > when > > we create the BrowserInstantController. > > > > If the new service calls into it on construction, we'd now be creating the > > InstantService before profile creation completes, before we even have a > > URLRequestContext. > > > > I assume that somewhere down the line, that results in calling SetUserData on > > the ResourceContext on the IO thread, before we've passed the ResourceContext > > from the IO thread over to the UI thread...Which causes a DCHECK when we > > continue setting up the ResourceContext on the UI thread. > > > > So...How could we fix this? On option would be to create the interceptor on > the > > IO thread, and then have it set up the service, instead of the other way > around, > > though that's still a little ugly. Open to other ideas. > > > > Currently, having the new service "depend" on a service that has to be created > > *AFTER* the new service seems really weird. > > Shouldn't we just be watching the TemplateURLService, like the InstantService > is? Newest patch does that, have a look.
https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor.h (right): https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor.h:12: class NewTabPageInterceptor : public net::URLRequestInterceptor { Should have class level documentation. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor.h:20: base::WeakPtr<NewTabPageInterceptor> GetWeakPtr(); It's generally consider a bad idea to expose these to the world, as it makes lifetime dependencies weird and can lead to unexpected shutdown bugs. I suggest making this method private and friending the service. Alternatively, could move this class into an anonymous namespace in the service's CC file. Please see my last set of comments. Looks like you ignored most of them.
Sorry, I had not ignored your comments on purpose :) Have another look! https://codereview.chromium.org/845973005/diff/90001/chrome/browser/browsing_... File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/browsing_... chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:33: BrowserThread::GetBlockingPool()); On 2015/01/15 22:35:30, mmenke wrote: > Optional: While you're here, could break the dependency on how mock handlers > are installed by using SetUrlRequestMocksEnabled(true) here instead. This file > will still need to depend on URLRequestMockHTTPJob, though. Still think it's an > improvement, though. > > Could do it for PolicyBrowserTest as well, but not for the content/ tests. Actually both this test and the PolicyBrowserTest need to serve from content::DIR_TEST_DATA. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:989: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/16 22:04:16, mmenke wrote: > Should we really make one of these for incognito? Are you saying we should? Currently we are not creating the interceptor in incognito. It's not the same NTP. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:59: (request->url() == GURL(chrome::kChromeSearchLocalNtpUrl))) { On 2015/01/15 22:35:30, mmenke wrote: > optional: The second comparison left me scratching my head. Suggest second > comparison may be a little clearer as: > > new_tab_url_ == GURL(chrome::kChromeSearchLocalNtpUrl) > > With maybe a comment. > > > Alternatively, think making these the last checks instead of the first, putting > it right next to where we make the URLRequestRedirectJob, would work as well. Done with your first comment. I think the point with putting them at the top is that it would be the first thing we check, since we expect it to trigger for all navigations. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:63: if (request->status().status() == net::URLRequestStatus::CANCELED) { On 2015/01/15 22:35:30, mmenke wrote: > Should also check for status() == net::URLRequestStatus::FAILED && error() == > ERR_ABORTED here. Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:69: request->GetResponseCode() >= 400) { On 2015/01/15 22:35:30, mmenke wrote: > optional: Early return is generally preferred. Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.cc:71: base::Bind(&RecordNTPLoadFailure)); On 2015/01/15 22:35:30, mmenke wrote: > Histograms are threadsafe, so no reason to post a task here. Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor.h (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:17: // Destroys the interceptor. On 2015/01/15 22:35:30, mmenke wrote: > nit: Don't think these two comments add anything. Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:20: base::WeakPtr<NewTabPageInterceptor> GetWeakPtr(); On 2015/01/15 22:35:30, mmenke wrote: > Making globally accessible weak pointers is strongly recommended against. > Admittedly, the scope of the URLRequestInterceptor is pretty limited, but I'd > still like to hide this method. > > Maybe move this class into the service file's CC file, making this a private > class? > > Could make it private and make the service a friend, but not a huge fan of that, > if we can avoid it. Let me know if that is what you had in mind. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor.h:36: DISALLOW_COPY_AND_ASSIGN(NewTabPageInterceptor); On 2015/01/15 22:35:30, mmenke wrote: > Should include base/macros.h for DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/01/15 22:35:30, mmenke wrote: > 2015 Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:45: void ChangeDefaultSearchProvider(const std::string& newtab_path) { On 2015/01/15 22:35:30, mmenke wrote: > Optional: Don't think we get anything out of making all these protected instead > of public. Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:65: base::FilePath path_; On 2015/01/15 22:35:30, mmenke wrote: > Should make these private, per Google style guidelines, and make setters/getters > as necessary Done. https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/ui/search... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:24: content::BrowserThread::IO, FROM_HERE, On 2015/01/15 22:35:30, mmenke wrote: > Should include base/location.h for FROM_HERE. Believe browser_thread currently > includes it, but shouldn't depend on that. Done. https://codereview.chromium.org/845973005/diff/90001/net/test/url_request/url... File net/test/url_request/url_request_mock_http_job.cc (right): https://codereview.chromium.org/845973005/diff/90001/net/test/url_request/url... net/test/url_request/url_request_mock_http_job.cc:180: GURL URLRequestMockHTTPJob::GetMockUrlForScheme(const base::FilePath& path, On 2015/01/15 22:35:30, mmenke wrote: > nit: Instead of making this a static member of URLRequestMockHTTPJob, can just > move this into the anonymous namespace up top. > > At least in net/, this is the preferred pattern, to keep stuff out of the > headers. Done. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor.h (right): https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor.h:12: class NewTabPageInterceptor : public net::URLRequestInterceptor { On 2015/01/22 16:35:26, mmenke wrote: > Should have class level documentation. Done. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor.h:20: base::WeakPtr<NewTabPageInterceptor> GetWeakPtr(); On 2015/01/22 16:35:26, mmenke wrote: > It's generally consider a bad idea to expose these to the world, as it makes > lifetime dependencies weird and can lead to unexpected shutdown bugs. I suggest > making this method private and friending the service. > > Alternatively, could move this class into an anonymous namespace in the > service's CC file. > > Please see my last set of comments. Looks like you ignored most of them. Done.
https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:989: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/22 19:21:58, Mathieu Perreault wrote: > On 2015/01/16 22:04:16, mmenke wrote: > > Should we really make one of these for incognito? > > Are you saying we should? Currently we are not creating the interceptor in > incognito. It's not the same NTP. Incognito mode doesn't use an online ntp. We don't need to intercept those NTP requests. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:28: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2015/01/21 21:29:39, Mathieu Perreault wrote: > kmadhusu@: Do you think it's worth replicating the logic in > InstantService::OnTemplateURLServiceChanged() here? The code is much simpler > here, but not sure how often it'll trigger for nothing. What do you think? I looked at the latest patch. These changes seems reasonable. I will do a quick test just to make sure everything works as expected.
LGTM, suggestions are mostly optional nits. Of the files modified, I'm an owner of net/, chrome/browser/net, and content/browser/loader/ (And profiles/, if you take that suggestion). https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:989: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/22 20:06:30, kmadhusu wrote: > On 2015/01/22 19:21:58, Mathieu Perreault wrote: > > On 2015/01/16 22:04:16, mmenke wrote: > > > Should we really make one of these for incognito? > > > > Are you saying we should? Currently we are not creating the interceptor in > > incognito. It's not the same NTP. > > Incognito mode doesn't use an online ntp. We don't need to intercept those NTP > requests. Right...I missed the face that in NewTabPageInterceptorServiceFactory::GetForProfile, we weren't creating a service for incognito. Had been thinking that were were making one for incognito, which seemed like a bad idea. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:982: request_interceptors.push_back(service->CreateInterceptor().release()); Is there a reason to prefer this here over ProfileImplIOData::Handle::CreateMainRequestContextGetter? If there's a reason, I'm fine with it here, but most of the URLRequestContext setup happens in Profile.*IOData, so it's easier to keep track of all the stuff that is attached to the poor beleaguered request context if we try and keep most of it there...Of course, my suggestion only overs main profiles, but since we don't want it for incognito, anyways, not sure that's an issue. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:28: } // namespace Suggest a linebreak just after the start and just before the end of the namespace. Seems the more common pattern, except for forward declarations, and a close bracket for the function definition right above one for the end of the namespace looks odd to me. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:40: const GURL& new_tab_url() { return new_tab_url_; } const GURL& new_tab_url() const { return new_tab_url_; } https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:42: GURL GetMockURL(const std::string& file) { This should be static and probably private as well. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:48: new_tab_url_ = GetMockURL(newtab_path); new_tab_path seems more consistent (Particularly given new_tab_url_) https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:114: controller->GetLastCommittedEntry()->GetURL()); Should probably include gurl.h and gtest.h https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:71: return nullptr; nit: Use braces when the if body takes more than one line https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:86: DCHECK(profile_); nit: include base/logging.h for DCHECK https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.h (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.h:39: class NewTabPageInterceptor : public net::URLRequestInterceptor { I'd suggest just forward declaring the class here, and moving the definition into the header file, too, to reduce stuff in the header and remove an include or two from it as well (url_request_interceptor.h and gurl.h). https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... File net/test/url_request/url_request_mock_http_job.cc (right): https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... net/test/url_request/url_request_mock_http_job.cc:105: url.append("/"); optional: May just want to merge these into one line.
And just to add, I reviewed everything but the changes to search_tab_helper. I didn't take a careful look at the new browser tests, but I did skim over them, and didn't notice any correctness issues. On 2015/01/22 20:53:51, mmenke wrote: > LGTM, suggestions are mostly optional nits. > > Of the files modified, I'm an owner of net/, chrome/browser/net, and > content/browser/loader/ (And profiles/, if you take that suggestion). > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/845973005/diff/90001/chrome/browser/chrome_co... > chrome/browser/chrome_content_browser_client.cc:989: > request_interceptors.push_back(service->CreateInterceptor().release()); > On 2015/01/22 20:06:30, kmadhusu wrote: > > On 2015/01/22 19:21:58, Mathieu Perreault wrote: > > > On 2015/01/16 22:04:16, mmenke wrote: > > > > Should we really make one of these for incognito? > > > > > > Are you saying we should? Currently we are not creating the interceptor in > > > incognito. It's not the same NTP. > > > > Incognito mode doesn't use an online ntp. We don't need to intercept those NTP > > requests. > > Right...I missed the face that in > NewTabPageInterceptorServiceFactory::GetForProfile, we weren't creating a > service for incognito. Had been thinking that were were making one for > incognito, which seemed like a bad idea. > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... > chrome/browser/chrome_content_browser_client.cc:982: > request_interceptors.push_back(service->CreateInterceptor().release()); > Is there a reason to prefer this here over > ProfileImplIOData::Handle::CreateMainRequestContextGetter? > > If there's a reason, I'm fine with it here, but most of the URLRequestContext > setup happens in Profile.*IOData, so it's easier to keep track of all the stuff > that is attached to the poor beleaguered request context if we try and keep most > of it there...Of course, my suggestion only overs main profiles, but since we > don't want it for incognito, anyways, not sure that's an issue. > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right): > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:28: } // > namespace > Suggest a linebreak just after the start and just before the end of the > namespace. Seems the more common pattern, except for forward declarations, and > a close bracket for the function definition right above one for the end of the > namespace looks odd to me. > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:40: const GURL& > new_tab_url() { return new_tab_url_; } > const GURL& new_tab_url() const { return new_tab_url_; } > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:42: GURL > GetMockURL(const std::string& file) { > This should be static and probably private as well. > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:48: > new_tab_url_ = GetMockURL(newtab_path); > new_tab_path seems more consistent (Particularly given new_tab_url_) > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:114: > controller->GetLastCommittedEntry()->GetURL()); > Should probably include gurl.h and gtest.h > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_service.cc:71: return nullptr; > nit: Use braces when the if body takes more than one line > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_service.cc:86: > DCHECK(profile_); > nit: include base/logging.h for DCHECK > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > File chrome/browser/ui/search/new_tab_page_interceptor_service.h (right): > > https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... > chrome/browser/ui/search/new_tab_page_interceptor_service.h:39: class > NewTabPageInterceptor : public net::URLRequestInterceptor { > I'd suggest just forward declaring the class here, and moving the definition > into the header file, too, to reduce stuff in the header and remove an include > or two from it as well (url_request_interceptor.h and gurl.h). > > https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... > File net/test/url_request/url_request_mock_http_job.cc (right): > > https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... > net/test/url_request/url_request_mock_http_job.cc:105: url.append("/"); > optional: May just want to merge these into one line.
Thanks Matt, Lingering question below. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:982: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/22 20:53:50, mmenke wrote: > Is there a reason to prefer this here over > ProfileImplIOData::Handle::CreateMainRequestContextGetter? > > If there's a reason, I'm fine with it here, but most of the URLRequestContext > setup happens in Profile.*IOData, so it's easier to keep track of all the stuff > that is attached to the poor beleaguered request context if we try and keep most > of it there...Of course, my suggestion only overs main profiles, but since we > don't want it for incognito, anyways, not sure that's an issue. I'll let kmadhusu comment on whether she considered it. So there seems to be a good candidate place here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... This is pretty deep down, but if you think it's a better place let me know. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:28: } // namespace On 2015/01/22 20:53:51, mmenke wrote: > Suggest a linebreak just after the start and just before the end of the > namespace. Seems the more common pattern, except for forward declarations, and > a close bracket for the function definition right above one for the end of the > namespace looks odd to me. Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:40: const GURL& new_tab_url() { return new_tab_url_; } On 2015/01/22 20:53:50, mmenke wrote: > const GURL& new_tab_url() const { return new_tab_url_; } Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:42: GURL GetMockURL(const std::string& file) { On 2015/01/22 20:53:51, mmenke wrote: > This should be static and probably private as well. Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:48: new_tab_url_ = GetMockURL(newtab_path); On 2015/01/22 20:53:50, mmenke wrote: > new_tab_path seems more consistent (Particularly given new_tab_url_) Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc:114: controller->GetLastCommittedEntry()->GetURL()); On 2015/01/22 20:53:51, mmenke wrote: > Should probably include gurl.h and gtest.h Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:71: return nullptr; On 2015/01/22 20:53:51, mmenke wrote: > nit: Use braces when the if body takes more than one line Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:86: DCHECK(profile_); On 2015/01/22 20:53:51, mmenke wrote: > nit: include base/logging.h for DCHECK Done. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.h (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.h:39: class NewTabPageInterceptor : public net::URLRequestInterceptor { On 2015/01/22 20:53:51, mmenke wrote: > I'd suggest just forward declaring the class here, and moving the definition > into the header file, too, to reduce stuff in the header and remove an include > or two from it as well (url_request_interceptor.h and gurl.h). Done. Have a look! https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... File net/test/url_request/url_request_mock_http_job.cc (right): https://codereview.chromium.org/845973005/diff/150001/net/test/url_request/ur... net/test/url_request/url_request_mock_http_job.cc:105: url.append("/"); On 2015/01/22 20:53:51, mmenke wrote: > optional: May just want to merge these into one line. Done.
Still looks good. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:982: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/22 21:44:22, Mathieu Perreault wrote: > On 2015/01/22 20:53:50, mmenke wrote: > > Is there a reason to prefer this here over > > ProfileImplIOData::Handle::CreateMainRequestContextGetter? > > > > If there's a reason, I'm fine with it here, but most of the URLRequestContext > > setup happens in Profile.*IOData, so it's easier to keep track of all the > stuff > > that is attached to the poor beleaguered request context if we try and keep > most > > of it there...Of course, my suggestion only overs main profiles, but since we > > don't want it for incognito, anyways, not sure that's an issue. > > I'll let kmadhusu comment on whether she considered it. > > So there seems to be a good candidate place here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... > > This is pretty deep down, but if you think it's a better place let me know. That's actually called on the IO thread, so wouldn't work. I believe ProfileIOData::InitializeOnUIThread is actually the latest place you could put it (Not sure you can access the interceptor list there, but you can stick it in params, and then hook it up later down the line on the IO thread). There is already at least one other interceptor created there. https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:22: nit: Remove blank line. https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:55: } nit: Blank line between inlined functions (With the possible exception of simple getters/setters)
Will wait to see kmadhusu's opinion. https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:22: On 2015/01/22 21:52:29, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/845973005/diff/170001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:55: } On 2015/01/22 21:52:29, mmenke wrote: > nit: Blank line between inlined functions (With the possible exception of > simple getters/setters) Done.
LGTM for search_tab_helper related changes. https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... File chrome/browser/ui/search/new_tab_page_interceptor_service.cc (right): https://codereview.chromium.org/845973005/diff/110001/chrome/browser/ui/searc... chrome/browser/ui/search/new_tab_page_interceptor_service.cc:28: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2015/01/22 20:06:30, kmadhusu wrote: > On 2015/01/21 21:29:39, Mathieu Perreault wrote: > > kmadhusu@: Do you think it's worth replicating the logic in > > InstantService::OnTemplateURLServiceChanged() here? The code is much simpler > > here, but not sure how often it'll trigger for nothing. What do you think? > > I looked at the latest patch. These changes seems reasonable. I will do a quick > test just to make sure everything works as expected. Tested the latest patch. NTP interceptor works as expected. https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/845973005/diff/150001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:982: request_interceptors.push_back(service->CreateInterceptor().release()); On 2015/01/22 21:52:29, mmenke wrote: > On 2015/01/22 21:44:22, Mathieu Perreault wrote: > > On 2015/01/22 20:53:50, mmenke wrote: > > > Is there a reason to prefer this here over > > > ProfileImplIOData::Handle::CreateMainRequestContextGetter? > > > > > > If there's a reason, I'm fine with it here, but most of the > URLRequestContext > > > setup happens in Profile.*IOData, so it's easier to keep track of all the > > stuff > > > that is attached to the poor beleaguered request context if we try and keep > > most > > > of it there...Of course, my suggestion only overs main profiles, but since > we > > > don't want it for incognito, anyways, not sure that's an issue. > > > > I'll let kmadhusu comment on whether she considered it. > > > > So there seems to be a good candidate place here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... > > > > This is pretty deep down, but if you think it's a better place let me know. > > That's actually called on the IO thread, so wouldn't work. I believe > ProfileIOData::InitializeOnUIThread is actually the latest place you could put > it (Not sure you can access the interceptor list there, but you can stick it in > params, and then hook it up later down the line on the IO thread). > > There is already at least one other interceptor created there. I didn't try to create an interceptor in ProfileImplIOData::Handle::CreateMainRequestContextGetter. There should not be any problem to move this code there because line 983 eventually calls that function. But I did try to create an interceptor instance in ProfileImplIOData::InitializeInternal() and it failed because of thread violation. Feel free to move this code to any better place as long as that code runs on an UI thread.
mathp@chromium.org changed reviewers: + jochen@chromium.org
Jochen, Can you look at chrome/browser/chrome_content_browser_client ? Your approval would go towards the rename I did in the many tests AddUrlHandler->AddUrlHandlers. Thanks
On 2015/01/23 20:03:07, Mathieu Perreault wrote: > Jochen, > > Can you look at chrome/browser/chrome_content_browser_client ? > > Your approval would go towards the rename I did in the many tests > AddUrlHandler->AddUrlHandlers. > > Thanks What happened to moving that into ProfileIOData setup, per discussion?
On 2015/01/23 20:07:36, mmenke wrote: > On 2015/01/23 20:03:07, Mathieu Perreault wrote: > > Jochen, > > > > Can you look at chrome/browser/chrome_content_browser_client ? > > > > Your approval would go towards the rename I did in the many tests > > AddUrlHandler->AddUrlHandlers. > > > > Thanks > > What happened to moving that into ProfileIOData setup, per discussion? I thought your suggestion was optional, and it seems a bit convoluted to do and keep it on UI thread. Should I investigate more?
On 2015/01/23 20:17:24, Mathieu Perreault wrote: > On 2015/01/23 20:07:36, mmenke wrote: > > On 2015/01/23 20:03:07, Mathieu Perreault wrote: > > > Jochen, > > > > > > Can you look at chrome/browser/chrome_content_browser_client ? > > > > > > Your approval would go towards the rename I did in the many tests > > > AddUrlHandler->AddUrlHandlers. > > > > > > Thanks > > > > What happened to moving that into ProfileIOData setup, per discussion? > > I thought your suggestion was optional, and it seems a bit convoluted to do and > keep it on UI thread. Should I investigate more? It was optional, but I suggested a point where you could put it (Two, actually) that should both work, and are both on the UI thread. If you decided not to go with the idea, I was expecting a comment... And the more I think about it, the more inclined I am to think Profile[Impl]IOData is the right place for this - we have so much stuff hooked up to the network stack, best to keep where we add them in one or two files.
On 2015/01/23 20:33:56, mmenke wrote: > On 2015/01/23 20:17:24, Mathieu Perreault wrote: > > On 2015/01/23 20:07:36, mmenke wrote: > > > On 2015/01/23 20:03:07, Mathieu Perreault wrote: > > > > Jochen, > > > > > > > > Can you look at chrome/browser/chrome_content_browser_client ? > > > > > > > > Your approval would go towards the rename I did in the many tests > > > > AddUrlHandler->AddUrlHandlers. > > > > > > > > Thanks > > > > > > What happened to moving that into ProfileIOData setup, per discussion? > > > > I thought your suggestion was optional, and it seems a bit convoluted to do > and > > keep it on UI thread. Should I investigate more? > > It was optional, but I suggested a point where you could put it (Two, actually) > that should both work, and are both on the UI thread. If you decided not to go > with the idea, I was expecting a comment... And the more I think about it, the > more inclined I am to think Profile[Impl]IOData is the right place for this - we > have so much stuff hooked up to the network stack, best to keep where we add > them in one or two files. And actually I said "If there's a reason, I'm fine with it here", but I never got a reason. :)
chrome content browser client lgtm
renames also lgtm
On 2015/01/26 15:45:14, jochen (slow) wrote: > renames also lgtm Hi Matt, please have a look, I moved it to profile_impl_io_data.
On 2015/01/27 15:47:10, Mathieu Perreault wrote: > On 2015/01/26 15:45:14, jochen (slow) wrote: > > renames also lgtm > > Hi Matt, please have a look, I moved it to profile_impl_io_data. Hold the review, some tests fail due to NTPInterceptor being deconstructed on UI thread. Will look soon.
Hi Matt, have a look. Tests should be good now.
Patchset #11 (id:250001) has been deleted
Patchset #10 (id:230001) has been deleted
Sorry for not getting to this earlier - decided to take a couple days off after the blizzard. LGTM. https://codereview.chromium.org/845973005/diff/270001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/845973005/diff/270001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:593: } Think this would make more sense in ProfileIOData::Init, just before the call to this method.
https://codereview.chromium.org/845973005/diff/270001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/845973005/diff/270001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:593: } On 2015/01/30 15:21:10, mmenke wrote: > Think this would make more sense in ProfileIOData::Init, just before the call to > this method. Done.
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/845973005/290001
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...)
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/845973005/290001
Message was sent while issue was closed.
Committed patchset #11 (id:290001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6f2b16735659425f6a0167754051df62b0b4431b Cr-Commit-Position: refs/heads/master@{#314004} |
