Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8198003:
Convert app_launch_index and page_index from int to StringOrdinal. (Closed)
|
Created:
9 years, 2 months ago by csharp Modified:
6 years, 1 month ago CC:
chromium-reviews, Erik does not do reviews, estade+watch_chromium.org, mihaip+watch_chromium.org, Paweł Hajdan Jr., SteveT, Rick Byers Visibility:
Public. |
DescriptionConvert app_launch_index and page_index from int to StringOrdinal.
This application icon index values are being changed from ints to StringOrdinals to decrease the potential number of syncs required when icons are moved, by only needing to change 1 StringOrdinal instead of all the integers.
BUG=61447
TEST=Open up an instance of chromium with multiple application icons and ensure
that icons can still have their page and order changed. There should be no
behaviour change.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114083
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changing strings to StringOrdinals #
Total comments: 8
Patch Set 3 : Adding constness and comments #
Total comments: 54
Patch Set 4 : Addressing review comments #
Total comments: 4
Patch Set 5 : New values called ordinals, old ones called indices #Patch Set 6 : Fixing compile error in integration test #
Total comments: 48
Patch Set 7 : Adding review comments #
Total comments: 2
Patch Set 8 : Fixing problem with setting app page names #
Total comments: 21
Patch Set 9 : Addressing review comments #
Total comments: 2
Patch Set 10 : Change NULl to NULL #
Total comments: 34
Patch Set 11 : Responding to Comments #Patch Set 12 : Fixing multimap assignment #Patch Set 13 : Fixing Page Name changes #
Total comments: 25
Patch Set 14 : Adding migrate unit test #Patch Set 15 : Correct inlining #
Total comments: 36
Patch Set 16 : Changing ..MinAndMax.. to ..MinOrMax.. #
Total comments: 10
Patch Set 17 : Adding typedef #Patch Set 18 : Syncing to ToT #Patch Set 19 : Fixing clang compile #Messages
Total messages: 65 (0 generated)
I've run the unit tests to ensure they all still pass and allow manually moved the icons around to ensure they still work. Are there any other tests I should run to make sure that nothing broke? I also don't have any tests for the migration code at the moment. I assume I'll need some but I was under sure how to test it and where the tests should live.
http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_prefs.cc:1399: base::StringToInt(max_value, &new_value); So all I'm seeing are conversions from int->string and string->int? I thought I was clear earlier, but I guess not. Basically, the point of using strings as indexes is to avoid the problem with ints, which is basically that it's not true that there is an int between two arbitrary ints (i.e., there's no dense ordering on the integers: http://en.wikipedia.org/wiki/Dense_order ). So an example of using strings would be to treat them as base-26 numbers in [0, 1), with a=0, b=1, ..., z=25. Then a valid string index would be any string that matches [a-z]*[b-z] (i.e., doesn't end in 'a'). So if you had 10 items, they may have indices: b, d, f, h, j, n, q, s, u, w. If you had to move the item with index 'w' between 'b' and 'd', then you'd set its index to 'c'. If you then had to move the item with index 'u' between 'b' and 'c', then you'd set its index to 'bm', since 'b' < 'bm' < 'c'. If you then had to move the item with index 's' before 'b', then you'd set its index to 'am', since 'am' < 'b'. And so on. A problem with this scheme is that the string indices may grow to arbitrary length. But they do so slowly enough that it probably doesn't matter. So you should either: - make utility functions to manipulate and compare these string indices. - make a class that's a thin wrapper around a string with methods to manipulate and compare these string indices. Then you can write code to migrate from ints to string indices.
Ok, this review is going to be put on hold for a bit. I'm going to create the cl that sets up the system to create the strings as akalin@ has suggested, but without replacing the integer values in app_launch_index and page_index. Once that is submitted I'll come back to this cl and update it then (the migration code will probably change a little bit).
On 2011/10/07 17:31:15, akalin wrote: > So an example of using strings would be to treat them as base-26 numbers in [0, > 1), with a=0, b=1, ..., z=25. Then a valid string index would be any string Small correction: [0, 1) should be (0, 1).
http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_prefs.cc:96: const char kPrefAppLaunchIndexDepreciated[] = "app_launcher_index"; Typo (use "Deprecated" instead of "Depreciated"). Applies below too. http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_prefs.cc:777: if (ReadExtensionPrefString(*ext_id, kPrefPageIndex, &index)) { I'm confused about this migration code, since this will only return true if you already have a (new-style) string page index key, in which case you _don't_ want to migrate, yet below you clobber it with the old value. Also, on a successful migration you should remove the old pref keys.
The code is now updated to use the new StringOrdinal class instead of integers. I've played around with it and everything seems to be working as intended, including the migration of data. http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_prefs.cc:96: const char kPrefAppLaunchIndexDepreciated[] = "app_launcher_index"; On 2011/10/07 23:15:12, Mihai Parparita wrote: > Typo (use "Deprecated" instead of "Depreciated"). Applies below too. Done. http://codereview.chromium.org/8198003/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_prefs.cc:777: if (ReadExtensionPrefString(*ext_id, kPrefPageIndex, &index)) { On 2011/10/07 23:15:12, Mihai Parparita wrote: > I'm confused about this migration code, since this will only return true if you > already have a (new-style) string page index key, in which case you _don't_ want > to migrate, yet below you clobber it with the old value. > > Also, on a successful migration you should remove the old pref keys. Done.
this looks relevant to dbeam's work
http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (left): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:860: When is the EOL of this migration code? http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1634: Can you put some comments here? Wtfs/line ratio can be helped by this, :). http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:310: StringOrdinal GetFirstAppPage(); From style guide: "Declare methods to be const whenever possible. Accessors should almost always be const. Other methods should be const if they do not modify any data members, do not call any non-const methods, and do not return a non-const pointer or non-const reference to a data member." Does this apply to the methods you're changing / adding? http://codereview.chromium.org/8198003/diff/14001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:357: return a.app_launch_index > b.app_launch_index; Hmmm, would it be better to return +a.app_launch_index - +b.app_launch_index; to cast to decimal number or return window.parseInt(a.app_launch_index, 10) - window.parseInt(b.app_launch_index); possibly? I guess this is OK as long as we never encounter something like "02" < "1" // true I'm just trying to think of if this would ever result in strange bugs...
http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (left): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:860: I don't know what the EOL should be for this code. I don't have any previous exposure to migration code before so I'm unsure how to decide what this should be. Hopefully someone with more experience can let me know :) On 2011/11/12 02:28:30, Dan Beam wrote: > When is the EOL of this migration code? http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1634: Added some comments, let me know if they clear things up enough. On 2011/11/12 02:28:30, Dan Beam wrote: > Can you put some comments here? Wtfs/line ratio can be helped by this, :). http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:310: StringOrdinal GetFirstAppPage(); Good point, I'd forgotten about this. The methods should be properly updated with constness now. On 2011/11/12 02:28:30, Dan Beam wrote: > From style guide: > > "Declare methods to be const whenever possible. Accessors should almost always > be const. Other methods should be const if they do not modify any data members, > do not call any non-const methods, and do not return a non-const pointer or > non-const reference to a data member." > > Does this apply to the methods you're changing / adding? http://codereview.chromium.org/8198003/diff/14001/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/new_tab.js (right): http://codereview.chromium.org/8198003/diff/14001/chrome/browser/resources/nt... chrome/browser/resources/ntp4/new_tab.js:357: return a.app_launch_index > b.app_launch_index; We don't want to do this because the app_launch_index values no longer have integer values, they are represented as [a..z]*. Look at chrome/common/string_ordinal.h for more information on exactly how they work. Basically we've replaced the integers with StringOrdinals (which are [b-z][a-z]*). They have the advantage that if we move an icon, we only need to change the index of that icon to represent the new order, all the other values can stay the same.
lgtm with comment (btw, I'm only provisional at the moment): The reason I asked you when the migration code might die is that it seems like anything that's meant to migrate data is stuck forever as less and less useful code. I think it'd be useful to try to record how many people this happens to over time and see when that number is small enough that we can remove it or figure out some other general strategy to not leave this code here forever.
Any comments from the other reviewers?
The number of index-related methods in ExtensionPrefs has doubled, which makes things harder to follow and maintain. I would suggest moving all of the index-related code to a separate class that wraps ExtensionPrefs. Beyond that high level comment I don't feel qualified to review this, since I haven't done anything with page/launch indices. Perhaps try Finnur since he added launch indices wit r62112?
Some initial comments and a high-level one: It would probably be cleaner to consistently refer to the old integer indices as "...Index", and the new string ordinals as "...Ordinal". http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/c... File chrome/browser/extensions/crx_installer.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.h:181: void set_page_index(StringOrdinal page_index) { const ref http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:103: // If |page_index| is kUnsetIndex, and the then a page will be found kUnsetIndex, and the then -> an invalid ordinal, then http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordinal.h File chrome/common/string_ordinal.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:35: static StringOrdinal CreateValidOrdinal(); maybe "CreateFirstOrdinal"? http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:67: struct Comparsion { Comparsion -> Comparison Although the usual name is LessThan. Perhaps make it a top-level class named "StringOrdinalLessThan". http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:69: return lhs.LessThan(rhs); de-inline this
BUG= is missing bug ID. Also, rbyers should at least get a CC, since this touches on the Page Index as well. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:244: // A simple structure used when sorting extension based on the order they extension -> apps? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:901: } No braces for single line if clauses. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:909: int old_app_launch_index(0); nit: Even though you can, we never initialize ints this way in the codebase... I'd just stick to the general way. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:945: &old_app_launch_index)) { old_page_index is read and used above. old_app_launch_index, on the other hand, is read but not used. Will this not change the order of apps, potentially? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1542: void ExtensionPrefs::GetAllAppsOnPageSorted( This doesn't really get all Apps on the page as much as all the app launch indices (sorted)... No? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1544: std::set<StringOrdinal, StringOrdinal::Comparsion>* indexes_on_page) const { indexes -> indices? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1580: } nit: no braces around single-line clauses. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1587: return StringOrdinal(raw_value); Is the idea here to return a blank ordinal if the pref read fails? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1621: } same here: no braces (also on line 1613, 1615). http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1673: return StringOrdinal(raw_string_data); Same here: Is the idea here to return a blank ordinal if the pref read fails? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1692: UpdatePageIndexMap(StringOrdinal(), GetPageIndex(*ext_it)); Are there cases here where you can end up with {StringOrdinal(), StringOrdinal()} ? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1729: } nit: You don't need braces, your teeth look fine... http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:319: // launch index a invalid StringOrdinal is returned. nit: a invalid -> an invalid. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:328: StringOrdinal GetNextAppLaunchIndex(StringOrdinal on_page) const; const ref? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:330: // Gets the page a new app should install to. Starts the first page, and if Starts the first page? You mean Starts on (or with) the first page? (or am I misreading this?) http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:478: // Reads a string pref |pref_key| from extension with id |extension_id| nit: end comments with punctuation marks. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:552: std::set<StringOrdinal, StringOrdinal::Comparsion>* indexs_on_page) const; indexs? Is that a typo? Do you mean indices? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:558: // Update a value in the page_index_map_. This needs more documenting... What does update mean in this context? What do old and new values mean? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:568: // A map of all the StringOrdinal page indexs mapping to how often they are Here it is again... indexs... ? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:191: : prefs->GetNextAppLaunchIndex(page_index); nit: operator (:) should be at the end of previous line. http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordinal.h File chrome/common/string_ordinal.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:35: static StringOrdinal CreateValidOrdinal(); I went back and forth on this. I vaguely prefer CreateValidOrdinal over CreateFirstOrdinal, since the latter seems to imply that you'll get an 'a' as opposed to an 'n'. Just to add to the mix, you might call it CreateInitialOrdinal, but I'll leave it up to you. On 2011/11/17 03:06:32, akalin wrote: > maybe "CreateFirstOrdinal"?
Most of the issues addressed but I have two questions: 1) akalin, I assume you're suggesting changing all the functions from ...Index to ...Ordinal, and not just the comments? (I just wanted to double check) 2) What do people think about adding a separate class to encapsulate the Index/Ordinal functions as suggested by mihaip. Should I do that in this change or add a bug to myself and do that after committing this change? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/c... File chrome/browser/extensions/crx_installer.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/c... chrome/browser/extensions/crx_installer.h:181: void set_page_index(StringOrdinal page_index) { On 2011/11/17 03:06:32, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:244: // A simple structure used when sorting extension based on the order they On 2011/11/17 14:59:25, Finnur wrote: > extension -> apps? Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:901: } On 2011/11/17 14:59:25, Finnur wrote: > No braces for single line if clauses. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:909: int old_app_launch_index(0); On 2011/11/17 14:59:25, Finnur wrote: > nit: Even though you can, we never initialize ints this way in the codebase... > I'd just stick to the general way. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:945: &old_app_launch_index)) { I don't think the order can change, because set that we are iterating though here is sorted so that when we are placing an app, all the ones before it have already been placed, so we know it will go into the next open position. This could break if we have positions that have no app, but are suppose to show up as empty space in the NTP, but as far as I'm aware we don't have that. On 2011/11/17 14:59:25, Finnur wrote: > old_page_index is read and used above. > old_app_launch_index, on the other hand, is read but not used. > > Will this not change the order of apps, potentially? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1542: void ExtensionPrefs::GetAllAppsOnPageSorted( Correct, function name updated On 2011/11/17 14:59:25, Finnur wrote: > This doesn't really get all Apps on the page as much as all the app launch > indices (sorted)... No? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1544: std::set<StringOrdinal, StringOrdinal::Comparsion>* indexes_on_page) const { On 2011/11/17 14:59:25, Finnur wrote: > indexes -> indices? Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1580: } On 2011/11/17 14:59:25, Finnur wrote: > nit: no braces around single-line clauses. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1587: return StringOrdinal(raw_value); Yes, which is an invalid ordinal which does represent an unset pref. I've added a comment to try and make this clearer. On 2011/11/17 14:59:25, Finnur wrote: > Is the idea here to return a blank ordinal if the pref read fails? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1621: } On 2011/11/17 14:59:25, Finnur wrote: > same here: no braces (also on line 1613, 1615). Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1673: return StringOrdinal(raw_string_data); Yes, and comment added to clarify. On 2011/11/17 14:59:25, Finnur wrote: > Same here: Is the idea here to return a blank ordinal if the pref read fails? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1692: UpdatePageIndexMap(StringOrdinal(), GetPageIndex(*ext_it)); It is possible, but the map only updates for valid ordinals. If either input is invalid it has no affect on the mapping (if the other value is valid it will still affect the map by itself). On 2011/11/17 14:59:25, Finnur wrote: > Are there cases here where you can end up with {StringOrdinal(), > StringOrdinal()} ? http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1729: } On 2011/11/17 14:59:25, Finnur wrote: > nit: You don't need braces, your teeth look fine... Done and :/ http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:103: // If |page_index| is kUnsetIndex, and the then a page will be found On 2011/11/17 03:06:32, akalin wrote: > kUnsetIndex, and the then -> an invalid ordinal, then Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:319: // launch index a invalid StringOrdinal is returned. On 2011/11/17 14:59:25, Finnur wrote: > nit: a invalid -> an invalid. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:328: StringOrdinal GetNextAppLaunchIndex(StringOrdinal on_page) const; On 2011/11/17 14:59:25, Finnur wrote: > const ref? Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:330: // Gets the page a new app should install to. Starts the first page, and if On 2011/11/17 14:59:25, Finnur wrote: > Starts the first page? You mean Starts on (or with) the first page? (or am I > misreading this?) Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:478: // Reads a string pref |pref_key| from extension with id |extension_id| On 2011/11/17 14:59:25, Finnur wrote: > nit: end comments with punctuation marks. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:552: std::set<StringOrdinal, StringOrdinal::Comparsion>* indexs_on_page) const; On 2011/11/17 14:59:25, Finnur wrote: > indexs? Is that a typo? > Do you mean indices? Fixed. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:558: // Update a value in the page_index_map_. Comment updated to provide more details, let me know if this is still a bit unclear. On 2011/11/17 14:59:25, Finnur wrote: > This needs more documenting... What does update mean in this context? What do > old and new values mean? Done. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:568: // A map of all the StringOrdinal page indexs mapping to how often they are On 2011/11/17 14:59:25, Finnur wrote: > Here it is again... indexs... ? Fixed. http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:191: : prefs->GetNextAppLaunchIndex(page_index); On 2011/11/17 14:59:25, Finnur wrote: > nit: operator (:) should be at the end of previous line. Done. http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordinal.h File chrome/common/string_ordinal.h (right): http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:35: static StringOrdinal CreateValidOrdinal(); Changing to CreateInitialOrdinal since that expresses what is being returned and when this should be called. On 2011/11/17 14:59:25, Finnur wrote: > I went back and forth on this. > > I vaguely prefer CreateValidOrdinal over CreateFirstOrdinal, since the latter > seems to imply that you'll get an 'a' as opposed to an 'n'. > > Just to add to the mix, you might call it CreateInitialOrdinal, but I'll leave > it up to you. > > On 2011/11/17 03:06:32, akalin wrote: > > maybe "CreateFirstOrdinal"? > http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:67: struct Comparsion { On 2011/11/17 03:06:32, akalin wrote: > Comparsion -> Comparison > > Although the usual name is LessThan. Perhaps make it a top-level class named > "StringOrdinalLessThan". Done. http://codereview.chromium.org/8198003/diff/20001/chrome/common/string_ordina... chrome/common/string_ordinal.h:69: return lhs.LessThan(rhs); On 2011/11/17 03:06:32, akalin wrote: > de-inline this Done.
On Thu, Nov 17, 2011 at 11:51 AM, <csharp@chromium.org> wrote: > Most of the issues addressed but I have two questions: > 1) akalin, I assume you're suggesting changing all the functions from > ...Index > to ...Ordinal, and not just the comments? (I just wanted to double check) Yeah, function names, variable names, etc.
LGTM > Should I do that in this change or add a bug to > myself and do that after committing this change? I would do it in a separate changelist so that you don't complicate this changelist and because it is easier to review if it is just moving code around (little/no functionality change). http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1729: } Awww... isn't that smile lovely? :) On 2011/11/17 19:51:58, csharp wrote: > On 2011/11/17 14:59:25, Finnur wrote: > > nit: You don't need braces, your teeth look fine... > > Done and :/ http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:191: : prefs->GetNextAppLaunchIndex(page_index); Did you? I don't see the change... On 2011/11/17 19:51:58, csharp wrote: > On 2011/11/17 14:59:25, Finnur wrote: > > nit: operator (:) should be at the end of previous line. > > Done.
Ok, all locations that use the new ordinals should refer to them as such and index should only appear when talking about the old values or the pages in the javascript code (since they are still index values there) http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/20001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:191: : prefs->GetNextAppLaunchIndex(page_index); On 2011/11/18 09:35:15, Finnur wrote: > Did you? I don't see the change... > > On 2011/11/17 19:51:58, csharp wrote: > > On 2011/11/17 14:59:25, Finnur wrote: > > > nit: operator (:) should be at the end of previous line. > > > > Done. > Done. http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:359: // This is necessary because in some previous versions of chrome, we set a I'd like to move this else into the migration code, but I can't seem to find anyway in ExtensionPrefs to figure out which extension should have ordinal values. Any advice?
http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:359: // This is necessary because in some previous versions of chrome, we set a Not sure who this question is addressed to, but my answer would be that all apps that appear on the NTP should have ordinal values. There are functions on the Extension object to determine whether an extension is an app, but I don't see why you need to make a determination. If any extension has an invalid page index, wouldn't you want to clear it? Or do we still use the page index for non-app extensions? On 2011/11/18 15:57:31, csharp wrote: > I'd like to move this else into the migration code, but I can't seem to find > anyway in ExtensionPrefs to figure out which extension should have ordinal > values. Any advice?
http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:359: // This is necessary because in some previous versions of chrome, we set a The problem is that some extensions (which don't appear on the NTP) have valid indices, but they shouldn't. Also, I don't think any of the functions to determine if a extension is app can be called from the migration code, which led me to this question. On 2011/11/21 12:16:14, Finnur wrote: > Not sure who this question is addressed to, but my answer would be that all apps > that appear on the NTP should have ordinal values. > > There are functions on the Extension object to determine whether an extension is > an app, but I don't see why you need to make a determination. If any extension > has an invalid page index, wouldn't you want to clear it? Or do we still use the > page index for non-app extensions? > > On 2011/11/18 15:57:31, csharp wrote: > > I'd like to move this else into the migration code, but I can't seem to find > > anyway in ExtensionPrefs to figure out which extension should have ordinal > > values. Any advice? >
http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:359: // This is necessary because in some previous versions of chrome, we set a > The problem is that some extensions (which don't appear > on the NTP) have valid indices, but they shouldn't. Um... that was my point. Should any extension (app or not) have an invalid PageIndex? Why is it not ok to clear any invalid PageIndices? In any case, I'm not sure you need to get hung up on this for this changelist, it seems like a nice to move to a better location but not crucial and could be done separately...
Ah, I see what you're saying, I misunderstood. I don't think any extension can have an invalid page index set, but they can have no page index set which would cause the get function here to return an Invalid value. The only reason I can see us not wanting to clear the page index when we have an invalid value is that nothing will change after doing that (it will still return an Invalid ordinal from GetPageIndex). I'll just leave this code the way it is. Everything seems to be settled for this review now, so I'll commit via the commit queue On 2011/11/21 15:23:20, Finnur wrote: > http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... > File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): > > http://codereview.chromium.org/8198003/diff/35001/chrome/browser/ui/webui/ntp... > chrome/browser/ui/webui/ntp/app_launcher_handler.cc:359: // This is necessary > because in some previous versions of chrome, we set a > > The problem is that some extensions (which don't appear > > on the NTP) have valid indices, but they shouldn't. > > Um... that was my point. Should any extension (app or not) have an invalid > PageIndex? Why is it not ok to clear any invalid PageIndices? > > In any case, I'm not sure you need to get hung up on this for this changelist, > it seems like a nice to move to a better location but not crucial and could be > done separately...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/46001
Commit queue showed me that I had forget to compile sync_integration_tests and one of those tests needed to be updated to use StringOrdinals. Fixed that test and build other tests to ensure I didn't miss any other places. Will put change back in commit queue after lunch.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/51002
Presubmit check for 8198003-51002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/sync/test/integration/sync_extension_helper.cc Presubmit checks took 3.0s to calculate.
mostly nits, a few more substantial comments http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:255: app_launch_index(app_launch) {} add a default constructor and init int args to 0. but see later comments, this class may not be necessary at all http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:899: if (extension_ids.begin() == extension_ids.end()) use .empty() http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:917: if (has_old_values) { Hmm, I don't understand why you have to stuff these into a set and then loop over them a second time. Presumably you want to do something if only one of the prefs need to be migrated, but looking at the second loop, you're doing nothing for the non-migration-needed pref, anyway. Why not: for (ExtensionidSet::const_iterator ...) { if (ReadExtensionPrefInteger(*ext_id, ..., &old_page_index)) { SetPageOrdinal(...); } if (ReadEx...(...&old_app_launch_undex)) { SetAppLaunchordinal(...); } } http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:930: for (std::set<ApplicationNTPOldPosition, SortByNTPOldPosition>::iterator ext = I think this can be a const iterator http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1544: std::set<StringOrdinal, StringOrdinalLessThan>* ordinals_on_page) const { looks like you only really care about the min/max ordinals. why not just have output params for those two instead and return false when the page is empty? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1546: if (!extensions || !target_page_ordinal.IsValid()) can this fn actually be called with an invalid page ordinal? CHECK instead? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1562: if (!extensions) can this actually happen? if not, CHECK. If so, do we really want to return an invalid ordinal instead of an initial one? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1565: if (page_ordinal_map_.size() == 0) use .empty() http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1593: const StringOrdinal& ordinal) { indent http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1611: if (!extensions) same question as GetFirstAppPage http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1614: if (page_ordinal_map_.size() == 0) use empty http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1633: size_t position = std::distance(extension_ids.begin(), it); use *it - begin() (since we know it's constant-time) CHECK that it != end()? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1655: advance(it, -2); it -= 2 http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1698: const StringOrdinal& new_value) { indent http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1730: if (page_ordinal_map_.size() == 0) use empty() http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // Page. A value close to |A*| generally indicates top left. If the value -> string value, A* -> a* http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:350: // Convert the page StringOrdinal value to its integer equivalent. Probably should mention that this method is worst-case O(# of apps) http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:353: // Converts the page index integer to its StringOrdinal equivalent. this too http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.h (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.h:378: StringOrdinal page_ordinal); const ref http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:177: StringOrdinal app_launch_ordinal = any reason why you switched it to handle page ordinal first? Just curious, as it confused me at first http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:426: static_cast<DictionaryValue*>( hmm are these casts necessary? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:430: static_cast<DictionaryValue*>( here too http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:726: StringOrdinal page_ordinal = const ref http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:751: StringOrdinal page_ordinal = const ref
On 2011/11/16 20:56:10, Mihai Parparita wrote: > The number of index-related methods in ExtensionPrefs has doubled, which makes > things harder to follow and maintain. I would suggest moving all of the > index-related code to a separate class that wraps ExtensionPrefs. +1 (I see that you are going to do this in a separate CL, I just wanted to add my vote).
http://codereview.chromium.org/8198003/diff/60001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/60001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:861: pref_service->RegisterDictionaryPref(prefs::kNTPAppPageNames, there are no problems with migration here?
Fixed an issue with changing the app page names and publishing comments from yesterday's update that I forgot to send. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:255: app_launch_index(app_launch) {} On 2011/11/21 19:49:24, akalin wrote: > add a default constructor and init int args to 0. but see later comments, this > class may not be necessary at all Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:899: if (extension_ids.begin() == extension_ids.end()) On 2011/11/21 19:49:24, akalin wrote: > use .empty() Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:917: if (has_old_values) { Ok, SetPageOrdinal was rolled up into the first loop, but setting app launch still has to stay in a second loop, because we need to processor the apps their app launch order, because otherwise GetNextAppLaunchOrdinal will return incorrect values because some of the ordinals that should be in front of it might not have been converted yet. I updated the comment for the On 2011/11/21 19:49:24, akalin wrote: > Hmm, I don't understand why you have to stuff these into a set and then loop > over them a second time. Presumably you want to do something if only one of the > prefs need to be migrated, but looking at the second loop, you're doing nothing > for the non-migration-needed pref, anyway. Why not: > > for (ExtensionidSet::const_iterator ...) { > if (ReadExtensionPrefInteger(*ext_id, ..., &old_page_index)) { > SetPageOrdinal(...); > } > if (ReadEx...(...&old_app_launch_undex)) { > SetAppLaunchordinal(...); > } > } http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:930: for (std::set<ApplicationNTPOldPosition, SortByNTPOldPosition>::iterator ext = On 2011/11/21 19:49:24, akalin wrote: > I think this can be a const iterator Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1544: std::set<StringOrdinal, StringOrdinalLessThan>* ordinals_on_page) const { On 2011/11/21 19:49:24, akalin wrote: > looks like you only really care about the min/max ordinals. why not just have > output params for those two instead and return false when the page is empty? Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1546: if (!extensions || !target_page_ordinal.IsValid()) They should always be valid, adding CHECK. On 2011/11/21 19:49:24, akalin wrote: > can this fn actually be called with an invalid page ordinal? CHECK instead? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1562: if (!extensions) It looks like this should always work, so I'll add the CHECK. On 2011/11/21 19:49:24, akalin wrote: > can this actually happen? if not, CHECK. If so, do we really want to return an > invalid ordinal instead of an initial one? http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1565: if (page_ordinal_map_.size() == 0) On 2011/11/21 19:49:24, akalin wrote: > use .empty() Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1593: const StringOrdinal& ordinal) { On 2011/11/21 19:49:24, akalin wrote: > indent Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1611: if (!extensions) On 2011/11/21 19:49:24, akalin wrote: > same question as GetFirstAppPage Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1614: if (page_ordinal_map_.size() == 0) On 2011/11/21 19:49:24, akalin wrote: > use empty Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1633: size_t position = std::distance(extension_ids.begin(), it); On 2011/11/21 19:49:24, akalin wrote: > use *it - begin() (since we know it's constant-time) > > CHECK that it != end()? Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1655: advance(it, -2); On 2011/11/21 19:49:24, akalin wrote: > it -= 2 Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1698: const StringOrdinal& new_value) { On 2011/11/21 19:49:24, akalin wrote: > indent Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1730: if (page_ordinal_map_.size() == 0) On 2011/11/21 19:49:24, akalin wrote: > use empty() Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // Page. A value close to |A*| generally indicates top left. If the On 2011/11/21 19:49:24, akalin wrote: > value -> string value, A* -> a* Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:350: // Convert the page StringOrdinal value to its integer equivalent. On 2011/11/21 19:49:24, akalin wrote: > Probably should mention that this method is worst-case O(# of apps) Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:353: // Converts the page index integer to its StringOrdinal equivalent. On 2011/11/21 19:49:24, akalin wrote: > this too Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.h (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.h:378: StringOrdinal page_ordinal); On 2011/11/21 19:49:24, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:177: StringOrdinal app_launch_ordinal = Switched it around because we need the page ordinal when creating the app launch ordinal (since the values are created relative to the other values on the page) On 2011/11/21 19:49:24, akalin wrote: > any reason why you switched it to handle page ordinal first? Just curious, as > it confused me at first http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:426: static_cast<DictionaryValue*>( On 2011/11/21 19:49:24, akalin wrote: > hmm are these casts necessary? Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:430: static_cast<DictionaryValue*>( On 2011/11/21 19:49:24, akalin wrote: > here too Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:726: StringOrdinal page_ordinal = On 2011/11/21 19:49:24, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/51002/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:751: StringOrdinal page_ordinal = On 2011/11/21 19:49:24, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/60001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/60001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:861: pref_service->RegisterDictionaryPref(prefs::kNTPAppPageNames, This did cause problems. Changed names wouldn't load properly the next time the NTP was loaded. See other comment for fix. On 2011/11/22 22:50:37, Evan Stade wrote: > there are no problems with migration here? http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:435: dictionary->Set("appPageNames", list); This seems to be the best way I could find to convert the keys of the dictionary to a list, but please point me to a better method if one exists.
Some more comments http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:938: if (ReadExtensionPrefInteger(*(ext->application_id), why is this conditional still needed? you already did it in the first loop, right? you're not even using the value http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:942: if (!page.IsValid()) { can you fold this into the first loop? it can be an 'else' statement to the first if statement of the first loop, right? http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:949: UpdateExtensionPref( can you do this in the first loop? http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1169: StringOrdinal page_ordinal) { const ref http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1693: // If the preference read fails then raw_value will still be unset and we will raw_value -> raw_data http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1700: const StringOrdinal& page_ordinal) { indent http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:108: StringOrdinal page_ordinal); const ref http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:337: void SetAppLauncherOrder(const std::vector<std::string>& extension_ids, this function doesn't really do what it says anymore. It's more like "OnExtensionMoved". I think it should look something like: void OnExtensionMoved( const std::string& moved_extension_id, const std::string& predecessor_extension_id, const std::string& successor_extension_id); where pred and succ can be empty (meaning the extension is at the beginning or at the end, respectively). Then the caller can do all the heavy lifting of figuring that info out.
http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:435: dictionary->Set("appPageNames", list); On 2011/11/23 15:38:07, csharp wrote: > This seems to be the best way I could find to convert the keys of the dictionary > to a list, but please point me to a better method if one exists. this seems fine, my original comment was concerned with whether changing from a list to a dictionary pref type would work correctly (can list json be read into a dictionary? what would the keys be?)
http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:938: if (ReadExtensionPrefInteger(*(ext->application_id), On 2011/11/23 18:50:19, akalin wrote: > why is this conditional still needed? you already did it in the first loop, > right? you're not even using the value Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:942: if (!page.IsValid()) { I thought it could, but unit tests proved me wrong. If we do it as an else to the first if statement we end up creating ordinal value for extensions that shouldn't have it, since we don't (at this point) have a good way of knowing what should have values and what shouldn't. The only reason we aren't bitten by this problem in the other loop is because we at least found a app launch value that needed to be migrated, we know a page ordinal is required. On 2011/11/23 18:50:19, akalin wrote: > can you fold this into the first loop? it can be an 'else' statement to the > first if statement of the first loop, right? http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:949: UpdateExtensionPref( On 2011/11/23 18:50:19, akalin wrote: > can you do this in the first loop? Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1169: StringOrdinal page_ordinal) { On 2011/11/23 18:50:19, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1693: // If the preference read fails then raw_value will still be unset and we will On 2011/11/23 18:50:19, akalin wrote: > raw_value -> raw_data Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1700: const StringOrdinal& page_ordinal) { On 2011/11/23 18:50:19, akalin wrote: > indent Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:108: StringOrdinal page_ordinal); On 2011/11/23 18:50:19, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:337: void SetAppLauncherOrder(const std::vector<std::string>& extension_ids, On 2011/11/23 18:50:19, akalin wrote: > this function doesn't really do what it says anymore. It's more like > "OnExtensionMoved". I think it should look something like: > > void OnExtensionMoved( > const std::string& moved_extension_id, > const std::string& predecessor_extension_id, > const std::string& successor_extension_id); > > where pred and succ can be empty (meaning the extension is at the beginning or > at the end, respectively). Then the caller can do all the heavy lifting of > figuring that info out. Done.
ping akalin, estade, mihaip Hey dudes, I need to (semi-non-trivially) integrate with this code as soon as possible for a patch I've been working on for a while that's scheduled for M17. I know we're likely all busy right now, but helping csharp resolve any remaining questions would be much appreciated. http://codereview.chromium.org/8198003/diff/70002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/70002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:561: // NULl values for |min_app_launch_value| or |max_app_launch_value|. NULL
Also, csharp, I think the issue's description (and future commit message) should be BUG=61447 now, yeah?
Bug id updated. http://codereview.chromium.org/8198003/diff/70002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/70002/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:561: // NULl values for |min_app_launch_value| or |max_app_launch_value|. On 2011/11/29 01:28:25, Dan Beam wrote: > NULL Done.
More comments http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:942: if (!page.IsValid()) { On 2011/11/23 22:10:01, csharp wrote: > I thought it could, but unit tests proved me wrong. > If we do it as an else to the first if statement we end up creating ordinal > value for extensions that shouldn't have it, since we don't (at this point) have > a good way of knowing what should have values and what shouldn't. > > The only reason we aren't bitten by this problem in the other loop is because we > at least found a app launch value that needed to be migrated, we know a page > ordinal is required. Ah, I see. But it can certainly still be in the first loop. Something like: for (...) { StringOrdinal page; if (ReadExtensionPref...kPrefPageIndex...) { page = PageIntegerAsStringOrdinal(old_page_index); ... } if (ReadExtensionPref...kPrefAppLaunchIndexDeprecated) { ... if (!page.isValid()) { ... } } } Basically, the second loop exists only because we have to set the app launch indices in a certain order. So that should be all it's doing. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:903: std::multiset<ApplicationNTPOldAppLaunch, SortByNTPOldAppLaunch> Any particular reason you're using a multiset? If you use a multimap from int -> const string* you can avoid the extra struct. Also, any particular reason for the 'multi'? Is it expected that apps would have the same launch index? Are you just being defensive? If so, that's okay. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1160: void ExtensionPrefs::SetAppOrdinalValues(DictionaryValue* extension_dict, this is called in essentially one place, right? I think it's better to inline it, since the name is kind of generic http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1162: extension_dict->Set(kPrefPageOrdinal, doesn't this overlap with SetPageOrdinal? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1564: min_value = &app_launch_ordinal; uh, how does this work? you're setting the address to a stack variable scoped to the for loop, and then accessing it outside the for loop. You probably just want to mutate {min,max}_app_launch_value directly. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1580: return min_app_launch_value->IsValid(); it looks like the callers can just check IsValid() on either min or max, so we may not need the retval after all http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1640: std::map<StringOrdinal, int>::const_iterator it = page_ordinal_map_.begin(); scope this to the for loop (and indent as needed) http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1656: if (!predecessor_extension_id.empty() || !successor_extension_id.empty()) { do an early exit if there are no neighbors to avoid posting a notification? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1668: StringOrdinal sucessor_ordinal = sucessor -> successor http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1718: if (page_ordinal_map_[old_value] == 0) is this correct? this changes all the indices of the following pages, right? also, you add in 0-valued pages in PageIntegerAsStringOrdinal. How is this supposed to work? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1725: const { move to prev line http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1746: advance(it, page_index); std::advance? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1751: page_ordinal_map_[StringOrdinal::CreateInitialOrdinal()] = 0; ah, i see why this isn't const. Perhaps split off the resizing into its own method, say 'RegisterPageInteger'? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:313: StringOrdinal CreateFirstAppLaunchOrdinal(const StringOrdinal& page_ordinal) Move this closer to GetNextAppLaunchOrdinal since they're complementary (don't forget the implementation in the .cc file, too) Maybe rename to GetFirst... to make the parallel clear http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:338: // Empty strings are used to indicate no successor or predecessor. move this near the other 'OnExtension...' functions http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:362: StringOrdinal PageIntegerAsStringOrdinal(size_t page_index); const? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:562: bool GetMaxAndMinAppLaunchOrdinalsOnPage( you definitely want to name this 'GetMinAndMax' to match param orders http://codereview.chromium.org/8198003/diff/84001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:171: // We convert the page_ordinal to an integer becuase the pages are referenced becuase -> because
http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:942: if (!page.IsValid()) { On 2011/11/29 18:39:09, akalin wrote: > On 2011/11/23 22:10:01, csharp wrote: > > I thought it could, but unit tests proved me wrong. > > If we do it as an else to the first if statement we end up creating ordinal > > value for extensions that shouldn't have it, since we don't (at this point) > have > > a good way of knowing what should have values and what shouldn't. > > > > The only reason we aren't bitten by this problem in the other loop is because > we > > at least found a app launch value that needed to be migrated, we know a page > > ordinal is required. > > Ah, I see. But it can certainly still be in the first loop. Something like: > > for (...) { > StringOrdinal page; > if (ReadExtensionPref...kPrefPageIndex...) { > page = PageIntegerAsStringOrdinal(old_page_index); > ... > } > > if (ReadExtensionPref...kPrefAppLaunchIndexDeprecated) { > ... > if (!page.isValid()) { > ... > } > } > } > > Basically, the second loop exists only because we have to set the app launch > indices in a certain order. So that should be all it's doing. Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:903: std::multiset<ApplicationNTPOldAppLaunch, SortByNTPOldAppLaunch> Changed to multimap (didn't think about doing this when I remove the page value from the struct). Multi is needed because all the pages have their apps placed in this one map and each page can (and usually has) an app with app_launch_index 0. On 2011/11/29 18:39:09, akalin wrote: > Any particular reason you're using a multiset? If you use a multimap from int > -> const string* you can avoid the extra struct. > > Also, any particular reason for the 'multi'? Is it expected that apps would > have the same launch index? Are you just being defensive? If so, that's okay. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1160: void ExtensionPrefs::SetAppOrdinalValues(DictionaryValue* extension_dict, On 2011/11/29 18:39:09, akalin wrote: > this is called in essentially one place, right? I think it's better to inline > it, since the name is kind of generic Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1162: extension_dict->Set(kPrefPageOrdinal, It does, changing to just call those functions. On 2011/11/29 18:39:09, akalin wrote: > doesn't this overlap with SetPageOrdinal? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1564: min_value = &app_launch_ordinal; On 2011/11/29 18:39:09, akalin wrote: > uh, how does this work? you're setting the address to a stack variable scoped > to the for loop, and then accessing it outside the for loop. You probably just > want to mutate {min,max}_app_launch_value directly. Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1580: return min_app_launch_value->IsValid(); On 2011/11/29 18:39:09, akalin wrote: > it looks like the callers can just check IsValid() on either min or max, so we > may not need the retval after all Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1640: std::map<StringOrdinal, int>::const_iterator it = page_ordinal_map_.begin(); On 2011/11/29 18:39:09, akalin wrote: > scope this to the for loop (and indent as needed) Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1656: if (!predecessor_extension_id.empty() || !successor_extension_id.empty()) { On 2011/11/29 18:39:09, akalin wrote: > do an early exit if there are no neighbors to avoid posting a notification? Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1668: StringOrdinal sucessor_ordinal = On 2011/11/29 18:39:09, akalin wrote: > sucessor -> successor Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1718: if (page_ordinal_map_[old_value] == 0) I'm pretty sure this is correct. When a page no longer has any icons on it, it disappears from the NTP, so we would want to remove it and decrease the index of all the pages after it. The zero-value pages should only come into affect for the migration code, because we could end up placing the apps on page 5 before the apps on page 2 (based on their app launch ordinals), so we need to temporarily add the 0-valued pages. I've updated the code so only the migration code can create 0-valued pages, which also lets PageIntegerAsStringOrdinal become a const function. On 2011/11/29 18:39:09, akalin wrote: > is this correct? this changes all the indices of the following pages, right? > > also, you add in 0-valued pages in PageIntegerAsStringOrdinal. How is this > supposed to work? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1725: const { On 2011/11/29 18:39:09, akalin wrote: > move to prev line Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1746: advance(it, page_index); On 2011/11/29 18:39:09, akalin wrote: > std::advance? Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1751: page_ordinal_map_[StringOrdinal::CreateInitialOrdinal()] = 0; Remove the need for adding extra values to the map, so there isn't much code here now. On 2011/11/29 18:39:09, akalin wrote: > ah, i see why this isn't const. > > Perhaps split off the resizing into its own method, say 'RegisterPageInteger'? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:313: StringOrdinal CreateFirstAppLaunchOrdinal(const StringOrdinal& page_ordinal) Moved together and renamed some of the function to be Create.. instead of Get.. since they aren't returning a pre-existing value but are creating a new one. On 2011/11/29 18:39:09, akalin wrote: > Move this closer to GetNextAppLaunchOrdinal since they're complementary (don't > forget the implementation in the .cc file, too) > > Maybe rename to GetFirst... to make the parallel clear http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:338: // Empty strings are used to indicate no successor or predecessor. On 2011/11/29 18:39:09, akalin wrote: > move this near the other 'OnExtension...' functions Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:362: StringOrdinal PageIntegerAsStringOrdinal(size_t page_index); Code changed so now it is :) On 2011/11/29 18:39:09, akalin wrote: > const? http://codereview.chromium.org/8198003/diff/84001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:562: bool GetMaxAndMinAppLaunchOrdinalsOnPage( On 2011/11/29 18:39:09, akalin wrote: > you definitely want to name this 'GetMinAndMax' to match param orders Done. http://codereview.chromium.org/8198003/diff/84001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/84001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:171: // We convert the page_ordinal to an integer becuase the pages are referenced On 2011/11/29 18:39:09, akalin wrote: > becuase -> because Done.
Fixed a bug with app_page_names being incorrectly converted from a list to a dictionary. Didn't realize that app_page_name is only every referenced by index, so it doesn't need to be a dictionary. Revert the code back to being a list.
More comments http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:878: // Convert all the page index values to page ordinals. If there are any can't you do a two-level map, the outer one keyed by page ordinal and the inner one keyed by page index values? That seems clearer than lumping all the app indices together http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:881: std::multimap<int,const std::string*> app_launches_to_convert; space after 'int,' http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:893: if (page_ordinal_map_.size() == 0) .empty() http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:917: std::pair<int,const std::string*>( use std::make_pair http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1204: remove extra space http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1206: StringOrdinal new_page_ordinal = GetNaturalAppPageOrdinal(); by 'inline this code' i meant inline and eliminate redundancy. The two clauses differ only in the page ordinal. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1253: StringOrdinal predecessor_ordinal = const ref http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1255: StringOrdinal successor_ordinal = const ref http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1588: StringOrdinal app_launch_ordinal = GetAppLaunchOrdinal(*ext_it); const ref http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1595: max_app_launch_value->LessThan(app_launch_ordinal)) surely you mean GreaterThan? Why didn't any tests catch this? Do you need to write one for this function? http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // This determines the order of which the applications appear on the New Tab update comment to reflect that the app ordinal is within a specified page http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:336: // Create a StringOrdinal that will be the firsrt page. firsrt -> first
So at each iteration, I keep finding new bugs, and I'm still just reviewing only one .h/.cc file pair. Are you writing tests for the bugs that you've found so far? If not, you should be. I'm also a bit distressed that these bugs slipped by all the other reviewers. It looks like this would have been checked in bugs and all had this CL not touched a file in sync-land by happenstance. :/
On 2011/12/01 03:00:23, akalin wrote: > So at each iteration, I keep finding new bugs, and I'm still just reviewing only > one .h/.cc file pair. Are you writing tests for the bugs that you've found so > far? If not, you should be. Forgot to mention that this might also be a sign that this CL needs to be split up into smaller ones. Although I'm not sure how much it can be split up. Maybe some smaller pieces, like the additions to StringOrdinal, etc.
Most of the code does have test, but the migration code didn't. I wasn't sure originally how to test it and I forgot to bug people to find a way to do so. I think the other changes are covered by the existing unit test (at least it seemed so when I looked at them). The migration code now has a unit test, I'm not sure if it is the best way to test it, but it does test it. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:878: // Convert all the page index values to page ordinals. If there are any On 2011/12/01 02:57:47, akalin wrote: > can't you do a two-level map, the outer one keyed by page ordinal and the inner > one keyed by page index values? That seems clearer than lumping all the app > indices together Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:881: std::multimap<int,const std::string*> app_launches_to_convert; On 2011/12/01 02:57:47, akalin wrote: > space after 'int,' Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:893: if (page_ordinal_map_.size() == 0) On 2011/12/01 02:57:47, akalin wrote: > .empty() Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:917: std::pair<int,const std::string*>( On 2011/12/01 02:57:47, akalin wrote: > use std::make_pair Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1204: On 2011/12/01 02:57:47, akalin wrote: > remove extra space Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1206: StringOrdinal new_page_ordinal = GetNaturalAppPageOrdinal(); Ok, I think I've fixed this but I'm not 100% sure what you mean. I now replace page_ordinal if it is invalid, but that means it is no longer passed in as a const reference since it can change. Is this what you meant? On 2011/12/01 02:57:47, akalin wrote: > by 'inline this code' i meant inline and eliminate redundancy. The two clauses > differ only in the page ordinal. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1253: StringOrdinal predecessor_ordinal = On 2011/12/01 02:57:47, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1255: StringOrdinal successor_ordinal = On 2011/12/01 02:57:47, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1588: StringOrdinal app_launch_ordinal = GetAppLaunchOrdinal(*ext_it); On 2011/12/01 02:57:47, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1595: max_app_launch_value->LessThan(app_launch_ordinal)) This was the correct call, the operators are in the different order than the one above. To prevent this confusion though I'd changed it to call GreaterThan, I also added a test for this function. On 2011/12/01 02:57:47, akalin wrote: > surely you mean GreaterThan? Why didn't any tests catch this? Do you need to > write one for this function? http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // This determines the order of which the applications appear on the New Tab On 2011/12/01 02:57:47, akalin wrote: > update comment to reflect that the app ordinal is within a specified page Done. http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:336: // Create a StringOrdinal that will be the firsrt page. On 2011/12/01 02:57:47, akalin wrote: > firsrt -> first Done.
http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/89001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1206: StringOrdinal new_page_ordinal = GetNaturalAppPageOrdinal(); On 2011/12/01 20:15:09, csharp wrote: > Ok, I think I've fixed this but I'm not 100% sure what you mean. I now replace > page_ordinal if it is invalid, but that means it is no longer passed in as a > const reference since it can change. Is this what you meant? I meant something like: StringOrdinal new_page_ordinal = page_ordinal.IsValid() ? page_ordinal : GetNatural...; SetPageOrdinal(...); SetAppLaunchOrdinal(...);
More comments http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:886: std::map<StringOrdinal, std::map<int, const std::string*>, use typedefs (scoped to this function) to make this more readable http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:896: // to properly convert from integers and we are iteratoring though them in iteratoring -> iterating http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:907: page = PageIntegerAsStringOrdinal(old_page_index); you can do: page = *page_ordinal_map_.rbegin(); DCHECK(page.Equals(PageIntegerAsStringOrdinal(old_page_index))); http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:921: if (page.IsValid()) use braces for non-simple if statement http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:922: app_launches_to_convert[page].insert( you can just do app_launches_to_convert[page][old_app_launch_index] = &*ext_id; http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1581: void ExtensionPrefs::GetMinAndMaxAppLaunchOrdinalsOnPage( hmm looks like the callers of this function care about either min or max. How about renaming this to GetMinOrMax..., changing the two output parameters to an input enum, say { MIN_ORDINAL, MAX_ORDINAL }, and having it return the min or max? http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // This determines the order of which the applications appear on the page it First sentence should change extension -> app Rewrite second sentence as: This determines the order in which the app appears on the page it's on in the New Tab Page. (Note that you can compare app launch ordinals only if the apps are on the same page.) http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:324: // Sets a specific launch ordinal for an extension with |extension_id|. extension -> app http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:328: // Create a StringOrdinal which will be the first element for the given page. // Returns a StringOrdinal that is lower than any app launch ordinal for the given page. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:332: // Creates a ordinal value that is higher than the highest current // Returns a StringOrdinal that is higher than any app launch ordinal for the given page. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:337: // Create a StringOrdinal that will be the first page. // Returns a StringOrdinal that is lower than any existing page ordinal. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:338: StringOrdinal CreateFirstAppPage() const; maybe rename to CreateFirstAppPageOrdinal() to be more consistent http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:341: // there are N or more apps on it, tries to install on the next page. // Gets the page a new app should install to, which is the earliest non-full page. The returned ordinal may correspond to a page that doesn't yet exist if all pages are full. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:344: // Get the application page ordinal for an extension with |extension_id|. application page -> page, extension -> app http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:349: // Sets a specific page ordinal for an extension with |extension_id|. extension -> app http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:353: // Removes the page ordinal for an extension. extension -> app http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:589: std::map<StringOrdinal, int, StringOrdinalLessThan> page_ordinal_map_; actually, i think it's better to have a two-layer map, from page ordinals to some structure that allows lookups of page ordinals by id and vice versa. But that seems like a big change, so maybe just add a TODO for that http://codereview.chromium.org/8198003/diff/99001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:734: static_cast<int>(page_index)); shouldn't this be size_t?
http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:886: std::map<StringOrdinal, std::map<int, const std::string*>, On 2011/12/02 19:48:23, akalin wrote: > use typedefs (scoped to this function) to make this more readable Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:896: // to properly convert from integers and we are iteratoring though them in On 2011/12/02 19:48:23, akalin wrote: > iteratoring -> iterating Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:907: page = PageIntegerAsStringOrdinal(old_page_index); old_page_index isn't always referring to the last page so we can't do this. On 2011/12/02 19:48:23, akalin wrote: > you can do: > > page = *page_ordinal_map_.rbegin(); > DCHECK(page.Equals(PageIntegerAsStringOrdinal(old_page_index))); http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:921: if (page.IsValid()) On 2011/12/02 19:48:23, akalin wrote: > use braces for non-simple if statement Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:922: app_launches_to_convert[page].insert( On 2011/12/02 19:48:23, akalin wrote: > you can just do > > app_launches_to_convert[page][old_app_launch_index] = &*ext_id; Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.cc:1581: void ExtensionPrefs::GetMinAndMaxAppLaunchOrdinalsOnPage( On 2011/12/02 19:48:23, akalin wrote: > hmm looks like the callers of this function care about either min or max. How > about renaming this to GetMinOrMax..., changing the two output parameters to an > input enum, say { MIN_ORDINAL, MAX_ORDINAL }, and having it return the min or > max? Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:318: // This determines the order of which the applications appear on the page it On 2011/12/02 19:48:23, akalin wrote: > First sentence should change extension -> app > > Rewrite second sentence as: > > This determines the order in which the app appears on the page it's on in the > New Tab Page. (Note that you can compare app launch ordinals only if the apps > are on the same page.) Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:324: // Sets a specific launch ordinal for an extension with |extension_id|. On 2011/12/02 19:48:23, akalin wrote: > extension -> app Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:328: // Create a StringOrdinal which will be the first element for the given page. On 2011/12/02 19:48:23, akalin wrote: > // Returns a StringOrdinal that is lower than any app launch ordinal for the > given page. Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:332: // Creates a ordinal value that is higher than the highest current On 2011/12/02 19:48:23, akalin wrote: > // Returns a StringOrdinal that is higher than any app launch ordinal for the > given page. Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:337: // Create a StringOrdinal that will be the first page. On 2011/12/02 19:48:23, akalin wrote: > // Returns a StringOrdinal that is lower than any existing page ordinal. Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:338: StringOrdinal CreateFirstAppPage() const; On 2011/12/02 19:48:23, akalin wrote: > maybe rename to CreateFirstAppPageOrdinal() to be more consistent Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:341: // there are N or more apps on it, tries to install on the next page. On 2011/12/02 19:48:23, akalin wrote: > // Gets the page a new app should install to, which is the earliest non-full > page. The returned ordinal may correspond to a page that doesn't yet exist if > all pages are full. Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:344: // Get the application page ordinal for an extension with |extension_id|. On 2011/12/02 19:48:23, akalin wrote: > application page -> page, extension -> app Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:349: // Sets a specific page ordinal for an extension with |extension_id|. On 2011/12/02 19:48:23, akalin wrote: > extension -> app Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:353: // Removes the page ordinal for an extension. On 2011/12/02 19:48:23, akalin wrote: > extension -> app Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/extensions/e... chrome/browser/extensions/extension_prefs.h:589: std::map<StringOrdinal, int, StringOrdinalLessThan> page_ordinal_map_; On 2011/12/02 19:48:23, akalin wrote: > actually, i think it's better to have a two-layer map, from page ordinals to > some structure that allows lookups of page ordinals by id and vice versa. But > that seems like a big change, so maybe just add a TODO for that Done. http://codereview.chromium.org/8198003/diff/99001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/99001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:734: static_cast<int>(page_index)); On 2011/12/02 19:48:23, akalin wrote: > shouldn't this be size_t? Done.
LGTM http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:933: for (std::map<StringOrdinal, int, StringOrdinalLessThan>::iterator it = use typedef here? http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1627: StringOrdinal min_ordinal = const ref http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1639: StringOrdinal max_ordinal = const ref http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:596: std::map<StringOrdinal, int, StringOrdinalLessThan> page_ordinal_map_; use a typedef to save some typing? http://codereview.chromium.org/8198003/diff/104001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:669: // We don't worry about checking if the gets work because if they fail Hmm, prefer explicit checking here. GetString may return false if the argument isn't a string, and we'd want to catch that, since passing down an empty string for pred/succ would be an error. Another theoretical case would be if GetSize() == (size_t)-1. Then GetString(i - 1, ...) would return a valid value. So something like: if (i > 0) { CHECK(app_order->GetString(i - 1, ...); } if (i + 1 < app_order->GetSize()) { CHECK(app_order->GetString(i + 1, ...); }
On 2011/12/06 17:48:24, akalin wrote: > LGTM Thanks for your patience! I think the code is much clearer now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/104001
Presubmit check for 8198003-104001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/resources/ntp4/page_list_view.js Presubmit checks took 3.8s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/104001
Presubmit check for 8198003-104001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/resources/ntp4/page_list_view.js Presubmit checks took 3.6s to calculate.
estade@, could you have another look at this review when you get a chance. I need a LGTM from you, thanks http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:933: for (std::map<StringOrdinal, int, StringOrdinalLessThan>::iterator it = On 2011/12/06 17:48:24, akalin wrote: > use typedef here? Done. http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1627: StringOrdinal min_ordinal = On 2011/12/06 17:48:24, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1639: StringOrdinal max_ordinal = On 2011/12/06 17:48:24, akalin wrote: > const ref Done. http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:596: std::map<StringOrdinal, int, StringOrdinalLessThan> page_ordinal_map_; On 2011/12/06 17:48:24, akalin wrote: > use a typedef to save some typing? Done. http://codereview.chromium.org/8198003/diff/104001/chrome/browser/ui/webui/nt... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/104001/chrome/browser/ui/webui/nt... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:669: // We don't worry about checking if the gets work because if they fail On 2011/12/06 17:48:24, akalin wrote: > Hmm, prefer explicit checking here. GetString may return false if the argument > isn't a string, and we'd want to catch that, since passing down an empty string > for pred/succ would be an error. > > Another theoretical case would be if GetSize() == (size_t)-1. Then GetString(i > - 1, ...) would return a valid value. > > So something like: > > if (i > 0) { > CHECK(app_order->GetString(i - 1, ...); > } > if (i + 1 < app_order->GetSize()) { > CHECK(app_order->GetString(i + 1, ...); > } Done.
if you responded to my last question, I can't find it. Can you repeat whatever you wrote?
Sorry for missing your question before, I didn't have a good answer and I wasn't sure if it still needed an answer. http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/app_launcher_handler.cc:435: dictionary->Set("appPageNames", list); I'm not exactly sure where the json and c++ code meet, so I'm not sure what the correct answer is (I had luck figuring it out when I looked into it). I don't think that they can be read in, but I could be wrong. In our code that isn't a problem (at least for the appPageNames) because we are never returning a whole list from json, it only passes back the name changes one at a time, so this issue don't occur. Sorry I don't have a better answer. On 2011/11/23 21:54:51, Evan Stade wrote: > On 2011/11/23 15:38:07, csharp wrote: > > This seems to be the best way I could find to convert the keys of the > dictionary > > to a list, but please point me to a better method if one exists. > > this seems fine, my original comment was concerned with whether changing from a > list to a dictionary pref type would work correctly (can list json be read into > a dictionary? what would the keys be?)
my question is this: If I have a profile where I have named all my apps pages, and then this patch lands and I start running the new code, will my app pages still have the names I gave them? since you are changing the type of the pref, I can't see how that data will be migrated. On 2011/12/07 15:00:59, csharp wrote: > Sorry for missing your question before, I didn't have a good answer and I wasn't > sure if it still needed an answer. > > http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... > File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): > > http://codereview.chromium.org/8198003/diff/64001/chrome/browser/ui/webui/ntp... > chrome/browser/ui/webui/ntp/app_launcher_handler.cc:435: > dictionary->Set("appPageNames", list); > I'm not exactly sure where the json and c++ code meet, so I'm not sure what the > correct answer is (I had luck figuring it out when I looked into it). I don't > think that they can be read in, but I could be wrong. In our code that isn't a > problem (at least for the appPageNames) because we are never returning a whole > list from json, it only passes back the name changes one at a time, so this > issue don't occur. > > Sorry I don't have a better answer. > > On 2011/11/23 21:54:51, Evan Stade wrote: > > On 2011/11/23 15:38:07, csharp wrote: > > > This seems to be the best way I could find to convert the keys of the > > dictionary > > > to a list, but please point me to a better method if one exists. > > > > this seems fine, my original comment was concerned with whether changing from > a > > list to a dictionary pref type would work correctly (can list json be read > into > > a dictionary? what would the keys be?)
The page names will not be changed. The code doesn't change the pref type anymore (I remove that change in patch 13). I hadn't completely realized at first that the page names prefs where only accessed by the old index values, and thus didn't need the change the store preference type.
but I definitely do think you want to sync this (fix that csilv TODO).
You are correct, I do plan add the syncing code for the page names. I've changed csilv's todo to be mine now. If there are any other issues you have with this code, please just ping me as I'd like to get it in fairly soon so I can move onto syncing the names and positions.
On 2011/12/12 16:57:19, csharp wrote: > You are correct, I do plan add the syncing code for the page names. I've changed > csilv's todo to be mine now. > If there are any other issues you have with this code, please just ping me as > I'd like to get it in fairly soon so I can move onto syncing the names and > positions. can you disable app position syncing until the page name syncing is landed?
lgtm -- Evan Stade On Mon, Dec 12, 2011 at 11:40 AM, <estade@chromium.org> wrote: > On 2011/12/12 16:57:19, csharp wrote: > >> You are correct, I do plan add the syncing code for the page names. I've >> > changed > >> csilv's todo to be mine now. >> If there are any other issues you have with this code, please just ping >> me as >> I'd like to get it in fairly soon so I can move onto syncing the names and >> positions. >> > > can you disable app position syncing until the page name syncing is landed? > > http://codereview.chromium.**org/8198003/<http://codereview.chromium.org/8198... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/119001
clang fix lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/8198003/123001
Change committed as 114083 |
