For now I've tried to focus on SpellCheckProvider::OnAdvanceToNextMisspelling. Unfortunately, I have not been able to come up with a repro that shows things not working in OOPIF case (most likely because I am not familiar with the spellchecking feature...).
I've also tried to see if SpellCheckProvider::OnAdvanceToNextMisspelling is covered by CQ tests, but unfortunately CL that removes body of that methods passes CQ dry run... (https://codereview.chromium.org/2235333004).
A concrete example of a problem that being a RVO causes is issue 625068 : we have to special case some code for spellcheck, since it can visit render views with no local frames at all.
SpellCheckProvider is weak referenced in WebViewImpl as a WebSpellCheckClient, and is used by the SpellCheckerClientImpl owned by the same WebViewImpl.
If we change SpellCheckProvider into a RenderFrameObserver, then it should be refererenced in a WebLocalFrameImpl instead of WebViewImpl. Then SpellCheckerClientImpl also needs to be moved to WebLocalFrameImpl.
So if we do this change, basically all renderer-side classes for spellchecking will be owned by frames. The only remaining thing owned by WebViewImpl is the current SpellCheckClientImpl::m_spellCheckThisFieldStatus, since we still want all frames in the same page to have the same spellcheck enabled status.
Making it frame-based sounds good to me. dcheng@ can probably comment on the m_spellCheckThisFieldStatus part, which I imagine might belong on something Page-related rather than View-related?
Thanks for looking into it-- hope we'll be able to get it into M59?
I think I got a better idea.
Blink has two abstract classes:
- SpellCheckerClient, which is a one-per-page class maintaining states of global spellchecking enabled and spelling UI. It doesn't handle any checking.
- TextCheckerClient passes all the checking requests to web/
However, at the web/ level, they are implemented by the same class SpellCheckerClientImpl, which communicates with SpellCheckProvider.
I think we can split SpellCheckerClientImpl into two classes to implement SpellCheckerClient and TextCheckerClient, respectively. We can also make TextCheckerClient frame-owned. Then we can do a similar split to SpellCheckProvider to two parts: an RVO and communicates with SpellCheckerClient, and an RFO that communicates with TextCheckerClient.
WDYT?
We're trying to eventually eliminate RenderViewObserver. Is there a way to avoid that part?
Otherwise it sounds reasonable to me (without knowing the spellcheck code), and I'd defer to dcheng@ on the Blink details.
I didn't notice the deprecation message of RenderView...
What's the Blink side plan about the deprecation? Are we going to do any change to WebView and WebViewClient?
I don't have a good idea to completely avoid RenderViewObserver. Anyway, since SpellCheckerProvider is already a RVO, I think it's OK to leave part of this class still as an RVO.
As we plan to fix it in M59, which is not too far away from now, I'll start the implementation as sketched in #22. Detailed plan is at: https://goo.gl/VfCENk
Can we ensure that spell checking is fixed in M59 first? We can look at completely removing RVO later (M60), if it isn't possible in the M59 timeframe.
Fixing OOPIF spell checking should be doable in M59, as the design is already fixed and there doesn't seem to be too much work.
Completely removing RVO sounds strechy. I don't have a clean idea how to do that, so let's revisit that later.
Comment 1 by lukasza@chromium.org
, Aug 16 2016