Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901443002:
Do not skip updating composition info on Mac. (Closed)
|
Created:
3 years, 7 months ago by Shuhei Takahashi Modified:
3 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, kinaba, krajshree_chromium.org, mlamouri+watch-content_chromium.org, yukawa Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not skip updating composition info on Mac.
This is a temporary workaround for crbug.com/714771 introduced in
https://codereview.chromium.org/2121953002.
The original change was intended to optimize IME performance on Android,
but during its review, the author was suggested to apply it to all
platforms, and the platform guard was removed without manual checking on
other platforms. It unfortunately broke Japanese IME input in text boxes
inside <webview> on Mac.
The issue was not very visible for long time though it was first
introduced in M54, possibly because it only affected text inputs inside
<webview>. However it has come to attention of users after Electron
rebased its Chromium to 54+ which extensively uses <webview>.
This change essentially disables the optimization introduced in the
original change only when OOPIF is disabled on Mac (the issue does not
reproduce if OOPIF is enabled). This workaround should be removed once
we have a proper fix to support <webview>, or OOPIF is fully launched.
BUG=chromium:714771
TEST=Repro steps in crbug.com/714771 work fine regardless of whether
OOPIF is enabled or disabled (set at chrome://flags)
Patch Set 1 #Patch Set 2 : Apply the guard only when OOPIF is disabled on Mac. #
Total comments: 4
Patch Set 3 : Update comments. #Messages
Total messages: 39 (20 generated)
The CQ bit was checked by nya@google.com 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...
Description was changed from ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the owner was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. Actually it broke Japanese IME input in text inputs inside <webview> on Mac. The issue was not very visible for long time (it was first introduced in M54), possibly because it only affected text inputs inside <webview>. However it has become very visible to users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization only on Mac until we get a proper fix for crbug.com/714771. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 ========== to ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only on Mac until we get a proper fix for the issue. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@google.com changed reviewers: + nona@chromium.org
nona: PTAL
nya@google.com changed reviewers: + aelias@chromium.org
aelias: PTAL
You want this workaround in for M60, but ekaramad@ said http://crbug.com/655753 will fix it and is also targeted for M60. So it seems no action is needed unless http://crbug.com/655753 is punted. Secondly, if we do end up wanting this, I notice that Windows was not tested and the issue looks more <webview>-specific than Mac-specific. So I'd suggest checking a runtime setting for <webview> rather than having a Mac platform #ifdef.
On 2017/05/22 20:01:33, aelias wrote: > You want this workaround in for M60, but ekaramad@ said http://crbug.com/655753 > will fix it and is also targeted for M60. So it seems no action is needed > unless http://crbug.com/655753 is punted. Generally I would not recommend relying on a not-yet-approved feature launch to fix a bug. I believe you have seen launches to be delayed countless times... > Secondly, if we do end up wanting this, I notice that Windows was not tested and > the issue looks more <webview>-specific than Mac-specific. So I'd suggest > checking a runtime setting for <webview> rather than having a Mac platform > #ifdef. I manually tested the same repro case in Windows but I could not reproduce the issue. I do prefer #ifdef so behavior on any other platform does not change. Doing general things is great and I really like it, but that's exactly the reason we failed in the original change https://codereview.chromium.org/2121953002. Since ekaramad@ will work on proper fixes, this change should be only for M60.
OK, then please have both the check for Mac and a check for the deprecated non-cross-process <webview>, so that it will become dead code when ekaramad@ launches his change and we will be sure it can be safely deleted.
On 2017/05/23 00:42:32, aelias wrote: > OK, then please have both the check for Mac and a check for the deprecated > non-cross-process <webview>, so that it will become dead code when ekaramad@ > launches his change and we will be sure it can be safely deleted. Thanks! Your suggestion sounds good, I will revise the change accordingly.
The CQ bit was checked by nya@google.com 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...
Description was changed from ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only on Mac until we get a proper fix for the issue. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 ========== to ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only when OOPIF is disabled on Mac (the issue does not reproduce if OOPIF is enabled). This workaround should be removed once we have a proper fix to support <webview>, or OOPIF is fully launched. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only when OOPIF is disabled on Mac (the issue does not reproduce if OOPIF is enabled). This workaround should be removed once we have a proper fix to support <webview>, or OOPIF is fully launched. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 ========== to ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only when OOPIF is disabled on Mac (the issue does not reproduce if OOPIF is enabled). This workaround should be removed once we have a proper fix to support <webview>, or OOPIF is fully launched. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 work fine regardless whether OOPIF is enabled or disabled (set at chrome://flags) ==========
Description was changed from ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only when OOPIF is disabled on Mac (the issue does not reproduce if OOPIF is enabled). This workaround should be removed once we have a proper fix to support <webview>, or OOPIF is fully launched. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 work fine regardless whether OOPIF is enabled or disabled (set at chrome://flags) ========== to ========== Do not skip updating composition info on Mac. This is a temporary workaround for crbug.com/714771 introduced in https://codereview.chromium.org/2121953002. The original change was intended to optimize IME performance on Android, but during its review, the author was suggested to apply it to all platforms, and the platform guard was removed without manual checking on other platforms. It unfortunately broke Japanese IME input in text boxes inside <webview> on Mac. The issue was not very visible for long time though it was first introduced in M54, possibly because it only affected text inputs inside <webview>. However it has come to attention of users after Electron rebased its Chromium to 54+ which extensively uses <webview>. This change essentially disables the optimization introduced in the original change only when OOPIF is disabled on Mac (the issue does not reproduce if OOPIF is enabled). This workaround should be removed once we have a proper fix to support <webview>, or OOPIF is fully launched. BUG=chromium:714771 TEST=Repro steps in crbug.com/714771 work fine regardless of whether OOPIF is enabled or disabled (set at chrome://flags) ==========
Updated the change and tested on Mac with OOPIF enabled/disabled. PTAL.
lgtm modulo comment nits https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:1846: // On Mac without OOPIF, IME malfunctions if we skip update. "On Mac without OOPIF, <webview> IME" https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:1848: // a proper fix for crbug.com/714771 or we fully launch OOPIF. doesn't need "fully launch OOPIF", just "launch OOPIF for <webview>.
Wait, sorry, not lgtm yet. https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:1850: base::FeatureList::IsEnabled(::features::kGuestViewCrossProcessFrames); Please check that the current renderer is a <webview> tag. As it is, this change also affects non-webview on Mac since the feature flag is global, so it doesn't really address my concern. ekaramad@, what is the way to check that we're within a webview?
ekaramad@chromium.org changed reviewers: + ekaramad@chromium.org
https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2901443002/diff/20001/content/renderer/render... content/renderer/render_widget.cc:1850: base::FeatureList::IsEnabled(::features::kGuestViewCrossProcessFrames); On 2017/05/23 18:19:25, aelias wrote: > Please check that the current renderer is a <webview> tag. As it is, this > change also affects non-webview on Mac since the feature flag is global, so it > doesn't really address my concern. > > ekaramad@, what is the way to check that we're within a webview? Yes this is the generic way.
On 2017/05/23 at 18:39:50, ekaramad wrote: > > ekaramad@, what is the way to check that we're within a webview? > > Yes this is the generic way. My question is how to check if the current renderer process is a non-OOPIF <webview>. Because this is a global feature flag, I believe it would affect all renderers, <webview> or not.
ekaramad@chromium.org changed reviewers: + lfg@chromium.org
Sorry I misread the question. I think it is not (and should not be) possible to tell we are inside a guest process. The guest should always believe it is rendered in tab. That being said, I am cc-ing lfg@ who knows more on the matter. +lfg@chromium.org: Is it possible to tell we are inside a <webview> guest process?
The CQ bit was checked by nya@google.com 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...
Meanwhile I've updated the comment as per aelias's suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/23 19:45:17, EhsanK wrote: > Sorry I misread the question. I think it is not (and should not be) possible to > tell we are inside a guest process. The guest should always believe it is > rendered in tab. > > That being said, I am cc-ing lfg@ who knows more on the matter. > > mailto:+lfg@chromium.org: Is it possible to tell we are inside a <webview> guest > process? It's not possible to determine whether you are inside a <webview> from the, renderer process. This is by design.
OK, in that case, I would suggest sending InputMsg_RequestCompositionUpdates(false, true) from BrowserPlugin side during initialization (still behind feature flag and Mac ifdef). Then, monitor_compositor_info_ will be true, having same effect as this patch.
On 2017/05/24 18:10:04, aelias wrote: > OK, in that case, I would suggest sending > InputMsg_RequestCompositionUpdates(false, true) from BrowserPlugin side during > initialization (still behind feature flag and Mac ifdef). Then, > monitor_compositor_info_ will be true, having same effect as this patch. +1. I was about to suggest something similar. This is not a long-term fix of course since any InputMsg should first go to embedder and then back to browser... to avoid race which could be quite likely in this case (the composition update is supposed to start when we focus <input> and stop when we blur one). But given that we are enabling it for the lifetime of <webview>, it should be fine. I think this might be the right place to send the IPC: https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p... perhaps something like: RenderWidgetHostImpl::From( GetWebContents()->GetRenderViewHost()->GetWidget())->RequestCompositionUpdates(false /* immediate */, true /* monitor */);
On 2017/05/25 19:02:46, EhsanK wrote: > On 2017/05/24 18:10:04, aelias wrote: > > OK, in that case, I would suggest sending > > InputMsg_RequestCompositionUpdates(false, true) from BrowserPlugin side during > > initialization (still behind feature flag and Mac ifdef). Then, > > monitor_compositor_info_ will be true, having same effect as this patch. > > +1. I was about to suggest something similar. This is not a long-term fix of > course since any InputMsg should first go > to embedder and then back to browser... to avoid race which could be quite > likely in this case (the composition update > is supposed to start when we focus <input> and stop when we blur one). But given > that we are enabling it for the lifetime > of <webview>, it should be fine. > > I think this might be the right place to send the IPC: > https://cs.chromium.org/chromium/src/content/browser/browser_plugin/browser_p... > > perhaps something like: > RenderWidgetHostImpl::From( > > GetWebContents()->GetRenderViewHost()->GetWidget())->RequestCompositionUpdates(false > /* immediate */, true /* monitor */); Sorry, I lost bandwidth to work on this issue, and anyway it's unlikely I can make it to M60. But a good news is that Electron picked up this WIP patch already so (IIUC) mainly affected users will be saved. We may not need to hurry to M60. So now I'm thinking of giving up submitting this change and wait for ekaramad@'s proper fix which will come by M61. WDYT?
Sounds good to me. I agree <webview> tag usage on Mac outside of Electron is likely to be very low anyway. Thanks for working on it.
Message was sent while issue was closed.
On 2017/05/26 18:36:21, aelias wrote: > Sounds good to me. I agree <webview> tag usage on Mac outside of Electron is > likely to be very low anyway. Thanks for working on it. FYI, we are on track for launching <webview> on top of the OOPIF architecture for M60, and as noted by ekaramad on bug 714771, this will also resolve the issue. |
