[MD Extensions] Fix item sorting

[MD Extensions] Fix item sorting

Items are sorted (unpacked first, then alphabetical by name, then alphabetical
by id). Move this sorting to the manager, since there are now sections (so
sorting them in the service doesn't make sense).

BUG=529395
Committed: https://crrev.com/3d6968a0264075f5c079f3de4c4e486507761f8f
Cr-Commit-Position: refs/heads/master@{#360477}

[Devlin, 2015-11-18 00:38:56]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Devlin, 2015-11-18 00:38:57]

Michael's up!

Dan FYI.

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/manager.js:9: * Compares two extensions
to determine which should come first in the list.
note: copy-paste from service.js

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/service.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:24: this.sidebar_ = /** @type
{extensions.Sidebar} */(manager.sidebar);
nasty cast needed because sidebar is a declared property on manager.

[michaelpg, 2015-11-18 09:08:46]

Not thrilled about making the sort O(n^2) just because sorting in the service
"doesn't make sense". The old addItem was pretty dumb and didn't care about
order; could you still sort items in the service using name & id, and just let
addItem append each item one by one to the proper list? Then the service can
continue not caring about sections.

(It's not a perfect suggestion since you still have to handle proper insertion
of new extensions after page load...)

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/manager.js:36: sidebar: {
/** @type {extensions.Sidebar} */

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/manager.js:49: * Creates and adds a new
extensions-item element to the list.
nit: ", inserting it into its sorted position in the relevant section" or
something.

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/service.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:24: this.sidebar_ = /** @type
{extensions.Sidebar} */(manager.sidebar);
On 2015/11/18 00:38:57, Devlin wrote:
> nasty cast needed because sidebar is a declared property on manager.

polymer_pass would normally pick these up if you @type the property.

unless the PolymerPass isn't working because the Manager lives inside a
cr.define?

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:36:
this.manager_.addItem(extension, this);
so... this is essentially bubble sort now, right?

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/cr_extensions_browsertest.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:60: function
CrExtensionsBrowserTestWithInstalledExtension() {}
organization

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:88:
CrExtensionsBrowserTestWithInstalledExtension.prototype = {
your

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:140:
TEST_F('CrExtensionsBrowserTest', 'ExtensionManagerItemOrderTest', function() {
confusing is

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/extension_manager_test.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_manager_test.js:99: {location:
'FROM_STORE', name: 'AA', id: 'd'.repeat(32)});
would you mind also adding items like
  {location: 'FROM_STORE', name: 'Aa', id: 'f'.repeat(32)}
and
  {location: 'FROM_STORE', name: 'aA', id: 'e'.repeat(32)}

to verify the sort is case-insensitive with respect to name? (not asking for
separate expectEquals blocks for them, just add two more expectEqualses at 105)

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_test_util.js:69: views: [{url:
baseUrl + 'foo.html'}, {url: baseUrl + 'bar.html'}],
not to nitpick, but if the caller passes {url: foo}, technically data.views will
be wrong...

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_test_util.js:71: if (opt_properties)
{
Object.assign(data, opt_properties);  // even works (as a no-op) if
opt_properties is undefined

or:

return Object.assign({
  description: 'This is an extension', .....
}, opt_properties);

[Devlin, 2015-11-18 17:17:00]

On 2015/11/18 09:08:46, michaelpg wrote:
> Not thrilled about making the sort O(n^2) just because sorting in the service
> "doesn't make sense". The old addItem was pretty dumb and didn't care about
> order; could you still sort items in the service using name & id, and just let
> addItem append each item one by one to the proper list? Then the service can
> continue not caring about sections.
> 
> (It's not a perfect suggestion since you still have to handle proper insertion
> of new extensions after page load...)

Somewhat addressed in another comment, but I don't think we should ever have
enough data that it really matters (the overwhelming majority of users don't
have more than 10 extensions), so I think it's worth the code simplicity (and I
feel pretty strongly that Service shouldn't care about the different sections). 
And, as you said, we'd still have to address insertion of extensions.  It's
perfectly doable - but is it worth the spaghetti-code?

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/manager.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/manager.js:36: sidebar: {
On 2015/11/18 09:08:45, michaelpg wrote:
> /** @type {extensions.Sidebar} */

Ah, didn't realize that worked.  Done.

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/manager.js:49: * Creates and adds a new
extensions-item element to the list.
On 2015/11/18 09:08:45, michaelpg wrote:
> nit: ", inserting it into its sorted position in the relevant section" or
> something.

Done.

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/service.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:24: this.sidebar_ = /** @type
{extensions.Sidebar} */(manager.sidebar);
On 2015/11/18 09:08:46, michaelpg wrote:
> On 2015/11/18 00:38:57, Devlin wrote:
> > nasty cast needed because sidebar is a declared property on manager.
> 
> polymer_pass would normally pick these up if you @type the property.
> 
> unless the PolymerPass isn't working because the Manager lives inside a
> cr.define?

Fixed with the @type in manager.js.

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:36:
this.manager_.addItem(extension, this);
On 2015/11/18 09:08:46, michaelpg wrote:
> so... this is essentially bubble sort now, right?

Hmm... I think it's actually insertion sort, right?  Which is still O(n^2), but
it's worth noting that the data set here is small (i.e., anything over 50 and
the user probably has other problems), and on reasonably small data sets,
insertion sort's actually decently fast.

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/cr_extensions_browsertest.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:140:
TEST_F('CrExtensionsBrowserTest', 'ExtensionManagerItemOrderTest', function() {
On 2015/11/18 09:08:46, michaelpg wrote:
> confusing is

Only if you look at the test class before what it's testing. :)  In this case,
the test class is actually an implementation detail - whether or not we want to
start with real data/items added to the list.  For this particular test, we
don't.  (We could probably restructure the other Manager test to also not need
it, but I'm not sure that's necessary right now.)  In any other type of test,
this would be accomplished by just calling the appropriate AddFooExtension()
from within the test body, but for these, those calls are made in C++, and I
think that's only doable from the testGenPreamble (correct me if I'm wrong).

But I see your point.  I've reorganized this a bit to just put all the test
classes (implementation details, in my mind) at the top, and sort more clearly
by what's being tested (item, sidebar, service, manager).  Lemme know if you
think that's okay, or if you'd prefer something else.

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/extension_manager_test.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_manager_test.js:99: {location:
'FROM_STORE', name: 'AA', id: 'd'.repeat(32)});
On 2015/11/18 09:08:46, michaelpg wrote:
> would you mind also adding items like
>   {location: 'FROM_STORE', name: 'Aa', id: 'f'.repeat(32)}
> and
>   {location: 'FROM_STORE', name: 'aA', id: 'e'.repeat(32)}
> 
> to verify the sort is case-insensitive with respect to name? (not asking for
> separate expectEquals blocks for them, just add two more expectEqualses at
105)

Done.

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_test_util.js:69: views: [{url:
baseUrl + 'foo.html'}, {url: baseUrl + 'bar.html'}],
On 2015/11/18 09:08:46, michaelpg wrote:
> not to nitpick, but if the caller passes {url: foo}, technically data.views
will
> be wrong...

It shouldn't be - views is the key on the object, and there is no url key.  So
if opt_properties contains 'url', won't it just assign an url property on the
object, but not affect the url properties of the views objects?

(Whether or not we want to worry about people passing in garbage data to
opt_properties is another question - we could limit [or assert] the copied
properties to the ones present on the base ExtensionInfo, but it seems a bit
extreme right now.  Lemme know if you feel differently.)

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_test_util.js:71: if (opt_properties)
{
On 2015/11/18 09:08:46, michaelpg wrote:
> Object.assign(data, opt_properties);  // even works (as a no-op) if
> opt_properties is undefined
> 
> or:
> 
> return Object.assign({
>   description: 'This is an extension', .....
> }, opt_properties);

Rad.  Done.

[michaelpg, 2015-11-18 20:01:17]

lgtm

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/service.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:36:
this.manager_.addItem(extension, this);
On 2015/11/18 17:16:59, Devlin wrote:
> On 2015/11/18 09:08:46, michaelpg wrote:
> > so... this is essentially bubble sort now, right?
> 
> Hmm... I think it's actually insertion sort, right?  Which is still O(n^2),
but
> it's worth noting that the data set here is small (i.e., anything over 50 and
> the user probably has other problems), and on reasonably small data sets,
> insertion sort's actually decently fast.

ok, insertion sort, but still not a native sort() or even a native compare
function :-P

as a user with >50 extensions, what other problems do I have? Having a bunch of
apps on CrOS is not abnormal.

all that being said, with some quick and dirty empirical testing it looks like
this would add ~20ms for 200 extensions. sooo, not worth optimizing for.

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/cr_extensions_browsertest.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/cr_extensions_browsertest.js:140:
TEST_F('CrExtensionsBrowserTest', 'ExtensionManagerItemOrderTest', function() {
On 2015/11/18 17:16:59, Devlin wrote:
> On 2015/11/18 09:08:46, michaelpg wrote:
> > confusing is
> 
> Only if you look at the test class before what it's testing. :)  In this case,
> the test class is actually an implementation detail - whether or not we want
to
> start with real data/items added to the list.  For this particular test, we
> don't.  (We could probably restructure the other Manager test to also not need
> it, but I'm not sure that's necessary right now.)  In any other type of test,
> this would be accomplished by just calling the appropriate AddFooExtension()
> from within the test body, but for these, those calls are made in C++, and I
> think that's only doable from the testGenPreamble (correct me if I'm wrong).
> 
> But I see your point.  I've reorganized this a bit to just put all the test
> classes (implementation details, in my mind) at the top, and sort more clearly
> by what's being tested (item, sidebar, service, manager).  Lemme know if you
> think that's okay, or if you'd prefer something else.

Thanks.

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
File chrome/test/data/webui/extensions/extension_test_util.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/test/data/webui/exte...
chrome/test/data/webui/extensions/extension_test_util.js:69: views: [{url:
baseUrl + 'foo.html'}, {url: baseUrl + 'bar.html'}],
On 2015/11/18 17:16:59, Devlin wrote:
> On 2015/11/18 09:08:46, michaelpg wrote:
> > not to nitpick, but if the caller passes {url: foo}, technically data.views
> will
> > be wrong...
> 
> It shouldn't be - views is the key on the object, and there is no url key.  So
> if opt_properties contains 'url', won't it just assign an url property on the
> object, but not affect the url properties of the views objects?
> 
> (Whether or not we want to worry about people passing in garbage data to
> opt_properties is another question - we could limit [or assert] the copied
> properties to the ones present on the base ExtensionInfo, but it seems a bit
> extreme right now.  Lemme know if you feel differently.)

Oh, baseUrl is generated from the id -- that was what I missed.

[Devlin, 2015-11-18 23:23:10]

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_extensions/service.js (right):

https://codereview.chromium.org/1447183005/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_extensions/service.js:36:
this.manager_.addItem(extension, this);
On 2015/11/18 20:01:17, michaelpg wrote:
> as a user with >50 extensions, what other problems do I have? Having a bunch
of
> apps on CrOS is not abnormal.

Was mostly referring to extensions, since apps (globally) are pretty uncommon -
and with >50 extensions, your browsing experience is almost certainly suffering.
 >50 apps isn't as bad, but is also pretty uncommon.  (And, like you said, the
~20ms probably isn't worth the work. :))

[Devlin, 2015-11-18 23:23:14]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-11-18 23:25:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1447183005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1447183005/20001

[commit-bot: I haz the power, 2015-11-19 01:53:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-19 01:55:15]

Patchset 2 (id:??) landed as
https://crrev.com/3d6968a0264075f5c079f3de4c4e486507761f8f
Cr-Commit-Position: refs/heads/master@{#360477}