Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894483003:
Initialize ios/web_view translate with a system-wide URLRequestContext. (Closed)
DescriptionInitialize ios/web_view translate with a system-wide URLRequestContext.
Add WebViewIOThread and ApplicationContext singleton which manage global
application state and WebViewTranslateService which manages the translate
lifecycle.
BUG=679895, 710948
Review-Url: https://codereview.chromium.org/2894483003
Cr-Commit-Position: refs/heads/master@{#476700}
Committed: https://chromium.googlesource.com/chromium/src/+/6c4f1da164a4c4450a9ff9d5ede739bd6f28eef5
Patch Set 1 #
Total comments: 46
Patch Set 2 : Respond to comments. #
Total comments: 1
Patch Set 3 : Respond to comments. #Patch Set 4 : Move WebViewIOThread to ios/components. #Patch Set 5 : Remove unneeded deps. #
Total comments: 4
Patch Set 6 : Rebase and respond to comments. #Patch Set 7 : Add namespace around WebViewIOThread. #Patch Set 8 : Rebase and add WebViewTranslateService #Patch Set 9 : Cleanup DEPS and old io_thread class. #Patch Set 10 : Cleanup. #
Total comments: 31
Patch Set 11 : Respond to comments. #
Total comments: 8
Patch Set 12 : Rebase. #Patch Set 13 : Respond to comments. #
Total comments: 6
Patch Set 14 : Respond to comments. #Messages
Total messages: 41 (11 generated)
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org, ichikawa@chromium.org
This is a first pass at adding the system wide request context. The logic here is a reduced set of the logic that is used in ios/chrome. I will still do a pass to review comments as they are mostly unchanged, but feel free to take a high level look at this CL. I believe everything here is required to setup the system context, but please let me know if it looks like anything can be removed.
I think someone from net folks should take a look. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:25: ApplicationContext* GetApplicationContext(); Should this be a singleton from base/memory/singleton.h? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:27: class ApplicationContext { Please add comments https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:29: ApplicationContext(); Should constructor/destructor be protected? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:23: base::FilePath ApplicationContextImpl::LocalStatePath() { Do you want to reorder the method to follow the order from header? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:57: web_view_io_thread_.reset(new WebViewIOThread(GetLocalState(), GetNetLog())); s/new/MakeUnique ? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:75: // object being NULL is considered synonymous with the IO thread nit: s/NULL/null https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:83: CreateLocalState(); Do you want to fold |CreateLocalState| into this method? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:123: scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple); s/new/MakeUnique ? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:31: // Most cleanup is done by these functions, driven from WebViewWebMainParts Please explain what these methods do, instead of where they are currently called. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:32: // rather than in the destructor, so that we can interleave cleanup with nit: Avoid "we" in the comments (go/avoidwe) https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:45: static base::FilePath LocalStatePath(); GetLocalStatePath ? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:50: // Create the local state. This comment simply repeats the method name. Would it be bette to explain what "Local State" is? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:62: bool created_local_state_; Why do we need a boolean? Can we check if local_state_ is not null instead? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... File ios/web_view/internal/app/web_view_io_thread.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... ios/web_view/internal/app/web_view_io_thread.h:4: Why we did not need this large file before, but need it now? Is this a code duplication? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... File ios/web_view/internal/cwv_web_view_configuration.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... ios/web_view/internal/cwv_web_view_configuration.mm:64: // Initialize translate. Is this something that can be deferred? Can this affect startup performance?
https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:27: class ApplicationContext { Shouldn't this class be inside a namespace? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:51: tracked_objects::ThreadData::EnsureCleanupWasCalled(4); Why is this 4? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:137: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), Any reason to cap this value with 99? Maybe define a constant for 99? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/web_v... File ios/web_view/internal/web_view_web_main_parts.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/web_v... ios/web_view/internal/web_view_web_main_parts.mm:44: PrefService* local_state = application_context_->GetLocalState(); Optional: Can be in one line: DCHECK(application_context_->GetLocalState());
michaeldo@chromium.org changed reviewers: + mmenke@chromium.org, sdefresne@chromium.org
+ sdefresne@ as components owner. I also agree about someone from net looking. + mmenke@, please feel free to re-direct if you'd like. This is the reasoning behind the CL you looked at earlier. (https://codereview.chromium.org/2888583002) https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:25: ApplicationContext* GetApplicationContext(); On 2017/05/18 23:54:54, Eugene But wrote: > Should this be a singleton from base/memory/singleton.h? This is not a singleton in chrome/browser/io_thread.h or ios/chrome/browser/ios_chrome_io_thread.h. I think it is not a singleton of that form because it can't be created automatically on first call because of the two required parameters (command_line and locale). The "singleton" getter is actually initialized when an instance of the object is created. PTAL at SetApplicationContext(this) inside application_context_impl.cc. This seems like a very odd pattern and I'll look to see if I can do something else. Maybe those required properties could be set statically on ApplicationContextImpl then the standard singleton pattern could be used? However, that may go against style guide restrictions on global vars. Note I've added a note about this inside application_context_impl.h as part of the class comment. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:27: class ApplicationContext { On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > Shouldn't this class be inside a namespace? I think so? While many other classes are namespaced, the ApplicationContext is not namespaced on other platforms. However, I can add the namespace and everything still seems ok, so yes I think the namespace is better to have unless anyone knows why it should be excluded. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:27: class ApplicationContext { On 2017/05/18 23:54:54, Eugene But wrote: > Please add comments Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:29: ApplicationContext(); On 2017/05/18 23:54:54, Eugene But wrote: > Should constructor/destructor be protected? Makes sense, Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:31: // Most cleanup is done by these functions, driven from WebViewWebMainParts On 2017/05/18 23:54:55, Eugene But wrote: > Please explain what these methods do, instead of where they are currently > called. Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:32: // rather than in the destructor, so that we can interleave cleanup with On 2017/05/18 23:54:55, Eugene But wrote: > nit: Avoid "we" in the comments (go/avoidwe) Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:45: static base::FilePath LocalStatePath(); On 2017/05/18 23:54:55, Eugene But wrote: > GetLocalStatePath ? Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:50: // Create the local state. On 2017/05/18 23:54:55, Eugene But wrote: > This comment simply repeats the method name. Would it be bette to explain what > "Local State" is? Updated comment here and in application_context.h explaining local state concept as preferences. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... File ios/web_view/internal/app/web_view_io_thread.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... ios/web_view/internal/app/web_view_io_thread.h:4: On 2017/05/18 23:54:55, Eugene But wrote: > Why we did not need this large file before, but need it now? Is this a code > duplication? This class is the owner of the system url request context (mimicking chrome) which I think is OK. It ties these objects to the IO thread so I didn't want to re-invent a different architecture. I also believe all the things here are used in some way throughout the creation and configuration of the system url request context. It may be possible to remove some of these items, but we should make concious decisions to do so to ensure we don't lose functionality as a result. You're right there is a lot of shared logic with ios_chrome_io_thread.h There are some chrome specific things in that class so they are not exactly the same but they could likely be merged together. Where could such a parent class that depends on so many external things live? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... File ios/web_view/internal/cwv_web_view_configuration.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... ios/web_view/internal/cwv_web_view_configuration.mm:64: // Initialize translate. On 2017/05/18 23:54:55, Eugene But wrote: > Is this something that can be deferred? Can this affect startup performance? This could affect startup performance depending on how the embedder is using the Framework. (It happens before ios_web_view_shell is done starting up because in viewDidLoad, the VC creates the web_view.) However, I don't know if we can delay it's creation. Maybe we could provide API to disable translate altogether which would likely allow us to not create this at all. I think for now this needs to happen early until we can do a more in depth investigation of how and where it could be moved to. (ios/chrome actually initializes this in IOSChromeMainParts::PreMainMessageLoopRun and I think in a followup it would be good to move it there for ios/web_view as well. Note this is crbug.com/723869. However this will still match the order and timing of how things are run now very closely.) https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/web_v... File ios/web_view/internal/web_view_web_main_parts.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/web_v... ios/web_view/internal/web_view_web_main_parts.mm:44: PrefService* local_state = application_context_->GetLocalState(); On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > Optional: Can be in one line: > > DCHECK(application_context_->GetLocalState()); Done.
https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:25: ApplicationContext* GetApplicationContext(); On 2017/05/19 22:02:35, michaeldo wrote: > On 2017/05/18 23:54:54, Eugene But wrote: > > Should this be a singleton from base/memory/singleton.h? > > This is not a singleton in chrome/browser/io_thread.h or > ios/chrome/browser/ios_chrome_io_thread.h. > > I think it is not a singleton of that form because it can't be created > automatically on first call because of the two required parameters (command_line > and locale). The "singleton" getter is actually initialized when an instance of > the object is created. PTAL at SetApplicationContext(this) inside > application_context_impl.cc. This seems like a very odd pattern and I'll look to > see if I can do something else. Maybe those required properties could be set > statically on ApplicationContextImpl then the standard singleton pattern could > be used? However, that may go against style guide restrictions on global vars. > > Note I've added a note about this inside application_context_impl.h as part of > the class comment. If this interface should not be singleton, then maybe GetApplicationContext and SetApplicationContext should not be a part of it? As it stands it looks like a singleton, but does not use base/memory/singleton. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... File ios/web_view/internal/app/web_view_io_thread.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... ios/web_view/internal/app/web_view_io_thread.h:4: On 2017/05/19 22:02:36, michaeldo wrote: > On 2017/05/18 23:54:55, Eugene But wrote: > > Why we did not need this large file before, but need it now? Is this a code > > duplication? > > This class is the owner of the system url request context (mimicking chrome) > which I think is OK. It ties these objects to the IO thread so I didn't want to > re-invent a different architecture. I also believe all the things here are used > in some way throughout the creation and configuration of the system url request > context. It may be possible to remove some of these items, but we should make > concious decisions to do so to ensure we don't lose functionality as a result. > > You're right there is a lot of shared logic with ios_chrome_io_thread.h There > are some chrome specific things in that class so they are not exactly the same > but they could likely be merged together. Where could such a parent class that > depends on so many external things live? We did not have this class before, and it's unclear from CL description why is it needed now. So that was my main question. If there is a lot of shared logic, then how about sharing that logic via components layer? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... File ios/web_view/internal/cwv_web_view_configuration.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... ios/web_view/internal/cwv_web_view_configuration.mm:64: // Initialize translate. On 2017/05/19 22:02:36, michaeldo wrote: > On 2017/05/18 23:54:55, Eugene But wrote: > > Is this something that can be deferred? Can this affect startup performance? > > This could affect startup performance depending on how the embedder is using the > Framework. (It happens before ios_web_view_shell is done starting up because in > viewDidLoad, the VC creates the web_view.) However, I don't know if we can delay > it's creation. Maybe we could provide API to disable translate altogether which > would likely allow us to not create this at all. I think for now this needs to > happen early until we can do a more in depth investigation of how and where it > could be moved to. > > (ios/chrome actually initializes this in > IOSChromeMainParts::PreMainMessageLoopRun and I think in a followup it would be > good to move it there for ios/web_view as well. Note this is crbug.com/723869. > However this will still match the order and timing of how things are run now > very closely.) Can we measure how much time does it take to execute the code? Translation does not work until the keys are set. So maybe this can be done after setting API keys?
https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... File ios/web_view/internal/app/web_view_io_thread.mm (right): https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... ios/web_view/internal/app/web_view_io_thread.mm:371: /*is_quic_force_enabled=*/false, quic_user_agent_id, ¶ms_); I guess this is modelled after IOThread, or the iOS equivalent? Is this going to live happily in tandem with ios_chrome_io_thread.mm, or are they both going to be sticking around for the foreseeable future? I'm currently working on getting IOThread using URLRequestContextBuilder, which should (hopefully) make for much cleaner initialization. Not quite far enough along for you use it now, unfortunately, but hopefully only a week or two away. Up to you if you want to hold off or not. Not sure how likely I am to update iOS code when I'm done (Though this call will need to be updated, at least). I'm certainly not going to update two iOS IOThread variants.
https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context.h:25: ApplicationContext* GetApplicationContext(); On 2017/05/19 23:32:05, Eugene But wrote: > On 2017/05/19 22:02:35, michaeldo wrote: > > On 2017/05/18 23:54:54, Eugene But wrote: > > > Should this be a singleton from base/memory/singleton.h? > > > > This is not a singleton in chrome/browser/io_thread.h or > > ios/chrome/browser/ios_chrome_io_thread.h. > > > > I think it is not a singleton of that form because it can't be created > > automatically on first call because of the two required parameters > (command_line > > and locale). The "singleton" getter is actually initialized when an instance > of > > the object is created. PTAL at SetApplicationContext(this) inside > > application_context_impl.cc. This seems like a very odd pattern and I'll look > to > > see if I can do something else. Maybe those required properties could be set > > statically on ApplicationContextImpl then the standard singleton pattern could > > be used? However, that may go against style guide restrictions on global vars. > > > > Note I've added a note about this inside application_context_impl.h as part of > > the class comment. > If this interface should not be singleton, then maybe GetApplicationContext and > SetApplicationContext should not be a part of it? As it stands it looks like a > singleton, but does not use base/memory/singleton. I took another look at this and was able to make it a real singleton!!! PTAL https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:23: base::FilePath ApplicationContextImpl::LocalStatePath() { On 2017/05/18 23:54:55, Eugene But wrote: > Do you want to reorder the method to follow the order from header? Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:51: tracked_objects::ThreadData::EnsureCleanupWasCalled(4); On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > Why is this 4? The 4 is for the threads that get spun up, but the value is never used (has been unsused for 5.5 years ever since it was added). Other platforms use 4 here, but I think the parameter should be deleted or the method needs to be re-enabled as stated in the comments "until it is working on XP". The bug referenced in the below CL (crbug.com/103209)is marked fixed even though the code in this method has never been executed. I'll ping an owner of //base about this. https://codereview.chromium.org/8606001 https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?t... https://cs.chromium.org/chromium/src/base/tracked_objects.cc?l=939&gsn=Ensure... https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:57: web_view_io_thread_.reset(new WebViewIOThread(GetLocalState(), GetNetLog())); On 2017/05/18 23:54:55, Eugene But wrote: > s/new/MakeUnique ? Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:75: // object being NULL is considered synonymous with the IO thread On 2017/05/18 23:54:55, Eugene But wrote: > nit: s/NULL/null Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:83: CreateLocalState(); On 2017/05/18 23:54:54, Eugene But wrote: > Do you want to fold |CreateLocalState| into this method? Yes, I had the same thought. Done. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:123: scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple); On 2017/05/18 23:54:55, Eugene But wrote: > s/new/MakeUnique ? I can't use MakeUnique because the destructor is protected. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:137: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > Any reason to cap this value with 99? Maybe define a constant for 99? No reason except consistency, which I agree is a bad reason. I think don't think a constant helps readability here because I don't have any good name for the value and my naming could end up being misleading to the real meaning of the value. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.h:62: bool created_local_state_; On 2017/05/18 23:54:55, Eugene But wrote: > Why do we need a boolean? Can we check if local_state_ is not null instead? After looking into it, I removed the bool as it isn't necessary. (Especially with the DCHECK that validates !local_state) I'm not sure why it existed in other ApplicationContextImpl. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... File ios/web_view/internal/app/web_view_io_thread.h (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/w... ios/web_view/internal/app/web_view_io_thread.h:4: On 2017/05/19 23:32:05, Eugene But wrote: > On 2017/05/19 22:02:36, michaeldo wrote: > > On 2017/05/18 23:54:55, Eugene But wrote: > > > Why we did not need this large file before, but need it now? Is this a code > > > duplication? > > > > This class is the owner of the system url request context (mimicking chrome) > > which I think is OK. It ties these objects to the IO thread so I didn't want > to > > re-invent a different architecture. I also believe all the things here are > used > > in some way throughout the creation and configuration of the system url > request > > context. It may be possible to remove some of these items, but we should make > > concious decisions to do so to ensure we don't lose functionality as a result. > > > > > You're right there is a lot of shared logic with ios_chrome_io_thread.h There > > are some chrome specific things in that class so they are not exactly the same > > but they could likely be merged together. Where could such a parent class that > > depends on so many external things live? > We did not have this class before, and it's unclear from CL description why is > it needed now. So that was my main question. > > If there is a lot of shared logic, then how about sharing that logic via > components layer? I updated the CL description here. I agree that this should be merged. I added a TODO below to crbug.com/725524 and will move it to components or somewhere better in another CL to avoid bloating this one. https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... File ios/web_view/internal/cwv_web_view_configuration.mm (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/cwv_w... ios/web_view/internal/cwv_web_view_configuration.mm:64: // Initialize translate. On 2017/05/19 23:32:05, Eugene But wrote: > On 2017/05/19 22:02:36, michaeldo wrote: > > On 2017/05/18 23:54:55, Eugene But wrote: > > > Is this something that can be deferred? Can this affect startup performance? > > > > This could affect startup performance depending on how the embedder is using > the > > Framework. (It happens before ios_web_view_shell is done starting up because > in > > viewDidLoad, the VC creates the web_view.) However, I don't know if we can > delay > > it's creation. Maybe we could provide API to disable translate altogether > which > > would likely allow us to not create this at all. I think for now this needs to > > happen early until we can do a more in depth investigation of how and where it > > could be moved to. > > > > (ios/chrome actually initializes this in > > IOSChromeMainParts::PreMainMessageLoopRun and I think in a followup it would > be > > good to move it there for ios/web_view as well. Note this is crbug.com/723869. > > However this will still match the order and timing of how things are run now > > very closely.) > Can we measure how much time does it take to execute the code? Translation does > not work until the keys are set. So maybe this can be done after setting API > keys? On my iPhone 7 this code takes about half a millisecond to execute the first time and only around 0.06 milliseconds on subsequent runs. I ran multiple times and measured with base::TimeTicks. I don't want to over-optimize this if it isn't necessary. However, I agree we should delay this if possible, especially if translate will never be used by the client. Do you think this can wait?
Description was changed from ========== Initialize ios/web_view translate with a system-wide URLRequestContext. BUG=679895, 710948 ========== to ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread which mimics IOSChromeIOThread and stores the application global objects exposed through the ApplicationContext singleton. The WebView and Chrome versions of the IOThread are quite similar on iOS and should be merged once there is a shared location for such code. (crbug.com/723869) BUG=679895, 710948 ==========
On 2017/05/22 17:14:58, mmenke wrote: > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > File ios/web_view/internal/app/web_view_io_thread.mm (right): > > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > ios/web_view/internal/app/web_view_io_thread.mm:371: > /*is_quic_force_enabled=*/false, quic_user_agent_id, ¶ms_); > I guess this is modelled after IOThread, or the iOS equivalent? Is this going > to live happily in tandem with ios_chrome_io_thread.mm, or are they both going > to be sticking around for the foreseeable future? > > I'm currently working on getting IOThread using URLRequestContextBuilder, which > should (hopefully) make for much cleaner initialization. Not quite far enough > along for you use it now, unfortunately, but hopefully only a week or two away. > Up to you if you want to hold off or not. Not sure how likely I am to update > iOS code when I'm done (Though this call will need to be updated, at least). > I'm certainly not going to update two iOS IOThread variants. Yes it is modeled after IOSChromeIOThread. They will hopefully be merged very soon, but since there isn't a good place to share code right now, I'll need to create a superclass (something like IOSIOThread) and put that in components. At that point, most of the logic will exist there, but some specific parts will exist here still. Thanks for the heads up, do you have a CL in progress now that I can take a look at? I definitely understand not wanting to update the (almost) same code twice. Maybe the new version could simply live in this "shared components" location and then I can convert/help convert this class and the ios/chrome version to use that new shared instance instead?
Description was changed from ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread which mimics IOSChromeIOThread and stores the application global objects exposed through the ApplicationContext singleton. The WebView and Chrome versions of the IOThread are quite similar on iOS and should be merged once there is a shared location for such code. (crbug.com/723869) BUG=679895, 710948 ========== to ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread which mimics IOSChromeIOThread and stores the application global objects exposed through the ApplicationContext singleton. The WebView and Chrome versions of the IOThread are quite similar on iOS and should be merged once there is a shared location for such code. (crbug.com/723869) BUG=679895, 710948 ==========
https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:51: tracked_objects::ThreadData::EnsureCleanupWasCalled(4); On 2017/05/23 22:38:36, michaeldo wrote: > On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > > Why is this 4? > > The 4 is for the threads that get spun up, but the value is never used (has been > unsused for 5.5 years ever since it was added). Other platforms use 4 here, but > I think the parameter should be deleted or the method needs to be re-enabled as > stated in the comments "until it is working on XP". > > The bug referenced in the below CL (crbug.com/103209)is marked fixed even though > the code in this method has never been executed. I'll ping an owner of //base > about this. > > https://codereview.chromium.org/8606001 > > https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?t... > > https://cs.chromium.org/chromium/src/base/tracked_objects.cc?l=939&gsn=Ensure... Thanks! Feel free to go ahead without waiting for the cleanup, but can you add a comment saying this 4 is not used? https://codereview.chromium.org/2894483003/diff/1/ios/web_view/internal/app/a... ios/web_view/internal/app/application_context_impl.cc:137: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/05/23 22:38:36, michaeldo wrote: > On 2017/05/19 02:37:51, Hiroshi Ichikawa wrote: > > Any reason to cap this value with 99? Maybe define a constant for 99? > > No reason except consistency, which I agree is a bad reason. I think don't think > a constant helps readability here because I don't have any good name for the > value and my naming could end up being misleading to the real meaning of the > value. Can you maybe just leave a comment saying this is inherited from somewhere else?
On 2017/05/23 22:48:44, michaeldo wrote: > On 2017/05/22 17:14:58, mmenke wrote: > > > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > > File ios/web_view/internal/app/web_view_io_thread.mm (right): > > > > > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > > ios/web_view/internal/app/web_view_io_thread.mm:371: > > /*is_quic_force_enabled=*/false, quic_user_agent_id, ¶ms_); > > I guess this is modelled after IOThread, or the iOS equivalent? Is this going > > to live happily in tandem with ios_chrome_io_thread.mm, or are they both going > > to be sticking around for the foreseeable future? > > > > I'm currently working on getting IOThread using URLRequestContextBuilder, > which > > should (hopefully) make for much cleaner initialization. Not quite far enough > > along for you use it now, unfortunately, but hopefully only a week or two > away. > > Up to you if you want to hold off or not. Not sure how likely I am to update > > iOS code when I'm done (Though this call will need to be updated, at least). > > I'm certainly not going to update two iOS IOThread variants. > > Yes it is modeled after IOSChromeIOThread. They will hopefully be merged very > soon, but since there isn't a good place to share code right now, I'll need to > create a superclass (something like IOSIOThread) and put that in components. At > that point, most of the logic will exist there, but some specific parts will > exist here still. > > Thanks for the heads up, do you have a CL in progress now that I can take a look > at? I definitely understand not wanting to update the (almost) same code twice. > Maybe the new version could simply live in this "shared components" location and > then I can convert/help convert this class and the ios/chrome version to use > that new shared instance instead? Unfortunately, this is a very incremental process with dozens of CLs, many not too relevant (Rework proxy config classes to be able to be created before IOThread construction, make a URLRequestContextBuilder subclass that supports V8 and Mojo proxy resolution). Others are more relevant, but due to sheer quantity of initialization code, I'm only doing a few things per CL (Splitting HttpNetworkSession::Params into two classes, one to pass to the URLRequestContextBuilder, one to be populated by the builder), etc.
On 2017/05/24 15:13:01, mmenke wrote: > On 2017/05/23 22:48:44, michaeldo wrote: > > On 2017/05/22 17:14:58, mmenke wrote: > > > > > > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > > > File ios/web_view/internal/app/web_view_io_thread.mm (right): > > > > > > > > > https://codereview.chromium.org/2894483003/diff/20001/ios/web_view/internal/a... > > > ios/web_view/internal/app/web_view_io_thread.mm:371: > > > /*is_quic_force_enabled=*/false, quic_user_agent_id, ¶ms_); > > > I guess this is modelled after IOThread, or the iOS equivalent? Is this > going > > > to live happily in tandem with ios_chrome_io_thread.mm, or are they both > going > > > to be sticking around for the foreseeable future? > > > > > > I'm currently working on getting IOThread using URLRequestContextBuilder, > > which > > > should (hopefully) make for much cleaner initialization. Not quite far > enough > > > along for you use it now, unfortunately, but hopefully only a week or two > > away. > > > Up to you if you want to hold off or not. Not sure how likely I am to > update > > > iOS code when I'm done (Though this call will need to be updated, at least). > > > > I'm certainly not going to update two iOS IOThread variants. > > > > Yes it is modeled after IOSChromeIOThread. They will hopefully be merged very > > soon, but since there isn't a good place to share code right now, I'll need to > > create a superclass (something like IOSIOThread) and put that in components. > At > > that point, most of the logic will exist there, but some specific parts will > > exist here still. > > > > Thanks for the heads up, do you have a CL in progress now that I can take a > look > > at? I definitely understand not wanting to update the (almost) same code > twice. > > Maybe the new version could simply live in this "shared components" location > and > > then I can convert/help convert this class and the ios/chrome version to use > > that new shared instance instead? > > Unfortunately, this is a very incremental process with dozens of CLs, many not > too relevant (Rework proxy config classes to be able to be created before > IOThread construction, make a URLRequestContextBuilder subclass that supports V8 > and Mojo proxy resolution). Others are more relevant, but due to sheer quantity > of initialization code, I'm only doing a few things per CL (Splitting > HttpNetworkSession::Params into two classes, one to pass to the > URLRequestContextBuilder, one to be populated by the builder), etc. Not a problem, thank you for the explanation. That process definitely makes sense. I will be merging the IOThread classes from ios/chrome and this new one for ios/web_view into a single class before landing this CL. I will update with a new patch soon.
I've moved IOThread to components/ios in this CL. However, I'm creating a separate CL to move the ios/chrome version to components/ios first and then I'll rebase this CL on top of that one. Please hold off on in-depth comments on the classes inside ios/components (as they'll be reveiewed and updated in the other CL which I haven't uploaded yet). That said, PTAL at the changes inside ios/web_view.
https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... ios/web_view/internal/app/application_context_impl.cc:139: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), Do you want to use local variables to improve the formatting? https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... File ios/web_view/internal/app/application_context_impl.h (right): https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... ios/web_view/internal/app/application_context_impl.h:21: class ApplicationContextImpl : public ApplicationContext { What is the reason in separating ApplicationContext and ApplicationContextImpl?
PTAL now at the latest patch as well as the dependent CL! https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... File ios/web_view/internal/app/application_context_impl.cc (right): https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... ios/web_view/internal/app/application_context_impl.cc:139: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/05/25 00:39:11, Eugene But wrote: > Do you want to use local variables to improve the formatting? I tried to make this more readable. This was an extra version of this file, so the change is in GetLocalState() inside applciation_context.cc https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... File ios/web_view/internal/app/application_context_impl.h (right): https://codereview.chromium.org/2894483003/diff/80001/ios/web_view/internal/a... ios/web_view/internal/app/application_context_impl.h:21: class ApplicationContextImpl : public ApplicationContext { On 2017/05/25 00:39:11, Eugene But wrote: > What is the reason in separating ApplicationContext and ApplicationContextImpl? No reason any longer. I removed the _impl class from the BUILD.gn file but forgot to delete the files :)
One note: I may also be able to move application_context to ios/componenets so please stand by on this change, but take a look at the dependent CL and if that looks good it will help me move faster on getting application_context in there too
On 2017/05/25 22:06:07, michaeldo wrote: > One note: I may also be able to move application_context to ios/componenets so > please stand by on this change, but take a look at the dependent CL and if that > looks good it will help me move faster on getting application_context in there > too Regarding sharing application_context, this is unfortunately not as easy as I had hoped. It may be too ambitious to also share the application_context. We also made some changes here to application_context that clean up this CL, but make it more difficult to share. In order to share application_context, we would need to go back to this version not being a true singleton or convert ios/chrome's version to a singleton. We will also need to update TestingApplicationContext which is used for all the tests within ios_chrome_unittests. What does everyone think about leaving this Application Context (and possibly work towards improving ios/chrome's before we combine them)?
On 2017/05/26 16:30:15, michaeldo wrote: > On 2017/05/25 22:06:07, michaeldo wrote: > > One note: I may also be able to move application_context to ios/componenets so > > please stand by on this change, but take a look at the dependent CL and if > that > > looks good it will help me move faster on getting application_context in there > > too > > Regarding sharing application_context, this is unfortunately not as easy as I > had hoped. It may be too ambitious to also share the application_context. > > We also made some changes here to application_context that clean up this CL, but > make it more difficult to share. In order to share application_context, we would > need to go back to this version not being a true singleton or convert > ios/chrome's version to a singleton. We will also need to update > TestingApplicationContext which is used for all the tests within > ios_chrome_unittests. > > What does everyone think about leaving this Application Context (and possibly > work towards improving ios/chrome's before we combine them)? I'm fine with duplicating Application Context. There is not much code and logic there.
Description was changed from ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread which mimics IOSChromeIOThread and stores the application global objects exposed through the ApplicationContext singleton. The WebView and Chrome versions of the IOThread are quite similar on iOS and should be merged once there is a shared location for such code. (crbug.com/723869) BUG=679895, 710948 ========== to ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread and ApplicationContext singleton which manage global application state and WebViewTranslateService which manages the translate lifecycle. BUG=679895, 710948 ==========
PTAL at this CL now. I've rebased it on top of the ios/components/io_thread CL and I've added a WebViewTranslateService class which manages the translate lifecycle.
Description was changed from ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread and ApplicationContext singleton which manage global application state and WebViewTranslateService which manages the translate lifecycle. BUG=679895, 710948 ========== to ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread and ApplicationContext singleton which manage global application state and WebViewTranslateService which manages the translate lifecycle. BUG=679895, 710948 ==========
https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:22: ~TranslateRequestsAllowedListener(){}; Remove trailing semi-colon. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:43: new TranslateRequestsAllowedListener()){}; Use "base::MakeUnique<>" here. Remove trailing semi-colon. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.h (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:33: // NOTE: This must be a separate class from WebViewTranslateService becuase becuase -> because https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:34: // base/memory/singleton.h won't see that the OnResourceRequestsAllowed I don't understand this comment at all. Is this because you want the method OnResourceRequestsAllowed to be invoked in the constructor/destructor? Then yes, it is not possible to do that, but this is not a problem with base/memory/singleton.h, just how C++ virtual methods are implemented. Since we have many singleton classes that do implements virtual base class (random example: all the KeyedService factories), I do not think you *need* a separate class, unless you are trying to call the virtual method from the destructor/constructor, which you do not appear to be doing (at least not in a different way in TranslateRequestsAllowedListener than it would be done in WebViewTranslateService AFAICT). Maybe the problem is that web_resource::ResourceRequestAllowedNotifier::Observer destructor is not virtual. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:40: ~TranslateRequestsAllowedListener(); This destructor should be virtual in the base class :-/ https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:59: std::unique_ptr<TranslateRequestsAllowedListener> You can change this to "std::unique_ptr<web_resource::ResourceRequestAllowedNotifier::Observer> translate_requests_allowed_listener_;" and move the declaration of the class TranslateRequestsAllowedListener to the implementation file. But I've just seen that the destructor in the base class is not virtual ... That is bad. Or you can also change this to "TranslateRequestsAllowedListener translate_requests_allowed_listener_;" to avoid a separate allocation.
lgtm https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:46: tracked_objects::ThreadData::EnsureCleanupWasCalled(4); Could you please create a constant for "4" with explanation why it is 4. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:58: if (local_state_) { Should we create local_state_ instead of having no-op behavior? If no, could you please clarify the reason in the comments. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:95: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), Do we know why we limit to 99 sockets? https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:50: // Creates state tied to application threads. When and why this should be called? Please specify in the comments https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:53: // Saves applicaiton context state. ditto https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:56: // Destroys state tied to application threads. ditto https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:17: nullptr) { nit: /*name_of_the_argument=*/nullptr https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:29: NOTREACHED(); This should be either NOTREACHED or early return, not both: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
I don't think you need me on this one any more, removing myself as a reviewer. If I missed something, please feel free to add me back, and tell me what I should be looking at. And thanks for taking the time to make this share io_thread code!
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:46: tracked_objects::ThreadData::EnsureCleanupWasCalled(4); On 2017/05/31 16:25:33, Eugene But wrote: > Could you please create a constant for "4" with explanation why it is 4. I deleted this instead. There is no sense in adding another call to this method as it is dead code. (I asked //base owners and got one agreement so I don't think we need this in here at all.) I will remove other calls like this if I get a chance. In the meantime I filed crbug.com/728299. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:58: if (local_state_) { On 2017/05/31 16:25:33, Eugene But wrote: > Should we create local_state_ instead of having no-op behavior? If no, could you > please clarify the reason in the comments. I don't think so. No reason to create Local state as there won't be any pending writes. I've updated the comment in the header. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:95: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/05/31 16:25:34, Eugene But wrote: > Do we know why we limit to 99 sockets? Unfortunately, I have no idea. I don't think it's beneficial for me to guess, but I will look through commit history to see if I can find an original reason for the constant. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:50: // Creates state tied to application threads. On 2017/05/31 16:25:34, Eugene But wrote: > When and why this should be called? Please specify in the comments Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:53: // Saves applicaiton context state. On 2017/05/31 16:25:34, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:56: // Destroys state tied to application threads. On 2017/05/31 16:25:34, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:17: nullptr) { On 2017/05/31 16:25:34, Eugene But wrote: > nit: /*name_of_the_argument=*/nullptr Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:22: ~TranslateRequestsAllowedListener(){}; On 2017/05/31 08:52:46, sdefresne wrote: > Remove trailing semi-colon. Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:29: NOTREACHED(); On 2017/05/31 16:25:34, Eugene But wrote: > This should be either NOTREACHED or early return, not both: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.cc:43: new TranslateRequestsAllowedListener()){}; On 2017/05/31 08:52:46, sdefresne wrote: > Use "base::MakeUnique<>" here. > > Remove trailing semi-colon. Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.h (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:33: // NOTE: This must be a separate class from WebViewTranslateService becuase On 2017/05/31 08:52:46, sdefresne wrote: > becuase -> because Done. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:34: // base/memory/singleton.h won't see that the OnResourceRequestsAllowed On 2017/05/31 08:52:46, sdefresne wrote: > I don't understand this comment at all. Is this because you want the method > OnResourceRequestsAllowed to be invoked in the constructor/destructor? Then yes, > it is not possible to do that, but this is not a problem with > base/memory/singleton.h, just how C++ virtual methods are implemented. > > Since we have many singleton classes that do implements virtual base class > (random example: all the KeyedService factories), I do not think you *need* a > separate class, unless you are trying to call the virtual method from the > destructor/constructor, which you do not appear to be doing (at least not in a > different way in TranslateRequestsAllowedListener than it would be done in > WebViewTranslateService AFAICT). > > Maybe the problem is that web_resource::ResourceRequestAllowedNotifier::Observer > destructor is not virtual. I updated the comment. IIUC then we need to fix the Observer class? A brief search for "class Observer {" shows that many of them do not have virtual destructors. Should they all? I looked into this a little and it seems this could lead to undefined deconstructor behavior and potential leaks!? https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:40: ~TranslateRequestsAllowedListener(); On 2017/05/31 08:52:46, sdefresne wrote: > This destructor should be virtual in the base class :-/ Acknowledged. https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:59: std::unique_ptr<TranslateRequestsAllowedListener> On 2017/05/31 08:52:46, sdefresne wrote: > You can change this to > "std::unique_ptr<web_resource::ResourceRequestAllowedNotifier::Observer> > translate_requests_allowed_listener_;" and move the declaration of the class > TranslateRequestsAllowedListener to the implementation file. But I've just seen > that the destructor in the base class is not virtual ... That is bad. > > Or you can also change this to "TranslateRequestsAllowedListener > translate_requests_allowed_listener_;" to avoid a separate allocation. I used your second suggestion.
On 2017/05/31 19:33:33, mmenke wrote: > I don't think you need me on this one any more, removing myself as a reviewer. > If I missed something, please feel free to add me back, and tell me what I > should be looking at. And thanks for taking the time to make this share > io_thread code! I agree I'm all set with sdefresne's approval for DEPS. Thank you for looking and providing feedback, I'm also glad we came up with a shared location!
lgtm https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:95: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/05/31 21:03:42, michaeldo wrote: > On 2017/05/31 16:25:34, Eugene But wrote: > > Do we know why we limit to 99 sockets? > > Unfortunately, I have no idea. I don't think it's beneficial for me to guess, > but I will look through commit history to see if I can find an original reason > for the constant. Can you at least add a comment which points to where this comes from (i.e., the original code location)?
https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.h (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:34: // base/memory/singleton.h won't see that the OnResourceRequestsAllowed On 2017/05/31 21:03:43, michaeldo wrote: > On 2017/05/31 08:52:46, sdefresne wrote: > > I don't understand this comment at all. Is this because you want the method > > OnResourceRequestsAllowed to be invoked in the constructor/destructor? Then > yes, > > it is not possible to do that, but this is not a problem with > > base/memory/singleton.h, just how C++ virtual methods are implemented. > > > > Since we have many singleton classes that do implements virtual base class > > (random example: all the KeyedService factories), I do not think you *need* a > > separate class, unless you are trying to call the virtual method from the > > destructor/constructor, which you do not appear to be doing (at least not in a > > different way in TranslateRequestsAllowedListener than it would be done in > > WebViewTranslateService AFAICT). > > > > Maybe the problem is that > web_resource::ResourceRequestAllowedNotifier::Observer > > destructor is not virtual. > > I updated the comment. IIUC then we need to fix the Observer class? A brief > search for "class Observer {" shows that many of them do not have virtual > destructors. Should they all? I looked into this a little and it seems this > could lead to undefined deconstructor behavior and potential leaks!? If a class is written to be sub-classed and it is possible to delete an instance through a pointer to base class (i.e. the destructor is public), then the base class destructor needs to be virtual otherwise the result is undefined behaviour. So Observer class destructor needs to be changed to be virtual or protected. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.h (right): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:33: // NOTE: This currently must be a separate class from WebViewTranslateService Can you create a bug to track fixing the destructor and add TODO(crbug.com/xxx) here? https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:59: std::unique_ptr<TranslateRequestsAllowedListener> You don't need a std::unique_ptr<> here, you can just have a instance of the correct type directly: TranslateRequestsAllowedListener translate_requests_allowed_listener_; IMO, I would only use a pointer here if I want to either: 1. allow to have the pointer points to different object or allow it to be null, 2. if I want to only forward-declare the type and not include the header. Since 2. is not true (as the type is defined a few lines before) and 1. is not true either (as the object is constructed in WebViewTranslateService constructor and only deallocated in the destructor), I think it is best to not use a pointer and reduce the number of separate allocations. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/web_view_web_client.mm (left): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/web_view_web_client.mm:25: web_main_parts_ = new WebViewWebMainParts(); This is obsoleted by https://chromium-review.googlesource.com/c/519263/. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/web_view_web_main_parts.mm (right): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/web_view_web_main_parts.mm:38: DCHECK(ApplicationContext::GetInstance()->GetLocalState()); Code in DCHECK is not invoked in official builds. If you want to ensure the local state is allocated, you need to make this call outside of DCHECK: // Initialize local state. PrefService* local_state = ApplicationContext::GetInstance()->GetLocalState(); DCHECK(local_state);
https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.cc (right): https://codereview.chromium.org/2894483003/diff/170001/ios/web_view/internal/... ios/web_view/internal/app/application_context.cc:95: std::max(std::min<int>(net::kDefaultMaxSocketsPerProxyServer, 99), On 2017/06/01 02:25:59, Hiroshi Ichikawa wrote: > On 2017/05/31 21:03:42, michaeldo wrote: > > On 2017/05/31 16:25:34, Eugene But wrote: > > > Do we know why we limit to 99 sockets? > > > > Unfortunately, I have no idea. I don't think it's beneficial for me to guess, > > but I will look through commit history to see if I can find an original reason > > for the constant. > > Can you at least add a comment which points to where this comes from (i.e., the > original code location)? I started to add a comment, but then looked into this more and simply removed the 99 and the std::min. There is a DCHECK inside |set_max_sockets_per_proxy_server| if the |socket_count| is greater than 100 so I don't think this 99 is necessary. Also, net::kDefaultMaxSocketsPerProxyServer hasn't changed in 6 years and if it does, the internals of that class will end up being adjusted accordingly. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/translate/web_view_translate_service.h (right): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:33: // NOTE: This currently must be a separate class from WebViewTranslateService On 2017/06/01 08:36:52, sdefresne wrote: > Can you create a bug to track fixing the destructor and add TODO(crbug.com/xxx) > here? Done. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/translate/web_view_translate_service.h:59: std::unique_ptr<TranslateRequestsAllowedListener> On 2017/06/01 08:36:52, sdefresne wrote: > You don't need a std::unique_ptr<> here, you can just have a instance of the > correct type directly: > > TranslateRequestsAllowedListener translate_requests_allowed_listener_; > > IMO, I would only use a pointer here if I want to either: > 1. allow to have the pointer points to different object or allow it to be null, > 2. if I want to only forward-declare the type and not include the header. > > Since 2. is not true (as the type is defined a few lines before) and 1. is not > true either (as the object is constructed in WebViewTranslateService constructor > and only deallocated in the destructor), I think it is best to not use a pointer > and reduce the number of separate allocations. Thank you for the explanation. Done. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/web_view_web_client.mm (left): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/web_view_web_client.mm:25: web_main_parts_ = new WebViewWebMainParts(); On 2017/06/01 08:36:52, sdefresne wrote: > This is obsoleted by https://chromium-review.googlesource.com/c/519263/. Thank you for doing that and pointing it out here. https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... File ios/web_view/internal/web_view_web_main_parts.mm (right): https://codereview.chromium.org/2894483003/diff/190001/ios/web_view/internal/... ios/web_view/internal/web_view_web_main_parts.mm:38: DCHECK(ApplicationContext::GetInstance()->GetLocalState()); On 2017/06/01 08:36:53, sdefresne wrote: > Code in DCHECK is not invoked in official builds. If you want to ensure the > local state is allocated, you need to make this call outside of DCHECK: > > // Initialize local state. > PrefService* local_state = ApplicationContext::GetInstance()->GetLocalState(); > DCHECK(local_state); > Thank you for catching this! Done.
lgtm https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:54: // Saves applicaiton context state if |local_state_| exists. This should be applicaiton → application https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:63: ApplicationContext(); nit: move this below "friend struct base::DefaultSingletonTraits<ApplicationContext>;" as the style guide recommends in a given section to "group[ing] similar kinds of declarations together" and here you are mixing "friend" and "methods". https://google.github.io/styleguide/cppguide.html#Declaration_Order So, this could be: private: friend struct base::DefaultSingletonTraits<ApplicationContext>; ApplicationContext(); ~ApplicationContext(); // Gets the ChromeNetLog. net_log::ChromeNetLog* GetNetLog(); https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:79: THREAD_CHECKER(thread_checker_); optional: I think you can use a SEQUENCE_CHECKER here. The instance does not uses thread-local storage and is created before any threads are started so they should give the same result, and SEQUENCE_CHECKER is recommended.
https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... File ios/web_view/internal/app/application_context.h (right): https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:54: // Saves applicaiton context state if |local_state_| exists. This should be On 2017/06/02 09:03:51, sdefresne wrote: > applicaiton → application Done. https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:63: ApplicationContext(); On 2017/06/02 09:03:51, sdefresne wrote: > nit: move this below "friend struct > base::DefaultSingletonTraits<ApplicationContext>;" as the style guide recommends > in a given section to "group[ing] similar kinds of declarations together" and > here you are mixing "friend" and "methods". > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > So, this could be: > > private: > friend struct base::DefaultSingletonTraits<ApplicationContext>; > > ApplicationContext(); > ~ApplicationContext(); > > // Gets the ChromeNetLog. > net_log::ChromeNetLog* GetNetLog(); Done. Thank you for reference. https://codereview.chromium.org/2894483003/diff/230001/ios/web_view/internal/... ios/web_view/internal/app/application_context.h:79: THREAD_CHECKER(thread_checker_); On 2017/06/02 09:03:51, sdefresne wrote: > optional: I think you can use a SEQUENCE_CHECKER here. The instance does not > uses thread-local storage and is created before any threads are started so they > should give the same result, and SEQUENCE_CHECKER is recommended. Done.
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, ichikawa@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2894483003/#ps250001 (title: "Respond to comments.")
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": 250001, "attempt_start_ts": 1496418432709130,
"parent_rev": "f60d062aad6f02456fb50f4f5d00936d47d08030", "commit_rev":
"6c4f1da164a4c4450a9ff9d5ede739bd6f28eef5"}
Message was sent while issue was closed.
Description was changed from ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread and ApplicationContext singleton which manage global application state and WebViewTranslateService which manages the translate lifecycle. BUG=679895, 710948 ========== to ========== Initialize ios/web_view translate with a system-wide URLRequestContext. Add WebViewIOThread and ApplicationContext singleton which manage global application state and WebViewTranslateService which manages the translate lifecycle. BUG=679895, 710948 Review-Url: https://codereview.chromium.org/2894483003 Cr-Commit-Position: refs/heads/master@{#476700} Committed: https://chromium.googlesource.com/chromium/src/+/6c4f1da164a4c4450a9ff9d5ede7... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001) as https://chromium.googlesource.com/chromium/src/+/6c4f1da164a4c4450a9ff9d5ede7... |
