Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7104106:
Unify the version string to be displayed on "About Chromium" dialog. (Closed)
|
Created:
9 years, 6 months ago by haraken1 Modified:
9 years, 4 months ago Reviewers:
Denis Lagno, tony, Elliot Glaysher, Evan Martin, Mark Mentovai, commit-bot: I haz the power, beng (no_code_reviews) CC:
chromium-reviews, pam+watch_chromium.org, dhollowa, Mark Mentovai Visibility:
Public. |
DescriptionUnify the version string to be displayed on "About Chromium" dialog.
BUG=37186
TEST=Observe that "About Chromium" dialog shows the version string like "14.0.787.0 (Developer Build Linux 88242)" on Mac and Linux. (88242 is the SVN revision.) The patch for Windows will be appeared as another patch.
Patch Set 1 : Unify the version string to be displayed on "About Chromium" dialog #
Total comments: 18
Patch Set 2 : Update lastchange.py and import it from tweak_info_plist #
Total comments: 6
Patch Set 3 : Improve the way to extract the SVN revision through Git #
Total comments: 2
Patch Set 4 : Changed the regular expression used for git-svn-id match #
Total comments: 3
Patch Set 5 : Change [\s\S]* to .* with re.DOTALL option #
Total comments: 4
Patch Set 6 : Add "git config --get-regexp ^svn-remote.svn.url" #
Total comments: 3
Patch Set 7 : Reuse the original lastchange.py as much as possible #
Total comments: 2
Patch Set 8 : Change the regular expression to extract git-svn-id #
Total comments: 1
Patch Set 9 : correct nit #Patch Set 10 : Move platform_util_* from chrome/browser/ to chrome/common/ #Patch Set 11 : Comment out platforum_util::GetVersionStringModifier() #Patch Set 12 : Rebased with the patch of issue 7249003 #
Total comments: 4
Patch Set 13 : Correct nit #Patch Set 14 : Add ui/base/l10n_util.* to app_base_nacl_win64 #Patch Set 15 : Skip CreateVersionString() for NACL_WIN64 #
Total comments: 16
Patch Set 16 : Modify nib files using Interface Builder #
Total comments: 12
Patch Set 17 : Adjust webkit_version.py #
Total comments: 1
Patch Set 18 : Update how to construct webkit_url #
Messages
Total messages: 60 (0 generated)
Previously, I fixed bug 18679 and 37186 in CL 6894037 (http://codereview.chromium.org/6894037/). However, I found that the version string displayed on "About Chromium" dialog differs among platforms. Thus, I created a patch that unifies the version string format, like "14.0.787.0 (Developer Build Linux 88242)". Here, 88242 is the SVN revision. Would you take a look at it? (When you click "View" of chrome/tools/build/win/version.bat in this review site, "Upload in progress" is displayed. I re-submitted the patch but the same error occurs. This seems to be a bug of the review system...)
http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:122: return None Did you intend to include this change?
http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:122: return None > Did you intend to include this change? Yes. My intention is to extract the revision number in the *same* way on all platforms so that the version string becomes consistent among all platforms. Specifically, the way is as follows (this is the way Mac does in src/chrome/tools/build/mac/tweak_info_plist): (1) Try to extract the SVN revision through SVN (2) If (1) fails, try to extract the SVN revision through Git (3) If (2) fails, try to extract the Git revision through Git (4) If (3) fails, give up
Would you please take a look?
sorry, I'm OOO this week dilmah can review the lastchange change, and tony can probably review the rest
http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:119: re.MULTILINE) MULTILINE is redundant here http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:121: return VersionInfo(match.group(1), 'git', match.group(2)) is it ok to return 'git' as SVN root? lastchange is called from webkit/build/webkit_version.py and it seems that code there expects that svn root is prefix of svn url.
On 2011/06/10 12:45:41, haraken wrote: > (When you click "View" of chrome/tools/build/win/version.bat in this review > site, "Upload in progress" is displayed. I re-submitted the patch but the same > error occurs. This seems to be a bug of the review system...) you can file a bug on review system there: http://code.google.com/p/rietveld/issues/list
John told me that Rietveld is in a confused state because the upload failed part of the way through. Can you try to upload the patch again? If that doesn't work, maybe try uploading it to a new issue number? I have comments, but I get errors when trying to publish them. I'll see if I can extract them.
Also, there was code in the .bat file to error out if we don't get a revision number in an official build. That seems to have been lost. http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:112: '--grep=git-svn-id', '--format=%b']) The previous code would add -dirty to if there were local commits on top of what was pulled from the repo. Did you mean to remove that feature? http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:118: match = re.search(r'git-svn-id: (svn://.*)@([0-9]+)', output, Does this always work? What if someone checked out the code from http://src.chromium.org (the read only repo)? The regular expression in tweak_info_plist seems to be different. http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:122: return None On 2011/06/10 23:01:00, haraken wrote: > Yes. My intention is to extract the revision number in the *same* way on all > platforms so that the version string becomes consistent among all platforms. > Specifically, the way is as follows (this is the way Mac does in > src/chrome/tools/build/mac/tweak_info_plist): Is it possible to have tweak_info_plist just import this file and call FetchVersionInfo? http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... File chrome/browser/ui/views/about_chrome_view.cc (right): http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... chrome/browser/ui/views/about_chrome_view.cc:672: if (!version_info.is_valid()) { Why do we have this check here? We don't seem to call is_valid() when we instantiate VersionInfo in other places. http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... File chrome/browser/ui/views/about_chrome_view.h (left): http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... chrome/browser/ui/views/about_chrome_view.h:98: views::Textfield* os_version_label_; I suspect this is here for a reason and we shouldn't remove it. It probably makes it easier for bug reporters to include the OS version.
> John told me that Rietveld is in a confused state because the upload failed part > of the way through. Can you try to upload the patch again? If that doesn't > work, maybe try uploading it to a new issue number? > > I have comments, but I get errors when trying to publish them. I'll see if I > can extract them. I tried to upload the patch again, but .bat file still has a wrong state (as you can see here). Then, I tried to upload the patch as a new issue number, but still wrong...(See here: http://codereview.chromium.org/7168009/) I will file this bug on the rietveld site. http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:112: '--grep=git-svn-id', '--format=%b']) Sorry. I reverted the feature. http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:118: match = re.search(r'git-svn-id: (svn://.*)@([0-9]+)', output, I corrected the regular expression into 'git-svn-id:\s*([^@]*)@([0-9]+)', but is it fine? http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:119: re.MULTILINE) I removed it. http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:121: return VersionInfo(match.group(1), 'git', match.group(2)) Of course, we should return the SVN root if we can get the SVN root through git, but I do not know the way. The previous code was using "git config --get-regexp ^svn-remote.svn.url$" to extract the SVN root (in FetchGitSVNRoot), but I found that it does not work actually. In the first place, there seems to be no information about the SVN root in .git/config. For the time being, I used "git-svn" as the SVN root so that the string does not pass |if| statement at line 54 of webkit_version.py: if (version_info.url.startswith(version_info.root) and version_info.url.endswith(version_file_dir)): ## not pass here else ## pass here http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:122: return None On 2011/06/14 18:56:31, tony wrote: > On 2011/06/10 23:01:00, haraken wrote: > > Yes. My intention is to extract the revision number in the *same* way on all > > platforms so that the version string becomes consistent among all platforms. > > Specifically, the way is as follows (this is the way Mac does in > > src/chrome/tools/build/mac/tweak_info_plist): > > Is it possible to have tweak_info_plist just import this file and call > FetchVersionInfo? Done. http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... File chrome/browser/ui/views/about_chrome_view.cc (right): http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... chrome/browser/ui/views/about_chrome_view.cc:672: if (!version_info.is_valid()) { I removed it. http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... File chrome/browser/ui/views/about_chrome_view.h (left): http://codereview.chromium.org/7104106/diff/2001/chrome/browser/ui/views/abou... chrome/browser/ui/views/about_chrome_view.h:98: views::Textfield* os_version_label_; I think it is OK to remove |os_version_label_| since in this patch the OS name is included in |version_label_|. Previously, Windows shows the OS name in |os_version_label_|, which is displayed just under |version_label_|. Linux shows the OS name in |version_label_|. Mac shows no OS name. Thus, my intent is to unify their formats, specifically, all version information is included in |version_label_| so that people can copy and paste it in one line. For this reason, I guess that |os_version_label_| can be removed from here.
http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:121: return VersionInfo(match.group(1), 'git', match.group(2)) On 2011/06/15 04:20:41, haraken wrote: > Of course, we should return the SVN root if we can get the SVN root through git, > but I do not know the way. The previous code was using "git config --get-regexp > ^svn-remote.svn.url$" to extract the SVN root (in FetchGitSVNRoot), but I found > that it does not work actually. In the first place, there seems to be no > information about the SVN root in .git/config. well, it works for some checkouts (mine for example) but apparently it does not work universally. http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:118: match = re.search(r'git-svn-id: (svn://.*)@([0-9]+)', output, sometimes it will give incorrect revision: for example, below is real commit message: Code will detect 17605 instead of correct 17638: commit 8688ab4a4657533405da0d2d247ff063b8e9c933 Author: willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> Date: Thu Jun 4 17:12:48 2009 +0000 Fix purify freeze. Revert "Use a priority queue to assure that subresources are resolved asap" This reverts git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17605 0039d316-1c4b-4281-b951-d872f2087c98 TBR=jar Review URL: http://codereview.chromium.org/118239 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17638 0039d316-1c4b-4281-b951-d872f2087c98 http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:127: Returns the last change (in the form of a branch, revision tuple), as long as we are here I'd prefer to do this in more correct way: it should be marked -dirty not only if there are local commits but also if there is any local uncommitted change (i.e. if "git checkout" command produces non-zero output).
I could upload .bat file correctly by allowing 'application/x-msdos-program' mime-type for 'git-cl upload'. Please see here for more details: http://code.google.com/p/rietveld/issues/detail?id=313 http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/2001/build/util/lastchange.py#new... build/util/lastchange.py:121: return VersionInfo(match.group(1), 'git', match.group(2)) hmmm..., my inclination is to unify the result as much as possible(, although I know it is impossible to unify it completely). In this sense, I would prefer "always git-svn" rather than "sometimes the SVN root but sometimes git-svn". If we use git-svn here, webkit_version.py uses the SVN url as a SVN root. http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:118: match = re.search(r'git-svn-id: (svn://.*)@([0-9]+)', output, I modified so that it chooses the "git-svn-id: ..." line that appears first after "Review URL: ..." line. How about it?? http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:127: Returns the last change (in the form of a branch, revision tuple), On 2011/06/15 12:25:34, Denis Lagno wrote: > as long as we are here I'd prefer to do this in more correct way: it should be > marked -dirty not only if there are local commits but also if there is any local > uncommitted change (i.e. if "git checkout" command produces non-zero output). Done.
http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:118: match = re.search(r'git-svn-id:\s*([^@]*)@([0-9]+)', output) On 2011/06/15 16:22:53, haraken wrote: > I modified so that it chooses the "git-svn-id: ..." line that appears first > after "Review URL: ..." line. How about it?? maybe just pick last match instead of first? http://codereview.chromium.org/7104106/diff/17001/build/util/lastchange.py#ne... build/util/lastchange.py:107: Fetch the SVN revision for a given directory through Git. nit: there is no such word "lastest"
We should not remove the OS version label. I looked in the git history and it was added specifically for ChromeOS to get the version of ChromeOS. Instead, I think we should try to add the OS to Mac and Linux in a separate patch. Also, the existing code in lastchange.py has been tested to work on lots of different setups (git only, git with svn, git with svn pointing to a branch, etc). How is your rewrite better (i.e., how is the new output different from the old output)?
> We should not remove the OS version label. I looked in the git history and it > was added specifically for ChromeOS to get the version of ChromeOS. Instead, I > think we should try to add the OS to Mac and Linux in a separate patch. Indeed previously |os_version_label_| was added for ChromeOS, but at r87308 all the code about ChromeOS was removed from about_chrome_view.cc (ChromeOS code was moved to chrome/browser/ui/webui/options/about_page_handler.cc). In this way, now, I think that |os_version_label_| is used only for Windows to show its OS name. If this observation is right, I guess we can remove |os_version_label_| and include the OS name into |version_label_|. Am I missing something? > Also, the existing code in lastchange.py has been tested to work on lots of > different setups (git only, git with svn, git with svn pointing to a branch, > etc). How is your rewrite better (i.e., how is the new output different from > the old output)? First of all, the new code can fetch the SVN revision through Git even if git-svn is not available. In the previous code, we can get the SVN revision only if git-svn is installed, since IsGitSVN() returns true only if 'git svn info' works as expected. On the other hand, the new code uses the way that was used in chrome/tools/build/mac/tweak_info_plist, which does not require git-svn. Second, the new code uses more "robust" rules to extract the SVN revision through 'git log', as is discussed with dilmah. http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/13001/build/util/lastchange.py#ne... build/util/lastchange.py:118: match = re.search(r'git-svn-id:\s*([^@]*)@([0-9]+)', output) On 2011/06/15 17:14:55, Denis Lagno wrote: > On 2011/06/15 16:22:53, haraken wrote: > > I modified so that it chooses the "git-svn-id: ..." line that appears first > > after "Review URL: ..." line. How about it?? > > maybe just pick last match instead of first? Done. http://codereview.chromium.org/7104106/diff/17001/build/util/lastchange.py#ne... build/util/lastchange.py:107: Fetch the SVN revision for a given directory through Git. Done. "latest".
LGTM (with nit) from my side http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py#ne... build/util/lastchange.py:114: match = re.search(r'([\s\S]*)git-svn-id:\s*([^@]*)@([0-9]+)', output) nit: maybe replace ([\s\S]*) with .* in my understanding \s\S is equivalent to . and parentheses are not needed here because this group of symbols is not used.
http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py#ne... build/util/lastchange.py:114: match = re.search(r'([\s\S]*)git-svn-id:\s*([^@]*)@([0-9]+)', output) On 2011/06/16 07:29:22, Denis Lagno wrote: > nit: maybe replace ([\s\S]*) with .* > in my understanding \s\S is equivalent to . oh, I see, dot does not match newline. Probably using dot together with re.DOTALL flag is still more clear than \s\S
Waiting for tony's opinion. tony: If you think another reviewer is necessary, I would be happy if you introduce the reviewer. Thanks. http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/20001/build/util/lastchange.py#ne... build/util/lastchange.py:114: match = re.search(r'([\s\S]*)git-svn-id:\s*([^@]*)@([0-9]+)', output) On 2011/06/16 07:44:42, Denis Lagno wrote: > On 2011/06/16 07:29:22, Denis Lagno wrote: > > nit: maybe replace ([\s\S]*) with .* > > in my understanding \s\S is equivalent to . > > oh, I see, dot does not match newline. Probably using dot together with > re.DOTALL flag is still more clear than \s\S Done.
On 2011/06/16 00:38:01, haraken wrote: > Indeed previously |os_version_label_| was added for ChromeOS, but at r87308 all > the code about ChromeOS was removed from about_chrome_view.cc (ChromeOS code was > moved to chrome/browser/ui/webui/options/about_page_handler.cc). In this way, > now, I think that |os_version_label_| is used only for Windows to show its OS > name. If this observation is right, I guess we can remove |os_version_label_| > and include the OS name into |version_label_|. Am I missing something? I see, yes, it appears that ChromeOS no longer needs this code. OK, I think it's fine to remove it, but I would probably do it in a separate CL and ask davemoore or seanparent to review since they added the code to views and moved it to webui. > First of all, the new code can fetch the SVN revision through Git even if > git-svn is not available. In the previous code, we can get the SVN revision only > if git-svn is installed, since IsGitSVN() returns true only if 'git svn info' > works as expected. On the other hand, the new code uses the way that was used in > chrome/tools/build/mac/tweak_info_plist, which does not require git-svn. I see, I think it's useful to be able to get the SVN revision if git-svn is not installed. However, by never checking to see if git-svn is installed, you lose information like the svn root (the svn-remote.svn.url mentioned above). Is it possible we can try git-svn and if it's not installed, fall back on this code path? > Second, the new code uses more "robust" rules to extract the SVN revision > through 'git log', as is discussed with dilmah. Using git log --grep is great, but can we just fix the existing LookupGitSVNRevision? http://codereview.chromium.org/7104106/diff/24001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/24001/build/util/lastchange.py#ne... build/util/lastchange.py:141: (dummy_url, dummy_revision) = MatchGitSVNRevision(output) Nit: The () on the left side of the = are not necessary. http://codereview.chromium.org/7104106/diff/24001/chrome/tools/build/mac/twea... File chrome/tools/build/mac/tweak_info_plist (right): http://codereview.chromium.org/7104106/diff/24001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:110: (scm_path, scm_revision) = (version_info.url, version_info.revision) Nit: The () on both sides of this assignment are not necessary.
> I see, yes, it appears that ChromeOS no longer needs this code. OK, I think > it's fine to remove it, but I would probably do it in a separate CL and ask > davemoore or seanparent to review since they added the code to views and moved > it to webui. Do you mean we should revert the views code back to the original and then make another CL for the change of the views code? Or, should we ask davemoore or seanparent for the review of this CL (without no change for the views code in the current patch)? Either is OK for me (but I would probably prefer the latter). > I see, I think it's useful to be able to get the SVN revision if git-svn is not > installed. However, by never checking to see if git-svn is installed, you lose > information like the svn root (the svn-remote.svn.url mentioned above). Is it > possible we can try git-svn and if it's not installed, fall back on this code > path? Done. I added the code that tries "git config --get-regexp ^svn-remote.svn.url". If it fails to extract the SVN root (maybe because git-svn is not available), it just uses a string "git-svn" as the SVN root. > > Second, the new code uses more "robust" rules to extract the SVN revision > > through 'git log', as is discussed with dilmah. > > Using git log --grep is great, but can we just fix the existing > LookupGitSVNRevision? In fact, LookupGitSVNRevision() uses "git log -999" to find the git-svn-id line. I guess that "git log -1 --grep=git-svn-id" is better way to do that, but why do you prefer LookupGitSVNRevision()? (I know that you want to keep the previous code (=lastchange.py) as much as possible, because the previous code have been working correctly. However, the previous code has some disadvantages, e.g. it does not work in the case where git-svn is not available. Then, if we intend to make the code more robust and clean up the logic, I guess the code may become something like the code in this patch.) http://codereview.chromium.org/7104106/diff/24001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/24001/build/util/lastchange.py#ne... build/util/lastchange.py:141: (dummy_url, dummy_revision) = MatchGitSVNRevision(output) On 2011/06/16 17:01:51, tony wrote: > Nit: The () on the left side of the = are not necessary. Done. http://codereview.chromium.org/7104106/diff/24001/chrome/tools/build/mac/twea... File chrome/tools/build/mac/tweak_info_plist (right): http://codereview.chromium.org/7104106/diff/24001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:110: (scm_path, scm_revision) = (version_info.url, version_info.revision) On 2011/06/16 17:01:51, tony wrote: > Nit: The () on both sides of this assignment are not necessary. Done.
On 2011/06/17 00:53:51, haraken wrote: > Do you mean we should revert the views code back to the original and then make > another CL for the change of the views code? Or, should we ask davemoore or > seanparent for the review of this CL (without no change for the views code in > the current patch)? Either is OK for me (but I would probably prefer the > latter). Either is OK, but I prefer the former and I suspect the former is faster (i.e., you can get that change landed in less than 1 day). Having 2 changes also means it's easier to identify a regression and easier to revert if something goes wrong. > > > Second, the new code uses more "robust" rules to extract the SVN revision > > > through 'git log', as is discussed with dilmah. > > > > Using git log --grep is great, but can we just fix the existing > > LookupGitSVNRevision? > > In fact, LookupGitSVNRevision() uses "git log -999" to find the git-svn-id line. > I guess that "git log -1 --grep=git-svn-id" is better way to do that, but why do > you prefer LookupGitSVNRevision()? I don't, I'm saying you could keep LookupGitSVNRevision as a function and just change the function body to use git log --grep. That makes it more clear in the diff what the change is. It also makes it possible to unit test that function if we wanted to. > (I know that you want to keep the previous code (lastchange.py) as much as > possible, because the previous code have been working correctly. Correct. > However, the > previous code has some disadvantages, e.g. it does not work in the case where > git-svn is not available. Then, if we intend to make the code more robust and > clean up the logic, I guess the code may become something like the code in this > patch.) I agree that making the code work in the cases where git-svn is not available is good, but the current diff makes it hard for me to tell if we've introduced any regressions or not because it's so different from the original code. I also find it harder to read because it's now one big function with lots of nesting. http://codereview.chromium.org/7104106/diff/27001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/27001/build/util/lastchange.py#ne... build/util/lastchange.py:127: if proc: Nit: Can we just early return if not proc to avoid the nesting? http://codereview.chromium.org/7104106/diff/27001/build/util/lastchange.py#ne... build/util/lastchange.py:135: dirty = True Nit: Can we make the dirty check a function? http://codereview.chromium.org/7104106/diff/27001/build/util/lastchange.py#ne... build/util/lastchange.py:163: root = match.group(0) Nit: Can we make this root extraction a function too?
> Either is OK, but I prefer the former and I suspect the former is faster (i.e., > you can get that change landed in less than 1 day). Having 2 changes also means > it's easier to identify a regression and easier to revert if something goes > wrong. I reverted the views codes back to the original ones. After this patch lands, I would like to upload the patch of those views codes. In addition, I rewrote this patch so that it uses the original lastchange.py (maybe) as much as possible, although I renamed LookupGitSVNRevision() to FetchGitSVNURLAndRevision().
http://codereview.chromium.org/7104106/diff/31002/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/31002/build/util/lastchange.py#ne... build/util/lastchange.py:167: match = re.search(r'^Review URL:', output, re.MULTILINE) since now you extract last match of 'git-svn-id' then you need not look it after 'Review URL:' match. Actually, around 10% of git-svn commmits lack 'Review URL', for example: commit 9ee5ccd9212a7f51cbdafcd2e4b27a088a4f9ec8 Author: chrome-release@google.com <chrome-release@google.com@0039d316-1c4b-4281-b951-d872f2087c98> Date: Thu Jun 16 09:00:50 2011 +0000 Updating trunk VERSION from 794.0 to 795.0 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@89308 0039d316-1c4b-4281-b951-d872f2087c98 commit a33b93ab82d20cc6f64673be2cb7d6707e911c77 Author: mnaganov@chromium.org <mnaganov@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> Date: Thu Feb 17 14:39:23 2011 +0000 Mark DevToolsSanityTest.TestScriptsTabIsPopulatedOnInspectedPageRefresh as failing after WK roll. BUG=73289 TEST=DevToolsSanityTest.TestScriptsTabIsPopulatedOnInspectedPageRefresh git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75265 0039d316-1c4b-4281-b951-d872f2087c98
http://codereview.chromium.org/7104106/diff/31002/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/31002/build/util/lastchange.py#ne... build/util/lastchange.py:167: match = re.search(r'^Review URL:', output, re.MULTILINE) On 2011/06/17 11:01:27, Denis Lagno wrote: > since now you extract last match of 'git-svn-id' then you need not look it after > 'Review URL:' match. > > Actually, around 10% of git-svn commmits lack 'Review URL', for example: > > commit 9ee5ccd9212a7f51cbdafcd2e4b27a088a4f9ec8 > Author: mailto:chrome-release@google.com > <chrome-release@google.com@0039d316-1c4b-4281-b951-d872f2087c98> > Date: Thu Jun 16 09:00:50 2011 +0000 > > Updating trunk VERSION from 794.0 to 795.0 > > git-svn-id: svn://svn.chromium.org/chrome/trunk/src@89308 > 0039d316-1c4b-4281-b951-d872f2087c98 > > > commit a33b93ab82d20cc6f64673be2cb7d6707e911c77 > Author: mailto:mnaganov@chromium.org > <mnaganov@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> > Date: Thu Feb 17 14:39:23 2011 +0000 > > Mark DevToolsSanityTest.TestScriptsTabIsPopulatedOnInspectedPageRefresh as > failing after WK roll. > > BUG=73289 > TEST=DevToolsSanityTest.TestScriptsTabIsPopulatedOnInspectedPageRefresh > > git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75265 > 0039d316-1c4b-4281-b951-d872f2087c98 Done.
Thanks, I find this diff much easier to read and see the changes from the original version. LGTM http://codereview.chromium.org/7104106/diff/32002/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/32002/build/util/lastchange.py#ne... build/util/lastchange.py:134: the SVN URL and revision. Nit: Please be explicit that this is a tuple. E.g., A tuple containing the SVN URL and revision.
Try job failure for 7104106-27021 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...
Windows build failed because of a dependency problem... In chrome/common/chrome_version_info.cc, we call platform_util::GetVersionStringModifier() in chrome/browser/platform_util_win.cc. The problem is that we are calling a method in *browser* from *common*. I am not sure why Linux build and Mac build succeed, but anyway, generally I think it is not good to call a method in *browser* from non-*browser* files even if it practically succeeds on Linux and Mac. There are several possible approaches to solve this problem, but I guess that moving chrome/browser/platform_util_* to chrome/common/ may be a good approach. Please note that here is not the only place where platform_util::GetVersionStringModifier() is called from non-*browser* files. For example, chrome/app/chrome_main.cc also calls platform_util::GetVersionStringModifier(), although it practically works well because only Linux and Mac call the method. I would like to hear the reviewers' ideas.
I moved chrome/browser/platform_util_* to chrome/common. I confirmed that build works fine on all of Mac, Linux and Windows. Would you please take a look at it? On 2011/06/20 02:32:03, haraken wrote: > Windows build failed because of a dependency problem... In > chrome/common/chrome_version_info.cc, we call > platform_util::GetVersionStringModifier() in > chrome/browser/platform_util_win.cc. The problem is that we are > calling a method in *browser* from *common*. I am not sure why Linux > build and Mac build succeed, but anyway, generally I think it is not > good to call a method in *browser* from non-*browser* files even if it > practically succeeds on Linux and Mac. > > There are several possible approaches to solve this problem, but I > guess that moving chrome/browser/platform_util_* to chrome/common/ may > be a good approach. Please note that here is not the only place where > platform_util::GetVersionStringModifier() is called from non-*browser* > files. For example, chrome/app/chrome_main.cc also calls > platform_util::GetVersionStringModifier(), although it practically > works well because only Linux and Mac call the method. > > I would like to hear the reviewers' ideas.
Sorry, I was on vacation Monday. I think we should only move GetVersionStringModifier() and GetChannel() to common, not the whole platform_util* files. Many of the other methods don't make sense in common (e.g., CanSetAsDefaultBrowser()). You could maybe even just move these methods into chrome_version_info.*. Please do this move as a separate patch (small patches are easier to review and faster to land).
> You could maybe even just move these methods into chrome_version_info.*. > Please do this move as a separate patch (small patches are easier to review and > faster to land). Sure. I will do it in next patch. For the time being, I commented out platform_util::GetVersionStringModifier() in order to pass Windows build. I confirmed that this latest patch works fine on all of Linux, Mac and Windows.
On 2011/06/22 05:49:33, haraken wrote: > Sure. I will do it in next patch. For the time being, I commented out > platform_util::GetVersionStringModifier() in order to pass Windows build. I > confirmed that this latest patch works fine on all of Linux, Mac and Windows. I don't think it's acceptable to comment out the versionstringmodifier code. What if we happened to cut a branch for a dev build or a canary build gets cut while this patch is in. Please do the refactoring of platform_util_* first.
> I think we should only move GetVersionStringModifier() and GetChannel() to > common, not the whole platform_util* files. Many of the other methods don't > make sense in common (e.g., CanSetAsDefaultBrowser()). > > You could maybe even just move these methods into chrome_version_info.*. > > Please do this move as a separate patch > > Please do the refactoring of platform_util_* first. I would feel that the most simple and reasonable way to fix it is to move GetVersionStringModifier() and GetChannel() into chrome_version_info.cc from platform_util_*. Do you mean that it is OK to do this move and rewrite several other codes that is also using those methods in this patch?
On 2011/06/22 18:15:10, haraken wrote: > I would feel that the most simple and reasonable way to fix it is to move > GetVersionStringModifier() and GetChannel() into chrome_version_info.cc from > platform_util_*. I agree. > Do you mean that it is OK to do this move and rewrite several > other codes that is also using those methods in this patch? I would do it as a separate patch. First patch: move GetVersionStringModifier() and GetChannel() into chrome_version_info.cc from platform_util_*. Second patch: The rest of this patch.
Thank you for your dedicated support. I just rebased with the patch of issue 7249003. I confirmed that this patch works fine on all of Mac, Linux and Windows.
LGTM http://codereview.chromium.org/7104106/diff/48001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/48001/build/util/lastchange.py#ne... build/util/lastchange.py:163: match = re.search(r'.*git-svn-id:\s*([^@]*)@([0-9]+)', output, re.DOTALL) Nit: Please move this regular expression out into a global. Something like: _GIT_SVN_ID_REGEX = re.compile(r'.*git-svn-id:\s*([^@]*)@([0-9]+)') Then you can reference it here and in FetchGitSVNURLAndRevision. http://codereview.chromium.org/7104106/diff/48001/chrome/tools/build/win/vers... File chrome/tools/build/win/version.bat (right): http://codereview.chromium.org/7104106/diff/48001/chrome/tools/build/win/vers... chrome/tools/build/win/version.bat:49: :GEN_FILE Nit: I think this label is no longer needed.
http://codereview.chromium.org/7104106/diff/48001/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/48001/build/util/lastchange.py#ne... build/util/lastchange.py:163: match = re.search(r'.*git-svn-id:\s*([^@]*)@([0-9]+)', output, re.DOTALL) On 2011/06/27 18:26:07, tony wrote: > Nit: Please move this regular expression out into a global. Something like: > > _GIT_SVN_ID_REGEX = re.compile(r'.*git-svn-id:\s*([^@]*)@([0-9]+)') > > Then you can reference it here and in FetchGitSVNURLAndRevision. Done. http://codereview.chromium.org/7104106/diff/48001/chrome/tools/build/win/vers... File chrome/tools/build/win/version.bat (right): http://codereview.chromium.org/7104106/diff/48001/chrome/tools/build/win/vers... chrome/tools/build/win/version.bat:49: :GEN_FILE On 2011/06/27 18:26:07, tony wrote: > Nit: I think this label is no longer needed. Done.
Try job failure for 7104106-52001 (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...
No change for now. Sorry for taking so long time. > Try job failure for 7104106-52001 (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... The above error happens at linking chrome_dll_nacl_win64, and the cause seems to be that we call a method in ui/base/ from chrome/common/. Specifically, it seems that we cannot call l10n_util::GetStringUTF8() from VersionInfo::CreateVersionString() because of a dependency problem. However, - ./src/tools/checkdeps/checkdeps.py says that there is no problem of dependency. - In my Windows environment, the build of chrome_dll_nacl_win64 succeeds. What should we do? I am not sure but moving VersionInfo::CreateVersionString() into chrome/browser/ may solve this problem (In this case, to which file?).
Can you try adding l10n_util.* to app_base_nacl_win64 (in app/app_base.gypi)? Alternately, maybe it's possible for us to remove chrome_version_info.cc from common_nacl_win64 (in chrome/chrome_common.gypi, you would move the file from the list at the top into just |common|). In your Windows environment, did you try clobbering? Also make sure you ran gclient runhooks.
> Can you try adding l10n_util.* to app_base_nacl_win64 (in app/app_base.gypi)? Done. > In your Windows environment, did you try clobbering? Also make sure you ran > gclient runhooks. I removed all the files in chrome/Debug, run 'gclient runhooks', and then build chrome and chrome_dll_nacl_win64. The build succeeded in my Windows 7 environment.
On 2011/06/29 12:01:12, haraken wrote: > > Can you try adding l10n_util.* to app_base_nacl_win64 (in app/app_base.gypi)? > > Done. Sorry, I ran it through the try bots and it didn't work. Trying another job ... > > In your Windows environment, did you try clobbering? Also make sure you ran > > gclient runhooks. > > I removed all the files in chrome/Debug, run 'gclient runhooks', and then build > chrome and chrome_dll_nacl_win64. The build succeeded in my Windows 7 > environment. Does it build in Release?
Success! Here's a patch that works on the try bots: http://codereview.chromium.org/7248049 Try bot results: http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/383 http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/34753 http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/35329 Mac is still running, but it will likely pass. Feel free to upload the patch here and check the commit box.
It seems that your Mac code hasn’t been reviewed at all. Do you need a Mac reviewer? All of the sudden, I understand why people “set noparent” in their OWNERS files.
I am sorry for clicking "commit" button without enough reviewers. Mark: Would you please take a look at chrome/browser/ui/cocoa/about_window_controller.mm, chrome/app/nibs/About.xib and chrome/tools/build/mac/tweak_info_plist? Erg: Would you please take a look at chrome/browser/ui/gtk/about_chrome_dialog.cc? If a fix is needed, I would like to make another patch.
+mark, +erg
http://codereview.chromium.org/7104106/diff/58001/chrome/app/nibs/About.xib File chrome/app/nibs/About.xib (right): http://codereview.chromium.org/7104106/diff/58001/chrome/app/nibs/About.xib#n... chrome/app/nibs/About.xib:44: <string key="NSWindowRect">{{196, 257}, {550, 277}}</string> Whenever you make a change to a .xib file, you change description must include a comment indicating what changed. You haven’t done that here. I can see that you’re changing something’s width to 550. What is it? The window? For what reason? And how does it compare with the dialog on other platforms? I have a strong suspicion that you edited this file manually. Don’t ask how I know, I just do. You can’t do that. Any .xib changes must be done in Interface Builder. If you try to make changes manually, you’ll at least break the UI, and likely break the file. Right now, the best case scenario is that this file is not canonically formed, which can lead to discrepancies at run-time. The UI may even behave differently on different OS releases. http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... File chrome/browser/ui/cocoa/about_window_controller.mm (right): http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:133: chrome::VersionInfo version_info; Put this below the comment, not above it. http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:136: NSString* version = [NSString stringWithFormat:@"%@", This is like saying snprintf(buf, arraysize(buf), "%s", FunctionThatReturnsCharPointer()); What you should have written: NSString* version = base::SysUTF8ToNSString(version_info.CreateVersionString()); http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... File chrome/common/chrome_version_info.cc (right): http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:101: IsOfficialBuild() ? What does this even mean in the context of !defined(GOOGLE_CHROME_BUILD)? http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:104: current_version += OSType(); Why? This seems to be all of obvious, out-of-place, and unnecessary. http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... File chrome/tools/build/mac/tweak_info_plist (right): http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:34: sys.path.insert(0, os.path.join(TOP, "build/util/")) No trailing slash. http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:109: version_info = lastchange.FetchVersionInfo(default_lastchange=None) If FetchVersionInfo fails, version_info will still be set to a 3-tuple containing values, but these values are not the same as those that the code below treats as “couldn’t find the data.” http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:110: scm_path, scm_revision = version_info.url, version_info.revision The value of scm_path will be wrong. The existing code (red, above) took care to strip the “repository root” portion off of the “URL” portion for Subversion, and to take only the portion after /chrome/ or /svn/ from a git-svn-id. It doesn’t appear that lastchange.py takes this into account at all. It should. It’s important to get this right, or you will leak internal Subversion or git addresses. In case it helps, when this script runs ont he Mac, we expect the relevant portion of Info.plist to wind up looking like this: <key>SVNPath</key> <string>/trunk/src</string> <key>SVNRevision</key> <string>90926</string>
http://codereview.chromium.org/7104106/diff/58001/chrome/app/nibs/About.xib File chrome/app/nibs/About.xib (right): http://codereview.chromium.org/7104106/diff/58001/chrome/app/nibs/About.xib#n... chrome/app/nibs/About.xib:44: <string key="NSWindowRect">{{196, 257}, {550, 277}}</string> I am sorry, as you say, I modified the nib file manually. In the latest patch, I modified it using Interface Builder. This change is going to expand the width of "About Chromium" dialog, so that the entire version string ("14.0.811.0 (Developer Bulid Mac OS 91455-dirty)") can be displayed from its beginning to its end in one line. Since this patch increases the length of the version string, the previous width is too small to show the entire version string. The similar change is unnecessary in GTK because the width of GTK's "About Chromium" dialog is wide enough all along. Views' change is also needed but we decided to make the change in another CL. http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... File chrome/browser/ui/cocoa/about_window_controller.mm (right): http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:133: chrome::VersionInfo version_info; On 2011/06/30 13:34:23, Mark Mentovai wrote: > Put this below the comment, not above it. Done. http://codereview.chromium.org/7104106/diff/58001/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:136: NSString* version = [NSString stringWithFormat:@"%@", On 2011/06/30 13:34:23, Mark Mentovai wrote: > This is like saying > > snprintf(buf, arraysize(buf), "%s", FunctionThatReturnsCharPointer()); > > What you should have written: > > NSString* version = > base::SysUTF8ToNSString(version_info.CreateVersionString()); Done. http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... File chrome/common/chrome_version_info.cc (right): http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:101: IsOfficialBuild() ? Fixed it. As you pointed out, IsOfficialBuild() is always false here. http://codereview.chromium.org/7104106/diff/58001/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:104: current_version += OSType(); No change for now. I think that OS type is necessary here. Indeed it is obvious for a user, but the purpose here is to create the version string that people can copy and paste to the bug report site. OS type is important information in that scenario. http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... File chrome/tools/build/mac/tweak_info_plist (right): http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:34: sys.path.insert(0, os.path.join(TOP, "build/util/")) On 2011/06/30 13:34:23, Mark Mentovai wrote: > No trailing slash. Done. http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:109: version_info = lastchange.FetchVersionInfo(default_lastchange=None) On 2011/06/30 13:34:23, Mark Mentovai wrote: > If FetchVersionInfo fails, version_info will still be set to a 3-tuple > containing values, but these values are not the same as those that the code > below treats as “couldn’t find the data.” Done. http://codereview.chromium.org/7104106/diff/58001/chrome/tools/build/mac/twea... chrome/tools/build/mac/tweak_info_plist:110: scm_path, scm_revision = version_info.url, version_info.revision Done. In addition, I modified the rule to extract the URL from Subversion so that it extracts the portion after /chrome/ or /svn/.
erg: Would you please take a look at chrome/browser/ui/gtk/about_chrome_dialog.cc?
I’m out this week. I’ll try to take another look at the Mac changes, time permitting.
The gtk changes LGTM
Mark: Would you please take a look at it?
It looks like the major issues are worked out, except for the one about webkit_version.py. http://codereview.chromium.org/7104106/diff/63011/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/63011/build/util/lastchange.py#ne... build/util/lastchange.py:56: match = _SVN_URL_REGEX.search(attrs['URL']) With this change and the one to FetchGitSVNRevision, it looks like you will need to adjust webkit/build/webkit_version.py too. http://codereview.chromium.org/7104106/diff/63011/chrome/browser/ui/cocoa/abo... File chrome/browser/ui/cocoa/about_window_controller.mm (right): http://codereview.chromium.org/7104106/diff/63011/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:137: base::SysUTF8ToNSString(version_info.CreateVersionString()); 4-space indent on continuation lines. http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... File chrome/common/chrome_version_info.cc (right): http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:94: std::string current_version = ""; You don’t need the |= ""| here, get rid of it. The default constructor for a std::string gives you an empty string, that’s fine. http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:101: current_version += OSType(); OK, so this will be Developer Build Windows 12345 but I think the order should be Developer Build 12345 Windows or Windows Developer Build 12345 http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:117: return "Mac OS"; If we’re going to use this, it should be Mac OS X. But when you change this, make sure that nobody else is currently using it on the Mac and relying on a specific value. http://codereview.chromium.org/7104106/diff/63011/chrome/tools/build/win/vers... File chrome/tools/build/win/version.bat (right): http://codereview.chromium.org/7104106/diff/63011/chrome/tools/build/win/vers... chrome/tools/build/win/version.bat:1: @echo off No changes visible in this file or version.rules. svn:eol-style problems? Do they belong in the changelist at all?
Mark: Would you please take a look at the change? http://codereview.chromium.org/7104106/diff/63011/build/util/lastchange.py File build/util/lastchange.py (right): http://codereview.chromium.org/7104106/diff/63011/build/util/lastchange.py#ne... build/util/lastchange.py:56: match = _SVN_URL_REGEX.search(attrs['URL']) On 2011/07/18 02:20:17, Mark Mentovai wrote: > With this change and the one to FetchGitSVNRevision, it looks like you will need > to adjust webkit/build/webkit_version.py too. Done. http://codereview.chromium.org/7104106/diff/63011/chrome/browser/ui/cocoa/abo... File chrome/browser/ui/cocoa/about_window_controller.mm (right): http://codereview.chromium.org/7104106/diff/63011/chrome/browser/ui/cocoa/abo... chrome/browser/ui/cocoa/about_window_controller.mm:137: base::SysUTF8ToNSString(version_info.CreateVersionString()); On 2011/07/18 02:20:17, Mark Mentovai wrote: > 4-space indent on continuation lines. Done. http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... File chrome/common/chrome_version_info.cc (right): http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:94: std::string current_version = ""; On 2011/07/18 02:20:17, Mark Mentovai wrote: > You don’t need the |= ""| here, get rid of it. The default constructor for a > std::string gives you an empty string, that’s fine. Done. http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:101: current_version += OSType(); I changed it to (Developer Build 12345 Windows) On 2011/07/18 02:20:17, Mark Mentovai wrote: > OK, so this will be > > Developer Build Windows 12345 > > but I think the order should be > > Developer Build 12345 Windows > > or > > Windows Developer Build 12345 http://codereview.chromium.org/7104106/diff/63011/chrome/common/chrome_versio... chrome/common/chrome_version_info.cc:117: return "Mac OS"; Changed to "Mac OS X". I confirmed that no place is depending on the specific value. On 2011/07/18 02:20:17, Mark Mentovai wrote: > If we’re going to use this, it should be Mac OS X. > > But when you change this, make sure that nobody else is currently using it on > the Mac and relying on a specific value. http://codereview.chromium.org/7104106/diff/63011/chrome/tools/build/win/vers... File chrome/tools/build/win/version.bat (right): http://codereview.chromium.org/7104106/diff/63011/chrome/tools/build/win/vers... chrome/tools/build/win/version.bat:1: @echo off Hmmm, strange. I can see the changes... If you still cannot see the change, please ping me. Anyway, the change is still in this CL and I have not yet touched version.bat and version.rules for this two weeks. On 2011/07/18 02:20:17, Mark Mentovai wrote: > No changes visible in this file or version.rules. svn:eol-style problems? Do > they belong in the changelist at all?
http://codereview.chromium.org/7104106/diff/73001/webkit/build/webkit_version.py File webkit/build/webkit_version.py (right): http://codereview.chromium.org/7104106/diff/73001/webkit/build/webkit_version... webkit/build/webkit_version.py:65: version_info.url.startswith(version_info.root) and What I meant was that you’ve changed the contents of version_info.url such that version_info.root may no longer be a prefix. Right?
Thanks, I fixed it. Also I changed _SVN_URL_REGEX as follows so that it matches with webkit URL: _SVN_URL_REGEX = re.compile(r'.*/(chrome|svn|webkit)(/.*)') On 2011/07/19 15:44:32, Mark Mentovai wrote: > http://codereview.chromium.org/7104106/diff/73001/webkit/build/webkit_version.py > File webkit/build/webkit_version.py (right): > > http://codereview.chromium.org/7104106/diff/73001/webkit/build/webkit_version... > webkit/build/webkit_version.py:65: > version_info.url.startswith(version_info.root) and > What I meant was that you’ve changed the contents of version_info.url such that > version_info.root may no longer be a prefix. > > Right?
This change touches a lot of code and I think the chance of it landing without any regressions is close to zero. Which probably means it'll get reverted and might cause you a lot of frustration (I would be frustrated if I spent over a month on a change and it got reverted). My recommendation (not a requirement, so feel free to ignore) would be to break this up into smaller changes to land separately. This is how I would break it up: 1) Add CreateVersionString() to chrome_version_info.*, update the GTK+ code to use it. erg and I can review it. 2) Update the mac code to use CreateVersionString(). mark can review it. 3) Update the views code to use CreateVersionString(). davemoore or I could review it (this can happen at the same time as (2)). 4) Update version.bat to use lastchange.py. evan is probably a good reviewer for that. 5) Update About.xib to use lastchange.py. mark can review (this can happen at the same time as (4)). 6) Make changes to lastchange.py. evan can review. I'm still not sure what the changes to lastchange.py accomplish so I'm a bit skeptical about this step. 7) Make changes to webkit_version.py. evan is probably also a good reviewer for this.
LGTM. Upon checking this in, please carefully review the first official canary (Mac and Windows) and dev (Linux) build with this change to ensure that everything is as it should be. I trust you to address any shortcomings quickly. version_info.root may now be obsolete. If so, you should remove it. Don’t do that in this change, please do it in a follow-up. We may also want to change _SVN_URL_REGEX at that time to be a regex that can be overridden by the user, so that webkit_verison.py pushes in the “webkit” string instead of having it hard-coded.
Thank you very much. Sure. I would like to check the change in in the following steps(, although I guess we should do (6) and (7) at one breath.) > 1) Add CreateVersionString() to chrome_version_info.*, update the GTK+ code to > use it. erg and I can review it. > 2) Update the mac code to use CreateVersionString(). mark can review it. > 3) Update the views code to use CreateVersionString(). davemoore or I could > review it (this can happen at the same time as (2)). > 4) Update version.bat to use lastchange.py. evan is probably a good reviewer > for that. > 5) Update About.xib to use lastchange.py. mark can review (this can happen at > the same time as (4)). > 6) Make changes to lastchange.py. evan can review. I'm still not sure what the > changes to lastchange.py accomplish so I'm a bit skeptical about this step. > 7) Make changes to webkit_version.py. evan is probably also a good reviewer for > this. Also, I would like to make the following change that Mark indicated in the step (6) and (7). > version_info.root may now be obsolete. If so, you should remove it. Don’t do > that in this change, please do it in a follow-up. We may also want to change > _SVN_URL_REGEX at that time to be a regex that can be overridden by the user, so > that webkit_verison.py pushes in the “webkit” string instead of having it > hard-coded.
Now that this is in today’s Canary and I’ve had a chance to see it, I wonder if 550px is too wide. Would 500 suffice?
All the patches for this issue seem to have been landed successfully. I close this issue. tony, mark, evan: thank you for the long support! On 2011/07/25 13:28:33, Mark Mentovai wrote: > Now that this is in today’s Canary and I’ve had a chance to see it, I wonder if > 550px is too wide. Would 500 suffice? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
