Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
92073003:
[SiteChip] Draw site chip icon and site title. Drag support. (Closed)
|
Created:
7 years ago by Greg Billock Modified:
7 years ago CC:
chromium-reviews, tfarina, Justin Donnelly Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[SiteChip] Draw site chip icon and site title. Drag support.
BUG=315944
R=pkasting@chromium.org
TBR=cpu
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240175
Patch Set 1 #Patch Set 2 : More icon and name improvements. #Patch Set 3 : Add 16x16 full icon accounting #Patch Set 4 : Rebase to master #Patch Set 5 : . #
Total comments: 80
Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : Lots of adjustments #Patch Set 9 : Rebased on new ToolbarModel method #Patch Set 10 : Site chip horiz spacing matches location bar #Patch Set 11 : Rebase #Patch Set 12 : Use new image assets #
Total comments: 39
Patch Set 13 : l10n str for ev ssl, 2 16x16 icons, etc. #
Total comments: 8
Patch Set 14 : . #Patch Set 15 : Change file url handling to use full url. #Patch Set 16 : Rebase #
Total comments: 5
Patch Set 17 : missing virtual #Patch Set 18 : Unused var #
Total comments: 27
Messages
Total messages: 28 (0 generated)
Still in progress, but have a look if you have time. This sets up basically all the behavior of the site chip. Drawing the background for exception states. Finding the title, etc. There are still some issues (i.e. known bug where we don't track malware state well enough between tab switches) I've gone over the pixel spacing with UI and found the right magic numbers. Drag support is in. Redraws on tab switches looks good. Location icon handled OK except for infobar repaint on tab switch Still showing the url -- Justin is investigating hiding that, so it's coming along.
On 2013/11/27 17:27:18, Greg Billock wrote: > Still in progress, but have a look if you have time. > > This sets up basically all the behavior of the site chip. Drawing the background > for exception states. Finding the title, etc. There are still some issues (i.e. > known bug where we don't track malware state well enough between tab switches) > > I've gone over the pixel spacing with UI and found the right magic numbers. > Drag support is in. > Redraws on tab switches looks good. > Location icon handled OK except for infobar repaint on tab switch > Still showing the url -- Justin is investigating hiding that, so it's coming > along. This is quite far along now. Draws the right site chip in most ordinary browsing, and a lot of edge cases. Still things to consider, but this is demo-able. Ready for a look.
Lemme punt reviewing this until the base CL lands.
On 2013/12/03 01:32:42, Peter Kasting wrote: > Lemme punt reviewing this until the base CL lands. OK, it's landed. I changed this a bit from previous state. I deferred malware warning handling -- I'm landing another patch to put some prerequisites into the safe browsing code. There's still a lot of TODOs, but this gets most all of the common behavior we want into the chip.
https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:680: ToolbarModel::EV_SECURE && !site_chip_view_) { Nit: Parens around binary subexprs If you put the site ship check first, I think the other subexpr can fit entirely on the next line. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:202: void SetSiteChip(SiteChipView* site_chip_view) { Inlined functions should always be named unix_hacker()-style. Setters should be set_name_of_member(). https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:40: const int kEdgeThickness = 4; Nit: Anonymous-namespace these and move them to the top of the SiteChipView section (see below) https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:45: class SiteChipExtensionIcon : public extensions::IconImage::Observer { Nit: Add a // SiteChipExtensionIcon ----... divider above this class as well as above the SiteChipView section below. -s go out to column 79, two blank lines above a divider and one below. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:50: : icon_view_(icon_view) { Don't inline definitions into the class declaration except in test files. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:51: icon_image_.reset(new extensions::IconImage( Nit: Avoid reset() call by initing this in the initializer list above. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:60: if (icon_image_->image_skia().image_reps().size() > 0) Nit: Use !empty() https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:61: icon_view_->SetImage(&(icon_image_->image_skia())); Nit: Call OnExtensionIconImageChanged() instead. Is the !empty() check necessary there too? It seems strange that we would need it in one place but not the other. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:66: icon_image_.reset(); Why are we doing either of these? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:73: icon_view_->SetImage(&(icon_image_->image_skia())); Nit: Are the innermost parens necessary? Normally I'd expect to not see them here. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:79: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:81: string16 HostnameFromURL(const GURL& url, Profile* profile) { This function name is misleading. We're not necessarily getting a hostname, so neither this function nor our caller should call it that. We should just call this SiteChipContents or SiteChipText or something. Also, I'd make this a private member so you can drop the second arg, since this is only called from inside SiteChipView anyway. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:83: if (!url.is_valid()) { Nit: No {} https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:84: return string16(UTF8ToUTF16("Chrome")); Thought we were doing "New Tab" here. Also, the explicit string16() should not be necessary here or on any of the similar calls below. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:95: Nit: Extra blank line https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:96: // For file: urls, return the BaseName of the file . That seems wrong. I'd expect to see the whole file path. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:106: // TODO(gbillock): think about view-source? Normally we'd strip the view-source: part and then process the remainder normally. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:112: if (!service) Can this actually fail? Maybe in tests? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:119: return string16(base::UTF8ToUTF16(extension->name())); Nit: Shorter: return base::UTF8ToUTF16(extension ? extension->name() : url.host()); https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:125: // length or the autocomplete parser. This whole section is way too complicated for what you want. Get url.host(), pass the result through net::StripWWW(), and you're done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:144: // For FTP, prepend "ftp:" to hostname. We should treat this like HTTP/HTTPS. Prepending "ftp:" not only results in some bizarre string that looks kind of like a URL but not really, it isn't necessary information in this chip. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:150: return string16(UTF8ToUTF16(url.host())); Just remove the scheme check above and let that code handle this case too. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:161: extension_icon_loader_.reset(); Why does this need to be done explicitly? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:185: void SiteChipView::Update(content::WebContents* tab) { Nit: While here: This should be named |web_contents| instead of |tab|. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:193: if (url == url_displayed_ && security_level == security_level_) Nit: Parens around binary subexprs (several places) https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:201: security_level == ToolbarModel::EV_SECURE) { Nit: No {} https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:209: // it'd be too low contrast, then switch to ?? inverse? Yeah, so what's the problem? That seems to be what you're doing. If you're worried about the background painter making the toolbar color too different, look at how the ev bubble and keyword search bubble code calculate the appropriate background color to compare against. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See ToolbarModelImpl::GetIcon() Instead of copying parts of that code here can we just share it? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:243: if (service) { Again, when can this fail? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:261: views::Painter::CreateImageGridPainter(kBackgroundImages)); Recreating these painters from scratch on every change sucks. Let's keep background painters for both states that we set up in the contstructor, and then simply set a Painter* member to point at the right one or NULL. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:280: // TODO(gbillock): Also need to potentially redo infobars. Why? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:289: (showing_16x16_icon_ ? 2 * k16x16IconExtraSpacing : 0), The whole point of the spacing we set up before was so 16x16 icons would look good without adding more extra space around them. We should definitely not be adding extra space here or anywhere else in this file. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:344: sender->GetWidget()); It seems like this ought to be the same data the omnibox would normally write. Can we share the code? https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.h:79: Nit: Understanding the logic behind where newlines are placed in here eludes me. Remove them all or else add useful comments about each separated group.
Also smoothed out the narrow-window layout case a bit. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.cc:680: ToolbarModel::EV_SECURE && !site_chip_view_) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Parens around binary subexprs > > If you put the site ship check first, I think the other subexpr can fit entirely > on the next line. won't quite. :-( But I think it's better re-ordered. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/l... chrome/browser/ui/views/location_bar/location_bar_view.h:202: void SetSiteChip(SiteChipView* site_chip_view) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Inlined functions should always be named unix_hacker()-style. Setters should be > set_name_of_member(). Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:40: const int kEdgeThickness = 4; On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Anonymous-namespace these and move them to the top of the SiteChipView > section (see below) Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:45: class SiteChipExtensionIcon : public extensions::IconImage::Observer { On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Add a // SiteChipExtensionIcon ----... divider above this class as well as > above the SiteChipView section below. -s go out to column 79, two blank lines > above a divider and one below. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:50: : icon_view_(icon_view) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Don't inline definitions into the class declaration except in test files. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:51: icon_image_.reset(new extensions::IconImage( On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Avoid reset() call by initing this in the initializer list above. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:60: if (icon_image_->image_skia().image_reps().size() > 0) On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Use !empty() Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:61: icon_view_->SetImage(&(icon_image_->image_skia())); On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Call OnExtensionIconImageChanged() instead. > > Is the !empty() check necessary there too? It seems strange that we would need > it in one place but not the other. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:66: icon_image_.reset(); On 2013/12/06 02:16:52, Peter Kasting wrote: > Why are we doing either of these? I hunted through the IconImage code for a while trying to figure out how it cancelled requests to make sure it doesn't use after free. I think I ended up with this as I figured out it was in the destructor, but I don't think we need them as it's all on the UI thread. A bit uncanny for my taste... https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:73: icon_view_->SetImage(&(icon_image_->image_skia())); On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Are the innermost parens necessary? Normally I'd expect to not see them > here. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:79: }; On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:81: string16 HostnameFromURL(const GURL& url, Profile* profile) { Renamed. On 2013/12/06 02:16:52, Peter Kasting wrote: > This function name is misleading. We're not necessarily getting a hostname, so > neither this function nor our caller should call it that. We should just call > this SiteChipContents or SiteChipText or something. > > Also, I'd make this a private member so you can drop the second arg, since this > is only called from inside SiteChipView anyway. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:83: if (!url.is_valid()) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:84: return string16(UTF8ToUTF16("Chrome")); On 2013/12/06 02:16:52, Peter Kasting wrote: > Thought we were doing "New Tab" here. > > Also, the explicit string16() should not be necessary here or on any of the > similar calls below. I have a TODO here below -- If we use the page name, I'll make a little translation table. I'm not sure whether we need translated strings or what, though. Let's talk about it. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:95: On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Extra blank line Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:96: // For file: urls, return the BaseName of the file . On 2013/12/06 02:16:52, Peter Kasting wrote: > That seems wrong. I'd expect to see the whole file path. I'm not sure what the use case is. If its basically developers, then perhaps we want to show the path in the omnibox in that case. (similarly for view-source) https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:106: // TODO(gbillock): think about view-source? On 2013/12/06 02:16:52, Peter Kasting wrote: > Normally we'd strip the view-source: part and then process the remainder > normally. ack. Leaving as TODO for now. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:112: if (!service) On 2013/12/06 02:16:52, Peter Kasting wrote: > Can this actually fail? Maybe in tests? Yeah, this should never fail in production. I don't think tests should touch this; I'll check. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:119: return string16(base::UTF8ToUTF16(extension->name())); On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Shorter: > > return base::UTF8ToUTF16(extension ? extension->name() : url.host()); Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:125: // length or the autocomplete parser. On 2013/12/06 02:16:52, Peter Kasting wrote: > This whole section is way too complicated for what you want. > > Get url.host(), pass the result through net::StripWWW(), and you're done. I'm worried about i18n domain names and other unicode gotchas that FormatUrl is accounting for inside. This produces identical output to what we draw in the omnibox, which is reassuring. I'm all for simplifying, but there's a couple hundred lines of code, at least, doing all kinds of magic in there that look like they may have started with StripWWW(host()) and grew hair. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:144: // For FTP, prepend "ftp:" to hostname. On 2013/12/06 02:16:52, Peter Kasting wrote: > We should treat this like HTTP/HTTPS. Prepending "ftp:" not only results in > some bizarre string that looks kind of like a URL but not really, it isn't > necessary information in this chip. I'm not sure where this originated -- it's in the experiment doc, so I put it here, but let's figure out if that came from a security concern or what. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:150: return string16(UTF8ToUTF16(url.host())); On 2013/12/06 02:16:52, Peter Kasting wrote: > Just remove the scheme check above and let that code handle this case too. Let's figure out whether we need net::FormatUrl first. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:161: extension_icon_loader_.reset(); On 2013/12/06 02:16:52, Peter Kasting wrote: > Why does this need to be done explicitly? I don't like leaving the icon fetch to clean itself up implicitly, but if that's how the API is supposed to work, I'll just use it. If it was called ScopedIconImageFetcher I think I'd be happier. :-) https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:185: void SiteChipView::Update(content::WebContents* tab) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: While here: This should be named |web_contents| instead of |tab|. Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:193: if (url == url_displayed_ && security_level == security_level_) On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Parens around binary subexprs (several places) Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:201: security_level == ToolbarModel::EV_SECURE) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:209: // it'd be too low contrast, then switch to ?? inverse? On 2013/12/06 02:16:52, Peter Kasting wrote: > Yeah, so what's the problem? That seems to be what you're doing. > > If you're worried about the background painter making the toolbar color too > different, look at how the ev bubble and keyword search bubble code calculate > the appropriate background color to compare against. stale comment, I ended up just doing this but didn't erase the comment. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See ToolbarModelImpl::GetIcon() On 2013/12/06 02:16:52, Peter Kasting wrote: > Instead of copying parts of that code here can we just share it? I'm not sure why this icon is calculated in so disjointed a way. I don't understand why there's a whole header for types, except if there's some invisible test or something that's consuming it. It looked like a yak shave to touch, but perhaps that's all just stale. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:243: if (service) { On 2013/12/06 02:16:52, Peter Kasting wrote: > Again, when can this fail? Done. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:261: views::Painter::CreateImageGridPainter(kBackgroundImages)); On 2013/12/06 02:16:52, Peter Kasting wrote: > Recreating these painters from scratch on every change sucks. Let's keep > background painters for both states that we set up in the contstructor, and then > simply set a Painter* member to point at the right one or NULL. Yeah, it's kind of painful. I'll make special-purpose ones. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:280: // TODO(gbillock): Also need to potentially redo infobars. On 2013/12/06 02:16:52, Peter Kasting wrote: > Why? The layout of the site chip can change depending on the site. In a couple cases, there are infobars that follow the toolbar around, and those need redrawn or they anchor into the wrong place. It's kind of an unusual thing, but awkward. We need to account for it somehow -- perhaps by anchoring those infobars elsewhere. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:289: (showing_16x16_icon_ ? 2 * k16x16IconExtraSpacing : 0), On 2013/12/06 02:16:52, Peter Kasting wrote: > The whole point of the spacing we set up before was so 16x16 icons would look > good without adding more extra space around them. We should definitely not be > adding extra space here or anywhere else in this file. Let's talk to Alan about this. The location icons are higher than they are wide -- they have a visual width quite a bit narrower than the typical 16x16 favicon type icon, particularly for the lock and the page icon, which are the most commonly seen. Alan thought extra padding was necessary for the Chrome 16x16, and I think the visual size of that icon is more typical of what we'll see for extensions than are the usual page icon sizes. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:344: sender->GetWidget()); On 2013/12/06 02:16:52, Peter Kasting wrote: > It seems like this ought to be the same data the omnibox would normally write. > Can we share the code? This is basically copy-pasted from the drag support for the location icon view. If the experiment succeeds, we'll eliminate that and it'll just live here. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.h:79: On 2013/12/06 02:16:52, Peter Kasting wrote: > Nit: Understanding the logic behind where newlines are placed in here eludes me. > Remove them all or else add useful comments about each separated group. First para is the views machinery. Second is layout state. Third is internal machinery. Fourth is display state. But I agree this is subtle and just collapsed it all.
https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:125: // length or the autocomplete parser. On 2013/12/06 19:54:26, Greg Billock wrote: > On 2013/12/06 02:16:52, Peter Kasting wrote: > > This whole section is way too complicated for what you want. > > > > Get url.host(), pass the result through net::StripWWW(), and you're done. > > I'm worried about i18n domain names and other unicode gotchas that FormatUrl is > accounting for inside. That's fair. It seems like in that case though we just need to run the hostname through net_util::IDNToUnicode(). That's still simpler than the sort of conditional stripping you're doing. > I'm all for simplifying, but there's a couple > hundred lines of code, at least, doing all kinds of magic in there that look > like they may have started with StripWWW(host()) and grew hair. I don't know precisely where you're looking so I can't comment. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:161: extension_icon_loader_.reset(); On 2013/12/06 19:54:26, Greg Billock wrote: > On 2013/12/06 02:16:52, Peter Kasting wrote: > > Why does this need to be done explicitly? > > I don't like leaving the icon fetch to clean itself up implicitly, but if that's > how the API is supposed to work, I'll just use it. If it was called > ScopedIconImageFetcher I think I'd be happier. :-) I don't understand your comment here or in ~SiteChipExtensionIcon(). The reason you don't need these reset()s is because these are scoped_ptrs, and thus on class destruction they delete themselves. Having an explicit reset() in both these functions is functionally identical to having no reset(), regardless of what class is contained in the scoped_ptr. The only difference is if you need to control the order in which the member variables are destroyed. If you're just trying to make sure destruction happens at all, explicit reset()s aren't needed. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See ToolbarModelImpl::GetIcon() On 2013/12/06 19:54:26, Greg Billock wrote: > On 2013/12/06 02:16:52, Peter Kasting wrote: > > Instead of copying parts of that code here can we just share it? > > I'm not sure why this icon is calculated in so disjointed a way. It doesn't look that bad -- it looks like GetIcon() does what you want except that it accounts for search term replacement, which is probably not a step you want here. That could be done conditionally or split into a separate function. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:280: // TODO(gbillock): Also need to potentially redo infobars. On 2013/12/06 19:54:26, Greg Billock wrote: > On 2013/12/06 02:16:52, Peter Kasting wrote: > > Why? > > The layout of the site chip can change depending on the site. In a couple cases, > there are infobars that follow the toolbar around, and those need redrawn or > they anchor into the wrong place. It's kind of an unusual thing, but awkward. We > need to account for it somehow -- perhaps by anchoring those infobars elsewhere. I would ask the UI leads about this and consider proposing that we anchor infobars to the Chrome menu ("wrench") button. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:289: (showing_16x16_icon_ ? 2 * k16x16IconExtraSpacing : 0), On 2013/12/06 19:54:26, Greg Billock wrote: > Alan thought extra padding was necessary for the Chrome 16x16 I haven't seen that comment. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:344: sender->GetWidget()); On 2013/12/06 19:54:26, Greg Billock wrote: > On 2013/12/06 02:16:52, Peter Kasting wrote: > > It seems like this ought to be the same data the omnibox would normally write. > > > Can we share the code? > > This is basically copy-pasted from the drag support for the location icon view. > If the experiment succeeds, we'll eliminate that and it'll just live here. Still seems like until then we could make this a static method that takes a WebContents* and just call it from both places. I guess I'm just always leery of "we can clean this up later once ______ occurs" because I've been burned too many times by it not getting cleaned up :/
https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:125: // length or the autocomplete parser. On 2013/12/06 21:26:11, Peter Kasting wrote: > On 2013/12/06 19:54:26, Greg Billock wrote: > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > This whole section is way too complicated for what you want. > > > > > > Get url.host(), pass the result through net::StripWWW(), and you're done. > > > > I'm worried about i18n domain names and other unicode gotchas that FormatUrl > is > > accounting for inside. > > That's fair. It seems like in that case though we just need to run the hostname > through net_util::IDNToUnicode(). That's still simpler than the sort of > conditional stripping you're doing. > > > I'm all for simplifying, but there's a couple > > hundred lines of code, at least, doing all kinds of magic in there that look > > like they may have started with StripWWW(host()) and grew hair. > > I don't know precisely where you're looking so I can't comment. FormatUrl calls through a couple layers internally, but eventually you get to a big pile of code that does most of the work. I'm sure at least some of it is unnecessary -- code specific to paths and so on. But URLs are weird enough that I'm reluctant to improvise too much. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:161: extension_icon_loader_.reset(); On 2013/12/06 21:26:11, Peter Kasting wrote: > On 2013/12/06 19:54:26, Greg Billock wrote: > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > Why does this need to be done explicitly? > > > > I don't like leaving the icon fetch to clean itself up implicitly, but if > that's > > how the API is supposed to work, I'll just use it. If it was called > > ScopedIconImageFetcher I think I'd be happier. :-) > > I don't understand your comment here or in ~SiteChipExtensionIcon(). The reason > you don't need these reset()s is because these are scoped_ptrs, and thus on > class destruction they delete themselves. Having an explicit reset() in both > these functions is functionally identical to having no reset(), regardless of > what class is contained in the scoped_ptr. The only difference is if you need > to control the order in which the member variables are destroyed. If you're > just trying to make sure destruction happens at all, explicit reset()s aren't > needed. Agreed. I got gunshy after getting unexpected crashes in the IconImage class. I don't like its API, and put in defenses around it, but I agree I went overboard. :-) I took them all out, I believe. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See ToolbarModelImpl::GetIcon() On 2013/12/06 21:26:11, Peter Kasting wrote: > On 2013/12/06 19:54:26, Greg Billock wrote: > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > Instead of copying parts of that code here can we just share it? > > > > I'm not sure why this icon is calculated in so disjointed a way. > > It doesn't look that bad -- it looks like GetIcon() does what you want except > that it accounts for search term replacement, which is probably not a step you > want here. That could be done conditionally or split into a separate function. I'll give it a shot. I'm trying to prioritize keeping experimental changes from disrupting the existing code too much, and when it happens, doing generally useful things. This seems on the edge of useful to me. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:280: // TODO(gbillock): Also need to potentially redo infobars. On 2013/12/06 21:26:11, Peter Kasting wrote: > On 2013/12/06 19:54:26, Greg Billock wrote: > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > Why? > > > > The layout of the site chip can change depending on the site. In a couple > cases, > > there are infobars that follow the toolbar around, and those need redrawn or > > they anchor into the wrong place. It's kind of an unusual thing, but awkward. > We > > need to account for it somehow -- perhaps by anchoring those infobars > elsewhere. > > I would ask the UI leads about this and consider proposing that we anchor > infobars to the Chrome menu ("wrench") button. will do. https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:344: sender->GetWidget()); On 2013/12/06 21:26:11, Peter Kasting wrote: > On 2013/12/06 19:54:26, Greg Billock wrote: > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > It seems like this ought to be the same data the omnibox would normally > write. > > > > > Can we share the code? > > > > This is basically copy-pasted from the drag support for the location icon > view. > > If the experiment succeeds, we'll eliminate that and it'll just live here. > > Still seems like until then we could make this a static method that takes a > WebContents* and just call it from both places. I guess I'm just always leery > of "we can clean this up later once ______ occurs" because I've been burned too > many times by it not getting cleaned up :/ :-) I hear you. I guess my intuition is that this experiment has a low probability of success, and so refactoring if it is successful will need to happen, whereas re-consolidating a bunch of marginal-utility refactorings after we delete it is the thing that'll be harder to make sure happens. If your intuition is opposite, though, that's probably a good thing, and I'd consider it more reliable. :-)
https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See ToolbarModelImpl::GetIcon() On 2013/12/06 22:11:50, Greg Billock wrote: > On 2013/12/06 21:26:11, Peter Kasting wrote: > > On 2013/12/06 19:54:26, Greg Billock wrote: > > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > > Instead of copying parts of that code here can we just share it? > > > > > > I'm not sure why this icon is calculated in so disjointed a way. > > > > It doesn't look that bad -- it looks like GetIcon() does what you want except > > that it accounts for search term replacement, which is probably not a step you > > want here. That could be done conditionally or split into a separate > function. > > I'll give it a shot. I'm trying to prioritize keeping experimental changes from > disrupting the existing code too much, and when it happens, doing generally > useful things. This seems on the edge of useful to me. Sent you a CL under separate cover to create this API. It looks about like you'd expect. It's a bit sad that it requires a new method, but I think I prefer that here to a mystery-meat boolean argument.
On 2013/12/06 22:42:40, Greg Billock wrote: > https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... > File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): > > https://codereview.chromium.org/92073003/diff/80001/chrome/browser/ui/views/t... > chrome/browser/ui/views/toolbar/site_chip_view.cc:217: // See > ToolbarModelImpl::GetIcon() > On 2013/12/06 22:11:50, Greg Billock wrote: > > On 2013/12/06 21:26:11, Peter Kasting wrote: > > > On 2013/12/06 19:54:26, Greg Billock wrote: > > > > On 2013/12/06 02:16:52, Peter Kasting wrote: > > > > > Instead of copying parts of that code here can we just share it? > > > > > > > > I'm not sure why this icon is calculated in so disjointed a way. > > > > > > It doesn't look that bad -- it looks like GetIcon() does what you want > except > > > that it accounts for search term replacement, which is probably not a step > you > > > want here. That could be done conditionally or split into a separate > > function. > > > > I'll give it a shot. I'm trying to prioritize keeping experimental changes > from > > disrupting the existing code too much, and when it happens, doing generally > > useful things. This seems on the edge of useful to me. > > Sent you a CL under separate cover to create this API. It looks about like you'd > expect. It's a bit sad that it requires a new method, but I think I prefer that > here to a mystery-meat boolean argument. This code is now rejiggered to match the location bar spacings exactly for the lock case. The constants look very aligned now, so if this is our destination, we can just kill basically all of them off. :-)
https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:680: (GetToolbarModel()->GetSecurityLevel(false) == Nit: Indent 4, a la } else if (!site_chip_view_ && (GetToolbarModel()->GetSecurityLevel(false) == ToolbarModel::EV_SECURE)) { This also allows you to save a linewrap. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:918: return site_chip_view_->location_icon_view(); Tiny nit: I'd probably use ?: here and in the next function. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:95: const int k16x16IconExtraSpacing = 3; Nit: Change this to two constants for now (a before and after extension icon); that way you can easily mock up my proposal (1 px before and 2 after), even if you don't check it in as part of this patch. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:114: // For file: urls, return the BaseName of the file . Nit: Lots of spaces before period? https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:134: return extension ? base::UTF8ToUTF16(extension->name()) : Nit: break after '?' instead of ':' https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:146: base::string16 formatted = net::FormatUrl(url.GetOrigin(), languages, I still think this should just be the host, run through StripWWW() and IDNToUnicode(). Not only is that shorter, it's actually more accurate to the intent of the code. We don't want the site chip to "match how the omnibox formats URLs" so much as we explicitly want "the hostname (in Unicode), sans 'www.'". So calling FormatURL() is a very roundabout way of doing what we want, and in theory could break someday (though I can't imagine why it would). https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:216: if (url == url_displayed_ && (security_level == security_level_)) Nit: Parens on both subexprs https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:228: ASCIIToUTF16(" ") + host; This isn't good l10n, even ignoring RTL. Use some kind of a substitutable string defined in the GRD file to be "$1 $2" or "$1 - $2" or something, then substitute the strings in. That lets localizers reorder and change delimiters as necessary. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:236: SkColor host_color = Nit: I would at least inline this into the next statement. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:288: // TODO(gbillock): Also need to potentially redo infobars. Nit: Might be nice to clarify that this is about where the infobar arrows point. Also seems like the toolbar, rather than the site chip, should be responsible for doing that update. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:342: content::WebContents* web_contents = toolbar_view_->GetWebContents(); Nit: At least add a TODO here about consolidating this, even if this patch doesn't do it. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:64: // views::DragController:: Nit: Extra trailing ':' https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:81: scoped_ptr<views::Painter> badssl_background_painter_; Nit: badssl -> bad_ssl (or broken_ssl?) https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:82: // Will point to one of the background painters. Nit: Or NULL? https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:85: scoped_ptr<SiteChipExtensionIcon> extension_icon_loader_; Nit: This should just be |extension_icon_|, both to match the class name better, and because "loader" seems misleading. (If we wanted to attach any words to the end I'd think "monitor" would be more accurate than "loader", since this monitors for icon changes, but I wouldn't even put that.) https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:249: show_home_button_.Init(prefs::kShowHomeButton, Nit: Blank line above this https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:502: 2 * kStandardSpacing + 2 * button_spacing) : I still think adding |button_spacing| in here is wrong? It doesn't seem to correspond to anything we do in Layout(). https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:582: site_chip_view_->GetPreferredSize().width() + Nit: This fits on fewer lines (and reads better): (site_chip_view_->ShouldShow() ? site_chip_view_->GetPreferredSize().width() + kStandardSpacing : 0); Though FWIW I honestly find it pretty confusing that we include kStandardSpacing in |site_chip_width|. It seems like this would be clearer: site_chip_view_->SetVisible(site_chip_view_->ShouldShow()); int site_chip_width = site_chip_view_->GetPreferredSize().width(); site_chip_width = std::max( 0, std::min(site_chip_width, (available_width - kStandardSpacing) / 2)); if (site_chip_view_->visible()) available_width -= (kStandardSpacing + site_chip_width); ... if (site_chip_view_->visible()) { site_chip_view_->SetBounds(browser_actions_x + kStandardSpacing, child_y, site_chip_width, child_height); browser_actions_x = site_chip_view_->bounds().right(); } Note also that this: (a) Fixes a bug where we don't clamp the width to 0 before subtracting it from |available_width| (thus possibly increasing |available_width|) (b) Uses visible() as the conditional check, which is lower-overhead
Saw you about, Peter. Thanks for the last-minute looks. Just finalizing the l10n string, but wanted to upload in case you had a sec for a look. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:680: (GetToolbarModel()->GetSecurityLevel(false) == On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Indent 4, a la > > } else if (!site_chip_view_ && > (GetToolbarModel()->GetSecurityLevel(false) == ToolbarModel::EV_SECURE)) { > > This also allows you to save a linewrap. Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:918: return site_chip_view_->location_icon_view(); On 2013/12/10 03:28:39, Peter Kasting wrote: > Tiny nit: I'd probably use ?: here and in the next function. Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:95: const int k16x16IconExtraSpacing = 3; On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Change this to two constants for now (a before and after extension icon); > that way you can easily mock up my proposal (1 px before and 2 after), even if > you don't check it in as part of this patch. Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:114: // For file: urls, return the BaseName of the file . On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Lots of spaces before period? Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:134: return extension ? base::UTF8ToUTF16(extension->name()) : On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: break after '?' instead of ':' Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:146: base::string16 formatted = net::FormatUrl(url.GetOrigin(), languages, On 2013/12/10 03:28:39, Peter Kasting wrote: > I still think this should just be the host, run through StripWWW() and > IDNToUnicode(). Not only is that shorter, it's actually more accurate to the > intent of the code. We don't want the site chip to "match how the omnibox > formats URLs" so much as we explicitly want "the hostname (in Unicode), sans > 'www.'". So calling FormatURL() is a very roundabout way of doing what we want, > and in theory could break someday (though I can't imagine why it would). I agree this is conservative. Let's keep it for this change and fix it up later. That gives us a baseline we know is identical to the display logic we have now. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:216: if (url == url_displayed_ && (security_level == security_level_)) On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Parens on both subexprs Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:228: ASCIIToUTF16(" ") + host; On 2013/12/10 03:28:39, Peter Kasting wrote: > This isn't good l10n, even ignoring RTL. > > Use some kind of a substitutable string defined in the GRD file to be "$1 $2" or > "$1 - $2" or something, then substitute the strings in. That lets localizers > reorder and change delimiters as necessary. I hadn't considered using L10N to deal with this -- it seems more like a display than formatting issue, but I think that's a good plan. I'll add it in. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:236: SkColor host_color = On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: I would at least inline this into the next statement. Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:288: // TODO(gbillock): Also need to potentially redo infobars. On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Might be nice to clarify that this is about where the infobar arrows point. > > Also seems like the toolbar, rather than the site chip, should be responsible > for doing that update. Done. The update signals come in lower, though, so I think we'll have to trigger it from here. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:342: content::WebContents* web_contents = toolbar_view_->GetWebContents(); On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: At least add a TODO here about consolidating this, even if this patch > doesn't do it. Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:64: // views::DragController:: On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Extra trailing ':' Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:81: scoped_ptr<views::Painter> badssl_background_painter_; On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: badssl -> bad_ssl (or broken_ssl?) Went with broken to match the image constants. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:82: // Will point to one of the background painters. On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Or NULL? Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:85: scoped_ptr<SiteChipExtensionIcon> extension_icon_loader_; On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: This should just be |extension_icon_|, both to match the class name better, > and because "loader" seems misleading. (If we wanted to attach any words to the > end I'd think "monitor" would be more accurate than "loader", since this > monitors for icon changes, but I wouldn't even put that.) ok, but I think that the base class is named "IconImage" is very confusing. This will be equally confusing, then, which might reduce it on aggregate... https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:249: show_home_button_.Init(prefs::kShowHomeButton, On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:502: 2 * kStandardSpacing + 2 * button_spacing) : On 2013/12/10 03:28:39, Peter Kasting wrote: > I still think adding |button_spacing| in here is wrong? It doesn't seem to > correspond to anything we do in Layout(). Yeah, I'm suspicious that's a leftover from before. Removing. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:582: site_chip_view_->GetPreferredSize().width() + On 2013/12/10 03:28:39, Peter Kasting wrote: > Nit: This fits on fewer lines (and reads better): > (site_chip_view_->ShouldShow() ? > site_chip_view_->GetPreferredSize().width() + kStandardSpacing : 0); > > Though FWIW I honestly find it pretty confusing that we include kStandardSpacing > in |site_chip_width|. It seems like this would be clearer: > > site_chip_view_->SetVisible(site_chip_view_->ShouldShow()); > int site_chip_width = site_chip_view_->GetPreferredSize().width(); > site_chip_width = std::max( > 0, std::min(site_chip_width, (available_width - kStandardSpacing) / 2)); > if (site_chip_view_->visible()) > available_width -= (kStandardSpacing + site_chip_width); > ... > if (site_chip_view_->visible()) { > site_chip_view_->SetBounds(browser_actions_x + kStandardSpacing, child_y, > site_chip_width, child_height); > browser_actions_x = site_chip_view_->bounds().right(); > } > > Note also that this: > (a) Fixes a bug where we don't clamp the width to 0 before subtracting it from > |available_width| (thus possibly increasing |available_width|) > (b) Uses visible() as the conditional check, which is lower-overhead I don't think the bug obtains -- I don't think site_chip_width can be < 0 -- but I do agree that having the width include the spacing is bad. I fiddled with this some yesterday, but this convinces me it is confusing. I'll switch it.
LGTM https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:146: base::string16 formatted = net::FormatUrl(url.GetOrigin(), languages, On 2013/12/10 17:37:59, Greg Billock wrote: > On 2013/12/10 03:28:39, Peter Kasting wrote: > > I still think this should just be the host, run through StripWWW() and > > IDNToUnicode(). Not only is that shorter, it's actually more accurate to the > > intent of the code. We don't want the site chip to "match how the omnibox > > formats URLs" so much as we explicitly want "the hostname (in Unicode), sans > > 'www.'". So calling FormatURL() is a very roundabout way of doing what we > want, > > and in theory could break someday (though I can't imagine why it would). > > I agree this is conservative. Let's keep it for this change and fix it up later. > That gives us a baseline we know is identical to the display logic we have now. OK I guess, but consider at least adding a TODO about simplifying this. https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:582: site_chip_view_->GetPreferredSize().width() + On 2013/12/10 17:37:59, Greg Billock wrote: > I don't think the bug obtains -- I don't think site_chip_width can be < 0 No, but (available_width - kStandardSpacing) possibly can be. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:916: return site_chip_view_ ? site_chip_view_->location_icon_view() Nit: Break after '?' instead of before ':' (2 places) https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:298: (2 * (k16x16IconLeadingSpacing + k16x16IconTrailingSpacing)) : 0; The "2 *" here is wrong. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:303: icon_size.height()); Nit: How about wrapping/indenting like: return gfx::Size( kEdgeThickness + icon_spacing + icon_size.width() + kIconTextSpacing + label_size.width() + kTrailingLabelMargin + kEdgeThickness, icon_size.height()); (I also reordered things to be roughly in visual order) https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:600: browser_actions_x = site_chip_view_->x() + site_chip_view_->width(); Nit: Just "site_chip_view_->bounds().right()" suffices
+cpu for resources https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/190001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:582: site_chip_view_->GetPreferredSize().width() + On 2013/12/10 19:44:31, Peter Kasting wrote: > On 2013/12/10 17:37:59, Greg Billock wrote: > > I don't think the bug obtains -- I don't think site_chip_width can be < 0 > > No, but (available_width - kStandardSpacing) possibly can be. Yeah, but I thought I'd accounted for that. Anyway, I switched it to use what you suggested, which I think is simpler to understand. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:916: return site_chip_view_ ? site_chip_view_->location_icon_view() On 2013/12/10 19:44:31, Peter Kasting wrote: > Nit: Break after '?' instead of before ':' (2 places) Done. Done. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:298: (2 * (k16x16IconLeadingSpacing + k16x16IconTrailingSpacing)) : 0; On 2013/12/10 19:44:31, Peter Kasting wrote: > The "2 *" here is wrong. Done. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:303: icon_size.height()); On 2013/12/10 19:44:31, Peter Kasting wrote: > Nit: How about wrapping/indenting like: > > return gfx::Size( > kEdgeThickness + icon_spacing + icon_size.width() + kIconTextSpacing + > label_size.width() + kTrailingLabelMargin + kEdgeThickness, > icon_size.height()); > > (I also reordered things to be roughly in visual order) I like it. https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/92073003/diff/210001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:600: browser_actions_x = site_chip_view_->x() + site_chip_view_->width(); On 2013/12/10 19:44:31, Peter Kasting wrote: > Nit: Just "site_chip_view_->bounds().right()" suffices OK. I noticed that but followed the browser_actions_x calculation above. Changed that, too.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/92073003/270001
I haven't looked at this in full, but will asap to get up to speed for the subsequent CL. Thought I'd let you know about the compile error and a couple minor nits for now. https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:50: ~SiteChipExtensionIcon(); Error from http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_clang/... [chromium-style] Overriding method must have "virtual" keyword. https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:363: const gfx::Point& p) { nit: fix indent. https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:368: const gfx::Point& press_pt, nit: fix indent.
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/92073003/280001
https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:50: ~SiteChipExtensionIcon(); On 2013/12/11 00:16:19, msw wrote: > Error from > http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_clang/... > [chromium-style] Overriding method must have "virtual" keyword. Thanks, just saw this on the bot. Re-CQ-ing. https://codereview.chromium.org/92073003/diff/270001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:363: const gfx::Point& p) { On 2013/12/11 00:16:19, msw wrote: > nit: fix indent. oops! will fix in next round.
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/92073003/300001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
Committed patchset #18 manually as r240175 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:974: return site_chip_view_ ? I think this change will alter the point returned by LocationBarView::GetLocationBarAnchorPoint, which is used to draw the infobar arrows, I suspect that's okay, but I want to make sure it's intended and working as expected (since it uses some view-coordinate translation that might break by not hosting the location icon view). Please follow up with some cleanup to make that a function of ToolbarView (or similar) and not LocationBarView if it's no longer a point necessarily contained by LocationBarView. Filing an Issue to followup would be appreciated. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:104: string16 SiteChipView::SiteLabelFromURL(const GURL& url) { You definition order should match the declaration order. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:135: base::UTF8ToUTF16(extension->name()) : UTF8ToUTF16(url.host()); nit: be consistent about using the base:: namespace qualifier. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:152: if (StartsWith(formatted, ASCIIToUTF16("http://"), false)) chrome/browser/autocomplete/url_prefix.h might help here. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:169: return string16(UTF8ToUTF16(url.host())); nit: the string16() here seems like an unnecessary copy. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:211: void SiteChipView::Update(content::WebContents* web_contents) { Why is this argument necessary? https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:212: if (!web_contents) What happens on chrome://crash and chrome://hang? Are there other cases with NULL contents? https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:241: color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background)); Label does this automatically if you SetBackgroundColor(toolbar_background); https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:249: url.SchemeIs(chrome::kChromeUIScheme)) { Should you check for kAboutScheme as you do above? I suggest your checks be consistent. Otherwise, this fits on the line above. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:271: location_icon_view_->SetImage(GetThemeProvider()->GetImageSkiaNamed(icon)); Won't this potentially stomp the extension icon changes applied by constructing the SiteChipExtensionIcon? https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:46: void OnChanged(); nit: Add a comment.
Message was sent while issue was closed.
https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:974: return site_chip_view_ ? On 2013/12/11 21:54:44, msw wrote: > I think this change will alter the point returned by > LocationBarView::GetLocationBarAnchorPoint, which is used to draw the infobar > arrows, I suspect that's okay, but I want to make sure it's intended and working > as expected (since it uses some view-coordinate translation that might break by > not hosting the location icon view). Yep. That's the intention -- infobars will be drawn pointing to the location icon in the site chip. > > Please follow up with some cleanup to make that a function of ToolbarView (or > similar) and not LocationBarView if it's no longer a point necessarily contained > by LocationBarView. Filing an Issue to followup would be appreciated. Will do. I simplified this a bit earlier, making this patch clean, and moving it up to the toolbar may be possible. I think the way things currently work the location bar needs to know the infobar painting situation, but perhaps that's no longer true. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:104: string16 SiteChipView::SiteLabelFromURL(const GURL& url) { On 2013/12/11 21:54:44, msw wrote: > You definition order should match the declaration order. Yeah. I left them skewed to simplify patch diffs; shall I change this now in the next patch? https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:135: base::UTF8ToUTF16(extension->name()) : UTF8ToUTF16(url.host()); On 2013/12/11 21:54:44, msw wrote: > nit: be consistent about using the base:: namespace qualifier. Yes. I've been fighting with merges on this during the lifetime of the patch, and it needs final clean-up. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:152: if (StartsWith(formatted, ASCIIToUTF16("http://"), false)) On 2013/12/11 21:54:44, msw wrote: > chrome/browser/autocomplete/url_prefix.h might help here. We hope to swap this out for a more direct approach as in the TODO. This is basically the most conservative approach that produces results directly equivalent to what we have now. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:169: return string16(UTF8ToUTF16(url.host())); On 2013/12/11 21:54:44, msw wrote: > nit: the string16() here seems like an unnecessary copy. Agreed. I removed this subsequently. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:211: void SiteChipView::Update(content::WebContents* web_contents) { On 2013/12/11 21:54:44, msw wrote: > Why is this argument necessary? It's used in subsequent changes -- easier to keep it in this change. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:212: if (!web_contents) On 2013/12/11 21:54:44, msw wrote: > What happens on chrome://crash and chrome://hang? > Are there other cases with NULL contents? Good question. I'll try those out, but in general we shouldn't have them. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:241: color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background)); On 2013/12/11 21:54:44, msw wrote: > Label does this automatically if you SetBackgroundColor(toolbar_background); ok, so it manages visibility itself? Should I do it on 'this' as a whole? Or I need to do it on the label directly? Will setting the label background mess up any themes, or they are all solid toolbar colored? https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:249: url.SchemeIs(chrome::kChromeUIScheme)) { On 2013/12/11 21:54:44, msw wrote: > Should you check for kAboutScheme as you do above? I suggest your checks be > consistent. Otherwise, this fits on the line above. This changes in the subsequent patch to deal with a lot of custom cases. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:271: location_icon_view_->SetImage(GetThemeProvider()->GetImageSkiaNamed(icon)); On 2013/12/11 21:54:44, msw wrote: > Won't this potentially stomp the extension icon changes applied by constructing > the SiteChipExtensionIcon? Good point. I think you're right this should move up. Adjusted in the next patch. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:46: void OnChanged(); On 2013/12/11 21:54:44, msw wrote: > nit: Add a comment. Done.
Message was sent while issue was closed.
https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:104: string16 SiteChipView::SiteLabelFromURL(const GURL& url) { On 2013/12/11 22:34:08, Greg Billock wrote: > On 2013/12/11 21:54:44, msw wrote: > > You definition order should match the declaration order. > > Yeah. I left them skewed to simplify patch diffs; shall I change this now in the > next patch? A separate move-only CL would probably work best. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:241: color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background)); On 2013/12/11 22:34:08, Greg Billock wrote: > On 2013/12/11 21:54:44, msw wrote: > > Label does this automatically if you SetBackgroundColor(toolbar_background); > > ok, so it manages visibility itself? Should I do it on 'this' as a whole? Or I > need to do it on the label directly? Will setting the label background mess up > any themes, or they are all solid toolbar colored? Label::SetBackgroundColor won't draw a background, just determine a readable color from the text color you specify, almost exactly like you're doing here.
Message was sent while issue was closed.
https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:104: string16 SiteChipView::SiteLabelFromURL(const GURL& url) { On 2013/12/11 22:42:19, msw wrote: > On 2013/12/11 22:34:08, Greg Billock wrote: > > On 2013/12/11 21:54:44, msw wrote: > > > You definition order should match the declaration order. > > > > Yeah. I left them skewed to simplify patch diffs; shall I change this now in > the > > next patch? > > A separate move-only CL would probably work best. OK. I'll leave the next one as-is, then. After that there's some additional small hook-ups, UMAs, etc. and we can do this in parallel. https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:241: color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background)); On 2013/12/11 22:42:19, msw wrote: > On 2013/12/11 22:34:08, Greg Billock wrote: > > On 2013/12/11 21:54:44, msw wrote: > > > Label does this automatically if you SetBackgroundColor(toolbar_background); > > > > ok, so it manages visibility itself? Should I do it on 'this' as a whole? Or I > > need to do it on the label directly? Will setting the label background mess up > > any themes, or they are all solid toolbar colored? > > Label::SetBackgroundColor won't draw a background, just determine a readable > color from the text color you specify, almost exactly like you're doing here. Nice! So I need to adjust for the specialized backgrounds we draw for EV SSL, broken SSL, or malware, too, right? That should be easy.
Message was sent while issue was closed.
https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/92073003/diff/300001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:241: color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background)); On 2013/12/11 23:00:45, Greg Billock wrote: > On 2013/12/11 22:42:19, msw wrote: > > On 2013/12/11 22:34:08, Greg Billock wrote: > > > On 2013/12/11 21:54:44, msw wrote: > > > > Label does this automatically if you > SetBackgroundColor(toolbar_background); > > > > > > ok, so it manages visibility itself? Should I do it on 'this' as a whole? Or > I > > > need to do it on the label directly? Will setting the label background mess > up > > > any themes, or they are all solid toolbar colored? > > > > Label::SetBackgroundColor won't draw a background, just determine a readable > > color from the text color you specify, almost exactly like you're doing here. > > Nice! So I need to adjust for the specialized backgrounds we draw for EV SSL, > broken SSL, or malware, too, right? That should be easy. Yeah, updating for those backgrounds should help. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
