Protect AppListItemList Add/Remove and fix sync bugs

Protect AppListItemList Add/Remove and fix sync bugs

This should fixe the issues seen in the bug, and includes some cleanup
in prep for syncing folders.

BUG=338520
TBR=jamescook@chromium.org for ash_shell.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247565

[stevenjb, 2014-01-28 01:12:53]

OK, this should be a bit simpler than the complete folder sync CL and includes
much of the cleanup, as well as some subtle non folder related bug fixes.

Once this is reviewed and in I will rebase the folder list CL.

[xiyuan, 2014-01-28 05:58:28]

Mostly nits.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.cc:158: std::string
label =
|label| is used for both function arg and a local varible. We should use
different names.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:146:
processing_sync_changes_ = processing_sync_changes;
Can we also Add/RemoveObserer here? If we can, we don't need to test the flag in
the notification handlers below.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_unittest.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_unittest.cc:275: 
nit: slightly prefer to keep one empty line after opening a namespace and before
closing it.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1337: std::string source_id =
nit: const std::string&

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1341: std::string target_id =
target_view->item()->id();
nit: const std::string&

[tapted, 2014-01-28 11:56:52]

Started skimming towards the end, since I think xiyuan and I were working in
parallel. Nothing stood out though.

Also there seems to be a bit more going on than the CL description suggests - I
usually see people restate a summary of the bug for a bugfix (i.e. rather than
just saying ~fixes the referenced bug).

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc
File ash/shell/app_list.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc#ol...
ash/shell/app_list.cc:201: PopulateApps(model_->item_list());
Maybe something to consider, but I guess it's not feasible for
AppListModel::item_list() to return a `const AppListItemList*` (e.g. I think
we'd still want AppListItemList::item_at(..) to return a non-const
`AppListItem*` [e.g. to add/remove obsververs], so it might look weird).

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.h (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.h:53: // Helper
function for debugging, logs into for an item.
nit: into -> info?

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:486:
model_observer_->set_processing_sync_changes(true);
It's a common pattern in the /ui/app_list code to temporarily remove the
observer, then add it back when finished when similar things like this occur -
should that be done here? (i.e. model_observer_.reset() here, and .reset(new ..)
down below)

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
File ui/app_list/app_list_item_list.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
ui/app_list/app_list_item_list.h:59: protected:
could these be private: ? (I don't think anything inherits from AppListItemList)

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:54: void AddItem(AppListItem* item);
I think the preferred way to document the ownership transferral here is to take
a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a raw
pointer, rather than .release()). More below, but those are private functions so
you could maybe argue against scoped_ptr

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:76: virtual void OnListItemMoved(size_t from_index,
nit: OVERRIDEs last?

[jennyz, 2014-01-28 18:02:47]

lgtm with nit.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (left):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder.cc:196: 
Is the model_ empty, or we just don't need to clear the old data before building
model?

[stevenjb, 2014-01-28 18:28:06]

Please let me know if you'd prefer to reset / construct ModelObserver in lieu of
set_processing_sync_changes(). I slightly prefer it as-is but don't feel
strongly.
PTAL

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc
File ash/shell/app_list.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc#ol...
ash/shell/app_list.cc:201: PopulateApps(model_->item_list());
On 2014/01/28 11:56:53, tapted wrote:
> Maybe something to consider, but I guess it's not feasible for
> AppListModel::item_list() to return a `const AppListItemList*` (e.g. I think
> we'd still want AppListItemList::item_at(..) to return a non-const
> `AppListItem*` [e.g. to add/remove obsververs], so it might look weird).

We specifically don't allow const methods to return non-const pointers. We could
make add/remove observer mutable, but I would just as soon avoid that.

Also, as we add complexity to the model (e.g. pages) we should be moving away
from manipulating the item lists directly and interact only with the model.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.cc:158: std::string
label =
On 2014/01/28 05:58:29, xiyuan wrote:
> |label| is used for both function arg and a local varible. We should use
> different names. 

Done.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.h (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.h:53: // Helper
function for debugging, logs into for an item.
On 2014/01/28 11:56:53, tapted wrote:
> nit: into -> info?

Done.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:146:
processing_sync_changes_ = processing_sync_changes;
On 2014/01/28 05:58:29, xiyuan wrote:
> Can we also Add/RemoveObserer here? If we can, we don't need to test the flag
in
> the notification handlers below.

I didn't go that route for two reasons:
1) This makes it easier to debug, and possibly modify the logic later.
2) Adding / Removing observers has additional overhead.

I can change to adding/removing as an observer if you feel that would be better
/ more clear.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:486:
model_observer_->set_processing_sync_changes(true);
On 2014/01/28 11:56:53, tapted wrote:
> It's a common pattern in the /ui/app_list code to temporarily remove the
> observer, then add it back when finished when similar things like this occur -
> should that be done here? (i.e. model_observer_.reset() here, and .reset(new
..)
> down below)

Xiyuan made a similar comment, I explained why I didn't do that above, but could
be persuaded otherwise.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (left):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder.cc:196: 
On 2014/01/28 18:02:48, jennyz wrote:
> Is the model_ empty, or we just don't need to clear the old data before
building
> model?

This only gets called once now. We used to call this when we changed profiles,
but that changed and this got left. I didn't want to maintain DeleteItemsByType
when we don't actually need it. I'll add some DCHECKs and remove the if
(tracker_) code to make this more clear.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
File ui/app_list/app_list_item_list.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
ui/app_list/app_list_item_list.h:59: protected:
On 2014/01/28 11:56:53, tapted wrote:
> could these be private: ? (I don't think anything inherits from
AppListItemList)

Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:54: void AddItem(AppListItem* item);
On 2014/01/28 11:56:53, tapted wrote:
> I think the preferred way to document the ownership transferral here is to
take
> a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a raw
> pointer, rather than .release()). More below, but those are private functions
so
> you could maybe argue against scoped_ptr

