Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12463042:
Shows chrome-extension urls and greys out the whole url. (Closed)
|
Created:
7 years, 9 months ago by Patrick Riordan Modified:
7 years, 8 months ago Reviewers:
Devlin, Peter Kasting, jam, Avi (use Gerrit), arv (Not doing code reviews), Matt Perry, Use pkasting(at)chromium.org, Jeffrey Yasskin CC:
chromium-reviews, Aaron Boodman, James Su, tfarina, sail+watch_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionShows chrome-extension urls and greys out the whole url.
Original: http://i.imgur.com/JICZ06y.png
New: http://i.imgur.com/2twzZDL.png
BUG=72021
TBR=pkasting@chromium.org
TEST= See if chrome-extension urls are shown and see if the whole url is grey.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195131
Patch Set 1 #Patch Set 2 : Not hacky anymore #
Total comments: 18
Patch Set 3 : Changed name to GreyOutURL, and misc. #Patch Set 4 : Removed the correct extra newline #
Total comments: 4
Patch Set 5 : Fixed comment nits #
Total comments: 11
Patch Set 6 : Implementation does not depend on having a host and fixed nits. #
Total comments: 12
Patch Set 7 : Simpler and fixed nits #Patch Set 8 : Does not go through content #Patch Set 9 : No longer involves toolbar model #
Total comments: 10
Patch Set 10 : Fixed GetText calls and fixed logic errors #Patch Set 11 : Made the new ShouldDisplayURL, display extension schemes #Patch Set 12 : Fixed mac typos and fixed unit test #
Messages
Total messages: 45 (0 generated)
Haven't gone through all the code yet, but a couple of things... - be sure to mention on crbug.com that you're looking into it. This helps to ensure that no one else is also working on it, verifies it's still wanted, etc. - for changes which result in a UI change, please include a screenshot (or, even better, multiple - before and after, for instance) to the CL. (We usually do this through an imgur link or similar.)
On 2013/03/27 00:31:49, D Cronin wrote: > Haven't gone through all the code yet, but a couple of things... > - be sure to mention on http://crbug.com that you're looking into it. This helps to > ensure that no one else is also working on it, verifies it's still wanted, etc. > - for changes which result in a UI change, please include a screenshot (or, even > better, multiple - before and after, for instance) to the CL. (We usually do > this through an imgur link or similar.) Original: http://i.imgur.com/JICZ06y.png New: http://i.imgur.com/2twzZDL.png
One of the main things is that I really don't like the names "ShouldNotEmphasizeHost" and "DontEmphasizeHost"; it's entirely accurate, but double negatives when dealing with bools are awkward. I think that something like "ShouldGreyOutHost" and "GreyOutHost" might be better, but: - I'm not entirely sure I like that one, either; in particular, if we ever de-emphasize in a way other than greying, it's bad. - I'm open to new suggestions. What makes the most sense is actually "ShouldEmphasizeHost" - but this goes against the convention described in web_ui_impl.h:94, since it would default to true. https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/exte... File chrome/browser/extensions/extension_web_ui.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/exte... chrome/browser/extensions/extension_web_ui.cc:153: // Current behavior of other chrome::// pages is to emphasize URL host. Don't add an extra : https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/g... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/g... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1640: url_parse::Component scheme, host; Chrome style says to put each variable declaration on its own line; I know it's not your code, but since you're here, would you mind fixing it? https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/g... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1646: GtkTextIter start, end; Same as 1640. https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/g... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1653: remove extra newline. https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/g... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1656: GetUTF8Offset(text, Need to fix line wrapping. https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/t... File chrome/browser/ui/toolbar/toolbar_model.h (right): https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/t... chrome/browser/ui/toolbar/toolbar_model.h:74: // NOT be emphasized in the location bar. You should emphasize that this is just a handle to return the value from its parent WebContents' WebUI, as opposed to something determined by its own data (in addition to changing the name). https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/v... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2466: url_parse::Component scheme, host; Each declaration on its own line. https://chromiumcodereview.appspot.com/12463042/diff/2001/chrome/browser/ui/v... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2486: // We've found a host name, give it more emphasis. Update comment to mention that it's a host to which we want to provide emphasis (i.e., not a chrome-extension:// url), since we've done more than just find a host name. https://chromiumcodereview.appspot.com/12463042/diff/2001/content/browser/web... File content/browser/webui/web_ui_impl.h (right): https://chromiumcodereview.appspot.com/12463042/diff/2001/content/browser/web... content/browser/webui/web_ui_impl.h:99: bool should_not_emphasize_host_; Might be worth commenting (probably at the getters, since that's where the comment on line 94 directs us) when this would be the case (right now, this is only for chrome-extension:// urls).
https://codereview.chromium.org/12463042/diff/2001/chrome/browser/extensions/... File chrome/browser/extensions/extension_web_ui.cc (right): https://codereview.chromium.org/12463042/diff/2001/chrome/browser/extensions/... chrome/browser/extensions/extension_web_ui.cc:153: // Current behavior of other chrome::// pages is to emphasize URL host. On 2013/03/27 15:59:52, D Cronin wrote: > Don't add an extra : Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/gtk/omni... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1640: url_parse::Component scheme, host; On 2013/03/27 15:59:52, D Cronin wrote: > Chrome style says to put each variable declaration on its own line; I know it's > not your code, but since you're here, would you mind fixing it? Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1646: GtkTextIter start, end; On 2013/03/27 15:59:52, D Cronin wrote: > Same as 1640. Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1653: On 2013/03/27 15:59:52, D Cronin wrote: > remove extra newline. Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1656: GetUTF8Offset(text, On 2013/03/27 15:59:52, D Cronin wrote: > Need to fix line wrapping. Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/toolbar_model.h (right): https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/toolbar_model.h:74: // NOT be emphasized in the location bar. On 2013/03/27 15:59:52, D Cronin wrote: > You should emphasize that this is just a handle to return the value from its > parent WebContents' WebUI, as opposed to something determined by its own data > (in addition to changing the name). Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2466: url_parse::Component scheme, host; On 2013/03/27 15:59:52, D Cronin wrote: > Each declaration on its own line. Done. https://codereview.chromium.org/12463042/diff/2001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2486: // We've found a host name, give it more emphasis. On 2013/03/27 15:59:52, D Cronin wrote: > Update comment to mention that it's a host to which we want to provide emphasis > (i.e., not a chrome-extension:// url), since we've done more than just find a > host name. Done. https://codereview.chromium.org/12463042/diff/2001/content/browser/webui/web_... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/12463042/diff/2001/content/browser/webui/web_... content/browser/webui/web_ui_impl.h:99: bool should_not_emphasize_host_; On 2013/03/27 15:59:52, D Cronin wrote: > Might be worth commenting (probably at the getters, since that's where the > comment on line 94 directs us) when this would be the case (right now, this is > only for chrome-extension:// urls). Done.
lgtm with nits. https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ext... File chrome/browser/extensions/extension_web_ui.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ext... chrome/browser/extensions/extension_web_ui.cc:153: // Current behavior of other chrome:// pages is to emphasize host, "emphasize the host" https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2488: // We've found a host name that we should provide emphasis, so grammatical error; maybe "We've found a non-internal host name; give it more emphasis."
https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ext... File chrome/browser/extensions/extension_web_ui.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ext... chrome/browser/extensions/extension_web_ui.cc:153: // Current behavior of other chrome:// pages is to emphasize host, On 2013/04/01 20:35:28, D Cronin wrote: > "emphasize the host" Done. https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/13004/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2488: // We've found a host name that we should provide emphasis, so On 2013/04/01 20:35:28, D Cronin wrote: > grammatical error; maybe "We've found a non-internal host name; give it more > emphasis." As of this patch its only extension host names that we don't provide emphasis to. chrome:// pages still have emphasis. I thought that if I mentioned that, the comment would be incorrect if ShouldGreyOutURL is ever used for something besides chrome-extension. So I changed the comment to be more grammatically correct, but kept the first meaning. If you think it's appropriate I can add that currently it's only chrome-extension:// pages.
+ sky for Omnibox ui changes. Hopefully I didn't miss a more suitable reviewer.
pkasting knows this code better than I. https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:184: if (web_contents && web_contents->GetWebUIForCurrentState()) nit: make this a single return, eg: return web_contents && web_contents->.. && web_contents()->GetWe...
On 2013/04/03 04:03:57, sky wrote: > pkasting knows this code better than I. I'll take a look tomorrow.
https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:453: const bool emphasize = model()->CurrentTextIsURL() && (host.len > 0); I don't think ShouldGreyOutURL()'s implementation should depend on having a host. I'd rewrite this block as follows: ... ParseForEmphasizeComponents(...); if (model()->CurrentTextIsURL() && (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL())) { ... Grey string if (!toolbar_model()->ShouldGreyOutURL()) { ... Black host Make this same change in the other platform-specific implementations too. https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1641: url_parse::Component host; Nit: Splitting these declarations onto multiple lines isn't necessary. The old way was fine, arguably superior. (2 places) https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model.h (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model.h:75: // location bar. Nit: The comment shouldn't be talking about implementation detail so much. Just say: // Returns whether the URL for the current navigation entry should be displayed in all grey, as opposed to with the host in black (the default behavior). https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2467: url_parse::Component host; Nit: Again, don't split these https://chromiumcodereview.appspot.com/12463042/diff/25001/content/public/bro... File content/public/browser/web_ui.h (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/content/public/bro... content/public/browser/web_ui.h:75: // Returns true if the pages URL should be entirely greyed out, currently Nit: Comma -> period and capitalize next word
https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:453: const bool emphasize = model()->CurrentTextIsURL() && (host.len > 0); On 2013/04/03 22:32:13, Peter Kasting wrote: > I don't think ShouldGreyOutURL()'s implementation should depend on having a > host. I'd rewrite this block as follows: > > ... > ParseForEmphasizeComponents(...); > if (model()->CurrentTextIsURL() && > (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL())) { > ... Grey string > > if (!toolbar_model()->ShouldGreyOutURL()) { > ... Black host > > Make this same change in the other platform-specific implementations too. Done. https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1641: url_parse::Component host; On 2013/04/03 22:32:13, Peter Kasting wrote: > Nit: Splitting these declarations onto multiple lines isn't necessary. The old > way was fine, arguably superior. (2 places) Done. https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model.h (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model.h:75: // location bar. On 2013/04/03 22:32:13, Peter Kasting wrote: > Nit: The comment shouldn't be talking about implementation detail so much. Just > say: > > // Returns whether the URL for the current navigation entry should be displayed > in all grey, as opposed to with the host in black (the default behavior). Done. https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2467: url_parse::Component host; On 2013/04/03 22:32:13, Peter Kasting wrote: > Nit: Again, don't split these Done. https://chromiumcodereview.appspot.com/12463042/diff/25001/content/public/bro... File content/public/browser/web_ui.h (right): https://chromiumcodereview.appspot.com/12463042/diff/25001/content/public/bro... content/public/browser/web_ui.h:75: // Returns true if the pages URL should be entirely greyed out, currently On 2013/04/03 22:32:13, Peter Kasting wrote: > Nit: Comma -> period and capitalize next word Done.
LGTM https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:458: if (host.is_nonempty() && !toolbar_model()->ShouldGreyOutURL()) { Nit: No need to add the first clause, it will always be true if the second clause holds. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: if (model()->CurrentTextIsURL() && Nit: Slightly simpler: bool grey_base = model()->CurrentTextIsURL() && (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); gtk_text_buffer_apply_tag( text_buffer_, grey_base ? faded_text_tag_ : normal_text_tag_, &start, &end); if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { // We've found a host name, give it more emphasis. gtk_text_buffer_get_iter_at_line_index( text_buffer_, &start, 0, GetUTF8Offset(text, host.begin)); gtk_text_buffer_get_iter_at_line_index( text_buffer_, &end, 0, GetUTF8Offset(text, host.end())); gtk_text_buffer_apply_tag(text_buffer_, normal_text_tag_, &start, &end); } https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:185: web_contents->GetWebUIForCurrentState()->ShouldGreyOutURL(); Nit: Indent 4, not even https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:831: if (model()->CurrentTextIsURL() && Nit: Slightly simpler: bool grey_base = model()->CurrentTextIsURL() && (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); SetColor(location_bar_view_->GetColor( security_level_, grey_base ? LocationBarView::DEEMPHASIZED_TEXT : LocationBarView::TEXT)); if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { ApplyColor( location_bar_view_->GetColor(security_level_, LocationBarView::TEXT), ui::Range(host.begin, host.end())); } https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2479: const bool emphasize = model()->CurrentTextIsURL() && (host.len > 0); Nit: Remove this, it's unused https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2487: if (model()->CurrentTextIsURL() && Nit: Slightly simpler: bool grey_base = model()->CurrentTextIsURL() && (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); cf.crTextColor = skia::SkColorToCOLORREF(parent_view_->GetColor( security_level_, grey_base ? LocationBarView::DEEMPHASIZED_TEXT : LocationBarView::TEXT)); // NOTE: Don't use SetDefaultCharFormat() instead of the below; that sets the // format that will get applied to text added in the future, not to text // already in the edit. SelectAll(false); SetSelectionCharFormat(cf); if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { // We've found a host name and we should provide emphasis to host names, so // emphasize it. cf.crTextColor = skia::SkColorToCOLORREF(parent_view_->GetColor( security_level_, LocationBarView::TEXT)); SetSelectionCharFormat(cf); }
https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:458: if (host.is_nonempty() && !toolbar_model()->ShouldGreyOutURL()) { On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: No need to add the first clause, it will always be true if the second > clause holds. Done. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1659: if (model()->CurrentTextIsURL() && On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: Slightly simpler: > > bool grey_base = model()->CurrentTextIsURL() && > (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); > gtk_text_buffer_apply_tag( > text_buffer_, grey_base ? faded_text_tag_ : normal_text_tag_, &start, > &end); > if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { > // We've found a host name, give it more emphasis. > gtk_text_buffer_get_iter_at_line_index( > text_buffer_, &start, 0, GetUTF8Offset(text, host.begin)); > gtk_text_buffer_get_iter_at_line_index( > text_buffer_, &end, 0, GetUTF8Offset(text, host.end())); > gtk_text_buffer_apply_tag(text_buffer_, normal_text_tag_, &start, &end); > } Done. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/toolbar/toolbar_model_impl.cc:185: web_contents->GetWebUIForCurrentState()->ShouldGreyOutURL(); On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: Indent 4, not even Done. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:831: if (model()->CurrentTextIsURL() && On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: Slightly simpler: > > bool grey_base = model()->CurrentTextIsURL() && > (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); > SetColor(location_bar_view_->GetColor( > security_level_, > grey_base ? LocationBarView::DEEMPHASIZED_TEXT : LocationBarView::TEXT)); > if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { > ApplyColor( > location_bar_view_->GetColor(security_level_, LocationBarView::TEXT), > ui::Range(host.begin, host.end())); > } Done. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2479: const bool emphasize = model()->CurrentTextIsURL() && (host.len > 0); On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: Remove this, it's unused Done. https://chromiumcodereview.appspot.com/12463042/diff/40001/chrome/browser/ui/... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2487: if (model()->CurrentTextIsURL() && On 2013/04/04 21:07:05, Peter Kasting wrote: > Nit: Slightly simpler: > > bool grey_base = model()->CurrentTextIsURL() && > (host.is_nonempty() || toolbar_model()->ShouldGreyOutURL()); > cf.crTextColor = skia::SkColorToCOLORREF(parent_view_->GetColor( > security_level_, > grey_base ? LocationBarView::DEEMPHASIZED_TEXT : LocationBarView::TEXT)); > // NOTE: Don't use SetDefaultCharFormat() instead of the below; that sets the > // format that will get applied to text added in the future, not to text > // already in the edit. > SelectAll(false); > SetSelectionCharFormat(cf); > if (grey_base && !toolbar_model()->ShouldGreyOutURL()) { > // We've found a host name and we should provide emphasis to host names, so > // emphasize it. > cf.crTextColor = skia::SkColorToCOLORREF(parent_view_->GetColor( > security_level_, LocationBarView::TEXT)); > SetSelectionCharFormat(cf); > } Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/12463042/4...
Presubmit check for 12463042-49001 failed and returned exit status 1.
INFO:root:Found 11 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/public/browser/web_ui.h
content/browser/webui/web_ui_impl.cc
chrome/browser/extensions/extension_web_ui.cc
content/browser/webui/web_ui_impl.h
Presubmit checks took 1.6s to calculate.
+ arv content/browser/webui/web_ui_impl.h and .cc + rdsmith content/public/browser/web_ui.h + jyasskin chrome/browser/extensions/extension_web_ui.cc
As noted in the OWNERS file, I'm only able to give reviews on download files, which these aren't. Substituting joi.
I'm not an owner for content/browser. Substituting avi@ who should be able to give you the review for all your stuff in content/. Cheers, Jói
lgtm for extension_web_ui
Extensions are a chrome feature. URL bars are a chrome feature. This patch drops both concepts straight into content, and I'm not comfortable with that. Why does content need to know about all of this?
On 2013/04/10 14:11:07, Avi wrote: > Extensions are a chrome feature. URL bars are a chrome feature. This patch drops > both concepts straight into content, and I'm not comfortable with that. Why does > content need to know about all of this? agreed. this was probably done to match FocusLocationBarByDefault and HideURL. these really shouldn't be there though. perhaps if WebUI derived from base::SupportsUserData, chrome can stash whatever information it wants on it that way?
On 2013/04/10 14:11:07, Avi wrote: > Extensions are a chrome feature. URL bars are a chrome feature. This patch drops > both concepts straight into content, and I'm not comfortable with that. Why does > content need to know about all of this? The existing methods right before Patrick's new GreyOutURL methods are: // Returns true if the favicon should be hidden for the current tab. virtual bool ShouldHideFavicon() const = 0; virtual void HideFavicon() = 0; // Returns true if the location bar should be focused by default rather than // the page contents. Some pages will want to use this to encourage the user // to type in the URL bar. virtual bool ShouldFocusLocationBarByDefault() const = 0; virtual void FocusLocationBarByDefault() = 0; // Returns true if the page's URL should be hidden. Some Web UI pages // like the new tab page will want to hide it. virtual bool ShouldHideURL() const = 0; virtual void HideURL() = 0; ... What do you mean that URL bars are not a content feature?
The dividing line between content and chrome is a bit fuzzy; we're trying to make content be "put this web content in a rectangle" and we've failed/succeeded to varying degrees. That's WebUI, and we tried to rip that out of content too and gave up because it was too much of an architectural headache. I'm hoping we can do better here.
On 2013/04/10 15:45:58, Avi wrote: > The dividing line between content and chrome is a bit fuzzy; we're trying to > make content be "put this web content in a rectangle" and we've failed/succeeded > to varying degrees. > > That's WebUI, and we tried to rip that out of content too and gave up because it > was too much of an architectural headache. I'm hoping we can do better here. see my previous line above, the methods you point to for justification shouldn't be there. so we shouldn't add more, but instead should fix them. re webui, we put it in content also because content has its own features that it wants to expose webui pages for. src\chromeos also depends on content and wants to serve webui pages (i.e. login).
On 2013/04/10 20:13:32, jam wrote: > On 2013/04/10 15:45:58, Avi wrote: > > The dividing line between content and chrome is a bit fuzzy; we're trying to > > make content be "put this web content in a rectangle" and we've > failed/succeeded > > to varying degrees. > > > > That's WebUI, and we tried to rip that out of content too and gave up because > it > > was too much of an architectural headache. I'm hoping we can do better here. > > see my previous line above, the methods you point to for justification shouldn't > be there. so we shouldn't add more, but instead should fix them. btw looking at the existing ones more, it looks like some of them aren't needed, or can be done in different ways. i'll look at cleaning this up now. > > re webui, we put it in content also because content has its own features that it > wants to expose webui pages for. src\chromeos also depends on content and wants > to serve webui pages (i.e. login).
> btw looking at the existing ones more, it looks like some of them aren't needed, > or can be done in different ways. i'll look at cleaning this up now. I've removed them all in https://codereview.chromium.org/14080004/
So I should implement ShouldGreyOutURL in ToolbarModelImpl like jam's cl and just check the host?
On 2013/04/11 00:01:25, Patrick Riordan wrote: > So I should implement ShouldGreyOutURL in ToolbarModelImpl like jam's cl and > just check the host? yes
On 2013/04/11 00:13:42, jam wrote: > On 2013/04/11 00:01:25, Patrick Riordan wrote: > > So I should implement ShouldGreyOutURL in ToolbarModelImpl like jam's cl and > > just check the host? > > yes Changed. Whoever's cl gets committed first will have edit ShouldDisplayURL to return true with kExtensionSchemes btw.
After thinking more, I'm wondering why we need to even involve the toolbar model. I'm worried that doing so can introduce bugs -- the text-coloring code in the address bar cares about the current text in the address bar, which the toolbar model doesn't actually know or care about. So asking that model if the current text should be greyed is wrong. Instead the code that colors the text should just check directly for an extension scheme itself, the way it already looks for a non-empty host. I think this will be more correct and will remove yet more files from this CL. I should have thought of this before. Sorry.
On 2013/04/11 05:44:52, Peter Kasting wrote: > After thinking more, I'm wondering why we need to even involve the toolbar > model. > > I'm worried that doing so can introduce bugs -- the text-coloring code in the > address bar cares about the current text in the address bar, which the toolbar > model doesn't actually know or care about. So asking that model if the current > text should be greyed is wrong. > > Instead the code that colors the text should just check directly for an > extension scheme itself, the way it already looks for a non-empty host. I think > this will be more correct and will remove yet more files from this CL. > > I should have thought of this before. Sorry. Done.
https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:452: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == Use |display_text| here instead of GetText(). https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:459: if (grey_out_url) { Isn't this conditional backwards? https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/gtk/omn... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/gtk/omn... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1660: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == Use |text| here instead of GetText(). https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:850: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == Use text() here instead of GetText(). https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2477: AutocompleteInput::ParseForEmphasizeComponents(GetText(), &scheme, &host); Pull this GetText() call out into a string16 temp and use that temp in your new code below.
https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:452: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == On 2013/04/11 23:10:40, Peter Kasting wrote: > Use |display_text| here instead of GetText(). Done. Sorry. This patch set was sloppy. https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/cocoa/o... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:459: if (grey_out_url) { On 2013/04/11 23:10:40, Peter Kasting wrote: > Isn't this conditional backwards? Done. https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/gtk/omn... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/gtk/omn... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1660: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == On 2013/04/11 23:10:40, Peter Kasting wrote: > Use |text| here instead of GetText(). Done. https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:850: bool grey_out_url = GetText().substr(scheme.begin, scheme.len) == On 2013/04/11 23:10:40, Peter Kasting wrote: > Use text() here instead of GetText(). Done. https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/12463042/diff/80001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2477: AutocompleteInput::ParseForEmphasizeComponents(GetText(), &scheme, &host); On 2013/04/11 23:10:40, Peter Kasting wrote: > Pull this GetText() call out into a string16 temp and use that temp in your new > code below. Done.
LGTM
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Wrong, commit-bot, I'm a valid committer. LGTM (again).
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On Sat, Apr 13, 2013 at 9:08 PM, <commit-bot@chromium.org> wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a lowly provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. > Marc, is there something wrong with CQ? Looks like it isn't accepting Peter's approval. > https://chromiumcodereview.appspot.com/12463042/ -- Thiago
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/12463042/9...
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/patrickriordan177@gmail.com/12463042/1...
Message was sent while issue was closed.
Change committed as 195131
Message was sent while issue was closed.
Awesome! So happy to see us finally try this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
