Order the script badges in the location bar consistently

Order the script badges in the location bar in the order that
they appeared, and fix the logic on each platform which 
assumes they never change.

BUG=133139
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142855

[not at google - send to devlin, 2012-06-15 23:02:48]

Epic code reviewer list.

aa: extensions
estade: gtk
sky: views
thakis: cocoa
willchan/jeffrey: scoped_ptr.h

[willchan no longer on Chromium, 2012-06-15 23:15:01]

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
File base/memory/scoped_ptr.h (right):

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
Can't we change this to take T*?

[Jeffrey Yasskin, 2012-06-15 23:17:38]

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
This could use a comment like

// Use this as:
//  make_scoped_ptr<MyClass>(constructor_arg1, constructor_arg2)
// For simplicity, we only forward 0-3 arguments, as const references. To
// construct with non-const references, create the scoped_ptr manually.


The interface matches make_shared<T>(...) from C++11 and the make_unique<T>(...)
that's been proposed for C++17, so I'm happy with it.

[Evan Stade, 2012-06-16 00:42:46]

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:703: if (new_page_actions !=
page_actions_) {
I've never actually tried to use comparison operators on std::vectors... does
this do what you expect?

[Jeffrey Yasskin, 2012-06-16 00:53:25]

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:703: if (new_page_actions !=
page_actions_) {
On 2012/06/16 00:42:46, Evan Stade wrote:
> I've never actually tried to use comparison operators on std::vectors... does
> this do what you expect?

Yes: http://en.cppreference.com/w/cpp/container/vector/operator_cmp (and
[lib.containers]p5 in the standard)

[Nico, 2012-06-16 04:58:03]

Please either add a BUG= line or add a TEST= line that describes how to
experience this fix :-)

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:604:
scoped_ptr<std::vector<ExtensionAction*> > new_page_actions =
This looks like a function called GetCurrentActions() returns something that's
supposed to be owned by the caller. Functions called GetBlah() shouldn't do
that, rename the function to NewBlah() or CreateBlah()

[Aaron Boodman, 2012-06-16 05:42:36]

Added a BUG line, but just to the generic feature.

[Nico, 2012-06-16 05:53:05]

Thanks, that's good enough for me.

[Aaron Boodman, 2012-06-17 14:44:31]