If you'll pardon the pun, I wanted to limit the scope of this change. We call
this a lot from tests (previously we called model->item_list->AddItem), and
changing those all to use scoped pointers would be tedious. Maybe in another
cleanup pass. I will add a comment about ownership though. (I don't personally
believe strongly in using scoping to enforce ownership; this is still C++, not
Java).

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:76: virtual void OnListItemMoved(size_t from_index,
On 2014/01/28 11:56:53, tapted wrote:
> nit: OVERRIDEs last?
I'm unaware of that convention, and prefer to list overrides before fully
private methods which tend to be implementation details.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_unittest.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_unittest.cc:275: 
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: slightly prefer to keep one empty line after opening a namespace and
before
> closing it.

Unintentional. Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1337: std::string source_id =
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: const std::string&

Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1341: std::string target_id =
target_view->item()->id();
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: const std::string&

Done.

[stevenjb, 2014-01-28 18:28:07]

Please let me know if you'd prefer to reset / construct ModelObserver in lieu of
set_processing_sync_changes(). I slightly prefer it as-is but don't feel
strongly.
PTAL

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc
File ash/shell/app_list.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ash/shell/app_list.cc#ol...
ash/shell/app_list.cc:201: PopulateApps(model_->item_list());
On 2014/01/28 11:56:53, tapted wrote:
> Maybe something to consider, but I guess it's not feasible for
> AppListModel::item_list() to return a `const AppListItemList*` (e.g. I think
> we'd still want AppListItemList::item_at(..) to return a non-const
> `AppListItem*` [e.g. to add/remove obsververs], so it might look weird).

We specifically don't allow const methods to return non-const pointers. We could
make add/remove observer mutable, but I would just as soon avoid that.

Also, as we add complexity to the model (e.g. pages) we should be moving away
from manipulating the item lists directly and interact only with the model.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.cc:158: std::string
label =
On 2014/01/28 05:58:29, xiyuan wrote:
> |label| is used for both function arg and a local varible. We should use
> different names. 

Done.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/sync_app_list_helper.h (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/sync_app_list_helper.h:53: // Helper
function for debugging, logs into for an item.
On 2014/01/28 11:56:53, tapted wrote:
> nit: into -> info?

Done.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:146:
processing_sync_changes_ = processing_sync_changes;
On 2014/01/28 05:58:29, xiyuan wrote:
> Can we also Add/RemoveObserer here? If we can, we don't need to test the flag
in
> the notification handlers below.

I didn't go that route for two reasons:
1) This makes it easier to debug, and possibly modify the logic later.
2) Adding / Removing observers has additional overhead.

I can change to adding/removing as an observer if you feel that would be better
/ more clear.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:486:
model_observer_->set_processing_sync_changes(true);
On 2014/01/28 11:56:53, tapted wrote:
> It's a common pattern in the /ui/app_list code to temporarily remove the
> observer, then add it back when finished when similar things like this occur -
> should that be done here? (i.e. model_observer_.reset() here, and .reset(new
..)
> down below)

