Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555063003:
Render extension URLs with chips (Closed)
DescriptionRender extension URLs with chips.
This CL reuses the security chip to display the extension name in the omnibox.
Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing
BUG=453093
Committed: https://crrev.com/172e00cdced29d37051bd7fc8faaeedb7416653c
Cr-Commit-Position: refs/heads/master@{#440298}
Patch Set 1 #Patch Set 2 : Fix linux and chromeos tests #Patch Set 3 : Fix Mac and tests #Patch Set 4 : Enlarge the window so that it's over the minimum window size #
Total comments: 24
Patch Set 5 : pkasting comments and rebase #
Total comments: 12
Patch Set 6 : pkasting comments #Patch Set 7 : IsEditingOrEmpty check #Patch Set 8 : pkasting comment #
Total comments: 2
Patch Set 9 : Chip -> text #Messages
Total messages: 40 (27 generated)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Render extension URLs with chips BUG=453093 ========== to ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/open?id=0B9q2eN9gDoUIelprUjk0dXlINHc BUG=453093 ==========
Description was changed from ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/open?id=0B9q2eN9gDoUIelprUjk0dXlINHc BUG=453093 ========== to ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing BUG=453093 ==========
meacer@chromium.org changed reviewers: + pkasting@chromium.org, rsesek@chromium.org
pkasting: Can you please take a look at c/b/ui/views/location_bar, c/b/ui/location_bar/location_bar.cc, components/toolbar? rsesek: Can you please take a look at chrome/browser/ui/cocoa/location_bar? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cocoa/ lgtm
https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: if (web_contents && url.SchemeIs(extensions::kExtensionScheme)) { Can there be no web_contents? Consider adding a comment about when. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:58: if (extension_registry) { Can this actually fail? https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:64: base::ASCIIToUTF16(" "), &extension_name); Why do you need this? It seems like CollapseWhitespace() will do this for you. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:109: // extension name. Nit: "associated with the page" is kinda vague; if a content script is acting on a page, is that "associated"? Maybe say "If |url| is an extension URL, returns the name of the associated extension, with whitespace collapsed. |web_contents| is used to get at the extension registry." https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:111: content::WebContents* web_contents) const; It seems like this method can be static, not just const. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:80: virtual const content::WebContents* GetWebContents() const = 0; Not clear to me why this is useful, since your GetExtensionName() function takes a non-const pointer. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:329: // URL. This seems inaccurate since we don't return true for chrome:// URLs. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:330: bool ShouldShowExtensionChip() const; Rather than adding this, and then having to add duplicate code blocks for the existing security chip stuff to handle the similar "extension chip" stuff, just change the "security chip" concept to speak broadly about the location icon also having text. (This also starts to phase out "chip", which is a meaningless word in our current presentation.) GetSecurityText() -> GetLocationIconText() ShouldShowSecurityChip() -> ShouldShowLocationIconText() ShouldAnimateSecurityChip() -> ShouldAnimateLocationIconTextVisibilityChange() etc. This will force you to think about how we should animate state changes in/out of showing the extension icon, to make sure ShouldAnimate...() returns the right thing. That's probably a good thing. I haven't thought deeply enough to be convinced the current patch does the right thing. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.h:52: void SetDisplayState(bool should_show, bool should_animate); Nit: I'd call this SetTextVisibility(). https://codereview.chromium.org/2555063003/diff/60001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2555063003/diff/60001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:72: if (GetURL().SchemeIs("chrome-extension")) { Nit: No {}
PTAL? (and sorry for the mid-review rebase) https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: if (web_contents && url.SchemeIs(extensions::kExtensionScheme)) { On 2016/12/14 22:47:07, Peter Kasting wrote: > Can there be no web_contents? Consider adding a comment about when. I don't see an obvious code path that would led to no web_contents, but LocationBarView code assumes it could be null in several places. Honestly, not sure what to document here. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:58: if (extension_registry) { On 2016/12/14 22:47:07, Peter Kasting wrote: > Can this actually fail? Looks like it can't, removed the check. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:64: base::ASCIIToUTF16(" "), &extension_name); On 2016/12/14 22:47:07, Peter Kasting wrote: > Why do you need this? It seems like CollapseWhitespace() will do this for you. This is the rebase of an old CL, and I seem to remember a case which CollapseWhitespace didn't handle. I can neither recall what it was nor reproduce it now, so removed. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:109: // extension name. On 2016/12/14 22:47:07, Peter Kasting wrote: > Nit: "associated with the page" is kinda vague; if a content script is acting on > a page, is that "associated"? Maybe say "If |url| is an extension URL, returns > the name of the associated extension, with whitespace collapsed. |web_contents| > is used to get at the extension registry." Done. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.h:111: content::WebContents* web_contents) const; On 2016/12/14 22:47:07, Peter Kasting wrote: > It seems like this method can be static, not just const. Done. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:80: virtual const content::WebContents* GetWebContents() const = 0; On 2016/12/14 22:47:07, Peter Kasting wrote: > Not clear to me why this is useful, since your GetExtensionName() function takes > a non-const pointer. LocationBarView::GetPreferredSize is a const method and it calls GetExtensionName, so I can't use the non-const GetWebContents there. So it's to keep the constness of the caller of GetExtensionName instead. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:329: // URL. On 2016/12/14 22:47:07, Peter Kasting wrote: > This seems inaccurate since we don't return true for chrome:// URLs. Removed and folded into ShouldShowSecurityText. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:330: bool ShouldShowExtensionChip() const; On 2016/12/14 22:47:07, Peter Kasting wrote: > Rather than adding this, and then having to add duplicate code blocks for the > existing security chip stuff to handle the similar "extension chip" stuff, just > change the "security chip" concept to speak broadly about the location icon also > having text. (This also starts to phase out "chip", which is a meaningless word > in our current presentation.) > > GetSecurityText() -> GetLocationIconText() > ShouldShowSecurityChip() -> ShouldShowLocationIconText() > ShouldAnimateSecurityChip() -> ShouldAnimateLocationIconTextVisibilityChange() > > etc. > > This will force you to think about how we should animate state changes in/out of > showing the extension icon, to make sure ShouldAnimate...() returns the right > thing. That's probably a good thing. I haven't thought deeply enough to be > convinced the current patch does the right thing. Sure, done. I checked with the UI folks and they said it's okay not to animate the extension text (reason being the animation is used to draw attention when the security state changes). I kept ShouldShowExtensionChip naming in the cocoa code though, as they already distinguish between security and EV chips. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.h:52: void SetDisplayState(bool should_show, bool should_animate); On 2016/12/14 22:47:07, Peter Kasting wrote: > Nit: I'd call this SetTextVisibility(). Done. https://codereview.chromium.org/2555063003/diff/60001/components/toolbar/tool... File components/toolbar/toolbar_model_impl.cc (right): https://codereview.chromium.org/2555063003/diff/60001/components/toolbar/tool... components/toolbar/toolbar_model_impl.cc:72: if (GetURL().SchemeIs("chrome-extension")) { On 2016/12/14 22:47:07, Peter Kasting wrote: > Nit: No {} Done.
https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: if (web_contents && url.SchemeIs(extensions::kExtensionScheme)) { On 2016/12/16 18:46:17, Mustafa Emre Acer wrote: > On 2016/12/14 22:47:07, Peter Kasting wrote: > > Can there be no web_contents? Consider adding a comment about when. > > I don't see an obvious code path that would led to no web_contents, but > LocationBarView code assumes it could be null in several places. Honestly, not > sure what to document here. IIRC, the places where LocationBarView does this are generally for tests. If you are being called from a chain that is doing such a check, you may need one too. Otherwise, I would avoid this until you prove you need it. (You can try CHECK(web_contents) temporarily to see if the trybots complain? If you're really unsure, you can ship that to users too, as long as you change it to a DCHECK after enough time has past that you're sure you've flushed out crashes.) https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:80: virtual const content::WebContents* GetWebContents() const = 0; On 2016/12/16 18:46:17, Mustafa Emre Acer wrote: > On 2016/12/14 22:47:07, Peter Kasting wrote: > > Not clear to me why this is useful, since your GetExtensionName() function > takes > > a non-const pointer. > > LocationBarView::GetPreferredSize is a const method and it calls > GetExtensionName, so I can't use the non-const GetWebContents there. So it's to > keep the constness of the caller of GetExtensionName instead. My point is, the const method you're adding returns a pointer-to-const. But GetExtensionName() requires a pointer-to-non-const. If the const method here were required, your code wouldn't compile, because the compiler would change from complaining that GetWebContents() is non-const to complaining that the argument to GetExtensionName() needs to be non-const. What's actually happening, I think, is that the compiler is not calling this new method at all. It's calling the non-const method, and it can get away with that because it's a method on |delegate_|, rather than a method on |this|, at the time of the call in GetPreferredSize(). Because what |delegate_| points to, and not the pointer itself, is being modified, that's legal. So I strongly suspect this is unused, and if you remove it, everything will keep working. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:52: base::string16 extension_name(base::UTF8ToUTF16(extension->name())); Nit: Prefer '=' to () for initializing strings; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: return base::CollapseWhitespace(extension_name, false); Nit: Using early-returns to unindent lets us rewrite this more briefly: if (!web_contents || !url.SchemeIs(extensions::kExtensionScheme) return base::string16(); ... base::string16 name = base::UTF8ToUTF16(extension->name()); return extension ? base::CollapseWhitespace(name) : base::string16(); https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:316: // security chip. This is somewhat misleading, since the extension name is not really about the URL's "security level". In general, since we're broadening the concept of this label beyond just "security text", I would avoid saying "security" in any of the function names here, as well as in comments (except where it is mentioned as one possible type of information to show). This is why I used "LocationIconText" and similar in the comment suggesting this refactor. That naming is also parallel to what the actual classes that display this are called anyway. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:318: content::WebContents* contents) const; Don't add these args; both callers pass the same values, and GetSecurityText() can call them itself as it's non-static and the methods can be called from const methods. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:327: // Returns true if the security text should be animated Nit: Trailing period
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: if (web_contents && url.SchemeIs(extensions::kExtensionScheme)) { On 2016/12/20 01:59:31, Peter Kasting wrote: > On 2016/12/16 18:46:17, Mustafa Emre Acer wrote: > > On 2016/12/14 22:47:07, Peter Kasting wrote: > > > Can there be no web_contents? Consider adding a comment about when. > > > > I don't see an obvious code path that would led to no web_contents, but > > LocationBarView code assumes it could be null in several places. Honestly, not > > sure what to document here. > > IIRC, the places where LocationBarView does this are generally for tests. If > you are being called from a chain that is doing such a check, you may need one > too. Otherwise, I would avoid this until you prove you need it. (You can try > CHECK(web_contents) temporarily to see if the trybots complain? If you're > really unsure, you can ship that to users too, as long as you change it to a > DCHECK after enough time has past that you're sure you've flushed out crashes.) Done, added a CHECK. https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:80: virtual const content::WebContents* GetWebContents() const = 0; On 2016/12/20 01:59:31, Peter Kasting wrote: > On 2016/12/16 18:46:17, Mustafa Emre Acer wrote: > > On 2016/12/14 22:47:07, Peter Kasting wrote: > > > Not clear to me why this is useful, since your GetExtensionName() function > > takes > > > a non-const pointer. > > > > LocationBarView::GetPreferredSize is a const method and it calls > > GetExtensionName, so I can't use the non-const GetWebContents there. So it's > to > > keep the constness of the caller of GetExtensionName instead. > > My point is, the const method you're adding returns a pointer-to-const. But > GetExtensionName() requires a pointer-to-non-const. If the const method here > were required, your code wouldn't compile, because the compiler would change > from complaining that GetWebContents() is non-const to complaining that the > argument to GetExtensionName() needs to be non-const. > > What's actually happening, I think, is that the compiler is not calling this new > method at all. It's calling the non-const method, and it can get away with that > because it's a method on |delegate_|, rather than a method on |this|, at the > time of the call in GetPreferredSize(). Because what |delegate_| points to, and > not the pointer itself, is being modified, that's legal. > > So I strongly suspect this is unused, and if you remove it, everything will keep > working. Done. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:52: base::string16 extension_name(base::UTF8ToUTF16(extension->name())); On 2016/12/20 01:59:31, Peter Kasting wrote: > Nit: Prefer '=' to () for initializing strings; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > . Done. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: return base::CollapseWhitespace(extension_name, false); On 2016/12/20 01:59:31, Peter Kasting wrote: > Nit: Using early-returns to unindent lets us rewrite this more briefly: > > if (!web_contents || !url.SchemeIs(extensions::kExtensionScheme) > return base::string16(); > Done. > ... > base::string16 name = base::UTF8ToUTF16(extension->name()); > return extension ? base::CollapseWhitespace(name) : base::string16(); This will do a null deref in extension->name() if extension is null. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:316: // security chip. On 2016/12/20 01:59:31, Peter Kasting wrote: > This is somewhat misleading, since the extension name is not really about the > URL's "security level". > > In general, since we're broadening the concept of this label beyond just > "security text", I would avoid saying "security" in any of the function names > here, as well as in comments (except where it is mentioned as one possible type > of information to show). > > This is why I used "LocationIconText" and similar in the comment suggesting this > refactor. That naming is also parallel to what the actual classes that display > this are called anyway. Sorry I missed your naming suggestion in the previous patchset. Done. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:318: content::WebContents* contents) const; On 2016/12/20 01:59:31, Peter Kasting wrote: > Don't add these args; both callers pass the same values, and GetSecurityText() > can call them itself as it's non-static and the methods can be called from const > methods. Done. https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:327: // Returns true if the security text should be animated On 2016/12/20 01:59:31, Peter Kasting wrote: > Nit: Trailing period Done.
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: return base::CollapseWhitespace(extension_name, false); On 2016/12/21 01:38:56, Mustafa Emre Acer wrote: > On 2016/12/20 01:59:31, Peter Kasting wrote: > > Nit: Using early-returns to unindent lets us rewrite this more briefly: > > > > if (!web_contents || !url.SchemeIs(extensions::kExtensionScheme) > > return base::string16(); > > > Done. > > > ... > > base::string16 name = base::UTF8ToUTF16(extension->name()); > > return extension ? base::CollapseWhitespace(name) : base::string16(); > > This will do a null deref in extension->name() if extension is null. Correct. This won't though: return extension ? base::CollapseWhitespace(base::UTF8ToUTF16(extension->name())) : base::string16();
https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... File chrome/browser/ui/location_bar/location_bar.cc (right): https://codereview.chromium.org/2555063003/diff/80001/chrome/browser/ui/locat... chrome/browser/ui/location_bar/location_bar.cc:53: return base::CollapseWhitespace(extension_name, false); On 2016/12/21 02:09:40, Peter Kasting wrote: > On 2016/12/21 01:38:56, Mustafa Emre Acer wrote: > > On 2016/12/20 01:59:31, Peter Kasting wrote: > > > Nit: Using early-returns to unindent lets us rewrite this more briefly: > > > > > > if (!web_contents || !url.SchemeIs(extensions::kExtensionScheme) > > > return base::string16(); > > > > > Done. > > > > > ... > > > base::string16 name = base::UTF8ToUTF16(extension->name()); > > > return extension ? base::CollapseWhitespace(name) : base::string16(); > > > > This will do a null deref in extension->name() if extension is null. > > Correct. This won't though: > > return extension > ? base::CollapseWhitespace(base::UTF8ToUTF16(extension->name())) > : base::string16(); Done.
LGTM https://codereview.chromium.org/2555063003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2555063003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:912: // Extension chips should not be animated (their security level is Nit: Trying to get away from the word "chip" -- maybe "Text for extension URLs should not be..."
Thanks. https://codereview.chromium.org/2555063003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2555063003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_view.cc:912: // Extension chips should not be animated (their security level is On 2016/12/22 00:14:46, Peter Kasting wrote: > Nit: Trying to get away from the word "chip" -- maybe "Text for extension URLs > should not be..." Sure, done.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2555063003/#ps180001 (title: "Chip -> text")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482366197892520,
"parent_rev": "98e387633cd723a0eed1b6517e05df17e84f30e6", "commit_rev":
"ba868ffc64ed1eda2dc069147ca6ce12a7f53861"}
Message was sent while issue was closed.
Description was changed from ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing BUG=453093 ========== to ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing BUG=453093 Review-Url: https://codereview.chromium.org/2555063003 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing BUG=453093 Review-Url: https://codereview.chromium.org/2555063003 ========== to ========== Render extension URLs with chips. This CL reuses the security chip to display the extension name in the omnibox. Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIelprUjk0dXlINHc/view?usp=sharing BUG=453093 Committed: https://crrev.com/172e00cdced29d37051bd7fc8faaeedb7416653c Cr-Commit-Position: refs/heads/master@{#440298} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/172e00cdced29d37051bd7fc8faaeedb7416653c Cr-Commit-Position: refs/heads/master@{#440298} |
