Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10796116:
[Web Intents] Basic location bar UI for window disposition picker affordance. (Closed)
|
Created:
8 years, 5 months ago by Greg Billock Modified:
8 years, 3 months ago CC:
chromium-reviews, gbillock+watch_chromium.org, smckay+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Web Intents] Basic location bar UI for window disposition picker affordance.
R=bauerb@chromium.org
TBR=phajdan.jr@chromium.org
BUG=139028
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152002
Patch Set 1 #
Total comments: 6
Patch Set 2 : Refactor content settings to PageTool base class. Add stubs for mac/win. #
Total comments: 3
Patch Set 3 : Integrate PageToolViewGtk refactor #Patch Set 4 : Merge to head #Patch Set 5 : Fix merge #
Total comments: 2
Patch Set 6 : Update 'tool' to 'button' #Patch Set 7 : Merge to head #
Total comments: 10
Patch Set 8 : Improve comments/logging. #Patch Set 9 : Shorten animation time #Patch Set 10 : Rebase #Patch Set 11 : Add LocationBar method to test mock. #Messages
Total messages: 30 (0 generated)
https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... chrome/browser/ui/gtk/location_bar_view_gtk.cc:1557: LOG(INFO) << "CLICK!!!"; ? :) https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... chrome/browser/ui/gtk/location_bar_view_gtk.h:147: virtual void UpdateWebIntentsTool() OVERRIDE; Is this method actually called from outside of this class? If not, you could remove it from the LocationBar interface and make it private. https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... chrome/browser/ui/gtk/location_bar_view_gtk.h:174: // model interface for all the tools displayed in the location bar. Can you explain what this class does? IIUC, at the moment it's still specific to Web Intents, but you want to abstract it in the future? For now, we should probably still have a name with Web Intents though, because a general name would imply that the class is general-use. https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... chrome/browser/ui/gtk/location_bar_view_gtk.h:215: base::WeakPtrFactory<PageToolViewGtk> weak_factory_; Is this used? https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... File chrome/browser/ui/intents/web_intent_picker_controller.h (right): https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... chrome/browser/ui/intents/web_intent_picker_controller.h:63: bool ShowLocationBarPickerTool(); Name this "ShouldShow..."? If you're starting with "Show", it sounds like an action. https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/location_bar.h (right): https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/location_bar.h:71: virtual void UpdateWebIntentsTool() = 0; Does this compile on platforms other than Linux?
Revamped this a bit. I've now refactored out the class better. I'm thinking maybe I'll split some of this preamble into another CL that'll be easier to read and get in, as well as the LocationBar method. Then adding the new class will be more focused, and I'll just do it as a subclass of the ContentSettingImageViewGtk class instead of the way it's done here (backwards). Sound good? On 2012/07/24 20:37:03, Bernhard Bauer wrote: > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > chrome/browser/ui/gtk/location_bar_view_gtk.cc:1557: LOG(INFO) << "CLICK!!!"; > ? :) > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > chrome/browser/ui/gtk/location_bar_view_gtk.h:147: virtual void > UpdateWebIntentsTool() OVERRIDE; > Is this method actually called from outside of this class? If not, you could > remove it from the LocationBar interface and make it private. > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > chrome/browser/ui/gtk/location_bar_view_gtk.h:174: // model interface for all > the tools displayed in the location bar. > Can you explain what this class does? IIUC, at the moment it's still specific to > Web Intents, but you want to abstract it in the future? For now, we should > probably still have a name with Web Intents though, because a general name would > imply that the class is general-use. > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > chrome/browser/ui/gtk/location_bar_view_gtk.h:215: > base::WeakPtrFactory<PageToolViewGtk> weak_factory_; > Is this used? > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > File chrome/browser/ui/intents/web_intent_picker_controller.h (right): > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > chrome/browser/ui/intents/web_intent_picker_controller.h:63: bool > ShowLocationBarPickerTool(); > Name this "ShouldShow..."? If you're starting with "Show", it sounds like an > action. > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > File chrome/browser/ui/omnibox/location_bar.h (right): > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > chrome/browser/ui/omnibox/location_bar.h:71: virtual void UpdateWebIntentsTool() > = 0; > Does this compile on platforms other than Linux?
On 2012/07/26 22:56:56, Greg Billock wrote: > Revamped this a bit. I've now refactored out the class better. I'm thinking > maybe I'll split some of this preamble into another CL that'll be easier to read > and get in, as well as the LocationBar method. Then adding the new class will be > more focused, and I'll just do it as a subclass of the > ContentSettingImageViewGtk class instead of the way it's done here (backwards). > Sound good? Sounds good, although I'll have to get a feeling for what these two classes will turn out to look like to decide the class hierarchy. Maybe we could have an abstract base class with two subclasses for content settings and web intents? In general, I prefer that over having a default class with some behavior overridden a subclass, but like I said, I'm going to need to see the final classes for this case. > On 2012/07/24 20:37:03, Bernhard Bauer wrote: > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > chrome/browser/ui/gtk/location_bar_view_gtk.cc:1557: LOG(INFO) << "CLICK!!!"; > > ? :) > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > chrome/browser/ui/gtk/location_bar_view_gtk.h:147: virtual void > > UpdateWebIntentsTool() OVERRIDE; > > Is this method actually called from outside of this class? If not, you could > > remove it from the LocationBar interface and make it private. > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > chrome/browser/ui/gtk/location_bar_view_gtk.h:174: // model interface for all > > the tools displayed in the location bar. > > Can you explain what this class does? IIUC, at the moment it's still specific > to > > Web Intents, but you want to abstract it in the future? For now, we should > > probably still have a name with Web Intents though, because a general name > would > > imply that the class is general-use. > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > > chrome/browser/ui/gtk/location_bar_view_gtk.h:215: > > base::WeakPtrFactory<PageToolViewGtk> weak_factory_; > > Is this used? > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > > File chrome/browser/ui/intents/web_intent_picker_controller.h (right): > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > > chrome/browser/ui/intents/web_intent_picker_controller.h:63: bool > > ShowLocationBarPickerTool(); > > Name this "ShouldShow..."? If you're starting with "Show", it sounds like an > > action. > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > > File chrome/browser/ui/omnibox/location_bar.h (right): > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > > chrome/browser/ui/omnibox/location_bar.h:71: virtual void > UpdateWebIntentsTool() > > = 0; > > Does this compile on platforms other than Linux?
http://codereview.chromium.org/10796116/diff/6001/chrome/browser/ui/gtk/locat... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/6001/chrome/browser/ui/gtk/locat... chrome/browser/ui/gtk/location_bar_view_gtk.cc:1486: void LocationBarViewGtk::PageToolViewGtk::Update( Where is this method called? http://codereview.chromium.org/10796116/diff/6001/chrome/browser/ui/gtk/locat... chrome/browser/ui/gtk/location_bar_view_gtk.cc:1489: !tab_contents->web_intent_picker_controller() || This still seems to be Web Intents specific? http://codereview.chromium.org/10796116/diff/6001/chrome/browser/ui/gtk/locat... File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): http://codereview.chromium.org/10796116/diff/6001/chrome/browser/ui/gtk/locat... chrome/browser/ui/gtk/location_bar_view_gtk.h:239: virtual void Update(TabContents* tab_contents) OVERRIDE; Add a comment which class this overrides?
On Thu, Jul 26, 2012 at 5:35 PM, <bauerb@chromium.org> wrote: > On 2012/07/26 22:56:56, Greg Billock wrote: >> >> Revamped this a bit. I've now refactored out the class better. I'm >> thinking >> maybe I'll split some of this preamble into another CL that'll be easier >> to > > read >> >> and get in, as well as the LocationBar method. Then adding the new class >> will > > be >> >> more focused, and I'll just do it as a subclass of the >> ContentSettingImageViewGtk class instead of the way it's done here > > (backwards). >> >> Sound good? > > > Sounds good, although I'll have to get a feeling for what these two classes > will > turn out to look like to decide the class hierarchy. Maybe we could have an > abstract base class with two subclasses for content settings and web > intents? In > general, I prefer that over having a default class with some behavior > overridden > a subclass, but like I said, I'm going to need to see the final classes for > this > case. Sounds good. I've gone back and forth between those options, but the base class is the most appealing -- I'd like to strip the impl classes from the header and put them in the .cc file alone, but I'm not totally sure that's going to be possible yet. I'll attempt it when doing just the refactor today. > > >> On 2012/07/24 20:37:03, Bernhard Bauer wrote: >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > chrome/browser/ui/gtk/location_bar_view_gtk.cc:1557: LOG(INFO) << > > "CLICK!!!"; >> >> > ? :) >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:147: virtual void >> > UpdateWebIntentsTool() OVERRIDE; >> > Is this method actually called from outside of this class? If not, you >> > could >> > remove it from the LocationBar interface and make it private. >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:174: // model interface >> > for > > all >> >> > the tools displayed in the location bar. >> > Can you explain what this class does? IIUC, at the moment it's still > > specific >> >> to >> > Web Intents, but you want to abstract it in the future? For now, we >> > should >> > probably still have a name with Web Intents though, because a general >> > name >> would >> > imply that the class is general-use. >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... >> >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:215: >> > base::WeakPtrFactory<PageToolViewGtk> weak_factory_; >> > Is this used? >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... >> >> > File chrome/browser/ui/intents/web_intent_picker_controller.h (right): >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... >> >> > chrome/browser/ui/intents/web_intent_picker_controller.h:63: bool >> > ShowLocationBarPickerTool(); >> > Name this "ShouldShow..."? If you're starting with "Show", it sounds >> > like an >> > action. >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... >> >> > File chrome/browser/ui/omnibox/location_bar.h (right): >> > >> > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... >> >> > chrome/browser/ui/omnibox/location_bar.h:71: virtual void >> UpdateWebIntentsTool() >> > = 0; >> > Does this compile on platforms other than Linux? > > > > > http://codereview.chromium.org/10796116/
On 2012/07/27 15:36:37, Greg Billock wrote: > On Thu, Jul 26, 2012 at 5:35 PM, <mailto:bauerb@chromium.org> wrote: > > On 2012/07/26 22:56:56, Greg Billock wrote: > >> > >> Revamped this a bit. I've now refactored out the class better. I'm > >> thinking > >> maybe I'll split some of this preamble into another CL that'll be easier > >> to > > > > read > >> > >> and get in, as well as the LocationBar method. Then adding the new class > >> will > > > > be > >> > >> more focused, and I'll just do it as a subclass of the > >> ContentSettingImageViewGtk class instead of the way it's done here > > > > (backwards). > >> > >> Sound good? > > > > > > Sounds good, although I'll have to get a feeling for what these two classes > > will > > turn out to look like to decide the class hierarchy. Maybe we could have an > > abstract base class with two subclasses for content settings and web > > intents? In > > general, I prefer that over having a default class with some behavior > > overridden > > a subclass, but like I said, I'm going to need to see the final classes for > > this > > case. > > Sounds good. I've gone back and forth between those options, but the > base class is the most appealing -- I'd like to strip the impl classes > from the header and put them in the .cc file alone, but I'm not > totally sure that's going to be possible yet. I'll attempt it when > doing just the refactor today. > > > > > > > >> On 2012/07/24 20:37:03, Bernhard Bauer wrote: > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > chrome/browser/ui/gtk/location_bar_view_gtk.cc:1557: LOG(INFO) << > > > > "CLICK!!!"; > >> > >> > ? :) > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > File chrome/browser/ui/gtk/location_bar_view_gtk.h (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:147: virtual void > >> > UpdateWebIntentsTool() OVERRIDE; > >> > Is this method actually called from outside of this class? If not, you > >> > could > >> > remove it from the LocationBar interface and make it private. > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:174: // model interface > >> > for > > > > all > >> > >> > the tools displayed in the location bar. > >> > Can you explain what this class does? IIUC, at the moment it's still > > > > specific > >> > >> to > >> > Web Intents, but you want to abstract it in the future? For now, we > >> > should > >> > probably still have a name with Web Intents though, because a general > >> > name > >> would > >> > imply that the class is general-use. > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/gtk/... > >> > >> > chrome/browser/ui/gtk/location_bar_view_gtk.h:215: > >> > base::WeakPtrFactory<PageToolViewGtk> weak_factory_; > >> > Is this used? > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > >> > >> > File chrome/browser/ui/intents/web_intent_picker_controller.h (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/inte... > >> > >> > chrome/browser/ui/intents/web_intent_picker_controller.h:63: bool > >> > ShowLocationBarPickerTool(); > >> > Name this "ShouldShow..."? If you're starting with "Show", it sounds > >> > like an > >> > action. > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > >> > >> > File chrome/browser/ui/omnibox/location_bar.h (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/10796116/diff/1/chrome/browser/ui/omni... > >> > >> > chrome/browser/ui/omnibox/location_bar.h:71: virtual void > >> UpdateWebIntentsTool() > >> > = 0; > >> > Does this compile on platforms other than Linux? > > > > > > > > > > http://codereview.chromium.org/10796116/ OK, I've merged in the prerequisite changes I pulled out separately now. Take another look? I'll send along the views/cocoa changes separately.
LGTM! http://codereview.chromium.org/10796116/diff/14005/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/14005/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:356: LOG(INFO) << "Web Intents button click"; Nit: Could you make this a DLOG? That way we don't ship the string in an official build.
http://codereview.chromium.org/10796116/diff/14005/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/14005/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:356: LOG(INFO) << "Web Intents button click"; On 2012/07/31 23:29:15, Bernhard Bauer wrote: > Nit: Could you make this a DLOG? That way we don't ship the string in an > official build. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10796116/20001
Presubmit check for 10796116-20001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/cocoa/location_bar
chrome/browser/ui/gtk
chrome/browser/ui/omnibox
chrome/browser/ui
Presubmit checks took 1.1s to calculate.
Owners, can you take a look at this CL please? Contains stubs only in cocoa, views.
cocoa lgtm.
LGTM
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:150: // Animation opening time for web intents button. (in ms) http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; I don't know what exactly this animation is, but usually we cap all animations at 250ms. Was this 2s for debugging? http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:358: DLOG(INFO) << "Web Intents button click"; TODO(gbillock): Implement. (?) I don't think the DLOG belongs here
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:150: // Animation opening time for web intents button. On 2012/08/10 19:36:56, Evan Stade wrote: > (in ms) Done. http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; On 2012/08/10 19:36:56, Evan Stade wrote: > I don't know what exactly this animation is, but usually we cap all animations > at 250ms. Was this 2s for debugging? I found it basically impossible to see at all at 250ms (it is showing up on the new tab render, which is a busy time for the thread). My idea is that it should attract more attention than that. If we cap them, though, I'm happy to change it. http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:358: DLOG(INFO) << "Web Intents button click"; On 2012/08/10 19:36:56, Evan Stade wrote: > TODO(gbillock): Implement. (?) > > I don't think the DLOG belongs here Done.
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; On 2012/08/10 19:44:17, Greg Billock wrote: > On 2012/08/10 19:36:56, Evan Stade wrote: > > I don't know what exactly this animation is, but usually we cap all animations > > at 250ms. Was this 2s for debugging? > > I found it basically impossible to see at all at 250ms (it is showing up on the > new tab render, which is a busy time for the thread). My idea is that it should > attract more attention than that. If we cap them, though, I'm happy to change > it. Whether we cap depends on the purpose of the animation. Most animations are capped because they're fairly incidental. We do very occasionally allow longer ones for special cases, but they're exceptional. Greg, is this the amount of time the button takes to animate out from 0 to full width? If so, does this match the animation rate for the "popup blocked" animation? That's the closest current analog here so we should probably have them both go at about the same speed.
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; On 2012/08/11 03:37:10, Peter Kasting wrote: > On 2012/08/10 19:44:17, Greg Billock wrote: > > On 2012/08/10 19:36:56, Evan Stade wrote: > > > I don't know what exactly this animation is, but usually we cap all > animations > > > at 250ms. Was this 2s for debugging? > > > > I found it basically impossible to see at all at 250ms (it is showing up on > the > > new tab render, which is a busy time for the thread). My idea is that it > should > > attract more attention than that. If we cap them, though, I'm happy to change > > it. > > Whether we cap depends on the purpose of the animation. Most animations are > capped because they're fairly incidental. We do very occasionally allow longer > ones for special cases, but they're exceptional. Greg, is this the amount of > time the button takes to animate out from 0 to full width? If so, does this > match the animation rate for the "popup blocked" animation? That's the closest > current analog here so we should probably have them both go at about the same > speed. The rate is definitely slower than that. Obviously it'll depend on translation and such, but I'd guess the string is about twice as long. That animation is fairly low-attention though. Like I said, I'm happy to change it if those goals are the ones to watch. A debug build may be a lot different. Shall I put it to 300?
On 2012/08/11 15:08:05, Greg Billock wrote: > Shall I put it to > 300? Why don't you make it take the same time as the "popup blocked" animation, then. Calculating the rate would be tricky as you note.
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; On 2012/08/11 15:08:05, Greg Billock wrote: > On 2012/08/11 03:37:10, Peter Kasting wrote: > > On 2012/08/10 19:44:17, Greg Billock wrote: > > > On 2012/08/10 19:36:56, Evan Stade wrote: > > > > I don't know what exactly this animation is, but usually we cap all > > animations > > > > at 250ms. Was this 2s for debugging? > > > > > > I found it basically impossible to see at all at 250ms (it is showing up on > > the > > > new tab render, which is a busy time for the thread). My idea is that it > > should > > > attract more attention than that. If we cap them, though, I'm happy to > change > > > it. > > > > Whether we cap depends on the purpose of the animation. Most animations are > > capped because they're fairly incidental. We do very occasionally allow > longer > > ones for special cases, but they're exceptional. Greg, is this the amount of > > time the button takes to animate out from 0 to full width? If so, does this > > match the animation rate for the "popup blocked" animation? That's the > closest > > current analog here so we should probably have them both go at about the same > > speed. > > The rate is definitely slower than that. Obviously it'll depend on translation > and such, but I'd guess the string is about twice as long. That animation is > fairly low-attention though. Like I said, I'm happy to change it if those goals > are the ones to watch. A debug build may be a lot different. Shall I put it to > 300? I would copy the popup blocked duration. I don't think the string length matters (we don't change the animation duration for German for example). And yea, if you're testing this in debug it's going to have be a very different experience than in release.
http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right): http://codereview.chromium.org/10796116/diff/20001/chrome/browser/ui/gtk/loca... chrome/browser/ui/gtk/location_bar_view_gtk.cc:151: const int kWebIntentsButtonAnimationTime = 2000; Sounds good. It's easy enough to change if another number is desired. On 2012/08/14 00:32:32, Evan Stade wrote: > On 2012/08/11 15:08:05, Greg Billock wrote: > > On 2012/08/11 03:37:10, Peter Kasting wrote: > > > On 2012/08/10 19:44:17, Greg Billock wrote: > > > > On 2012/08/10 19:36:56, Evan Stade wrote: > > > > > I don't know what exactly this animation is, but usually we cap all > > > animations > > > > > at 250ms. Was this 2s for debugging? > > > > > > > > I found it basically impossible to see at all at 250ms (it is showing up > on > > > the > > > > new tab render, which is a busy time for the thread). My idea is that it > > > should > > > > attract more attention than that. If we cap them, though, I'm happy to > > change > > > > it. > > > > > > Whether we cap depends on the purpose of the animation. Most animations are > > > capped because they're fairly incidental. We do very occasionally allow > > longer > > > ones for special cases, but they're exceptional. Greg, is this the amount > of > > > time the button takes to animate out from 0 to full width? If so, does this > > > match the animation rate for the "popup blocked" animation? That's the > > closest > > > current analog here so we should probably have them both go at about the > same > > > speed. > > > > The rate is definitely slower than that. Obviously it'll depend on translation > > and such, but I'd guess the string is about twice as long. That animation is > > fairly low-attention though. Like I said, I'm happy to change it if those > goals > > are the ones to watch. A debug build may be a lot different. Shall I put it to > > 300? > > I would copy the popup blocked duration. I don't think the string length matters > (we don't change the animation duration for German for example). > > And yea, if you're testing this in debug it's going to have be a very different > experience than in release.
gtk lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10796116/7015
Try job failure for 10796116-7015 (previous was lost) (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10796116/20004
Presubmit check for 10796116-20004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/test
Presubmit checks took 1.5s to calculate.
On 2012/08/16 00:43:54, I haz the power (commit-bot) wrote: > Presubmit check for 10796116-20004 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > chrome/test > > Presubmit checks took 1.5s to calculate. phajdan, can I get owners approval for test?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10796116/20004
Change committed as 152002
chrome/test LGTM |