extensions lgtm

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/extensions/s...
File chrome/browser/extensions/script_badge_controller.cc (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.cc:37: return
make_scoped_ptr<std::vector<ExtensionAction*> >(current_actions_);
Nice utility. Sorry for assuming it was jyasskin's ideas.

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/extensions/s...
chrome/browser/extensions/script_badge_controller.cc:185: // Also note that this
means that when extensions are uninstalled their
Yeah, tricky. Both keeping it alive and killing it have problems. This seems
fine.

[sky, 2012-06-17 22:29:48]

views LGTM

[willchan no longer on Chromium, 2012-06-18 03:16:53]

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
File base/memory/scoped_ptr.h (right):

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
On 2012/06/15 23:17:38, Jeffrey Yasskin wrote:
> This could use a comment like
> 
> // Use this as:
> //  make_scoped_ptr<MyClass>(constructor_arg1, constructor_arg2)
> // For simplicity, we only forward 0-3 arguments, as const references. To
> // construct with non-const references, create the scoped_ptr manually.
> 
> 
> The interface matches make_shared<T>(...) from C++11 and the
make_unique<T>(...)
> that's been proposed for C++17, so I'm happy with it.

My initial inclination was to reject this, because this is not consistent with
make_linked_ptr() and make_scoped_refptr(). cs.chromium.org indicates that there
are on the order of 130~ instances of those two patterns in our code base.
make_linked_ptr() should not increase much more, but I suspect that
make_scoped_refptr() will continue to increase, primarily due to needing to do
so for passing function parameters to base::Bind().

In between beers, I've spent some background cycles pondering this. What's going
to be more common long-term? I see us likely switching to unique_ptr in the next
few years, at which point we could easily write our own make_unique<T> to match
the proposal for C++17. But I don't see us ever switching away from our uses of
make_scoped_refptr(), and I suspect they will be more commonly used than
make_scoped_ptr/make_unique.

There's another question...how much does consistency matter here? I guess my
inclination is that consistency with C++17 doesn't matter much, since that's so
far out, and it's not like many people know the standard. On the other hand,
most Chromium developers will expect make_*() to be consistent, so there are
real (albeit small, but sizable in aggregate) advantages to having these
functions be consistent in the Chromium codebase.

I don't really think this is worth discussing further, so I'm going to weakly
insist on being consistent with make_scoped_refptr() for now, unless others
strongly disagree here. Please chime in if you do!

[Jeffrey Yasskin, 2012-06-18 05:42:15]

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
File base/memory/scoped_ptr.h (right):

https://chromiumcodereview.appspot.com/10544185/diff/1013/base/memory/scoped_...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
On 2012/06/18 03:16:53, willchan wrote:
> On 2012/06/15 23:17:38, Jeffrey Yasskin wrote:
> > This could use a comment like
> > 
> > // Use this as:
> > //  make_scoped_ptr<MyClass>(constructor_arg1, constructor_arg2)
> > // For simplicity, we only forward 0-3 arguments, as const references. To
> > // construct with non-const references, create the scoped_ptr manually.
> > 
> > 
> > The interface matches make_shared<T>(...) from C++11 and the
> make_unique<T>(...)
> > that's been proposed for C++17, so I'm happy with it.
> 
> My initial inclination was to reject this, because this is not consistent with
> make_linked_ptr() and make_scoped_refptr(). http://cs.chromium.org indicates
that there
> are on the order of 130~ instances of those two patterns in our code base.
> make_linked_ptr() should not increase much more, but I suspect that
> make_scoped_refptr() will continue to increase, primarily due to needing to do
> so for passing function parameters to base::Bind().
> 
> In between beers, I've spent some background cycles pondering this. What's
going
> to be more common long-term? I see us likely switching to unique_ptr in the
next
> few years, at which point we could easily write our own make_unique<T> to
match
> the proposal for C++17. But I don't see us ever switching away from our uses
of
> make_scoped_refptr(), and I suspect they will be more commonly used than
> make_scoped_ptr/make_unique.
> 
> There's another question...how much does consistency matter here? I guess my
> inclination is that consistency with C++17 doesn't matter much, since that's
so
> far out, and it's not like many people know the standard. On the other hand,
> most Chromium developers will expect make_*() to be consistent, so there are
> real (albeit small, but sizable in aggregate) advantages to having these
> functions be consistent in the Chromium codebase.
> 
> I don't really think this is worth discussing further, so I'm going to weakly
> insist on being consistent with make_scoped_refptr() for now, unless others
> strongly disagree here. Please chime in if you do!

Interestingly, make_scoped_refptr doesn't take ownership of any objects, unlike
the other make_* functions (it AddRef's, leaving a share of ownership with the
caller). For make_linked_ptr and the version of make_scoped_ptr that would take
a T*, passing the same pointer to two different calls would result in a
double-free. For make_scoped_refptr, it's perfectly fine, which makes it
semantically unreasonable to convert make_scoped_refptr to the construction form
that we see here.

To some extent I dislike the semantic inconsistency between make_shared_refptr
and the other make_* functions, but given that they're already
inconsistent-in-ownership but consistent-in-argument-types, I have no objection
to having make_scoped_ptr take a T*.

[not at google - send to devlin, 2012-06-18 17:50:26]

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
On 2012/06/18 05:42:15, Jeffrey Yasskin wrote:
> On 2012/06/18 03:16:53, willchan wrote:
> > On 2012/06/15 23:17:38, Jeffrey Yasskin wrote:
> > > This could use a comment like
> > > 
> > > // Use this as:
> > > //  make_scoped_ptr<MyClass>(constructor_arg1, constructor_arg2)
> > > // For simplicity, we only forward 0-3 arguments, as const references. To
> > > // construct with non-const references, create the scoped_ptr manually.
> > > 
> > > 
> > > The interface matches make_shared<T>(...) from C++11 and the
> > make_unique<T>(...)
> > > that's been proposed for C++17, so I'm happy with it.
> > 
> > My initial inclination was to reject this, because this is not consistent
with
> > make_linked_ptr() and make_scoped_refptr(). http://cs.chromium.org indicates
> that there
> > are on the order of 130~ instances of those two patterns in our code base.
> > make_linked_ptr() should not increase much more, but I suspect that
> > make_scoped_refptr() will continue to increase, primarily due to needing to
do
> > so for passing function parameters to base::Bind().
> > 
> > In between beers, I've spent some background cycles pondering this. What's
> going
> > to be more common long-term? I see us likely switching to unique_ptr in the
> next
> > few years, at which point we could easily write our own make_unique<T> to
> match
> > the proposal for C++17. But I don't see us ever switching away from our uses
> of
> > make_scoped_refptr(), and I suspect they will be more commonly used than
> > make_scoped_ptr/make_unique.
> > 
> > There's another question...how much does consistency matter here? I guess my
> > inclination is that consistency with C++17 doesn't matter much, since that's
> so
> > far out, and it's not like many people know the standard. On the other hand,
> > most Chromium developers will expect make_*() to be consistent, so there are
> > real (albeit small, but sizable in aggregate) advantages to having these
> > functions be consistent in the Chromium codebase.
> > 
> > I don't really think this is worth discussing further, so I'm going to
weakly
> > insist on being consistent with make_scoped_refptr() for now, unless others
> > strongly disagree here. Please chime in if you do!
> 
> Interestingly, make_scoped_refptr doesn't take ownership of any objects,
unlike
> the other make_* functions (it AddRef's, leaving a share of ownership with the
> caller). For make_linked_ptr and the version of make_scoped_ptr that would
take
> a T*, passing the same pointer to two different calls would result in a
> double-free. For make_scoped_refptr, it's perfectly fine, which makes it
> semantically unreasonable to convert make_scoped_refptr to the construction
form
> that we see here.
> 
> To some extent I dislike the semantic inconsistency between make_shared_refptr
> and the other make_* functions, but given that they're already
> inconsistent-in-ownership but consistent-in-argument-types, I have no
objection
> to having make_scoped_ptr take a T*.

I think that... consistency with make_linked_ptr wins this one. I'll copy the
comment from there too.

Btw Will, why will make_linked_ptr go away?

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:604:
scoped_ptr<std::vector<ExtensionAction*> > new_page_actions =
On 2012/06/16 04:58:03, Nico wrote:
> This looks like a function called GetCurrentActions() returns something that's
> supposed to be owned by the caller. Functions called GetBlah() shouldn't do
> that, rename the function to NewBlah() or CreateBlah()

This is equivalent to a function like

void GetPageActions(std::vector<ExtensionAction*>* out) {
  while (something)
    out->push_back(action);
}

except instead of implicitly creating the return value (via appending to out) it
explicitly creates it. It's still a "getter" in that it's accessing internal
state (the ExtensionActions) not creating anything new besides the container for
the ExtensionActions. Calling this Create gives the impression to me that the
ExtensionActions are also being created.

I can change it to return just a plain std::vector<> but I was trying to avoid
extra unnecessarily copying originally (when it built up the vector on-demand;
PageActionsController still does this).

Could also change it to return a const* or const& but this is an implementation
of an interface, and that would be enforcing a stricter implementation detail on
this class and PageActionsController.

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:703: if (new_page_actions !=
page_actions_) {
On 2012/06/16 00:53:25, Jeffrey Yasskin wrote:
> On 2012/06/16 00:42:46, Evan Stade wrote:
> > I've never actually tried to use comparison operators on std::vectors...
does
> > this do what you expect?
> 
> Yes: http://en.cppreference.com/w/cpp/container/vector/operator_cmp (and
> [lib.containers]p5 in the standard)

Yep I assumed it did.

Thanks Jeffrey for finding the actual spec for it.

[Nico, 2012-06-18 17:54:23]

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:604:
scoped_ptr<std::vector<ExtensionAction*> > new_page_actions =
On 2012/06/18 17:50:27, kalman wrote:
> On 2012/06/16 04:58:03, Nico wrote:
> > This looks like a function called GetCurrentActions() returns something
that's
> > supposed to be owned by the caller. Functions called GetBlah() shouldn't do
> > that, rename the function to NewBlah() or CreateBlah()
> 
> This is equivalent to a function like
> 
> void GetPageActions(std::vector<ExtensionAction*>* out) {
>   while (something)
>     out->push_back(action);
> }
> 
> except instead of implicitly creating the return value (via appending to out)
it
> explicitly creates it. It's still a "getter" in that it's accessing internal
> state (the ExtensionActions) not creating anything new besides the container
for
> the ExtensionActions. Calling this Create gives the impression to me that the
> ExtensionActions are also being created.
> 
> I can change it to return just a plain std::vector<> but I was trying to avoid
> extra unnecessarily copying originally (when it built up the vector on-demand;
> PageActionsController still does this).
> 
> Could also change it to return a const* or const& but this is an
implementation
> of an interface, and that would be enforcing a stricter implementation detail
on
> this class and PageActionsController.

Right, it's just a vector of pointers, so just return it by value.

[not at google - send to devlin, 2012-06-18 18:05:59]

So I guess make_scoped_ptr isn't used anymore.

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10544185/diff/1013/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:604:
scoped_ptr<std::vector<ExtensionAction*> > new_page_actions =
On 2012/06/18 17:54:23, Nico wrote:
> On 2012/06/18 17:50:27, kalman wrote:
> > On 2012/06/16 04:58:03, Nico wrote:
> > > This looks like a function called GetCurrentActions() returns something
> that's
> > > supposed to be owned by the caller. Functions called GetBlah() shouldn't
do
> > > that, rename the function to NewBlah() or CreateBlah()
> > 
> > This is equivalent to a function like
> > 
> > void GetPageActions(std::vector<ExtensionAction*>* out) {
> >   while (something)
> >     out->push_back(action);
> > }
> > 
> > except instead of implicitly creating the return value (via appending to
out)
> it
> > explicitly creates it. It's still a "getter" in that it's accessing internal
> > state (the ExtensionActions) not creating anything new besides the container
> for
> > the ExtensionActions. Calling this Create gives the impression to me that
the
> > ExtensionActions are also being created.
> > 
> > I can change it to return just a plain std::vector<> but I was trying to
avoid
> > extra unnecessarily copying originally (when it built up the vector
on-demand;
> > PageActionsController still does this).
> > 
> > Could also change it to return a const* or const& but this is an
> implementation
> > of an interface, and that would be enforcing a stricter implementation
detail
> on
> > this class and PageActionsController.
> 
> Right, it's just a vector of pointers, so just return it by value.

Done.

[Nico, 2012-06-18 18:44:44]

cocoa lgtm

[willchan no longer on Chromium, 2012-06-18 18:52:32]

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/10544185/diff/1013/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:497: scoped_ptr<T> make_scoped_ptr() {
On 2012/06/18 17:50:27, kalman wrote:
> On 2012/06/18 05:42:15, Jeffrey Yasskin wrote:
> > On 2012/06/18 03:16:53, willchan wrote:
> > > On 2012/06/15 23:17:38, Jeffrey Yasskin wrote:
> > > > This could use a comment like
> > > > 
> > > > // Use this as:
> > > > //  make_scoped_ptr<MyClass>(constructor_arg1, constructor_arg2)
> > > > // For simplicity, we only forward 0-3 arguments, as const references.
To
> > > > // construct with non-const references, create the scoped_ptr manually.
> > > > 
> > > > 
> > > > The interface matches make_shared<T>(...) from C++11 and the
> > > make_unique<T>(...)
> > > > that's been proposed for C++17, so I'm happy with it.
> > > 
> > > My initial inclination was to reject this, because this is not consistent
> with
> > > make_linked_ptr() and make_scoped_refptr(). http://cs.chromium.org
indicates
> > that there
> > > are on the order of 130~ instances of those two patterns in our code base.
> > > make_linked_ptr() should not increase much more, but I suspect that
> > > make_scoped_refptr() will continue to increase, primarily due to needing
to
> do
> > > so for passing function parameters to base::Bind().
> > > 
> > > In between beers, I've spent some background cycles pondering this. What's
> > going
> > > to be more common long-term? I see us likely switching to unique_ptr in
the
> > next
> > > few years, at which point we could easily write our own make_unique<T> to
> > match
> > > the proposal for C++17. But I don't see us ever switching away from our
uses
> > of
> > > make_scoped_refptr(), and I suspect they will be more commonly used than
> > > make_scoped_ptr/make_unique.
> > > 
> > > There's another question...how much does consistency matter here? I guess
my
> > > inclination is that consistency with C++17 doesn't matter much, since
that's
> > so
> > > far out, and it's not like many people know the standard. On the other
hand,
> > > most Chromium developers will expect make_*() to be consistent, so there
are
> > > real (albeit small, but sizable in aggregate) advantages to having these
> > > functions be consistent in the Chromium codebase.
> > > 
> > > I don't really think this is worth discussing further, so I'm going to
> weakly
> > > insist on being consistent with make_scoped_refptr() for now, unless
others
> > > strongly disagree here. Please chime in if you do!
> > 
> > Interestingly, make_scoped_refptr doesn't take ownership of any objects,
> unlike
> > the other make_* functions (it AddRef's, leaving a share of ownership with
the
> > caller). For make_linked_ptr and the version of make_scoped_ptr that would
> take
> > a T*, passing the same pointer to two different calls would result in a
> > double-free. For make_scoped_refptr, it's perfectly fine, which makes it
> > semantically unreasonable to convert make_scoped_refptr to the construction
> form
> > that we see here.
> > 
> > To some extent I dislike the semantic inconsistency between
make_shared_refptr
> > and the other make_* functions, but given that they're already
> > inconsistent-in-ownership but consistent-in-argument-types, I have no
> objection
> > to having make_scoped_ptr take a T*.
> 
> I think that... consistency with make_linked_ptr wins this one. I'll copy the
> comment from there too.
> 
> Btw Will, why will make_linked_ptr go away?

I don't think it'll go away (did I say that? if so, I was wrong, sorry).
linked_ptr<T> is just not used much in general in our code, since it's not
threadsafe and also people are much more used to using RefCounted{ThreadSafe}. I
think its rate of new use is very low.

[Evan Stade, 2012-06-18 19:03:14]

gtk lgtm

[not at google - send to devlin, 2012-06-18 19:03:37]

> I don't think it'll go away (did I say that? if so, I was wrong, sorry).
> linked_ptr<T> is just not used much in general in our code, since it's not
> threadsafe and also people are much more used to using RefCounted{ThreadSafe}.
I
> think its rate of new use is very low.

I meant to say linked_ptr not make_linked_ptr, sorry. But cool, I thought
perhaps you meant there was some replacement in c++11... I mainly use linked
ptrs as a way of giving ownership of pointers to stl containers. We have
ScopedVector I suppose, but not ScopedSet or ScopedMap.

[commit-bot: I haz the power, 2012-06-18 19:08:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10544185/2002

[commit-bot: I haz the power, 2012-06-18 20:02:43]

Try job failure for 10544185-2002 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-18 20:16:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10544185/2002

[commit-bot: I haz the power, 2012-06-18 21:11:13]

Try job failure for 10544185-2002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-06-18 21:12:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10544185/2002

[commit-bot: I haz the power, 2012-06-18 23:23:44]

Change committed as 142855