Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9129024:
Fix for crbug.com/111185. (Closed)
|
Created:
8 years, 11 months ago by andreip3000 Modified:
8 years, 11 months ago Reviewers:
darin (slow to review) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix for crbug.com/111185.
Count input events and the corresponding acks coming from the renderer
and only consider a renderer responsive when all acks have been
received.
BUG=111185
TEST=RenderWidgetHostTest.MultipleInputEvents
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119112
Patch Set 1 #Patch Set 2 : Add more comments. #Patch Set 3 : Remove dead code #Patch Set 4 : Fix initialization order #Patch Set 5 : Fix initialization list again. #
Total comments: 2
Patch Set 6 : Addressed Darin's comments. #Patch Set 7 : Forgot to commit all changes before uploading. #
Total comments: 1
Patch Set 8 : Addressed Darin comments. #
Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... File content/browser/renderer_host/render_widget_host.cc (right): https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... content/browser/renderer_host/render_widget_host.cc:1536: if (!CommandLine::ForCurrentProcess()->HasSwitch( I'm not sure it is a good idea to check command line flags on each input event. That seems wasteful. Also, why conditionalize this at all? https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... content/browser/renderer_host/render_widget_host.cc:1550: return --event_count_ == 0; The name of this function, RecordInputEventAck, doesn't make its return value immediately obvious. It makes me think that you should just have the caller check event_count_ directly. It may also be nice to give this variable a slightly more descriptive name. Maybe: in_flight_event_count_ You should also take care to reset this event counter when we recover from a crash.
On 2012/01/24 17:58:01, darin wrote: > https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... > File content/browser/renderer_host/render_widget_host.cc (right): > > https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... > content/browser/renderer_host/render_widget_host.cc:1536: if > (!CommandLine::ForCurrentProcess()->HasSwitch( > I'm not sure it is a good idea to check command line flags on each input event. > That seems wasteful. Also, why conditionalize this at all? > Ah yes, I don't actually need to check here at all. > https://chromiumcodereview.appspot.com/9129024/diff/7003/content/browser/rend... > content/browser/renderer_host/render_widget_host.cc:1550: return --event_count_ > == 0; > The name of this function, RecordInputEventAck, doesn't make its return value > immediately obvious. It makes me think that you should just have the caller > check event_count_ directly. > Done. > It may also be nice to give this variable a slightly more descriptive name. > Maybe: in_flight_event_count_ > Done. > You should also take care to reset this event counter when we recover from a > crash. Done.
Note: Normally, it is the case that there can be several in-flight events. https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... File content/browser/renderer_host/render_widget_host.cc (right): https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... content/browser/renderer_host/render_widget_host.cc:1160: !CommandLine::ForCurrentProcess()->HasSwitch( I still don't quite understand the need for checking this command line flag.
On 2012/01/24 20:06:55, darin wrote: > Note: Normally, it is the case that there can be several in-flight events. > Sure, I think that's fine. > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > File content/browser/renderer_host/render_widget_host.cc (right): > > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > content/browser/renderer_host/render_widget_host.cc:1160: > !CommandLine::ForCurrentProcess()->HasSwitch( > I still don't quite understand the need for checking this command line flag. Ok, let me try to explain: My goal is to fix the problem when the compositor thread is used and not change the behavior when it isn't. Let's say I don't check. Then we have two possibilities: 1. The command line flag isn't defined. In this situation, there is no guarantee that all input events get an ACK. This isn't because the renderer is blocked, it's just seems to be an optimization in how input events are currently handled. For this reason, our counter may not reach 0 on pages that are actually responsive, so we trigger the "page unresponsive" dialog when we shouldn't, making the situation worse than it is today. 2. The command line flag is defined. This means that now the compositor thread is up and running in the renderer process. In this configuration, as far as I understand and as far as I was able to verify, we are guaranteed to get ACKs for every single input event. This then means that the counter is guaranteed to reach 0, unless the renderer is blocked so the solution in this patch solves the problem. Given that I don't want to make the situation worse for the case where the command line flag isn't defined, it follows that I must check.
On Tue, Jan 24, 2012 at 12:51 PM, <andreip@chromium.org> wrote: > On 2012/01/24 20:06:55, darin wrote: > >> Note: Normally, it is the case that there can be several in-flight >> events. >> > > > Sure, I think that's fine. > > > > https://chromiumcodereview.**appspot.com/9129024/diff/7005/** > content/browser/renderer_host/**render_widget_host.cc<https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/renderer_host/render_widget_host.cc> > >> File content/browser/renderer_host/**render_widget_host.cc (right): >> > > > https://chromiumcodereview.**appspot.com/9129024/diff/7005/** > content/browser/renderer_host/**render_widget_host.cc#**newcode1160<https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/renderer_host/render_widget_host.cc#newcode1160> > >> content/browser/renderer_host/**render_widget_host.cc:1160: >> !CommandLine::**ForCurrentProcess()->**HasSwitch( >> I still don't quite understand the need for checking this command line >> flag. >> > > Ok, let me try to explain: > > My goal is to fix the problem when the compositor thread is used and not > change > the behavior when it isn't. > > Let's say I don't check. Then we have two possibilities: > > 1. The command line flag isn't defined. > > In this situation, there is no guarantee that all input events get an ACK. > This > isn't because the renderer is blocked, it's just seems to be an > optimization in > how input events are currently handled. > All input events are supposed to generate an ACK. Which ones do not? This seems like a bug. -Darin > For this reason, our counter may not reach 0 on pages that are actually > responsive, so we trigger the "page unresponsive" dialog when we shouldn't, > making the situation worse than it is today. > > 2. The command line flag is defined. > > This means that now the compositor thread is up and running in the renderer > process. In this configuration, as far as I understand and as far as I was > able > to verify, we are guaranteed to get ACKs for every single input event. > This then > means that the counter is guaranteed to reach 0, unless the renderer is > blocked > so the solution in this patch solves the problem. > > Given that I don't want to make the situation worse for the case where the > command line flag isn't defined, it follows that I must check. > > > http://chromiumcodereview.**appspot.com/9129024/<http://chromiumcodereview.ap... >
On Tue, Jan 24, 2012 at 9:21 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Tue, Jan 24, 2012 at 12:51 PM, <andreip@chromium.org> wrote: >> >> On 2012/01/24 20:06:55, darin wrote: >>> >>> Note: Normally, it is the case that there can be several in-flight >>> events. >> >> >> >> Sure, I think that's fine. >> >> >> >> >> https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... >>> >>> File content/browser/renderer_host/render_widget_host.cc (right): >> >> >> >> >> https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... >>> >>> content/browser/renderer_host/render_widget_host.cc:1160: >>> !CommandLine::ForCurrentProcess()->HasSwitch( >>> I still don't quite understand the need for checking this command line >>> flag. >> >> >> Ok, let me try to explain: >> >> My goal is to fix the problem when the compositor thread is used and not >> change >> the behavior when it isn't. >> >> Let's say I don't check. Then we have two possibilities: >> >> 1. The command line flag isn't defined. >> >> In this situation, there is no guarantee that all input events get an ACK. >> This >> isn't because the renderer is blocked, it's just seems to be an >> optimization in >> how input events are currently handled. > > > All input events are supposed to generate an ACK. Which ones do not? This > seems like a bug. > It could well be a bug but it definitely happens. I am home now but I will re-check the code tomorrow and explain why some ACKs are not sent back. Thanks, Andrei > -Darin > > >> >> For this reason, our counter may not reach 0 on pages that are actually >> responsive, so we trigger the "page unresponsive" dialog when we >> shouldn't, >> making the situation worse than it is today. >> >> 2. The command line flag is defined. >> >> This means that now the compositor thread is up and running in the >> renderer >> process. In this configuration, as far as I understand and as far as I was >> able >> to verify, we are guaranteed to get ACKs for every single input event. >> This then >> means that the counter is guaranteed to reach 0, unless the renderer is >> blocked >> so the solution in this patch solves the problem. >> >> Given that I don't want to make the situation worse for the case where the >> command line flag isn't defined, it follows that I must check. >> >> >> http://chromiumcodereview.appspot.com/9129024/ > >
On Tue, Jan 24, 2012 at 3:17 PM, Andrei Popescu <andreip@chromium.org>wrote: > On Tue, Jan 24, 2012 at 9:21 PM, Darin Fisher <darin@chromium.org> wrote: > > > > > > On Tue, Jan 24, 2012 at 12:51 PM, <andreip@chromium.org> wrote: > >> > >> On 2012/01/24 20:06:55, darin wrote: > >>> > >>> Note: Normally, it is the case that there can be several in-flight > >>> events. > >> > >> > >> > >> Sure, I think that's fine. > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > >>> > >>> File content/browser/renderer_host/render_widget_host.cc (right): > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > >>> > >>> content/browser/renderer_host/render_widget_host.cc:1160: > >>> !CommandLine::ForCurrentProcess()->HasSwitch( > >>> I still don't quite understand the need for checking this command line > >>> flag. > >> > >> > >> Ok, let me try to explain: > >> > >> My goal is to fix the problem when the compositor thread is used and not > >> change > >> the behavior when it isn't. > >> > >> Let's say I don't check. Then we have two possibilities: > >> > >> 1. The command line flag isn't defined. > >> > >> In this situation, there is no guarantee that all input events get an > ACK. > >> This > >> isn't because the renderer is blocked, it's just seems to be an > >> optimization in > >> how input events are currently handled. > > > > > > All input events are supposed to generate an ACK. Which ones do not? > This > > seems like a bug. > > > > It could well be a bug but it definitely happens. I am home now but I > will re-check the code tomorrow and explain why some ACKs are not sent > back. > Thanks! -Darin > > Thanks, > Andrei > > > -Darin > > > > > >> > >> For this reason, our counter may not reach 0 on pages that are actually > >> responsive, so we trigger the "page unresponsive" dialog when we > >> shouldn't, > >> making the situation worse than it is today. > >> > >> 2. The command line flag is defined. > >> > >> This means that now the compositor thread is up and running in the > >> renderer > >> process. In this configuration, as far as I understand and as far as I > was > >> able > >> to verify, we are guaranteed to get ACKs for every single input event. > >> This then > >> means that the counter is guaranteed to reach 0, unless the renderer is > >> blocked > >> so the solution in this patch solves the problem. > >> > >> Given that I don't want to make the situation worse for the case where > the > >> command line flag isn't defined, it follows that I must check. > >> > >> > >> http://chromiumcodereview.appspot.com/9129024/ > > > > >
On 2012/01/25 00:52:24, darin wrote: > On Tue, Jan 24, 2012 at 3:17 PM, Andrei Popescu <andreip@chromium.org>wrote: > > > On Tue, Jan 24, 2012 at 9:21 PM, Darin Fisher <mailto:darin@chromium.org> wrote: > > > > > > > > > On Tue, Jan 24, 2012 at 12:51 PM, <mailto:andreip@chromium.org> wrote: > > >> > > >> On 2012/01/24 20:06:55, darin wrote: > > >>> > > >>> Note: Normally, it is the case that there can be several in-flight > > >>> events. > > >> > > >> > > >> > > >> Sure, I think that's fine. > > >> > > >> > > >> > > >> > > >> > > > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > > >>> > > >>> File content/browser/renderer_host/render_widget_host.cc (right): > > >> > > >> > > >> > > >> > > >> > > > https://chromiumcodereview.appspot.com/9129024/diff/7005/content/browser/rend... > > >>> > > >>> content/browser/renderer_host/render_widget_host.cc:1160: > > >>> !CommandLine::ForCurrentProcess()->HasSwitch( > > >>> I still don't quite understand the need for checking this command line > > >>> flag. > > >> > > >> > > >> Ok, let me try to explain: > > >> > > >> My goal is to fix the problem when the compositor thread is used and not > > >> change > > >> the behavior when it isn't. > > >> > > >> Let's say I don't check. Then we have two possibilities: > > >> > > >> 1. The command line flag isn't defined. > > >> > > >> In this situation, there is no guarantee that all input events get an > > ACK. > > >> This > > >> isn't because the renderer is blocked, it's just seems to be an > > >> optimization in > > >> how input events are currently handled. > > > > > > > > > All input events are supposed to generate an ACK. Which ones do not? > > This > > > seems like a bug. > > > > > > > It could well be a bug but it definitely happens. I am home now but I > > will re-check the code tomorrow and explain why some ACKs are not sent > > back. > > > > Thanks! I humbly take it back :) I tried really hard today to replicate the missing acks but couldn't. Perhaps I wasn't as thorough when I tested the first time. Anyway, you are right and the check isn't needed. New patch uploaded. Andrei > -Darin > > > > > > Thanks, > > Andrei > > > > > -Darin > > > > > > > > >> > > >> For this reason, our counter may not reach 0 on pages that are actually > > >> responsive, so we trigger the "page unresponsive" dialog when we > > >> shouldn't, > > >> making the situation worse than it is today. > > >> > > >> 2. The command line flag is defined. > > >> > > >> This means that now the compositor thread is up and running in the > > >> renderer > > >> process. In this configuration, as far as I understand and as far as I > > was > > >> able > > >> to verify, we are guaranteed to get ACKs for every single input event. > > >> This then > > >> means that the counter is guaranteed to reach 0, unless the renderer is > > >> blocked > > >> so the solution in this patch solves the problem. > > >> > > >> Given that I don't want to make the situation worse for the case where > > the > > >> command line flag isn't defined, it follows that I must check. > > >> > > >> > > >> http://chromiumcodereview.appspot.com/9129024/ > > > > > > > >
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andreip@chromium.org/9129024/11001
Change committed as 119112 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
