Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10830098:
Get the Firefox branding name dynamically (Closed)
|
Created:
8 years, 4 months ago by cristian.patrasciuc Modified:
8 years, 4 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, tfarina, Elliot Glaysher Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionGet the Firefox branding name dynamically
from the application.ini file.
This way we can display different names in the importer combobox,
depending on the Firefox flavour that is installed on the machine.
One such example is the Debian rebranding of Firefox, i.e. Iceweasel.
BUG=119279
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151032
Patch Set 1 : #
Total comments: 31
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 25 (0 generated)
Ilya, I added you as a reviewer. If you think I should add someone else too (component owners?), please tell me. I tested it with Iceweasel 3.5.16, on Debian 6.
Thanks, this looks like it's totally headed in the right direction :) Lots of small comments inline. (Apologies for the delay in getting to this review -- I've had a lot of meetings this week, so not much time for actual code.) https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/app/generated_r... chrome/app/generated_resources.grd:6627: + Mozilla Iceweasel nit: I think the product's name is either "GNU Iceweasel" or just "Iceweasel" -- it's not a Mozilla product. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:423: if (file_util::PathExists(app_ini_file)) { nit: To reduce nesting, prefer writing code like this with early returns: if (!file_util::PathExists(app_ini_file)) return; // remainder of the code https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:430: const std::string name_attr("Name="); nit: This looks like it should be a file-level constant defined in the anonymous namespace at the top of the file. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:431: for (unsigned int i = 0; i < lines.size(); ++i) { nit: For Chromium code, prefer |size_t| to |unsigned int|. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:438: } nit: Since the if- and else-statements have one-line bodies, no need for curly braces. Also, we generally prefer to avoid else statements balancing if statements that return a value; i.e. we prefer to write if (foo) return bar; else fizzbang(); as if (foo) return bar; fizzbang() https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:440: in_app_section = true; Are there any other sections for which "Name=" can be defined? If so, you should probably set in_app_section back to false when any other section is encountered. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:447: << " has no [App] section"; nit: Ditto to the comment below for these logging statements. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:450: << app_ini_file.value() << " does not exist."; nit: No need to log a warning in this case. Log statements add to the binary size, and if all code were to log all possible rare cases, the logs would grow to be completely unmanageable. Logging statements like this one are generally only added temporarily to debug a problem. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.h (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.h:114: // the function returns an empty string. Optional nit: This comment is very nice and detailed (thanks for taking the time to write it!), but is perhaps a little much to include in the header file. I would recommend trimming it down to the essentials for the header, and moving the rest into the implementation file. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:79: << " is not supported."; Please revert this change. Chrome generally only logs warnings and errors in cases of exceptional code paths. Anything that is intended to be user-visible should be shown in the browser's UI. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:84: const std::string branding_name = GetFirefoxBrandingName(app_path); I would suggestion moving the below if-else logic into GetFirefoxBrandingName, and renaming that function to GetFirefoxImporterName. The code in this file should need to know as little as possible about each of the different import sources.
Also, have you completed the Contributor License Agreement, as described here? http://dev.chromium.org/developers/contributing-code#TOC-Get-your-code-ready
I will upload a new diff, once the above questions are answered. Thanks. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:430: const std::string name_attr("Name="); It is only used here. Should I really make it a file-level constant in this case? On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: This looks like it should be a file-level constant defined in the anonymous > namespace at the top of the file. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:438: } I added the curly braces because otherwise the compiler would give me a warning (treated as an error by Chromium): error: suggest explicit braces to avoid ambiguous ‘else’ I can probably remove some of them while still keeping the compiler happy, but I chose to add all for clarity. On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: Since the if- and else-statements have one-line bodies, no need for curly > braces. Also, we generally prefer to avoid else statements balancing if > statements that return a value; i.e. we prefer to write > > if (foo) > return bar; > else > fizzbang(); > > as > > if (foo) > return bar; > > fizzbang() https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:440: in_app_section = true; When you are in the [App] section and another section starts, the for-loop exits (see line 436-437). On 2012/08/03 23:12:29, Ilya Sherman wrote: > Are there any other sections for which "Name=" can be defined? If so, you > should probably set in_app_section back to false when any other section is > encountered. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:84: const std::string branding_name = GetFirefoxBrandingName(app_path); This will make unit tests depend on l10n/generated resources. Also, it would be harder to test if the Firefox importer name came up because that's what the application.ini file contains or just because something unexpected happened and the Firefox name is used as a fallback. So, should I still move this to GetFirefoxImporterName in this case? On 2012/08/03 23:12:29, Ilya Sherman wrote: > I would suggestion moving the below if-else logic into GetFirefoxBrandingName, > and renaming that function to GetFirefoxImporterName. The code in this file > should need to know as little as possible about each of the different import > sources.
https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:430: const std::string name_attr("Name="); On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > It is only used here. Should I really make it a file-level constant in this > case? It's fine either way. Typically, we prefer to have file-level constants to inline strings; but looking at this particular code again, I can see why it might be more convenient to just have the constant inlined here. > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > nit: This looks like it should be a file-level constant defined in the > anonymous > > namespace at the top of the file. > https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:436: } else if (lines[i][0] == '[') { You should check whether lines[i] is empty prior to indexing into it. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:438: } On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > I added the curly braces because otherwise the compiler would give me a warning > (treated as an error by Chromium): > error: suggest explicit braces to avoid ambiguous ‘else’ The else is not ambiguous if the outer if-stmt has curly braces (which it is required to by the Chromium style guide). > I can probably remove some of them while still keeping the compiler happy, but I > chose to add all for clarity. This is obviously a very minor style issue, but the common style for Chromium code is to omit curly braces for if- and else-stmts whose conditions and bodies are exactly one line each, and to include them otherwise. Since this is the common style throughout the codebase, adhering to it helps readability; otherwise, developers familiar with the working style will be distracted by the curly braces, spending a few thought cycles wondering if they are there because there's something subtle about the code that requires them. Again, it's not a big deal, but this is why I'd encourage you to omit the curly braces. > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > nit: Since the if- and else-statements have one-line bodies, no need for curly > > braces. Also, we generally prefer to avoid else statements balancing if > > statements that return a value; i.e. we prefer to write > > > > if (foo) > > return bar; > > else > > fizzbang(); > > > > as > > > > if (foo) > > return bar; > > > > fizzbang() > https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:440: in_app_section = true; On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > When you are in the [App] section and another section starts, the for-loop exits > (see line 436-437). Ah, you're right. Apologies for missing that. > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > Are there any other sections for which "Name=" can be defined? If so, you > > should probably set in_app_section back to false when any other section is > > encountered. > https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:84: const std::string branding_name = GetFirefoxBrandingName(app_path); On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > This will make unit tests depend on l10n/generated resources. That's ok. > Also, it would be > harder to test if the Firefox importer name came up because that's what the > application.ini file contains or just because something unexpected happened and > the Firefox name is used as a fallback. You'll still be able to test with both test inputs, so you're not actually losing any test coverage. In fact, you're gaining coverage, as this code in importer_list is currently untested. > So, should I still move this to GetFirefoxImporterName in this case? I think so, yes :) > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > I would suggestion moving the below if-else logic into GetFirefoxBrandingName, > > and renaming that function to GetFirefoxImporterName. The code in this file > > should need to know as little as possible about each of the different import > > sources. >
I uploaded a new patch set. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/app/generated_r... chrome/app/generated_resources.grd:6627: + Mozilla Iceweasel On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: I think the product's name is either "GNU Iceweasel" or just "Iceweasel" -- > it's not a Mozilla product. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:423: if (file_util::PathExists(app_ini_file)) { Now, if the file does not exist we don't just return, we have to obtain the localized name for Firefox. I chose to leave it this way to keep the default fallback value in one single place (at the end of the function). On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: To reduce nesting, prefer writing code like this with early returns: > > if (!file_util::PathExists(app_ini_file)) > return; > > // remainder of the code https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:430: const std::string name_attr("Name="); On 2012/08/06 21:07:37, Ilya Sherman wrote: > On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > > It is only used here. Should I really make it a file-level constant in this > > case? > > It's fine either way. Typically, we prefer to have file-level constants to > inline strings; but looking at this particular code again, I can see why it > might be more convenient to just have the constant inlined here. > > > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > > nit: This looks like it should be a file-level constant defined in the > > anonymous > > > namespace at the top of the file. > > Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:431: for (unsigned int i = 0; i < lines.size(); ++i) { On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: For Chromium code, prefer |size_t| to |unsigned int|. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:436: } else if (lines[i][0] == '[') { On 2012/08/06 21:07:37, Ilya Sherman wrote: > You should check whether lines[i] is empty prior to indexing into it. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:438: } On 2012/08/06 21:07:37, Ilya Sherman wrote: > On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > > I added the curly braces because otherwise the compiler would give me a > warning > > (treated as an error by Chromium): > > error: suggest explicit braces to avoid ambiguous ‘else’ > > The else is not ambiguous if the outer if-stmt has curly braces (which it is > required to by the Chromium style guide). > > > I can probably remove some of them while still keeping the compiler happy, but > I > > chose to add all for clarity. > > This is obviously a very minor style issue, but the common style for Chromium > code is to omit curly braces for if- and else-stmts whose conditions and bodies > are exactly one line each, and to include them otherwise. Since this is the > common style throughout the codebase, adhering to it helps readability; > otherwise, developers familiar with the working style will be distracted by the > curly braces, spending a few thought cycles wondering if they are there because > there's something subtle about the code that requires them. Again, it's not a > big deal, but this is why I'd encourage you to omit the curly braces. > > > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > > nit: Since the if- and else-statements have one-line bodies, no need for > curly > > > braces. Also, we generally prefer to avoid else statements balancing if > > > statements that return a value; i.e. we prefer to write > > > > > > if (foo) > > > return bar; > > > else > > > fizzbang(); > > > > > > as > > > > > > if (foo) > > > return bar; > > > > > > fizzbang() > > > Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:447: << " has no [App] section"; On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: Ditto to the comment below for these logging statements. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.cc:450: << app_ini_file.value() << " does not exist."; On 2012/08/03 23:12:29, Ilya Sherman wrote: > nit: No need to log a warning in this case. Log statements add to the binary > size, and if all code were to log all possible rare cases, the logs would grow > to be completely unmanageable. Logging statements like this one are generally > only added temporarily to debug a problem. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/firefox_importer_utils.h (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/firefox_importer_utils.h:114: // the function returns an empty string. On 2012/08/03 23:12:29, Ilya Sherman wrote: > Optional nit: This comment is very nice and detailed (thanks for taking the time > to write it!), but is perhaps a little much to include in the header file. I > would recommend trimming it down to the essentials for the header, and moving > the rest into the implementation file. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:79: << " is not supported."; On 2012/08/03 23:12:29, Ilya Sherman wrote: > Please revert this change. Chrome generally only logs warnings and errors in > cases of exceptional code paths. Anything that is intended to be user-visible > should be shown in the browser's UI. Done. https://chromiumcodereview.appspot.com/10830098/diff/7/chrome/browser/importe... chrome/browser/importer/importer_list.cc:84: const std::string branding_name = GetFirefoxBrandingName(app_path); On 2012/08/06 21:07:37, Ilya Sherman wrote: > On 2012/08/06 15:12:39, cristian.patrasciuc wrote: > > This will make unit tests depend on l10n/generated resources. > > That's ok. > > > Also, it would be > > harder to test if the Firefox importer name came up because that's what the > > application.ini file contains or just because something unexpected happened > and > > the Firefox name is used as a fallback. > > You'll still be able to test with both test inputs, so you're not actually > losing any test coverage. In fact, you're gaining coverage, as this code in > importer_list is currently untested. > > > So, should I still move this to GetFirefoxImporterName in this case? > > I think so, yes :) > > > On 2012/08/03 23:12:29, Ilya Sherman wrote: > > > I would suggestion moving the below if-else logic into > GetFirefoxBrandingName, > > > and renaming that function to GetFirefoxImporterName. The code in this file > > > should need to know as little as possible about each of the different import > > > sources. > > > Done.
Lookin' good! Just a few small style comments left :) https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/app/generat... chrome/app/generated_resources.grd:6626: + <message name="IDS_IMPORT_FROM_ICEWEASEL" desc="browser combo box: Mozilla Iceweasel"> nit: Please remove "Mozilla" from this line as well https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:441: ReplaceSubstringsAfterOffset(&content, 0, "\r\n", "\n"); nit: Is this necessary? It seems like the TrimWhitespace() call in the for loop should take care of it. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:450: branding_name = lines[i].substr(name_attr.size()); nit: You probably want to break out of the for loop in this case. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:452: break; nit: Please add a quick comment along the lines of "No longer in the app section." https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:455: } Optional nit: I would move this bit of logic to be the first part of the if-else branching, for two reasons: (1) It will happen first when scanning over the lines; there's no way to enter the if-stmt prior to ever entering this else-stmt. (2) It's slightly simpler/briefer, and typically code is organized so that simpler/briefer logic precedes meatier logic. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:457: } nit: Please add a blank line after this line https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:459: branding_name.find("iceweasel") != std::string::npos) nit: Rather than searching for both strings, how about converting the branding_name to all lowercase first? That way IceWeasel would also be fine, for example. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/firefox_importer_utils_unittest.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils_unittest.cc:83: "Name=iceweasel\n" nit: Perhaps add a separate test case for case-insensitivity? https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils_unittest.cc:90: }; nit: Please move this definition either into the test function that uses it, or into an anonymous namespace at the top of the file. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/importer_list.cc:79: } nit: If you're removing the comment from the final else-stmt, you should remove the curly braces from all the statements as well. Probably worth leaving the comment in place, though.
I uploaded a new patch with the changes that you suggested. Thanks. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/app/generat... chrome/app/generated_resources.grd:6626: + <message name="IDS_IMPORT_FROM_ICEWEASEL" desc="browser combo box: Mozilla Iceweasel"> On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Please remove "Mozilla" from this line as well Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/firefox_importer_utils.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:441: ReplaceSubstringsAfterOffset(&content, 0, "\r\n", "\n"); On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Is this necessary? It seems like the TrimWhitespace() call in the for loop > should take care of it. Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:450: branding_name = lines[i].substr(name_attr.size()); On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: You probably want to break out of the for loop in this case. Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:452: break; On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Please add a quick comment along the lines of "No longer in the app > section." Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:455: } On 2012/08/07 21:52:21, Ilya Sherman wrote: > Optional nit: I would move this bit of logic to be the first part of the if-else > branching, for two reasons: > (1) It will happen first when scanning over the lines; there's no way to enter > the if-stmt prior to ever entering this else-stmt. > (2) It's slightly simpler/briefer, and typically code is organized so that > simpler/briefer logic precedes meatier logic. Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:457: } On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Please add a blank line after this line Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils.cc:459: branding_name.find("iceweasel") != std::string::npos) On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Rather than searching for both strings, how about converting the > branding_name to all lowercase first? That way IceWeasel would also be fine, > for example. Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/firefox_importer_utils_unittest.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils_unittest.cc:83: "Name=iceweasel\n" On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Perhaps add a separate test case for case-insensitivity? Done. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/firefox_importer_utils_unittest.cc:90: }; Done. I also moved the other definition inside the anonymous namespace. On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: Please move this definition either into the test function that uses it, or > into an anonymous namespace at the top of the file. https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... File chrome/browser/importer/importer_list.cc (right): https://chromiumcodereview.appspot.com/10830098/diff/13001/chrome/browser/imp... chrome/browser/importer/importer_list.cc:79: } On 2012/08/07 21:52:21, Ilya Sherman wrote: > nit: If you're removing the comment from the final else-stmt, you should remove > the curly braces from all the statements as well. Probably worth leaving the > comment in place, though. Done.
LGTM :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10830098...
Presubmit check for 10830098-6008 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/importer
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
*sigh* this OWNERS file is out of date. I'll re-tick the commit queue checkbox once https://chromiumcodereview.appspot.com/10827226/ lands.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10830098...
Try job failure for 10830098-6008 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The Windows compile is failing with the following error message:
firefox_importer_utils.cc
E:\b\build\slave\win\build\src\chrome\browser\importer\firefox_importer_utils.cc(436)
:error C2664: 'FilePath FilePath::Append(const FilePath::StringType &) const' :
cannot convert parameter 1 from 'const char [16]' to 'const FilePath::StringType
&'
Reason: cannot convert from 'const char [16]' to 'const
FilePath::StringType'
No constructor could take the source type, or constructor overload
resolution was ambiguous
(If you are planning to continue working on Chromium, and aren't using it yet,
the BuildBot Error extension is very handy: [
https://chrome.google.com/webstore/detail/iehocdgbbocmkdidlbnnfbmbinnahbae ].)
Thanks for the tip. I fixed the compile error, but I did not upload a new patch
set yet because I found a new issue.
The bookmarks that are imported are placed under a new folder called "Imported
from Firefox". That string should also be changed to reflect the Firefox
branding name. It is retrieved using IDS_BOOKMARK_GROUP_FROM_FIREFOX.
Possible solutions:
1. Add a new resource IDS_BOOKMARK_GROUP_FROM_ICEWEASEL. However, we would have
to know from within the firefox2/3_importer instance if it is used for Firefox
or Iceweasel. For this we must either compare the importer_name with
IDS_IMPORT_FROM_FIREFOX or IDS_IMPORT_FROM_ICEWEASEL, or add a new importer_type
for Iceweasel and use this to figure out the bookmark folder name.
2. Add a $1 placeholder to IDS_BOOKMARK_GROUP_FROM_FIREFOX that would be
replaced with the importer_name at runtime. This will work pretty well for the
external importer since it creates a dictionary with the localized strings used
by the importers. We just have to change the following lines to something like
this:
localized_strings.SetString(
base::IntToString(IDS_BOOKMARK_GROUP_FROM_FIREFOX),
l10n_util::GetStringFUTF8(IDS_BOOKMARK_GROUP_FROM_FIREFOX,
source_profile_.importer_name));
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/ex...
The problem here is with the InProcessImporterBridge::GetLocalizedString method
that does not allow us to get parametrized strings. We could modify it to treat
the IDS_BOOKMARK_GROUP_FROM_FIREFOX id in a special way and call
l10n_util::GetStringFUTF8(), but this looks like a hack and you don't have easy
access to the source_profile/importer_name either.
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/in...
The same problem appears with the IDS_IMPORTER_LOCK_TITLE string (i.e "Close
Firefox Before Importing"). This might be even trickier since at this moment the
code that uses it, does not have access to the importer_name determined from the
application.ini file.
What do you think?
On 2012/08/08 21:10:52, Ilya Sherman wrote:
> The Windows compile is failing with the following error message:
>
> firefox_importer_utils.cc
>
E:\b\build\slave\win\build\src\chrome\browser\importer\firefox_importer_utils.cc(436)
> :error C2664: 'FilePath FilePath::Append(const FilePath::StringType &) const'
:
> cannot convert parameter 1 from 'const char [16]' to 'const
FilePath::StringType
> &'
> Reason: cannot convert from 'const char [16]' to 'const
> FilePath::StringType'
> No constructor could take the source type, or constructor overload
> resolution was ambiguous
>
> (If you are planning to continue working on Chromium, and aren't using it yet,
> the BuildBot Error extension is very handy: [
> https://chrome.google.com/webstore/detail/iehocdgbbocmkdidlbnnfbmbinnahbae ].)
On 2012/08/09 16:51:36, cristian.patrasciuc wrote: > Thanks for the tip. I fixed the compile error, but I did not upload a new patch > set yet because I found a new issue. > > The bookmarks that are imported are placed under a new folder called "Imported > from Firefox". That string should also be changed to reflect the Firefox > branding name. It is retrieved using IDS_BOOKMARK_GROUP_FROM_FIREFOX. Ah, interesting. Let's address this in a separate (follow-up) CL. This one is pretty much ready-to-go, and smaller CLs are generally easier to work with :) > Possible solutions: > > 1. Add a new resource IDS_BOOKMARK_GROUP_FROM_ICEWEASEL. However, we would have > to know from within the firefox2/3_importer instance if it is used for Firefox > or Iceweasel. For this we must either compare the importer_name with > IDS_IMPORT_FROM_FIREFOX or IDS_IMPORT_FROM_ICEWEASEL, or add a new importer_type > for Iceweasel and use this to figure out the bookmark folder name. This seems fine to me. It wouldn't be the most beautiful code ever, but it would still be fairly clear, etc. > 2. Add a $1 placeholder to IDS_BOOKMARK_GROUP_FROM_FIREFOX that would be > replaced with the importer_name at runtime. This will work pretty well for the > external importer since it creates a dictionary with the localized strings used > by the importers. We just have to change the following lines to something like > this: > > localized_strings.SetString( > base::IntToString(IDS_BOOKMARK_GROUP_FROM_FIREFOX), > l10n_util::GetStringFUTF8(IDS_BOOKMARK_GROUP_FROM_FIREFOX, > source_profile_.importer_name)); Hmm, isn't the importer name for Firefox "Mozilla Firefox"? If so, this approach wouldn't work -- we want the folder name to use the shorter "Firefox" name. > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/ex... > > The problem here is with the InProcessImporterBridge::GetLocalizedString method > that does not allow us to get parametrized strings. We could modify it to treat > the IDS_BOOKMARK_GROUP_FROM_FIREFOX id in a special way and call > l10n_util::GetStringFUTF8(), but this looks like a hack and you don't have easy > access to the source_profile/importer_name either. > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/in... > > The same problem appears with the IDS_IMPORTER_LOCK_TITLE string (i.e "Close > Firefox Before Importing"). This might be even trickier since at this moment the > code that uses it, does not have access to the importer_name determined from the > application.ini file.
I uploaded a new patch set that just fixes the previous compiler error then. For the other issue(s) I'll submit a separate CL once I implement a fix. If I'm about to go with option 1 from the above email, what's better: comparing the importer_name against IDS_IMPORT_FROM_ICEWEASEL or adding a new importer type constant (that would still instantiate a firefox importer, but would set the strings corresponding to Iceweasel)? On 2012/08/09 18:38:05, Ilya Sherman wrote: > On 2012/08/09 16:51:36, cristian.patrasciuc wrote: > > Thanks for the tip. I fixed the compile error, but I did not upload a new > patch > > set yet because I found a new issue. > > > > The bookmarks that are imported are placed under a new folder called "Imported > > from Firefox". That string should also be changed to reflect the Firefox > > branding name. It is retrieved using IDS_BOOKMARK_GROUP_FROM_FIREFOX. > > Ah, interesting. Let's address this in a separate (follow-up) CL. This one is > pretty much ready-to-go, and smaller CLs are generally easier to work with :) > > > Possible solutions: > > > > 1. Add a new resource IDS_BOOKMARK_GROUP_FROM_ICEWEASEL. However, we would > have > > to know from within the firefox2/3_importer instance if it is used for Firefox > > or Iceweasel. For this we must either compare the importer_name with > > IDS_IMPORT_FROM_FIREFOX or IDS_IMPORT_FROM_ICEWEASEL, or add a new > importer_type > > for Iceweasel and use this to figure out the bookmark folder name. > > This seems fine to me. It wouldn't be the most beautiful code ever, but it > would still be fairly clear, etc. > > > 2. Add a $1 placeholder to IDS_BOOKMARK_GROUP_FROM_FIREFOX that would be > > replaced with the importer_name at runtime. This will work pretty well for the > > external importer since it creates a dictionary with the localized strings > used > > by the importers. We just have to change the following lines to something like > > this: > > > > localized_strings.SetString( > > base::IntToString(IDS_BOOKMARK_GROUP_FROM_FIREFOX), > > l10n_util::GetStringFUTF8(IDS_BOOKMARK_GROUP_FROM_FIREFOX, > > source_profile_.importer_name)); > > Hmm, isn't the importer name for Firefox "Mozilla Firefox"? If so, this > approach wouldn't work -- we want the folder name to use the shorter "Firefox" > name. > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/ex... > > > > The problem here is with the InProcessImporterBridge::GetLocalizedString > method > > that does not allow us to get parametrized strings. We could modify it to > treat > > the IDS_BOOKMARK_GROUP_FROM_FIREFOX id in a special way and call > > l10n_util::GetStringFUTF8(), but this looks like a hack and you don't have > easy > > access to the source_profile/importer_name either. > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/in... > > > > The same problem appears with the IDS_IMPORTER_LOCK_TITLE string (i.e "Close > > Firefox Before Importing"). This might be even trickier since at this moment > the > > code that uses it, does not have access to the importer_name determined from > the > > application.ini file.
On 2012/08/09 20:02:24, cristian.patrasciuc wrote: > I uploaded a new patch set that just fixes the previous compiler error then. > > For the other issue(s) I'll submit a separate CL once I implement a fix. If I'm > about to go with option 1 from the above email, what's better: comparing the > importer_name against IDS_IMPORT_FROM_ICEWEASEL or adding a new importer type > constant (that would still instantiate a firefox importer, but would set the > strings corresponding to Iceweasel)? I think just comparing the name is probably a little simpler, and hence what I would recommend trying first. > On 2012/08/09 18:38:05, Ilya Sherman wrote: > > On 2012/08/09 16:51:36, cristian.patrasciuc wrote: > > > Thanks for the tip. I fixed the compile error, but I did not upload a new > > patch > > > set yet because I found a new issue. > > > > > > The bookmarks that are imported are placed under a new folder called > "Imported > > > from Firefox". That string should also be changed to reflect the Firefox > > > branding name. It is retrieved using IDS_BOOKMARK_GROUP_FROM_FIREFOX. > > > > Ah, interesting. Let's address this in a separate (follow-up) CL. This one > is > > pretty much ready-to-go, and smaller CLs are generally easier to work with :) > > > > > Possible solutions: > > > > > > 1. Add a new resource IDS_BOOKMARK_GROUP_FROM_ICEWEASEL. However, we would > > have > > > to know from within the firefox2/3_importer instance if it is used for > Firefox > > > or Iceweasel. For this we must either compare the importer_name with > > > IDS_IMPORT_FROM_FIREFOX or IDS_IMPORT_FROM_ICEWEASEL, or add a new > > importer_type > > > for Iceweasel and use this to figure out the bookmark folder name. > > > > This seems fine to me. It wouldn't be the most beautiful code ever, but it > > would still be fairly clear, etc. > > > > > 2. Add a $1 placeholder to IDS_BOOKMARK_GROUP_FROM_FIREFOX that would be > > > replaced with the importer_name at runtime. This will work pretty well for > the > > > external importer since it creates a dictionary with the localized strings > > used > > > by the importers. We just have to change the following lines to something > like > > > this: > > > > > > localized_strings.SetString( > > > base::IntToString(IDS_BOOKMARK_GROUP_FROM_FIREFOX), > > > l10n_util::GetStringFUTF8(IDS_BOOKMARK_GROUP_FROM_FIREFOX, > > > source_profile_.importer_name)); > > > > Hmm, isn't the importer name for Firefox "Mozilla Firefox"? If so, this > > approach wouldn't work -- we want the folder name to use the shorter "Firefox" > > name. > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/ex... > > > > > > The problem here is with the InProcessImporterBridge::GetLocalizedString > > method > > > that does not allow us to get parametrized strings. We could modify it to > > treat > > > the IDS_BOOKMARK_GROUP_FROM_FIREFOX id in a special way and call > > > l10n_util::GetStringFUTF8(), but this looks like a hack and you don't have > > easy > > > access to the source_profile/importer_name either. > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/importer/in... > > > > > > The same problem appears with the IDS_IMPORTER_LOCK_TITLE string (i.e "Close > > > Firefox Before Importing"). This might be even trickier since at this moment > > the > > > code that uses it, does not have access to the importer_name determined from > > the > > > application.ini file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10830098...
Try job failure for 10830098-11007 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10830098...
Try job failure for 10830098-7017 (retry) on win_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cristian.patrasciuc@gmail.com/10830098...
Change committed as 151032 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
