Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
217373003:
Dismissing the GestureNav overlay after the first non-empty paint to improve UX/responsiveness (Closed)
|
Created:
6 years, 9 months ago by mfomitchev Modified:
6 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding a flag to allow the GestureNav overlay to be dismissed after the first
non-empty paint, without waiting for the main frame to load fully.
BUG=357751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261347
Patch Set 1 #Patch Set 2 : Get rid of the flag and just make the fast dismiss the default behavior. #Patch Set 3 : Updating unit tests. #
Total comments: 1
Patch Set 4 : Removing {} #Patch Set 5 : Re-adding StopObservingIfDone() calls to load handlers - needed when need_paint_update_ is false. #
Messages
Total messages: 28 (0 generated)
Sadrul, can you please review? This came out of yesterday's meeting. Thanks!
Can we collect some UMA to see how often this happens, and then make this change if we decide it's worth it?
Sorry, how often what happens? This does make the overlay go away faster if that's what you are asking. The first non-visually paint comes before the load event almost always. On some sites, like theverge.com, the difference is quite noticeable. UX and kuscher@ rose the late dismiss of the overlay as a major issue, and were leaning towards switching to the simplified UI because of it. So we decided to experiment with dismissing the overlay sooner. This CL puts the early dismiss under a flag (not enabled by default), so we can try it. If we like it, we can enable it in M35.
On 2014/03/31 21:27:25, mfomitchev wrote: > Sorry, how often what happens? How often we get a first non-empty paint before the page is completely loaded. > This does make the overlay go away faster if > that's what you are asking. The first non-visually paint comes before the load > event almost always. On some sites, like http://theverge.com, the difference is quite > noticeable. > > UX and kuscher@ rose the late dismiss of the overlay as a major issue, and were > leaning towards switching to the simplified UI because of it. So we decided to > experiment with dismissing the overlay sooner. This CL puts the early dismiss > under a flag (not enabled by default), so we can try it. If we like it, we can > enable it in M35. My preference would be to just make this the default behaviour and see how well it works in the dev channel. We can always revert back if we don't like it (the change would be smaller, and we won't have to introduce a new string after the branch point). But, if we really think being able to turn this behaviour on/off in the same build is going to be useful, then lgtm. There should be an ONO unit-test for this changed behaviour.
Good point, I didn't think about the string. I will do as you suggest.
@sky: Can you please review? Thanks!
LGTM https://codereview.chromium.org/217373003/diff/40001/content/browser/web_cont... File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right): https://codereview.chromium.org/217373003/diff/40001/content/browser/web_cont... content/browser/web_contents/aura/overscroll_navigation_overlay.cc:312: if (committed_entry_id == pending_entry_id_ || !pending_entry_id_) { nit: no {}
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/217373003/80001
Message was sent while issue was closed.
Change committed as 261347 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
