Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7645041:
mac: Makeshift UI for scroll feedback. (Closed)
|
Created:
9 years, 4 months ago by Nico Modified:
9 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionmac: Makeshift UI for scroll feedback.
Heavily based on code by Alexei Svitkine <asvitkine@chromium.org>;
BUG=90228
TEST=two-finger history scroll. get some ui feedback.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97022
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 24
Patch Set 5 : comments #
Total comments: 10
Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : rebase #
Messages
Total messages: 12 (0 generated)
Cool. Thanks to Alexei, too. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_mac.mm:1606: // toward 0. Since you describe the API here and isComplete is now significant, can you expand on this comment slightly to indicate when the block gets called with isComplete YES? http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.h (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.h:7: You of all people should know… #pragma once http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:9: #include "base/logging.h" Used? Doesn’t look it. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:11: #include "base/sys_string_conversions.h" Used? Doesn’t look it. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:26: - (id)initWithFrame:(NSRect)frameRect { I know where this code is ultimately ripped from: chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm. Right down to the comments and stuff. Do you want to refactor things to share the common code? http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:30: [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)]); NSZeroRect? http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:70: // Fixup the frame of the string. “Fix up” as a verb is two words (although the problem is present in the file that this borrows from.) http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:138: alpha = -(gestureAmount + 1) * (gestureAmount + 1) + 1; Can you pull the alpha computation out of the conditional by using std::abs? I think that would be -(std::abs(gestureAmount) - 1) * (std::abs(gestureAmount) - 1) + 1. I think squaring it as you’ve done is a good idea, but should the alpha be scaled such that instead of being in the range of 0 to 1, it starts somewhere higher than 0? http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:147: p.x = x; I wonder if we need a “target” for the arrow to hit to make it obvious that the navigation happens when the things touch. It might be obvious enough with the arrow inside the overlay now. I haven’t actually looked at this implementation yet. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:153: const CGFloat kFadeOutDuration = 0.2; // In seconds. Instead of the comment, name the variable kFadeOutDurationSeconds. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:168: - (void)windowWillClose:(NSNotification*)notif { Don’t name variables notif, even if they’re unused, and even if the file you took this from did it. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:175: parent_ = window; Is it possible for parent_ to go away while this object hangs on to a reference to it? It seems like I can press command-W while my fingers are on the touchpad in the middle of a rubber band, for example.
http://codereview.chromium.org/7645041/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_mac.mm:1606: // toward 0. On 2011/08/16 01:01:20, Mark Mentovai wrote: > Since you describe the API here and isComplete is now significant, can you > expand on this comment slightly to indicate when the block gets called with > isComplete YES? Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.h (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.h:7: On 2011/08/16 01:01:20, Mark Mentovai wrote: > You of all people should know… > > #pragma once Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:9: #include "base/logging.h" On 2011/08/16 01:01:20, Mark Mentovai wrote: > Used? Doesn’t look it. Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:11: #include "base/sys_string_conversions.h" On 2011/08/16 01:01:20, Mark Mentovai wrote: > Used? Doesn’t look it. Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:26: - (id)initWithFrame:(NSRect)frameRect { On 2011/08/16 01:01:20, Mark Mentovai wrote: > I know where this code is ultimately ripped from: > chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm. Right down to the > comments and stuff. Do you want to refactor things to share the common code? I'm hoping to get a bigger version of our back button picture from Cole, and then I'll replace the text here with an NSImage thingie. So it might not be worth to factor out right now. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:30: [[NSTextField alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)]); On 2011/08/16 01:01:20, Mark Mentovai wrote: > NSZeroRect? Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:70: // Fixup the frame of the string. On 2011/08/16 01:01:20, Mark Mentovai wrote: > “Fix up” as a verb is two words (although the problem is present in the file > that this borrows from.) Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:138: alpha = -(gestureAmount + 1) * (gestureAmount + 1) + 1; On 2011/08/16 01:01:20, Mark Mentovai wrote: > Can you pull the alpha computation out of the conditional by using std::abs? I > think that would be -(std::abs(gestureAmount) - 1) * (std::abs(gestureAmount) - > 1) + 1. > > I think squaring it as you’ve done is a good idea, but should the alpha be > scaled such that instead of being in the range of 0 to 1, it starts somewhere > higher than 0? It feels good starting at 0 I think. Did the rest. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:147: p.x = x; On 2011/08/16 01:01:20, Mark Mentovai wrote: > I wonder if we need a “target” for the arrow to hit to make it obvious that the > navigation happens when the things touch. > > It might be obvious enough with the arrow inside the overlay now. I haven’t > actually looked at this implementation yet. Yes, this will need tweaking. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:153: const CGFloat kFadeOutDuration = 0.2; // In seconds. On 2011/08/16 01:01:20, Mark Mentovai wrote: > Instead of the comment, name the variable kFadeOutDurationSeconds. Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:168: - (void)windowWillClose:(NSNotification*)notif { On 2011/08/16 01:01:20, Mark Mentovai wrote: > Don’t name variables notif, even if they’re unused, and even if the file you > took this from did it. Done. http://codereview.chromium.org/7645041/diff/5001/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:175: parent_ = window; On 2011/08/16 01:01:20, Mark Mentovai wrote: > Is it possible for parent_ to go away while this object hangs on to a reference > to it? It seems like I can press command-W while my fingers are on the touchpad > in the middle of a rubber band, for example. Great catch, done. I made it a scoped_nsobject (hitting cmd-w during a swipe still hides the window immediately, but the track animation continues until completion, and nothing crashes.)
LGTM to the code. But you should be aware that I just synced up to HEAD and applied this to try it out. I’m getting a lot of rubber-banding (using the new gesture stuff), but wasn’t able to get a back/forward page transition on google.com, nytimes.com, or cnn.com with gestures at all. Two fingers, three fingers, whatever. (There’s also some “jank” with the rubber bands on the more complex pages like nytimes.com and cnn.com.) I’m not sure if that’s a factor of this patch or of the new gesture stuff. I was able to see the overlays while looking at a window that just had the NTP on it. By rapidly swiping left and right, I was able to get lots and lots of arrows to fly across the screen. More than one arrow would be visible at a time. That was kind of weird. I was also surprised to find that I could get the overlay arrows to actually go outside their parent windows. http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_mac.mm:1611: dampenAmountThresholdMin:-50 Why the change to ±50 here? Learn anything you’re not sharing?
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_mac.mm:1596: HistoryOverlayController* historyOverlay = I was going to ask you about this - but Mark beat me to it by trying it out and noticing the issue with multiple windows being created. You probably want to keep a scoped ptr to the history controller so the already open window can be re-used (or maybe just closed) if its still open. http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:101: mode_ = mode; Can you DCHECK() the mode to be one of the expected ones? http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:135: x = minX + gestureAmount * (maxX - minX); Maybe just initialize x to "gestureAmount * (maxX - minX)" and then just add maxX or minX based on the mode? Also, if mode can only be one of those two values, you can change the else if to an else. http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:138: p.y += (NSHeight(windowFrame) - NSHeight([[self window] frame])) * 0.65; Can you make move 0.65 to a constant? Something like kVerticalPositionRatio.
> You probably want to keep a scoped ptr to the history controller so the already > open window can be re-used (or maybe just closed) if its still open. To clarify, I mean to keep the scoped ns ptr on the rwhvm.
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:101: mode_ = mode; asvitkine_ wrote: > Can you DCHECK() the mode to be one of the expected ones? Such a check is likely to be optimized away. enums aren’t ints, so compilers take some tricky liberties.
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... File chrome/browser/ui/cocoa/history_overlay_controller.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist... chrome/browser/ui/cocoa/history_overlay_controller.mm:101: mode_ = mode; On 2011/08/16 14:17:24, Mark Mentovai wrote: > asvitkine_ wrote: > > Can you DCHECK() the mode to be one of the expected ones? > > Such a check is likely to be optimized away. enums aren’t ints, so compilers > take some tricky liberties. Good point - I take that back, the compiler would already validate the call sites.
mark: that's because the WebKit part this depends on isn't rolled in yet (it is landed upstream) On Aug 15, 2011 10:47 PM, <mark@chromium.org> wrote: > LGTM to the code. > > But you should be aware that I just synced up to HEAD and applied this to > try it > out. I’m getting a lot of rubber-banding (using the new gesture stuff), but > wasn’t able to get a back/forward page transition on google.com, > nytimes.com, or > cnn.com with gestures at all. Two fingers, three fingers, whatever. (There’s > also some “jank” with the rubber bands on the more complex pages like > nytimes.com and cnn.com.) > > I’m not sure if that’s a factor of this patch or of the new gesture stuff. > > I was able to see the overlays while looking at a window that just had the > NTP > on it. By rapidly swiping left and right, I was able to get lots and lots of > arrows to fly across the screen. More than one arrow would be visible at a > time. > That was kind of weird. > > I was also surprised to find that I could get the overlay arrows to > actually go > outside their parent windows. > > > http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... > File chrome/browser/renderer_host/render_widget_host_view_mac.mm > (right): > > http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... > chrome/browser/renderer_host/render_widget_host_view_mac.mm:1611: > dampenAmountThresholdMin:-50 > Why the change to ±50 here? Learn anything you’re not sharing? > > http://codereview.chromium.org/7645041/
asvitkine: Having a scoped_nsobject on the rwhvmac doesn't work so well when the user does two gestures quickly in succession and two of these things are on the screen. (Having just one window is ugly, too: You need to hide it in the old position before you can show it in the new position, etc) http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/7645041/diff/7002/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_mac.mm:1611: dampenAmountThresholdMin:-50 On 2011/08/16 05:47:22, Mark Mentovai wrote: > Why the change to ±50 here? Learn anything you’re not sharing? Hah, no. I played with this and couldn't really see a difference (if I make the number 0.0001, then the swipe feels very slow, but 1 and 50 feel mostly the same). I'll change this back to 1 before landing.
Tryjob error:
"__Block_object_dispose", referenced from:
___destroy_helper_block_ in libbrowser.a(render_widget_host_view_mac.o)
___destroy_helper_block_ in libbrowser.a(render_widget_host_view_mac.o)
"__Block_object_assign", referenced from:
___copy_helper_block_ in libbrowser.a(render_widget_host_view_mac.o)
___copy_helper_block_ in libbrowser.a(render_widget_host_view_mac.o)
ld: symbol(s) not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)
(The linker line has "-lclosure_blocks_leopard_compat_stub" on it)
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Mark: Do ___destroy_helper_block_ and ___copy_helper_block_ have to be added to
the block hack file?
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist...
File chrome/browser/ui/cocoa/history_overlay_controller.mm (right):
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_overlay_controller.mm:135: x = minX +
gestureAmount * (maxX - minX);
On 2011/08/16 14:10:04, asvitkine_ wrote:
> Maybe just initialize x to "gestureAmount * (maxX - minX)" and then just add
> maxX or minX based on the mode?
That's shorter, but harder to understand imho.
http://codereview.chromium.org/7645041/diff/7002/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_overlay_controller.mm:138: p.y +=
(NSHeight(windowFrame) - NSHeight([[self window] frame])) * 0.65;
On 2011/08/16 14:10:04, asvitkine_ wrote:
> Can you make move 0.65 to a constant? Something like kVerticalPositionRatio.
Done.
This now depends on http://codereview.chromium.org/7657014/ to add some more symbols to my closure-blocks compatibility library. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
