Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529283002:
Save favicon during reading list distillation (Closed)
DescriptionSave favicon during reading list distillation
As the lifespan of the DistillerPage is shorter than the one needed by the
WebState to download the favicon, the WebState needs to outlive the
DistillerPage.
As the favicon download should not impact too much the distillation time, the
next distillation should be launch before the previous one is finished.
This Dispatcher will makes sure the WebState survive long enough to download
the Favicon after it is returned.
BUG=664988
Committed: https://crrev.com/59fe574e35aecc8732b382751257bfef883514e2
Cr-Commit-Position: refs/heads/master@{#440065}
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : Add a comment #
Total comments: 17
Patch Set 4 : Test using the WebObserver #Patch Set 5 : Rename classes #Patch Set 6 : Add comments #Patch Set 7 : Using web state #
Total comments: 32
Patch Set 8 : Use WebState pool #
Total comments: 2
Patch Set 9 : Use dispatcher #Patch Set 10 : Cleanup #
Total comments: 10
Patch Set 11 : Address olivierrobin's comments #Patch Set 12 : Add DependsOn #
Total comments: 7
Patch Set 13 : Address comments #Patch Set 14 : Fix constructor #
Total comments: 4
Patch Set 15 : Adding tests #Patch Set 16 : Fix BUILD.gn #Patch Set 17 : Add constructor comments #
Total comments: 29
Patch Set 18 : Address comments #
Total comments: 28
Patch Set 19 : Change map to vector #
Total comments: 17
Patch Set 20 : Address comments #Patch Set 21 : Fix test #Messages
Total messages: 73 (19 generated)
gambard@chromium.org changed reviewers: + eugenebut@chromium.org
PTAL. https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... components/dom_distiller/ios/distiller_page_ios.mm:24: std::string favicon_script_ = This script is the same as in ios/web/web_state/js/resources/common.js. Should it be shared between this code and the CRWWebController? https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... File ios/chrome/browser/dom_distiller/distiller_favicon.mm (right): https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:36: web::WebState* web_state = provider->GetWebState(); This might not be the good way to get the web_state, it is the same as in the distiller_page_ios.
gambard@chromium.org changed reviewers: + kkhorimoto@chromium.org
+kkhorimoto@ as eugene is OOO. PTAL.
High level comment: I'm not exactly sure what this CL is changing. It looks like after we distill the page, we're kicking off the favicon fetch script, then passing the result of this script to the WebFaviconDriver. I assume that the favicon service does some caching in a layer that's accessible from multiple WebStates since the WebFaviconDriver is created for an unused WebState. It seems that creating a WebFaviconDriver before kicking off the page load from DistillerPageIOS would begin observing the WebStateObserver favicon callback, which would accomplish the same thing you're trying to do here. https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... File components/dom_distiller/ios/distiller_favicon_ios.h (right): https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... components/dom_distiller/ios/distiller_favicon_ios.h:18: class DistillerFaviconIOS { This needs class comments. Additionally, we only name classes with the *IOS suffix if they're iOS implementations of interfaces that are shared across platforms. Since this class is specific to iOS, I don't think the suffix is necessary here; maybe just use an abstract interface in components/ then an implementation with the *Impl suffix would work better. https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... components/dom_distiller/ios/distiller_page_ios.mm:24: std::string favicon_script_ = On 2016/11/28 10:20:04, gambard wrote: > This script is the same as in ios/web/web_state/js/resources/common.js. Should > it be shared between this code and the CRWWebController? WebStateObserver already exposes FaviconUrlUpdated(), which seems like it contains the same information returned by this script. Is there a reason you needed to duplicate this code here? https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... File ios/chrome/browser/dom_distiller/distiller_favicon.h (right): https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.h:16: class DistillerFavicon : public DistillerFaviconIOS { Add class comments https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... File ios/chrome/browser/dom_distiller/distiller_favicon.mm (right): https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:33: ios::GetWebControllerProviderFactory()->CreateWebControllerProvider( This is creating a new WebState, so this will be different than the WebState being used to load the page/run the distillation scripts. https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:36: web::WebState* web_state = provider->GetWebState(); On 2016/11/28 10:20:04, gambard wrote: > This might not be the good way to get the web_state, it is the same as in the > distiller_page_ios. Ideally, we should be removing the web controller provider altogether; I'm pretty sure that all the necessary functionality is already present in WebState's public interface at this point. For now, accessing like this is fine. https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:49: favicon_driver->FetchFavicon(page_url); Is this doing the same as executing the fetch favicon script from common.js? Why are we fetching here? Can you add comments? https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:51: ((web::WebStateObserver*)favicon_driver)->FaviconUrlUpdated(urls); I'm unclear on what exactly is happening here. Couldn't we just create the WebFaviconDriver before loading the page in DistillerPageIOS? That way, it will automatically receive this WebStateObserver callback when the page loads initially. https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:51: ((web::WebStateObserver*)favicon_driver)->FaviconUrlUpdated(urls); Also, we need to be using static_cast<> for these types of casting operations. https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... File ios/web/public/favicon_url.cc (right): https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... ios/web/public/favicon_url.cc:49: return true; Should we return false if |result_urls| is empty at the end here? I notice that you check for that when calling this from CRWWebController. https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... File ios/web/public/favicon_url.h (right): https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... ios/web/public/favicon_url.h:50: std::vector<web::FaviconURL>& result_urls); The style guide dictates that out-parameters should be pointers. Passing by reference should only be used for const references in order to avoid copying larger objects. Additionally, the comment here should explain what conditions will cause this function to return false. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
Thanks for your comments. I am just addressing the high level comment. Yes, we want to download the favicon after the distillation. Setting the FaviconDriver to observe the WebState does not work for the same reason my code does not work (see comment), sorry. Do you know if there is a way to either bypass it or to add an entry for the visible item? https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... File ios/chrome/browser/dom_distiller/distiller_favicon.mm (right): https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:51: ((web::WebStateObserver*)favicon_driver)->FaviconUrlUpdated(urls); On 2016/11/28 23:48:25, kkhorimoto_ wrote: > I'm unclear on what exactly is happening here. Couldn't we just create the > WebFaviconDriver before loading the page in DistillerPageIOS? That way, it will > automatically receive this WebStateObserver callback when the page loads > initially. You are right, but it is because this code isn't working (I have been unlucky when testing it). The implementation of FaviconUrlUpdated in web_favicon_driver ask for GetActiveURL() which uses web_state()->GetNavigationManager()->GetVisibleItem() which is not set in my case. The method I really want to call (and fixing the code) is FaviconDriverImpl::OnUpdateFaviconURL() in which I can set the url.
I am doing a new approach in this CL: https://codereview.chromium.org/2541093002 following the recommendation of not duplicating code. Please make comments on it.
I took a look at your other CL. It looks like you're following my advice of setting up the favicon driver before kicking off the load and allowing the preexisting WSO favicon callback to handle the urls. If I understand correctly, the only difference being that you store the URL we want to associate the favicon with upon initialization. If you initialize the WebFaviconDriver in the same location (i.e. when instantiating the provider/before kicking off the load), is GetVisibleItem() still null when the favicon callback is called? The favicon callback is called as the result of |-didFinishNavigation|, which is called after the NavigationItem is committed, so we should definitely have a visible item at that point. I'm wondering if it's just because (in this CL), you instantiate the driver on an unused WebState. I think that my original suggestion of just instantiating the WebFaviconDriver before using the web controller provider's WebState would be the best approach here; we should debug why the visible item is null rather than creating the workaround using subclassing.
https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... File components/dom_distiller/ios/distiller_favicon_ios.h (right): https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... components/dom_distiller/ios/distiller_favicon_ios.h:21: virtual void HandleFaviconURL(std::vector<web::FaviconURL> urls, Optional nit: Per C+++ Style Guide this should be HandleFaviconUrl, however we don't follow that rule and generally very inconsistent with URL capitalization. https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... File components/dom_distiller/ios/distiller_page_ios.h (right): https://codereview.chromium.org/2529283002/diff/40001/components/dom_distille... components/dom_distiller/ios/distiller_page_ios.h:51: // evaluated. Extracts the data from the |result| and let |distiller_favicon_| Please describe what |result| is, including it's actual expected type. https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... File ios/chrome/browser/dom_distiller/distiller_favicon.mm (right): https://codereview.chromium.org/2529283002/diff/40001/ios/chrome/browser/dom_... ios/chrome/browser/dom_distiller/distiller_favicon.mm:36: web::WebState* web_state = provider->GetWebState(); On 2016/11/28 23:48:25, kkhorimoto_ wrote: > On 2016/11/28 10:20:04, gambard wrote: > > This might not be the good way to get the web_state, it is the same as in the > > distiller_page_ios. > > Ideally, we should be removing the web controller provider altogether; I'm > pretty sure that all the necessary functionality is already present in > WebState's public interface at this point. For now, accessing like this is > fine. There should be no need to create new WebControllerProvider. You can just create a new WebState instead. https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... File ios/web/public/favicon_url.h (right): https://codereview.chromium.org/2529283002/diff/40001/ios/web/public/favicon_... ios/web/public/favicon_url.h:49: bool ExtractFaviconURL(base::ListValue* favicons, Do we need this as a part of public API? WebStateObserver already provides a way to get favicon URLs. If ios/web does not provide necessary API then we should look at other platforms for inspiration and use the same approach where possible.
This is a new version, based on the high level feedbacks provided. PTAL.
Our architectural goal is to remove WebControllerProvider/WebControllerProviderFactory etc... So I don't think we should write more code which we have to clean up later (like new factory class). Can we just use web::WebState instead of WebControllerProvider (which does not give any advantages over web state)?
On 2016/12/02 16:44:41, Eugene But wrote: > Our architectural goal is to remove > WebControllerProvider/WebControllerProviderFactory etc... So I don't think we > should write more code which we have to clean up later (like new factory class). > > > Can we just use web::WebState instead of WebControllerProvider (which does not > give any advantages over web state)? I was going to suggest removing the web controller provider in a separate CL, but given the current state of this CL, it might be easier to just switch to using WebState in DistillerPageIOS. Instead of the factory, we can lazily create a new WebState in DistillerPageIOS::DistillPageImpl() and instantiate the WebFaviconDriver there. I've reassigned crbug.com/546231 to you, as DistillerPageIOS is the only remaining user of the web controller provider.
On 2016/12/02 21:48:14, kkhorimoto_ wrote: > On 2016/12/02 16:44:41, Eugene But wrote: > > Our architectural goal is to remove > > WebControllerProvider/WebControllerProviderFactory etc... So I don't think we > > should write more code which we have to clean up later (like new factory > class). > > > > > > Can we just use web::WebState instead of WebControllerProvider (which does not > > give any advantages over web state)? > > I was going to suggest removing the web controller provider in a separate CL, > but given the current state of this CL, it might be easier to just switch to > using WebState in DistillerPageIOS. Instead of the factory, we can lazily > create a new WebState in DistillerPageIOS::DistillPageImpl() and instantiate the > WebFaviconDriver there. I've reassigned crbug.com/546231 to you, as > DistillerPageIOS is the only remaining user of the web controller provider. Thanks, I tried with a WebState but it seems to need a tab to load the URL as a web controller delegate. Even if I take the tab implementation, it seems to use a NavigationManager, which forwards the call to CRWWebController, which saves the url in the history. Do you know how I can load an URL without it being saved in the history, and without a tab?
On 2016/12/05 10:15:54, gambard wrote: > On 2016/12/02 21:48:14, kkhorimoto_ wrote: > > On 2016/12/02 16:44:41, Eugene But wrote: > > > Our architectural goal is to remove > > > WebControllerProvider/WebControllerProviderFactory etc... So I don't think > we > > > should write more code which we have to clean up later (like new factory > > class). > > > > > > > > > Can we just use web::WebState instead of WebControllerProvider (which does > not > > > give any advantages over web state)? > > > > I was going to suggest removing the web controller provider in a separate CL, > > but given the current state of this CL, it might be easier to just switch to > > using WebState in DistillerPageIOS. Instead of the factory, we can lazily > > create a new WebState in DistillerPageIOS::DistillPageImpl() and instantiate > the > > WebFaviconDriver there. I've reassigned crbug.com/546231 to you, as > > DistillerPageIOS is the only remaining user of the web controller provider. > > Thanks, I tried with a WebState but it seems to need a tab to load the URL as a > web controller delegate. > Even if I take the tab implementation, it seems to use a NavigationManager, > which forwards the call to CRWWebController, which saves the url in the history. > > Do you know how I can load an URL without it being saved in the history, and > without a tab? You can load a URL via NavigationManager, which you can get from WebState.
On 2016/12/05 17:42:40, Eugene But wrote: > On 2016/12/05 10:15:54, gambard wrote: > > On 2016/12/02 21:48:14, kkhorimoto_ wrote: > > > On 2016/12/02 16:44:41, Eugene But wrote: > > > > Our architectural goal is to remove > > > > WebControllerProvider/WebControllerProviderFactory etc... So I don't think > > we > > > > should write more code which we have to clean up later (like new factory > > > class). > > > > > > > > > > > > Can we just use web::WebState instead of WebControllerProvider (which does > > not > > > > give any advantages over web state)? > > > > > > I was going to suggest removing the web controller provider in a separate > CL, > > > but given the current state of this CL, it might be easier to just switch to > > > using WebState in DistillerPageIOS. Instead of the factory, we can lazily > > > create a new WebState in DistillerPageIOS::DistillPageImpl() and instantiate > > the > > > WebFaviconDriver there. I've reassigned crbug.com/546231 to you, as > > > DistillerPageIOS is the only remaining user of the web controller provider. > > > > Thanks, I tried with a WebState but it seems to need a tab to load the URL as > a > > web controller delegate. > > Even if I take the tab implementation, it seems to use a NavigationManager, > > which forwards the call to CRWWebController, which saves the url in the > history. > > > > Do you know how I can load an URL without it being saved in the history, and > > without a tab? > You can load a URL via NavigationManager, which you can get from WebState. Additionally, you need to call InitializeSession() on the NavigationManager and set |webUsageEnabled| to YES on CRWWebController.
On 2016/12/05 18:04:52, kkhorimoto_ wrote: > On 2016/12/05 17:42:40, Eugene But wrote: > > On 2016/12/05 10:15:54, gambard wrote: > > > On 2016/12/02 21:48:14, kkhorimoto_ wrote: > > > > On 2016/12/02 16:44:41, Eugene But wrote: > > > > > Our architectural goal is to remove > > > > > WebControllerProvider/WebControllerProviderFactory etc... So I don't > think > > > we > > > > > should write more code which we have to clean up later (like new factory > > > > class). > > > > > > > > > > > > > > > Can we just use web::WebState instead of WebControllerProvider (which > does > > > not > > > > > give any advantages over web state)? > > > > > > > > I was going to suggest removing the web controller provider in a separate > > CL, > > > > but given the current state of this CL, it might be easier to just switch > to > > > > using WebState in DistillerPageIOS. Instead of the factory, we can lazily > > > > create a new WebState in DistillerPageIOS::DistillPageImpl() and > instantiate > > > the > > > > WebFaviconDriver there. I've reassigned crbug.com/546231 to you, as > > > > DistillerPageIOS is the only remaining user of the web controller > provider. > > > > > > Thanks, I tried with a WebState but it seems to need a tab to load the URL > as > > a > > > web controller delegate. > > > Even if I take the tab implementation, it seems to use a NavigationManager, > > > which forwards the call to CRWWebController, which saves the url in the > > history. > > > > > > Do you know how I can load an URL without it being saved in the history, and > > > without a tab? > > You can load a URL via NavigationManager, which you can get from WebState. > > Additionally, you need to call InitializeSession() on the NavigationManager WebState::Create calls InitializeSession for you, so there is no need to call it manually. > set |webUsageEnabled| to YES on CRWWebController. Please call WebState::SetWebUsageEnabled(true) instead.
Thanks, I tried using NavigationController's loadURLWithParams but it does not works because it's calling CRWWebController's loadWithParams, then CRWWebController's loadCurrentURL which returns immediately because |_containerView| is nil. The only place were it seems to be set is in |triggerPendingLoad| and I don't see a way to trigger this. (BTW this is what WebControllerProvider is doing: |loadWithParams| then |triggerPendingLoad| on the CRWWebController directly).
On 2016/12/06 15:23:32, gambard wrote: > Thanks, I tried using NavigationController's loadURLWithParams but it does not > works because it's calling CRWWebController's loadWithParams, then > CRWWebController's loadCurrentURL which returns immediately because > |_containerView| is nil. > The only place were it seems to be set is in |triggerPendingLoad| and I don't > see a way to trigger this. > (BTW this is what WebControllerProvider is doing: |loadWithParams| then > |triggerPendingLoad| on the CRWWebController directly). It does not load because there is no view (which is probably incorrect, but that's current behavior). This is example how to load a URL with WebState: https://cs.chromium.org/chromium/src/ios/web/webui/web_ui_mojo_inttest.mm?q=i...
Thanks for the comments, I have made a new version using WebState. PTAL, high level review. I also have a problem: I shared the WebState through all the distill pages to make sure it outlive the page. This is needed because the favicon download is asynchronous and takes more time than the page distillation. This creates a problem when the requests for the distillation are queued: the next distillation can happen before the first has finished downloading its favicon, creating a new favicon request which would cancel the first one. Do you have any idea of how I can make sure the WebState live long enough to download the favicon and have all the favicon even if a new distillation starts before the previous one gets its favicon?
On 2016/12/08 15:01:42, gambard wrote: > Thanks for the comments, I have made a new version using WebState. > PTAL, high level review. > I also have a problem: I shared the WebState through all the distill pages to > make sure it outlive the page. This is needed because the favicon download is > asynchronous and takes more time than the page distillation. > This creates a problem when the requests for the distillation are queued: the > next distillation can happen before the first has finished downloading its > favicon, creating a new favicon request which would cancel the first one. > > Do you have any idea of how I can make sure the WebState live long enough to > download the favicon and have all the favicon even if a new distillation starts > before the previous one gets its favicon? Can you use separate WebState for each distillation? Seems like cancelling previous favicon download seems acceptable behavior. Just replying to your question. Will comment on CL separately.
Thank you for refactoring! High level this looks good. SO I just commented on everything. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/DEPS (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/DEPS:3: "+ios/public/provider/web", Do you still need this? https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:18: const web::WebState::CreateParams webStateCreateParams(browser_state); web_state_create_params https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.h (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.h:30: void test(const base::Value*); I guess this was added for testing or something. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:21: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:160: web_state_->GetView(); nit: It worth commenting why you have to call this method (this is more like a workaround). https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:161: } else nit: Needs braces, because previous block has them https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:178: [[web_state_->GetJSInjectionReceiver() Ideally you want to use WebState::ExecuteJavaScript here if possible. InjectionReceiver exists for historical reasons and we should get rid of it at some point. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/favicon_observer_factory.h (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/favicon_observer_factory.h:18: virtual void AddFaviconObserver(web::WebState* web_state, Please add comments https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.h (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.h:25: } // namespace dom_distiller https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm:13: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm:31: } // namespace dom_distiller https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_observer_factory_impl~.h (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl~.h:25: } // namespace dom_distiller
olivierrobin@chromium.org changed reviewers: + olivierrobin@chromium.org
I am not sure if this works but it may be worth trying this. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.h (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.h:26: class DistillerPageIOS : public DistillerPage { May be you can implement FaviconDriverObserver here. That way you will have a OnFaviconUpdated call. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:155: favicon_driver->FetchFavicon(url); favicon_driver->AddObserver(this); https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:193: OnDistillationDone(url_, resultValue.get()); Wait for OnFaviconUpdated to be called.
https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:19: web_state_ = web::WebState::Create(webStateCreateParams); As per higher level discussion on this CL, we should simply store a reference to |browser_state| in this factory class, then vend WebStates to the individual DistillerPageIOS objects. That way queued distillation tasks can occur in separate contexts and we don't need to worry about interleaving of distillation/favicon scripts. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:21: #include "ios/web/public/web_state/web_state.h" On 2016/12/08 15:34:53, Eugene But wrote: > s/include/import More of a comment for Eugene than Gauthier, but it looks like web_state.mm doesn't actually use any ObjC. Should we just update it to .cc file and use #include in the future? https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:134: web_state_observer_.reset(new DistillerWebStateObserver(web_state_, this)); DCHECK(web_state) before this line? https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:95: std::unique_ptr<FaviconObserverFactory> web_controller_provider_factory = s/web_controller_provider/favicon_observer for variable name
https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:21: #include "ios/web/public/web_state/web_state.h" On 2016/12/08 22:15:24, kkhorimoto_ wrote: > On 2016/12/08 15:34:53, Eugene But wrote: > > s/include/import > > More of a comment for Eugene than Gauthier, but it looks like web_state.mm > doesn't actually use any ObjC. Should we just update it to .cc file and use > #include in the future? Header has Objective-C code: https://cs.chromium.org/chromium/src/ios/web/public/web_state/web_state.h?q=w...
Thanks, PTAL! https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/DEPS (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/DEPS:3: "+ios/public/provider/web", On 2016/12/08 15:34:53, Eugene But wrote: > Do you still need this? Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:18: const web::WebState::CreateParams webStateCreateParams(browser_state); On 2016/12/08 15:34:53, Eugene But wrote: > web_state_create_params Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:19: web_state_ = web::WebState::Create(webStateCreateParams); On 2016/12/08 22:15:24, kkhorimoto_ wrote: > As per higher level discussion on this CL, we should simply store a reference to > |browser_state| in this factory class, then vend WebStates to the individual > DistillerPageIOS objects. That way queued distillation tasks can occur in > separate contexts and we don't need to worry about interleaving of > distillation/favicon scripts. Acknowledged. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.h (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.h:26: class DistillerPageIOS : public DistillerPage { On 2016/12/08 18:28:02, Olivier Robin wrote: > May be you can implement FaviconDriverObserver here. > That way you will have a OnFaviconUpdated call. Acknowledged. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.h:30: void test(const base::Value*); On 2016/12/08 15:34:53, Eugene But wrote: > I guess this was added for testing or something. Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:21: #include "ios/web/public/web_state/web_state.h" On 2016/12/08 15:34:53, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:160: web_state_->GetView(); On 2016/12/08 15:34:53, Eugene But wrote: > nit: It worth commenting why you have to call this method (this is more like a > workaround). Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:161: } else On 2016/12/08 15:34:53, Eugene But wrote: > nit: Needs braces, because previous block has them Done. https://codereview.chromium.org/2529283002/diff/120001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:178: [[web_state_->GetJSInjectionReceiver() On 2016/12/08 15:34:53, Eugene But wrote: > Ideally you want to use WebState::ExecuteJavaScript here if possible. > InjectionReceiver exists for historical reasons and we should get rid of it at > some point. I can use it because WebState::ExecuteJavaScript returns a base::Value result and I need to extract the value using the script of this page. See https://codereview.chromium.org/2327783002 for context. https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.h (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.h:25: } On 2016/12/08 15:34:53, Eugene But wrote: > // namespace dom_distiller Done. https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm (right): https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm:13: #include "ios/web/public/web_state/web_state.h" On 2016/12/08 15:34:53, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2529283002/diff/120001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_observer_factory_impl.mm:31: } On 2016/12/08 15:34:53, Eugene But wrote: > // namespace dom_distiller Done. https://codereview.chromium.org/2529283002/diff/140001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/140001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:187: [[web_state_->GetJSInjectionReceiver() Used to make sure I can use the script to parse the id to base::Value present in this file and not the generic one.
Resource Pools are used when resource creation is time consuming operation (like threads creation) and it is definitely not the case with WebState. Using this pattern here can be somewhat misleading and it makes code more complex than it should be. Is there anything I misunderstood and you actually need resource pool here? It would be helpful if CL description be more specific in describing what was done in this CL. I would suggest making a paragraph in reading list design doc to describe favicon issue and discuss high level solution there. https://codereview.chromium.org/2529283002/diff/140001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/140001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:11: #include "ios/web/public/web_state/web_state.h" s/include/import
Description was changed from ========== Save favicon during reading list distillation When a page is distilled in the reading list the favicon is downloaded and saved to be used later. BUG=664988 ========== to ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. As the favicon download can take some time and the user's device should not be overwhelmed by too many network request, the maximum number of WebState run at the same time needs to be limited (10 in this CL). BUG=664988 ==========
Thanks. I have updated the CL description to explain what I am doing. The pool is not here to prevent resource usage, but to have a good tradeoff between distillation speed and network usage.
On 2016/12/12 16:58:06, gambard wrote: > Thanks. > I have updated the CL description to explain what I am doing. > The pool is not here to prevent resource usage, but to have a good tradeoff > between distillation speed and network usage. Is there a reason why we want to limit the network usage? Assuming cellular connection, for mobile battery life it is always better to downloads everything as quick as possible and put radio to sleep (instead of waking it up periodically and/or keeping it active for a long time).
Description was changed from ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. As the favicon download can take some time and the user's device should not be overwhelmed by too many network request, the maximum number of WebState run at the same time needs to be limited (10 in this CL). BUG=664988 ========== to ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. This Dispatcher will makes sure the WebState survive long enough to download the Favicon after it is returned. BUG=664988 ==========
Thanks, PTAL.
https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:24: DistillerPageFactoryIOS::~DistillerPageFactoryIOS() {} before CreateDistillerPage https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:19: // Returns a WebState with a Favicon Driver attached. You need a virtual destructor. https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:12: #include "base/memory/ptr_util.h" remove https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:69: : BrowserStateKeyedServiceFactory( I think you know need some DependsOn here.
Thanks, PTAL. https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:24: DistillerPageFactoryIOS::~DistillerPageFactoryIOS() {} On 2016/12/13 14:29:10, Olivier Robin wrote: > before CreateDistillerPage Done. https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/180001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:19: // Returns a WebState with a Favicon Driver attached. On 2016/12/13 14:29:10, Olivier Robin wrote: > You need a virtual destructor. Done. https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:12: #include "base/memory/ptr_util.h" On 2016/12/13 14:29:10, Olivier Robin wrote: > remove Done. https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:69: : BrowserStateKeyedServiceFactory( On 2016/12/13 14:29:10, Olivier Robin wrote: > I think you know need some DependsOn here. Depends on what?
Looks like the logic of icon download is shared between 2 classes dispatcher (which manages web state lifetime) and distiller which performs the download. But also it's not very obvious that dispatcher has something to do with icon download, which makes things even more complicated. Can we have a single responsible class for the icon download? Per my previous comment, can we go back to design doc stage and discuss the solution there? Codereview tool is not very good instrument for design review, and this is excacly where we are now. Reviewing design for icon download.
https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:69: : BrowserStateKeyedServiceFactory( On 2016/12/13 16:25:53, gambard wrote: > On 2016/12/13 14:29:10, Olivier Robin wrote: > > I think you know need some DependsOn here. > > Depends on what? You are calling code that calls FaviconServiceFactory, . BookmarksService and HistoryServiceFactory. I think you need to add dependencies here.
lgtm
noyau@chromium.org changed reviewers: + noyau@chromium.org
All this code in in dom_distiller. lgtm.
https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/180001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:69: : BrowserStateKeyedServiceFactory( On 2016/12/13 16:31:21, Olivier Robin wrote: > On 2016/12/13 16:25:53, gambard wrote: > > On 2016/12/13 14:29:10, Olivier Robin wrote: > > > I think you know need some DependsOn here. > > > > Depends on what? > > You are calling code that calls FaviconServiceFactory, . BookmarksService and > HistoryServiceFactory. I think you need to add dependencies here. Done.
https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.h (right): https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.h:32: std::unique_ptr<WebStateDispatcher> web_state_dispatcher_; NIT: disallow copy https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:24: virtual void ReturnWebState(web::WebState* web_state) = 0; NIT: Disallow copy https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm:52: (int64_t)(kMaxDelayFavicon * NSEC_PER_SEC)), Do we need a cleaner on shutdown?
Thanks, PTAL. https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.h (right): https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.h:32: std::unique_ptr<WebStateDispatcher> web_state_dispatcher_; On 2016/12/16 10:31:31, Olivier Robin wrote: > NIT: disallow copy Done. https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/220001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:24: virtual void ReturnWebState(web::WebState* web_state) = 0; On 2016/12/16 10:31:31, Olivier Robin wrote: > NIT: Disallow copy Done. https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm:52: (int64_t)(kMaxDelayFavicon * NSEC_PER_SEC)), On 2016/12/16 10:31:31, Olivier Robin wrote: > Do we need a cleaner on shutdown? On shutdown the WebStateDispatchers should be destroyed and the map also. No objects should be leaked. If the callback is called after the object is destroyed the weak_this.get() should be nil. Did I miss something?
https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/220001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/web_state_dispatcher_impl.mm:52: (int64_t)(kMaxDelayFavicon * NSEC_PER_SEC)), On 2016/12/16 13:49:19, gambard wrote: > On 2016/12/16 10:31:31, Olivier Robin wrote: > > Do we need a cleaner on shutdown? > > On shutdown the WebStateDispatchers should be destroyed and the map also. No > objects should be leaked. > If the callback is called after the object is destroyed the weak_this.get() > should be nil. > > Did I miss something? If the map is destroyed, the web_state is destroyed. If a distiller_page is using it, is this a problem?
Thank you for your patience. lgtm. Could you please add a unittest for WebStateDispatcher? https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:14: // Dispatcher for WebState with Favicon Driver attached. The Webstate are kept This also attaches HistoryService and BookmarkModel. Please document that. https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:17: class WebStateDispatcher { If offline conversation we determined that there is no good way to make this class follow Single Responsibility Principle because of the way how dom_distiller component is designed (and it will too much efforts to refactor the shared code). Since that's the case could you please include "Favicon" word in class name. FaviconWebStateDispatcher seems to be a reasonable name.
Thanks, PTAL. Tests added. https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... File components/dom_distiller/ios/web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:14: // Dispatcher for WebState with Favicon Driver attached. The Webstate are kept On 2016/12/16 15:03:49, Eugene But wrote: > This also attaches HistoryService and BookmarkModel. Please document that. Done. https://codereview.chromium.org/2529283002/diff/260001/components/dom_distill... components/dom_distiller/ios/web_state_dispatcher.h:17: class WebStateDispatcher { On 2016/12/16 15:03:50, Eugene But wrote: > If offline conversation we determined that there is no good way to make this > class follow Single Responsibility Principle because of the way how > dom_distiller component is designed (and it will too much efforts to refactor > the shared code). Since that's the case could you please include "Favicon" word > in class name. FaviconWebStateDispatcher seems to be a reasonable name. Done.
gambard@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne for DEPS include
LGTM one suggestion. https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:201: void DistillerPageIOS::HandleJavaScriptResult(id result) { Early return if (!web_state_)
Thank you for the test! https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.h (right): https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.h:21: DistillerPageFactoryIOS( explicit https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:24: FaviconWebStateDispatcherImpl(web::BrowserState* browser_state); explicit https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm (right): https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:8: #include "ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h" This include should go first, then a line-break then the rest of includes/imports. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:13: #include "testing/gtest_mac.h" Do you need this include? https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:32: web::TestWebThreadBundle thread_bundle_; Can you subclass from web::WebTest which already has TestChromeBrowserState and web::TestWebThreadBundle? https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:38: class FaviconWebStateDispatcherObserverTest : public web::WebStateObserver { Should this be TestFaviconWebStateDispatcherObserver? Test suffix normally used for test fixtures (not stubs). https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:53: FaviconWebStateDispatcherImpl* dispatcher = This looks like a leak. Do you want to use stack dispatcher like this?: FaviconWebStateDispatcherImpl dispatcher(GetBrowserState()); https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:55: web::WebState* webState = dispatcher->RequestWebState(); s/webState/web_state https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:58: favicon::WebFaviconDriver::FromWebState(webState); Do you want to test that history and bookmarks services were attached as well? https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:66: web::WebState* webState = dispatcher->RequestWebState(); s/webState/web_state https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:71: ConditionBlock condition = ^{ Optional nit: consider inlining this block. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:77: ASSERT_TRUE(testing::WaitUntilConditionOrTimeout(0.5, condition)); How come that 0.5 seconds is enough for WebState destruction? kDefaultDelayFavicon is 10 seconds. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:79: } // namespace dom_distiller
Thanks! https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.h (right): https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.h:21: DistillerPageFactoryIOS( On 2016/12/19 17:30:06, Eugene But wrote: > explicit Done. https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/320001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:201: void DistillerPageIOS::HandleJavaScriptResult(id result) { On 2016/12/19 14:05:32, Olivier Robin wrote: > Early return if (!web_state_) No, the web state is not used anymore, it is not a problem for it to be nil. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:24: FaviconWebStateDispatcherImpl(web::BrowserState* browser_state); On 2016/12/19 17:30:06, Eugene But wrote: > explicit Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm (right): https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:8: #include "ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h" On 2016/12/19 17:30:06, Eugene But wrote: > This include should go first, then a line-break then the rest of > includes/imports. Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:13: #include "testing/gtest_mac.h" On 2016/12/19 17:30:06, Eugene But wrote: > Do you need this include? Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:32: web::TestWebThreadBundle thread_bundle_; On 2016/12/19 17:30:07, Eugene But wrote: > Can you subclass from web::WebTest which already has TestChromeBrowserState and > web::TestWebThreadBundle? No the BrowserState is not initialized enough. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:38: class FaviconWebStateDispatcherObserverTest : public web::WebStateObserver { On 2016/12/19 17:30:06, Eugene But wrote: > Should this be TestFaviconWebStateDispatcherObserver? Test suffix normally used > for test fixtures (not stubs). Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:53: FaviconWebStateDispatcherImpl* dispatcher = On 2016/12/19 17:30:06, Eugene But wrote: > This looks like a leak. Do you want to use stack dispatcher like this?: > > FaviconWebStateDispatcherImpl dispatcher(GetBrowserState()); > Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:55: web::WebState* webState = dispatcher->RequestWebState(); On 2016/12/19 17:30:06, Eugene But wrote: > s/webState/web_state Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:58: favicon::WebFaviconDriver::FromWebState(webState); On 2016/12/19 17:30:07, Eugene But wrote: > Do you want to test that history and bookmarks services were attached as well? History and Bookmarks are attached to the favicon driver, so no. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:66: web::WebState* webState = dispatcher->RequestWebState(); On 2016/12/19 17:30:06, Eugene But wrote: > s/webState/web_state Done. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:71: ConditionBlock condition = ^{ On 2016/12/19 17:30:06, Eugene But wrote: > Optional nit: consider inlining this block. Acknowledged. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:77: ASSERT_TRUE(testing::WaitUntilConditionOrTimeout(0.5, condition)); On 2016/12/19 17:30:06, Eugene But wrote: > How come that 0.5 seconds is enough for WebState destruction? > kDefaultDelayFavicon is 10 seconds. There is a new constructor for the dispatcher where you can specify the delay and override the default (unittest cannot wait for 10s). Here the delay is 0. https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:79: } On 2016/12/19 17:30:06, Eugene But wrote: > // namespace dom_distiller Done.
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:22: return base::WrapUnique<DistillerPage>( nit: please use base::MakeUnique<> here (or add a comment saying that it crash with Xcode's clang so that I don't have to ask you everytime I review this file). If you add a comment, please create a bug and add a TODO too ;-) https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:29: return base::WrapUnique<DistillerPage>( nit: please use base::MakeUnique<> here (or add a comment saying that it crash with Xcode's clang so that I don't have to ask you everytime I review this file). If you add a comment, please create a bug and add a TODO too ;-) https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... File components/dom_distiller/ios/favicon_web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/favicon_web_state_dispatcher.h:14: // Dispatcher for WebState havind a Favicon Driver, with BookmarkModel and s/havind/hanging/ https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/favicon_web_state_dispatcher.h:15: // HistoryService attached, as observer. The Webstate are kept alive between s/WebState/WebStates/ https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/DEPS (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/DEPS:2: "+components/favicon/ios", Move to ios/chrome/browser/DEPS. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:89: new leveldb_proto::ProtoDatabaseImpl<ArticleEntry>( Please convert all those naked new to base::MakeUnique<> (maybe as a followup). https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:25: // Constructor for keeping the WebStates alive for |keep_alive_time|. Can you add as comment that -1 means to use the default value? Please also state what is the unit used for keep_alive_time (maybe renamed the variable to keep_alive_seconds). https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:26: explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state, Remove explicit here, it is only for constructor that can be called with just one argument. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:37: std::map<web::WebState*, std::unique_ptr<web::WebState>> web_states_; I think it would be simpler to use a std::vector<std::unique_ptr<web::WebState>> here . https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:17: const int kDefaultDelayFavicon = 10; Please add unit in the variable name. Like kDefaultDelayFaviconInSeconds. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:25: FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl( Please leave an empty line between methods https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:32: FaviconWebStateDispatcherImpl::~FaviconWebStateDispatcherImpl() {} ditto https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:40: web_states_[web_state] = std::move(web_state_unique); This is really strange. Do you really need a std::map<>? I think a std::vector<> should be enough (deletion requires a linear search, but I don't think we have enough WebState for a std::map to outperform linear search in a vector). https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:62: dispatch_time(DISPATCH_TIME_NOW, (int64_t)(callback_time * NSEC_PER_SEC)), C style cast are forbidden by the style guide, use static_cast<int64_t>() here.
Thanks, sdefresne@: PTAL, specifically the map->vector change. https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... File components/dom_distiller/ios/distiller_page_factory_ios.mm (right): https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:22: return base::WrapUnique<DistillerPage>( On 2016/12/20 15:40:59, sdefresne wrote: > nit: please use base::MakeUnique<> here (or add a comment saying that it crash > with Xcode's clang so that I don't have to ask you everytime I review this > file). If you add a comment, please create a bug and add a TODO too ;-) Done. https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/distiller_page_factory_ios.mm:29: return base::WrapUnique<DistillerPage>( On 2016/12/20 15:40:59, sdefresne wrote: > nit: please use base::MakeUnique<> here (or add a comment saying that it crash > with Xcode's clang so that I don't have to ask you everytime I review this > file). If you add a comment, please create a bug and add a TODO too ;-) Done. https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... File components/dom_distiller/ios/favicon_web_state_dispatcher.h (right): https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/favicon_web_state_dispatcher.h:14: // Dispatcher for WebState havind a Favicon Driver, with BookmarkModel and On 2016/12/20 15:40:59, sdefresne wrote: > s/havind/hanging/ Done. https://codereview.chromium.org/2529283002/diff/340001/components/dom_distill... components/dom_distiller/ios/favicon_web_state_dispatcher.h:15: // HistoryService attached, as observer. The Webstate are kept alive between On 2016/12/20 15:40:59, sdefresne wrote: > s/WebState/WebStates/ Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/DEPS (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/DEPS:2: "+components/favicon/ios", On 2016/12/20 15:40:59, sdefresne wrote: > Move to ios/chrome/browser/DEPS. Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/dom_distiller_service_factory.cc:89: new leveldb_proto::ProtoDatabaseImpl<ArticleEntry>( On 2016/12/20 15:41:00, sdefresne wrote: > Please convert all those naked new to base::MakeUnique<> (maybe as a followup). Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:25: // Constructor for keeping the WebStates alive for |keep_alive_time|. On 2016/12/20 15:41:00, sdefresne wrote: > Can you add as comment that -1 means to use the default value? Please also state > what is the unit used for keep_alive_time (maybe renamed the variable to > keep_alive_seconds). Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:26: explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state, On 2016/12/20 15:41:00, sdefresne wrote: > Remove explicit here, it is only for constructor that can be called with just > one argument. Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:37: std::map<web::WebState*, std::unique_ptr<web::WebState>> web_states_; On 2016/12/20 15:41:00, sdefresne wrote: > I think it would be simpler to use a std::vector<std::unique_ptr<web::WebState>> > here . Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:17: const int kDefaultDelayFavicon = 10; On 2016/12/20 15:41:00, sdefresne wrote: > Please add unit in the variable name. Like kDefaultDelayFaviconInSeconds. Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:25: FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl( On 2016/12/20 15:41:00, sdefresne wrote: > Please leave an empty line between methods Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:32: FaviconWebStateDispatcherImpl::~FaviconWebStateDispatcherImpl() {} On 2016/12/20 15:41:00, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:40: web_states_[web_state] = std::move(web_state_unique); On 2016/12/20 15:41:00, sdefresne wrote: > This is really strange. Do you really need a std::map<>? I think a std::vector<> > should be enough (deletion requires a linear search, but I don't think we have > enough WebState for a std::map to outperform linear search in a vector). Done. https://codereview.chromium.org/2529283002/diff/340001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:62: dispatch_time(DISPATCH_TIME_NOW, (int64_t)(callback_time * NSEC_PER_SEC)), On 2016/12/20 15:41:00, sdefresne wrote: > C style cast are forbidden by the style guide, use static_cast<int64_t>() here. The C style cast is the default snippet for dispatch_after, but Done.
https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm (right): https://codereview.chromium.org/2529283002/diff/320001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl_unittest.mm:32: web::TestWebThreadBundle thread_bundle_; On 2016/12/20 08:54:22, gambard wrote: > On 2016/12/19 17:30:07, Eugene But wrote: > > Can you subclass from web::WebTest which already has TestChromeBrowserState > and > > web::TestWebThreadBundle? > > No the BrowserState is not initialized enough. Can we initialize browser_state_ in WebTest correctly and then subclass? web::WebTest was created for code reuse and we should take advantage of that. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:28: int keep_alive_time_second); Do you want to drop |FaviconWebStateDispatcherImpl(web::BrowserState*)| and make clients always pass timeout? This way tests and production code would use the same code (which is what we should do whenever possible). https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:31: keep_alive_time_second_(keep_alive_time_second), How about this?: keep_alive_time_second_(kDefaultDelayFaviconSecond) This way you can drop |keep_alive_time_second_ < 0| ternary. Otherwise unittests tests code path that is not running in production.
lgtm with some suggestions https://codereview.chromium.org/2529283002/diff/360001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/360001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:156: nit: I think using early return here could improve readability, but probably better to do as a followup web_state_ = web_state_dispatcher_->RequestWebState(); if (!web_state_) { OnLoadURLDone(web::PageLoadCompletionStatus::FAILURE); return; } web_state_observer_ = base::MakeUnique<DistillerWebStateObserver>(web_state_, this); ... https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:26: // seconds. If |keep_alive_time_second| < 0 then the default value is used. nit: I think |keep_alive_seconds| is better (using unit implies it is a time) https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:28: int keep_alive_time_second); On 2016/12/20 17:36:18, Eugene But wrote: > Do you want to drop |FaviconWebStateDispatcherImpl(web::BrowserState*)| and make > clients always pass timeout? This way tests and production code would use the > same code (which is what we should do whenever possible). Another option would be to have a default value for keep_alive_time_second. // Constructor for keeping the WebStates alive for |keep_alive_time_second| // seconds. If |keep_alive_time_second| < 0 then the default value is used. explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state, int keep_alive_time_second = -1); Then production and test will use the same code path, while production does not have to specify a timeout. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:31: keep_alive_time_second_(keep_alive_time_second), On 2016/12/20 17:36:18, Eugene But wrote: > How about this?: > keep_alive_time_second_(kDefaultDelayFaviconSecond) > > This way you can drop |keep_alive_time_second_ < 0| ternary. Otherwise unittests > tests code path that is not running in production. I think you wanted to write: keep_alive_time_second_( keep_alive_time_second < 0 ? kDefaultDelayFaviconSecond : keep_alive_time_second), https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:38: std::unique_ptr<web::WebState> web_state_unique = To avoid creating two local variables, I would do (simpler, no move, no web_state_uniwe, ...): web_states_.push_back(web::WebState::Create(web_state_create_params)); web::WebState* web_state = web_state_.back(); https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:61: int callback_time = keep_alive_time_second_ < 0 ? kDefaultDelayFaviconSecond I like eugenebut@ suggestion of doing the check in the constructor since this value cannot be changed after that point. To make the dispatch_after cleaner, I would still use a local variable here for the cast: int64_t keep_alive_nsec = static_cast<int64_t>(keep_alive_time_second_) * NSEC_PER_SEC; dispatch_after(dispatch_time(DISPATCH_TIME_NOW, keep_alive_nsec), ... https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:74: }); I think you can DCHECK here: DCHECK(it != web_state_dispatcher->web_states_.end()); web_state_dispatcher->web_states_.erase(it);
And just to clarify, you have my lgtm already... These were just follow up nits. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:28: int keep_alive_time_second); On 2016/12/20 18:05:31, sdefresne wrote: > On 2016/12/20 17:36:18, Eugene But wrote: > > Do you want to drop |FaviconWebStateDispatcherImpl(web::BrowserState*)| and > make > > clients always pass timeout? This way tests and production code would use the > > same code (which is what we should do whenever possible). > > Another option would be to have a default value for keep_alive_time_second. > > // Constructor for keeping the WebStates alive for |keep_alive_time_second| > // seconds. If |keep_alive_time_second| < 0 then the default value is used. > explicit FaviconWebStateDispatcherImpl(web::BrowserState* browser_state, > int keep_alive_time_second = -1); > > Then production and test will use the same code path, while production does not > have to specify a timeout. Default values are somewhat discouraged by C++ Style Guide and rarely used in Chromium: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:31: keep_alive_time_second_(keep_alive_time_second), On 2016/12/20 18:05:31, sdefresne wrote: > On 2016/12/20 17:36:18, Eugene But wrote: > > How about this?: > > keep_alive_time_second_(kDefaultDelayFaviconSecond) > > > > This way you can drop |keep_alive_time_second_ < 0| ternary. Otherwise > unittests > > tests code path that is not running in production. > > I think you wanted to write: > > keep_alive_time_second_( > keep_alive_time_second < 0 > ? kDefaultDelayFaviconSecond > : keep_alive_time_second), Sorry. I wanted to suggest changing the first constructor: FaviconWebStateDispatcherImpl::FaviconWebStateDispatcherImpl( web::BrowserState* browser_state) : FaviconWebStateDispatcherImpl(browser_state, kDefaultDelayFaviconSecond) {} and drop the usage or ternary operator. Or just fully drop the first constructor and make all clients pass kDefaultDelayFaviconSecond as an argument.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, noyau@chromium.org, olivierrobin@chromium.org, eugenebut@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2529283002/#ps380001 (title: "Address comments")
Thanks! https://codereview.chromium.org/2529283002/diff/360001/components/dom_distill... File components/dom_distiller/ios/distiller_page_ios.mm (right): https://codereview.chromium.org/2529283002/diff/360001/components/dom_distill... components/dom_distiller/ios/distiller_page_ios.mm:156: On 2016/12/20 18:05:31, sdefresne wrote: > nit: I think using early return here could improve readability, but probably > better to do as a followup > > web_state_ = web_state_dispatcher_->RequestWebState(); > if (!web_state_) { > OnLoadURLDone(web::PageLoadCompletionStatus::FAILURE); > return; > } > > web_state_observer_ = base::MakeUnique<DistillerWebStateObserver>(web_state_, > this); > ... Done. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:26: // seconds. If |keep_alive_time_second| < 0 then the default value is used. On 2016/12/20 18:05:31, sdefresne wrote: > nit: I think |keep_alive_seconds| is better (using unit implies it is a time) Done. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.h:28: int keep_alive_time_second); On 2016/12/20 17:36:18, Eugene But wrote: > Do you want to drop |FaviconWebStateDispatcherImpl(web::BrowserState*)| and make > clients always pass timeout? This way tests and production code would use the > same code (which is what we should do whenever possible). Done. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm (right): https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:38: std::unique_ptr<web::WebState> web_state_unique = On 2016/12/20 18:05:31, sdefresne wrote: > To avoid creating two local variables, I would do (simpler, no move, no > web_state_uniwe, ...): > > web_states_.push_back(web::WebState::Create(web_state_create_params)); > web::WebState* web_state = web_state_.back(); > Acknowledged. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:61: int callback_time = keep_alive_time_second_ < 0 ? kDefaultDelayFaviconSecond On 2016/12/20 18:05:31, sdefresne wrote: > I like eugenebut@ suggestion of doing the check in the constructor since this > value cannot be changed after that point. To make the dispatch_after cleaner, I > would still use a local variable here for the cast: > > int64_t keep_alive_nsec = static_cast<int64_t>(keep_alive_time_second_) * > NSEC_PER_SEC; > dispatch_after(dispatch_time(DISPATCH_TIME_NOW, keep_alive_nsec), ... Done. Making the constructor take a int64_t as argument. https://codereview.chromium.org/2529283002/diff/360001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/favicon_web_state_dispatcher_impl.mm:74: }); On 2016/12/20 18:05:31, sdefresne wrote: > I think you can DCHECK here: > > DCHECK(it != web_state_dispatcher->web_states_.end()); > web_state_dispatcher->web_states_.erase(it); Acknowledged.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, noyau@chromium.org, eugenebut@chromium.org, olivierrobin@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2529283002/#ps400001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1482310551184380,
"parent_rev": "a579e8a42a4f0095fe3a8fe0ca9c7d66afa9e82d", "commit_rev":
"5530c2dd5d1e4831495fe7c8b8bd3dd7639bec0b"}
Message was sent while issue was closed.
Description was changed from ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. This Dispatcher will makes sure the WebState survive long enough to download the Favicon after it is returned. BUG=664988 ========== to ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. This Dispatcher will makes sure the WebState survive long enough to download the Favicon after it is returned. BUG=664988 Review-Url: https://codereview.chromium.org/2529283002 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. This Dispatcher will makes sure the WebState survive long enough to download the Favicon after it is returned. BUG=664988 Review-Url: https://codereview.chromium.org/2529283002 ========== to ========== Save favicon during reading list distillation As the lifespan of the DistillerPage is shorter than the one needed by the WebState to download the favicon, the WebState needs to outlive the DistillerPage. As the favicon download should not impact too much the distillation time, the next distillation should be launch before the previous one is finished. This Dispatcher will makes sure the WebState survive long enough to download the Favicon after it is returned. BUG=664988 Committed: https://crrev.com/59fe574e35aecc8732b382751257bfef883514e2 Cr-Commit-Position: refs/heads/master@{#440065} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/59fe574e35aecc8732b382751257bfef883514e2 Cr-Commit-Position: refs/heads/master@{#440065} |
