Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9372091:
Polished on side-loaded extension install alert. (Closed)
|
Created:
8 years, 10 months ago by chebert Modified:
8 years, 9 months ago CC:
Matt Tytel, Devlin, cduvall, mitchellwrosen, eriq.augustine_gmail.com, clintstaley Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPolished on side-loaded extension install alert.
BUG=111674
TEST=Install 2 external extensions. Check popup bubble on installation.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124874
Patch Set 1 #
Total comments: 1
Patch Set 2 : Issue 111674 #
Total comments: 2
Patch Set 3 : Corrections #
Total comments: 2
Patch Set 4 : Minor Corrections #
Total comments: 3
Patch Set 5 : Style fix. #Messages
Total messages: 18 (0 generated)
Please review my first CL: Polish on side-loaded extension install alert
Updated CL for Polished side-loaded extension alert bubble. https://chromiumcodereview.appspot.com/9372091/diff/1/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://chromiumcodereview.appspot.com/9372091/diff/1/chrome/app/chromium_str... chrome/app/chromium_strings.grd:613: <ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph> has been added. Why did you do this here, but not in the google_chrome_strings.grd? Make a decision but be consistent.
LGTM
LGTM. Note that I am not a Chromium committer, so my approval is neither required nor meaningful. Crawl up the OWNERS chain to find a suitable reviewer.
Please provide a screenshot of the new behavior... you can upload to imgur.com.
On 2012/02/28 22:55:16, Aaron Boodman wrote: > Please provide a screenshot of the new behavior... you can upload to http://imgur.com. [IMG]http://i42.tinypic.com/2u5vo69.png[/IMG]
We now have a double-line-break after the last entry. A hacky way to fix this would be to just remove the last character from the text if it is a newline.
http://codereview.chromium.org/9372091/diff/3001/chrome/app/chromium_strings.grd File chrome/app/chromium_strings.grd (right): http://codereview.chromium.org/9372091/diff/3001/chrome/app/chromium_strings.... chrome/app/chromium_strings.grd:608: <message name="IDS_EXTENSION_ALERT_ITEM_EXTERNAL" desc="A statement that an external extension has been newly installed. End users have no idea what an 'external' extension is, so we simply call them extensions."> It would be good to note here that the trailing character is expected to be a newline.
Here is an updated screenshot: http://imgur.com/JdlY2
https://chromiumcodereview.appspot.com/9372091/diff/3001/chrome/app/chromium_... File chrome/app/chromium_strings.grd (right): https://chromiumcodereview.appspot.com/9372091/diff/3001/chrome/app/chromium_... chrome/app/chromium_strings.grd:608: <message name="IDS_EXTENSION_ALERT_ITEM_EXTERNAL" desc="A statement that an external extension has been newly installed. End users have no idea what an 'external' extension is, so we simply call them extensions."> On 2012/02/29 01:39:57, Aaron Boodman wrote: > It would be good to note here that the trailing character is expected to be a > newline. Done.
LGTM Please correct the two minor comments before landing. https://chromiumcodereview.appspot.com/9372091/diff/15001/chrome/app/chromium... File chrome/app/chromium_strings.grd (right): https://chromiumcodereview.appspot.com/9372091/diff/15001/chrome/app/chromium... chrome/app/chromium_strings.grd:608: <message name="IDS_EXTENSION_ALERT_ITEM_EXTERNAL" desc="A statement that an external extension has been newly installed. End users have no idea what an 'external' extension is, so we simply call them extensions. The trailing character is expected to be a newline."> Sorry to nitpick here, but can you change this to: NOTE: The last character must be a newline. These files get translated by non-programmers. The message in "desc" gets presented to them in a UI. Any odd expectations must be carefully called out there. https://chromiumcodereview.appspot.com/9372091/diff/15001/chrome/browser/exte... File chrome/browser/extensions/extension_global_error.cc (right): https://chromiumcodereview.appspot.com/9372091/diff/15001/chrome/browser/exte... chrome/browser/extensions/extension_global_error.cc:97: if (message_[message_.size()-1] == '\n') { No curly brace needed for two-line if statements.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/93720...
Change committed as 124874
http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_global_error.cc (right): http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... chrome/browser/extensions/extension_global_error.cc:98: message_.resize(message_.size()-1); spaces around operators.
http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_global_error.cc (right): http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... chrome/browser/extensions/extension_global_error.cc:98: message_.resize(message_.size()-1); On 2012/03/19 21:45:56, Evan Stade wrote: > spaces around operators. Dang, how did I miss this... Thanks Evan.
http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_global_error.cc (right): http://codereview.chromium.org/9372091/diff/22001/chrome/browser/extensions/e... chrome/browser/extensions/extension_global_error.cc:98: message_.resize(message_.size()-1); On 2012/03/19 21:45:56, Evan Stade wrote: > spaces around operators. Done. |