Xiyuan made a similar comment, I explained why I didn't do that above, but could
be persuaded otherwise.

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (left):

https://codereview.chromium.org/148403007/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder.cc:196: 
On 2014/01/28 18:02:48, jennyz wrote:
> Is the model_ empty, or we just don't need to clear the old data before
building
> model?

This only gets called once now. We used to call this when we changed profiles,
but that changed and this got left. I didn't want to maintain DeleteItemsByType
when we don't actually need it. I'll add some DCHECKs and remove the if
(tracker_) code to make this more clear.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
File ui/app_list/app_list_item_list.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_ite...
ui/app_list/app_list_item_list.h:59: protected:
On 2014/01/28 11:56:53, tapted wrote:
> could these be private: ? (I don't think anything inherits from
AppListItemList)

Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:54: void AddItem(AppListItem* item);
On 2014/01/28 11:56:53, tapted wrote:
> I think the preferred way to document the ownership transferral here is to
take
> a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a raw
> pointer, rather than .release()). More below, but those are private functions
so
> you could maybe argue against scoped_ptr

If you'll pardon the pun, I wanted to limit the scope of this change. We call
this a lot from tests (previously we called model->item_list->AddItem), and
changing those all to use scoped pointers would be tedious. Maybe in another
cleanup pass. I will add a comment about ownership though. (I don't personally
believe strongly in using scoping to enforce ownership; this is still C++, not
Java).

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:76: virtual void OnListItemMoved(size_t from_index,
On 2014/01/28 11:56:53, tapted wrote:
> nit: OVERRIDEs last?
I'm unaware of that convention, and prefer to list overrides before fully
private methods which tend to be implementation details.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_unittest.cc (left):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_unittest.cc:275: 
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: slightly prefer to keep one empty line after opening a namespace and
before
> closing it.

Unintentional. Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1337: std::string source_id =
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: const std::string&

Done.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1341: std::string target_id =
target_view->item()->id();
On 2014/01/28 05:58:29, xiyuan wrote:
> nit: const std::string&

Done.

[xiyuan, 2014-01-28 18:50:25]

LGTM

I am slightly preferred to do the RemoveObserver/AddObserver business but don't
feel strongly about it.

[stevenjb, 2014-01-28 19:34:49]

I went ahead and made the change to add / remove the model observer.

[commit-bot: I haz the power, 2014-01-28 19:37:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/148403007/80001

[commit-bot: I haz the power, 2014-01-28 19:55:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-28 20:01:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/148403007/80001

[tapted, 2014-01-28 22:37:05]

On 2014/01/28 11:56:52, tapted wrote:
> Also there seems to be a bit more going on than the CL description suggests -
I
> usually see people restate a summary of the bug for a bugfix (i.e. rather than
> just saying ~fixes the referenced bug).

I think the CL description still needs a tune-up.

It's common to have a TEST= line for bugfixes too.

Also, I have a concern that if the regression in http://crbug.com/334963 can't
be fixed we will need to revert http://crrev.com/244846 and all the things
layered on top of it to get it working again in M34.

[stevenjb, 2014-01-28 22:47:59]

On 2014/01/28 22:37:05, tapted wrote:
> On 2014/01/28 11:56:52, tapted wrote:
> > Also there seems to be a bit more going on than the CL description suggests
-
> I
> > usually see people restate a summary of the bug for a bugfix (i.e. rather
than
> > just saying ~fixes the referenced bug).
> 
> I think the CL description still needs a tune-up.
> 
> It's common to have a TEST= line for bugfixes too.
> 
> Also, I have a concern that if the regression in http://crbug.com/334963 can't
> be fixed we will need to revert http://crrev.com/244846 and all the things
> layered on top of it to get it working again in M34.

I apologize for not adding more detail in the issue description, I meant to do
that but forgot. Unfortunately if I change it now I'm afraid the CQ will blow
up. I feel like the changes themselves should be pretty well documented and self
explanatory. It does say "should fix".

We mostly stopped using the TEST=, since the referenced issue should do a better
job of providing that information.

Backing up now would be challenging. We are still quite a ways from the M34
release date, I am confident that we can address  http://crbug.com/334963 before
then. I will be investigating more later today and tomorrow.

[benwells, 2014-01-28 23:09:00]

On 2014/01/28 22:47:59, stevenjb wrote:
> On 2014/01/28 22:37:05, tapted wrote:
> > On 2014/01/28 11:56:52, tapted wrote:
> > > Also there seems to be a bit more going on than the CL description
suggests
> -
> > I
> > > usually see people restate a summary of the bug for a bugfix (i.e. rather
> than
> > > just saying ~fixes the referenced bug).
> > 
> > I think the CL description still needs a tune-up.
> > 
> > It's common to have a TEST= line for bugfixes too.
> > 
> > Also, I have a concern that if the regression in http://crbug.com/334963
can't
> > be fixed we will need to revert http://crrev.com/244846 and all the things
> > layered on top of it to get it working again in M34.
> 
> I apologize for not adding more detail in the issue description, I meant to do
> that but forgot. Unfortunately if I change it now I'm afraid the CQ will blow
> up. I feel like the changes themselves should be pretty well documented and
self
> explanatory. It does say "should fix".
> 
> We mostly stopped using the TEST=, since the referenced issue should do a
better
> job of providing that information.
> 
> Backing up now would be challenging. We are still quite a ways from the M34
> release date, I am confident that we can address  http://crbug.com/334963
before
> then. I will be investigating more later today and tomorrow.

As a general rule you should be responding to all feedback and waiting for lg tm
from the reviewers you add, assuming they leave some feedback. In this case it
isn't such a big deal, however in https://codereview.chromium.org/118463002 if
you had responded to Trent's feedback and waited for his lg tm, we may have
avoided introducing that bug in the first place.

Just to reiterate, my understanding of chromium etiquette is that developers
wait for lg tm from their reviewers and I'm surprised to see this not happening.

[stevenjb, 2014-01-28 23:26:41]

My apologies, I will be more careful in the future.



On Tue, Jan 28, 2014 at 3:09 PM, <benwells@chromium.org> wrote:

> On 2014/01/28 22:47:59, stevenjb wrote:
>
>> On 2014/01/28 22:37:05, tapted wrote:
>> > On 2014/01/28 11:56:52, tapted wrote:
>> > > Also there seems to be a bit more going on than the CL description
>>
> suggests
>
>> -
>> > I
>> > > usually see people restate a summary of the bug for a bugfix (i.e.
>> rather
>> than
>> > > just saying ~fixes the referenced bug).
>> >
>> > I think the CL description still needs a tune-up.
>> >
>> > It's common to have a TEST= line for bugfixes too.
>> >
>> > Also, I have a concern that if the regression in
>> http://crbug.com/334963
>>
> can't
>
>> > be fixed we will need to revert http://crrev.com/244846 and all the
>> things
>> > layered on top of it to get it working again in M34.
>>
>
>  I apologize for not adding more detail in the issue description, I meant
>> to do
>> that but forgot. Unfortunately if I change it now I'm afraid the CQ will
>> blow
>> up. I feel like the changes themselves should be pretty well documented
>> and
>>
> self
>
>> explanatory. It does say "should fix".
>>
>
>  We mostly stopped using the TEST=, since the referenced issue should do a
>>
> better
>
>> job of providing that information.
>>
>
>  Backing up now would be challenging. We are still quite a ways from the
>> M34
>> release date, I am confident that we can address  http://crbug.com/334963
>>
> before
>
>> then. I will be investigating more later today and tomorrow.
>>
>
> As a general rule you should be responding to all feedback and waiting for
> lg tm
> from the reviewers you add, assuming they leave some feedback. In this
> case it
> isn't such a big deal, however in https://codereview.chromium.
> org/118463002 if
> you had responded to Trent's feedback and waited for his lg tm, we may have
> avoided introducing that bug in the first place.
>
> Just to reiterate, my understanding of chromium etiquette is that
> developers
> wait for lg tm from their reviewers and I'm surprised to see this not
> happening.
>
> https://codereview.chromium.org/148403007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2014-01-28 23:33:53]

lgtm in the interests of not booting you out of the queue (although CQ would
probably be fine - it's resilient to these things, just slow).

Also note that you need to add people in TBR to the list of reviewers manually
unless it's in the CL description on first upload, otherwise they won't see it.

('corse if I do it now, you will get a reject from CQ with "list of reviewers
changed")

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:54: void AddItem(AppListItem* item);
On 2014/01/28 18:28:08, stevenjb wrote:
> On 2014/01/28 11:56:53, tapted wrote:
> > I think the preferred way to document the ownership transferral here is to
> take
> > a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a raw
> > pointer, rather than .release()). More below, but those are private
functions
> so
> > you could maybe argue against scoped_ptr
> 
> If you'll pardon the pun, I wanted to limit the scope of this change. We call
> this a lot from tests (previously we called model->item_list->AddItem), and
> changing those all to use scoped pointers would be tedious. Maybe in another
> cleanup pass. I will add a comment about ownership though. (I don't personally
> believe strongly in using scoping to enforce ownership; this is still C++, not
> Java).

I was going by a chromium dev thread -
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/9Q2pc9jqxMQ/94AZw...
. Latest word was "use raw pointers only when ownership is not passed."

TBH, the ownership transferrals here are pretty hard to follow, so I would be in
favour of scoped_ptr.

In tests, I think it mostly goes through AppListTestModel so shouldn't make too
much churn.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:76: virtual void OnListItemMoved(size_t from_index,
On 2014/01/28 18:28:08, stevenjb wrote:
> On 2014/01/28 11:56:53, tapted wrote:
> > nit: OVERRIDEs last?
> I'm unaware of that convention, and prefer to list overrides before fully
> private methods which tend to be implementation details.

I thought it was convention, but don't really feel strongly. I've seen it in
codereview a few times like ~"newly introduced functions come first in each
section", but it comes up more often in non-private sections.

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/cocoa/apps_g...
File ui/app_list/cocoa/apps_grid_controller_unittest.mm (left):

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/cocoa/apps_g...
ui/app_list/cocoa/apps_grid_controller_unittest.mm:525:
TEST_F(AppsGridControllerTest, ModelRemoveAlll) {
Just a note, this test case initially existed to ensure there was coverage for
ListModelObserver::ListItemsRemoved(size_t start, size_t count) when count != 1
(it should have a comment to that effect - sorry). That's not relevant any more,
since AppListItemListObserver can only remove one item at a time (and would have
become irrelevant when the line below changed to DeleteItemsByType in
http://crrev.com/231923 ).

I guess it's still good to have a test that gives some coverage when the "last"
item is removed from the model.

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1341: const std::string&  target_id =
target_view->item()->id();
nit: there's two spaces after the '&'

[stevenjb, 2014-01-29 00:11:30]

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:54: void AddItem(AppListItem* item);
On 2014/01/28 23:33:54, tapted wrote:
> On 2014/01/28 18:28:08, stevenjb wrote:
> > On 2014/01/28 11:56:53, tapted wrote:
> > > I think the preferred way to document the ownership transferral here is to
> > take
> > > a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a
raw
> > > pointer, rather than .release()). More below, but those are private
> functions
> > so
> > > you could maybe argue against scoped_ptr
> > 
> > If you'll pardon the pun, I wanted to limit the scope of this change. We
call
> > this a lot from tests (previously we called model->item_list->AddItem), and
> > changing those all to use scoped pointers would be tedious. Maybe in another
> > cleanup pass. I will add a comment about ownership though. (I don't
personally
> > believe strongly in using scoping to enforce ownership; this is still C++,
not
> > Java).
> 
> I was going by a chromium dev thread -
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/9Q2pc9jqxMQ/94AZw...
> . Latest word was "use raw pointers only when ownership is not passed."
> 
> TBH, the ownership transferrals here are pretty hard to follow, so I would be
in
> favour of scoped_ptr.
> 
> In tests, I think it mostly goes through AppListTestModel so shouldn't make
too
> much churn.

Yeah, that's generally the consensus, and I don't disagree that it would be more
clear here. You make a good point about AppListTestModel, the change may not be
as bad as I fear, I will definitely consider it the next time I am in this code.

https://codereview.chromium.org/148403007/diff/20001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:76: virtual void OnListItemMoved(size_t from_index,
On 2014/01/28 23:33:54, tapted wrote:
> On 2014/01/28 18:28:08, stevenjb wrote:
> > On 2014/01/28 11:56:53, tapted wrote:
> > > nit: OVERRIDEs last?
> > I'm unaware of that convention, and prefer to list overrides before fully
> > private methods which tend to be implementation details.
> 
> I thought it was convention, but don't really feel strongly. I've seen it in
> codereview a few times like ~"newly introduced functions come first in each
> section", but it comes up more often in non-private sections.

If it is I'm unaware. Frequently they are public and first.

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/148403007/diff/80001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1341: const std::string&  target_id =
target_view->item()->id();
On 2014/01/28 23:33:54, tapted wrote:
> nit: there's two spaces after the '&'

Thanks, I'll fix in the follow-up.

[commit-bot: I haz the power, 2014-01-29 01:23:02]

Change committed as 247565