Media Galleries: Keep media gallery permission dialogs in sync with the gallery

Media Galleries: Keep media gallery permission dialogs in sync with the gallery
prefs.

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

[Lei Zhang, 2013-01-31 01:12:26]

This depends on a couple other CLs:
https://chromiumcodereview.appspot.com/12114019/
https://chromiumcodereview.appspot.com/12091054/

Patch set 1 is just the core dialog controller changes. I still need to add the
platform-dependent ForgetGallery() UI code. However, you can review the core
logic independent of the UI changes.

[Lei Zhang, 2013-01-31 01:13:37]

+greg FYI.

[vandebo (ex-Chrome), 2013-01-31 01:54:07]

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:273: // First,
update |known_galleries_| from |pref_galleries|.
Seems that you could just put this all in OnGalleryChanged

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:280: if
(gallery.type == MediaGalleryPrefInfo::kBlackListed)
Seems that this shares some code with InitializePermissions

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:287:
DCHECK_EQ(existing_gallery.display_name, gallery.display_name);
We might get a gallery change event because the name of a gallery was updated.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:298: // Next,
find all the galleries in |known_galleries_| that no longer exist
Would it be easier to sort the new and old collections and walk them together?
Same gallery -> update, only in new list -> add, only in old list -> remove

Actually, maps are already sorted.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:39: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
It seems to me that there should be three methods, or one, but not two:
UpdateGallery(PrefInfo, enum {unchecked, checked, removed}), or
AddGallery(PrefInfo), RemoveGallery(PrefInfo), UpdateGalleryPermission(PrefInfo,
bool permitted). What do you think?
Also, in case you missed it, as an abstract method, you'll have to update the
implementations before committing.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:125: // auto
detected gallery coming available.
nit: coming->becoming

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:129: void
UpdateGalleriesOnDeviceEvent(const std::string& device_id);
Do you still need this when you have the OnPreferencesEvent ?

[Lei Zhang, 2013-01-31 06:59:21]

I'd like some feedback before proceeding to patch set 2.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:273: // First,
update |known_galleries_| from |pref_galleries|.
On 2013/01/31 01:54:07, vandebo wrote:
> Seems that you could just put this all in OnGalleryChanged

Yes, this is not strictly necessary. I hope it's more readable and for it to
mirror the storage attached event handlers.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:287:
DCHECK_EQ(existing_gallery.display_name, gallery.display_name);
On 2013/01/31 01:54:07, vandebo wrote:
> We might get a gallery change event because the name of a gallery was updated.

I don't think that can happen. Do you have an example in mind? Maybe I shouldn't
worry about this and just accept that the prefs is the more authoritative
information source.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:298: // Next,
find all the galleries in |known_galleries_| that no longer exist
On 2013/01/31 01:54:07, vandebo wrote:
> Would it be easier to sort the new and old collections and walk them together?
> Same gallery -> update, only in new list -> add, only in old list -> remove
> 
> Actually, maps are already sorted.

This seems to go against the suggestion above to re-use InitializePermissions().
It's not fun to walk an iterator and erase from it at the same time.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:39: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
On 2013/01/31 01:54:07, vandebo wrote:
> It seems to me that there should be three methods, or one, but not two:
> UpdateGallery(PrefInfo, enum {unchecked, checked, removed}), or
> AddGallery(PrefInfo), RemoveGallery(PrefInfo),
UpdateGalleryPermission(PrefInfo,
> bool permitted). What do you think?

You should know that Update does more than just update the checkbox. It's also
used to refresh the tooltip and gallery name. The comment is slightly out of
date.

Really, what I have is AddOrUpdate and Forget. We can do Add/Remove/Update, but
that's a lot less forgiving on the dialog controller, which has to be more
careful about when to call Add vs Update.

> Also, in case you missed it, as an abstract method, you'll have to update the
> implementations before committing.

I mentioned the lack of UI specific implementations in my original message.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:129: void
UpdateGalleriesOnDeviceEvent(const std::string& device_id);
On 2013/01/31 01:54:07, vandebo wrote:
> Do you still need this when you have the OnPreferencesEvent ?

There's no preference event when a device gets detached. We can probably get rid
of OnRemovableStorageAttached(), but I need to test that.

[vandebo (ex-Chrome), 2013-01-31 19:38:50]

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:273: // First,
update |known_galleries_| from |pref_galleries|.
On 2013/01/31 06:59:21, Lei Zhang wrote:
> On 2013/01/31 01:54:07, vandebo wrote:
> > Seems that you could just put this all in OnGalleryChanged
> 
> Yes, this is not strictly necessary. I hope it's more readable and for it to
> mirror the storage attached event handlers.

Didn't notice that part.  The storage attached/detach event handlers are done
that way to remove code duplication, but mirroring that seems fine.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:287:
DCHECK_EQ(existing_gallery.display_name, gallery.display_name);
On 2013/01/31 06:59:21, Lei Zhang wrote:
> On 2013/01/31 01:54:07, vandebo wrote:
> > We might get a gallery change event because the name of a gallery was
updated.
> 
> I don't think that can happen. Do you have an example in mind? Maybe I
shouldn't
> worry about this and just accept that the prefs is the more authoritative
> information source.

See chrome/browser/media_gallery/media_galleries_preferences.cc:268
(update_gallery_name).  We added that so that if the drive letter or volume name
changed on a device, the display name would update as well.

You can treat prefs as the source of truth.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:298: // Next,
find all the galleries in |known_galleries_| that no longer exist
On 2013/01/31 06:59:21, Lei Zhang wrote:
> On 2013/01/31 01:54:07, vandebo wrote:
> > Would it be easier to sort the new and old collections and walk them
together?
> > Same gallery -> update, only in new list -> add, only in old list -> remove
> > 
> > Actually, maps are already sorted.
> 
> This seems to go against the suggestion above to re-use
InitializePermissions().
> It's not fun to walk an iterator and erase from it at the same time.

It may not be practical to do both sure, but trying to do one or the other seems
like it could reduce the code and make it more maintainable.

You can certainly play the queue deletes in another list trick.  Even then you'd
still only have two loops.  Right now there are like six loops.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:39: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
On 2013/01/31 06:59:21, Lei Zhang wrote:
> On 2013/01/31 01:54:07, vandebo wrote:
> > It seems to me that there should be three methods, or one, but not two:
> > UpdateGallery(PrefInfo, enum {unchecked, checked, removed}), or
> > AddGallery(PrefInfo), RemoveGallery(PrefInfo),
> UpdateGalleryPermission(PrefInfo,
> > bool permitted). What do you think?
> 
> You should know that Update does more than just update the checkbox. It's also
> used to refresh the tooltip and gallery name. The comment is slightly out of
> date.
> 
> Really, what I have is AddOrUpdate and Forget. We can do Add/Remove/Update,
but
> that's a lot less forgiving on the dialog controller, which has to be more
> careful about when to call Add vs Update.

What about UpdateGallery(PrefInfo, enum {unchecked, checked, removed})? 
Removing is a way of updating, and it would remove a bool in favor of an enum. 
If the implementation with Update and Forget is easier, that's fine.

> > Also, in case you missed it, as an abstract method, you'll have to update
the
> > implementations before committing.
> 
> I mentioned the lack of UI specific implementations in my original message.

Oops, missed that, sorry.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:129: void
UpdateGalleriesOnDeviceEvent(const std::string& device_id);
On 2013/01/31 06:59:21, Lei Zhang wrote:
> On 2013/01/31 01:54:07, vandebo wrote:
> > Do you still need this when you have the OnPreferencesEvent ?
> 
> There's no preference event when a device gets detached. We can probably get
rid
> of OnRemovableStorageAttached(), but I need to test that.

Oh, you still need these to update the attached/detached status.  Got it.

[Lei Zhang, 2013-02-01 22:04:11]

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:280: if
(gallery.type == MediaGalleryPrefInfo::kBlackListed)
On 2013/01/31 01:54:07, vandebo wrote:
> Seems that this shares some code with InitializePermissions

Done.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:287:
DCHECK_EQ(existing_gallery.display_name, gallery.display_name);
On 2013/01/31 19:38:50, vandebo wrote:
> On 2013/01/31 06:59:21, Lei Zhang wrote:
> > On 2013/01/31 01:54:07, vandebo wrote:
> > > We might get a gallery change event because the name of a gallery was
> updated.
> > 
> > I don't think that can happen. Do you have an example in mind? Maybe I
> shouldn't
> > worry about this and just accept that the prefs is the more authoritative
> > information source.
> 
> See chrome/browser/media_gallery/media_galleries_preferences.cc:268
> (update_gallery_name).  We added that so that if the drive letter or volume
name
> changed on a device, the display name would update as well.
> 
> You can treat prefs as the source of truth.

Ya, I'm doing that since I just call InitializePermissions now.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:298: // Next,
find all the galleries in |known_galleries_| that no longer exist
On 2013/01/31 19:38:50, vandebo wrote:
> On 2013/01/31 06:59:21, Lei Zhang wrote:
> > On 2013/01/31 01:54:07, vandebo wrote:
> > > Would it be easier to sort the new and old collections and walk them
> together?
> > > Same gallery -> update, only in new list -> add, only in old list ->
remove
> > > 
> > > Actually, maps are already sorted.
> > 
> > This seems to go against the suggestion above to re-use
> InitializePermissions().
> > It's not fun to walk an iterator and erase from it at the same time.
> 
> It may not be practical to do both sure, but trying to do one or the other
seems
> like it could reduce the code and make it more maintainable.
> 
> You can certainly play the queue deletes in another list trick.  Even then
you'd
> still only have two loops.  Right now there are like six loops.

Got it down to InitializePermissions + 3 loops.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:39: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
On 2013/01/31 19:38:50, vandebo wrote:
> What about UpdateGallery(PrefInfo, enum {unchecked, checked, removed})? 
> Removing is a way of updating, and it would remove a bool in favor of an enum.

> If the implementation with Update and Forget is easier, that's fine.

I really would like to keep Remove separate from Update and not overload the
|permitted| parameter to become a tri-state variable.

https://codereview.chromium.org/12095074/diff/4001/chrome/browser/media_galle...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:125: // auto
detected gallery coming available.
On 2013/01/31 01:54:07, vandebo wrote:
> nit: coming->becoming

Done.

[Lei Zhang, 2013-02-05 00:20:28]

Patch set 3 remembers the user toggles, so when a new device gets attached, the
user-toggled checkboxes keep their permissions.

I added the GTK and Views implementations. Cocoa code still looks like a foreign
language...

[Greg Billock, 2013-02-05 18:07:05]

I'm going to be working on the cocoa dialog for the new look. I can work on
this at the same time.


On Mon, Feb 4, 2013 at 4:20 PM, <thestig@chromium.org> wrote:

> Patch set 3 remembers the user toggles, so when a new device gets
> attached, the
> user-toggled checkboxes keep their permissions.
>
> I added the GTK and Views implementations. Cocoa code still looks like a
> foreign
> language...
>
>
https://codereview.chromium.**org/12095074/<https://codereview.chromium.org/1...
>

[vandebo (ex-Chrome), 2013-02-06 00:42:23]

On 2013/02/05 00:20:28, Lei Zhang wrote:
> I added the GTK and Views implementations. Cocoa code still looks like a
foreign
> language...

Can you add a NULL implementation for Cocoa so that the CL can go in?

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:142:
iter->second.allowed = enabled;
DCHECK_NE(iter->second.allowed, enabled) ?

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:165: // only
non-test code path that deletes |this|. The test ctor never adds
How can the test ctor never add the observer? The test ctor has to call the base
class's ctor...

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:278: void
MediaGalleriesDialogController::UpdateGalleriesOnPreferencesEvent() {
I've previously suggested walking both lists concurrently.  I wanted to see if
it was easier to reed, so I wrote it.  I'm not sure it's easier to read, but it
is more explicit about information flow.  What do you think?

void MediaGalleriesDialogController::UpdateGalleriesOnPreferencesEvent() {
  MediaGalleryPrefIdSet galleries_to_forget;
  const MediaGalleriesPrefInfoMap& pref_galleries =
      preferences_->known_galleries();
  KnownGalleryPermissions::const_iterator existing = known_galleries_.begin();

  // Walk the existing set of galleries along with the new set retrieved from
  // preferences.
  for (MediaGalleriesPrefInfoMap::const_iterator it = pref_galleries.begin();
       it != pref_galleries.end();
       ++it) {
    // Anything that's in the existing set and not in the new set should be
    // removed.
    while (existing->first < it->first) {
      galleries_to_forget.insert(existing->first);
      dialog_->ForgetGallery(&existing->second.pref_info);
      ++existing;
    }
    // Entries that are in both sets need to be updated.
    if (existing->first == it->first)
      if (it->second.type == MediaGalleryPrefInfo::kBlackListed)) {
        galleries_to_forget.insert(existing->first);
        dialog_->ForgetGallery(&existing->second.pref_info);
      } else {
        bool allowed = it->second.allowed;
        if (ContainsKey(toggled_galleries_, it->first))
          allowed = existing->second.allowed;
        dialog_->UpdateGallery(&existing->second.pref_info, allowed);
      }
      ++existing;
    } else {
      // Add entries that are just in preferences.
      DCHECK_GT(existing->first > it->first);
      bool allowed = it->second.allowed;
      // If it was already added in this dialog, remove the added one first.
      for (NewGalleryPermissions::iterator added_it = new_galleries_.begin();
           added_it != new_galleries_.end();
           ++added_it) {
        if (added_it->pref_info.path == it->second.path &&
            added_it->pref_info.device_id == it->second.device_id) {
          allowed = new_if->allowed;
          dialog_->ForgetGallery(&new_it->pref_info);
          new_galleries_.erase(added_it);
          break;
        }
      }

      existing = known_galleries_.insert(
          std::pair<MediaGalleryPrefId, GalleryPermission>(
              it->first,
              GalleryPermission(it->second.pref_info, allowed)))->first;
      dialog_->UpdateGallery(&existing->second.pref_info, allowed);
      ++existing;
    }
  }

  // Remove the galleries to forget from |known_galleries_|. Doing it in the
  // above loop would invalidate the iterator there.
  for (MediaGalleryPrefIdSet::const_iterator it = galleries_to_forget.begin();
       it != galleries_to_forget.end();
       ++it) {
    known_galleries_.erase(*it);
  }
}

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:312: //
Resolve |new_galleries_| against the updated |known_galleries_|.
Does this need to go first?  We will have already called UpdateGallery for the
replacement view of the gallery.

[Lei Zhang, 2013-02-07 01:44:56]

I need to do the Cocoa implementation too, otherwise the dialog can point to
invalid memory.

Doing the |known_galleries_| update in one loop works, but it's a big loop
that's a bit harder to read. I'd like to keep my version, and I have updated a
couple more comments to clarify what's going on.

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:142:
iter->second.allowed = enabled;
On 2013/02/06 00:42:23, vandebo wrote:
> DCHECK_NE(iter->second.allowed, enabled) ?

Done.

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:165: // only
non-test code path that deletes |this|. The test ctor never adds
On 2013/02/06 00:42:23, vandebo wrote:
> How can the test ctor never add the observer? The test ctor has to call the
base
> class's ctor...

The tests go through MediaGalleriesDialogControllerMock which calls the second
version of the base ctor which does not add an observer.

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:312: //
Resolve |new_galleries_| against the updated |known_galleries_|.
On 2013/02/06 00:42:23, vandebo wrote:
> Does this need to go first?  We will have already called UpdateGallery for the
> replacement view of the gallery.

I think it's fine to do it here. There's no way to reuse the existing entry
because it's pointing to memory in |new_galleries_| but now the entry's info is
in |known_galleries_|.

For the case this loop is trying to solve, no matter how you do it, there's
going to be a moment when there's two entries for a gallery, one in
|known_galleries_| and one in |new_galleries_|. The update happens fast enough
that users are not going to see any visual glitches, if that's what you are
worried about.

[vandebo (ex-Chrome), 2013-02-07 02:08:04]

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/16001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:312: //
Resolve |new_galleries_| against the updated |known_galleries_|.
On 2013/02/07 01:44:56, Lei Zhang wrote:
> On 2013/02/06 00:42:23, vandebo wrote:
> > Does this need to go first?  We will have already called UpdateGallery for
the
> > replacement view of the gallery.
> 
> I think it's fine to do it here. There's no way to reuse the existing entry
> because it's pointing to memory in |new_galleries_| but now the entry's info
is
> in |known_galleries_|.
> 
> For the case this loop is trying to solve, no matter how you do it, there's
> going to be a moment when there's two entries for a gallery, one in
> |known_galleries_| and one in |new_galleries_|. The update happens fast enough
> that users are not going to see any visual glitches, if that's what you are
> worried about.

I just don't know how the dialog actually works so I didn't know if it was ok to
have two entries for the same gallery.

[Lei Zhang, 2013-02-07 02:16:10]

On 2013/02/07 02:08:04, vandebo wrote:
> I just don't know how the dialog actually works so I didn't know if it was ok
to
> have two entries for the same gallery.

It would be really confusing to have two entries for the same gallery. Since the
copy in the |preferences_| is "more established" and the copy in
|new_galleries_| has yet to be saved, |preferences_| wins.

[vandebo (ex-Chrome), 2013-02-07 18:44:07]

On 2013/02/07 02:16:10, Lei Zhang wrote:
> On 2013/02/07 02:08:04, vandebo wrote:
> > I just don't know how the dialog actually works so I didn't know if it was
ok
> to
> > have two entries for the same gallery.
> 
> It would be really confusing to have two entries for the same gallery. Since
the
> copy in the |preferences_| is "more established" and the copy in
> |new_galleries_| has yet to be saved, |preferences_| wins.

Not sure how to take your reply.  The current code has the existing "new"
gallery entry.  Calls update to add the one from preferences and then removes
the "new" one.  My concern is that will confuse the dialog because at step two
in that flow there are two entries for the same gallery.  If that's ok, then I
don't object.

[Lei Zhang, 2013-02-08 05:09:52]

On 2013/02/07 18:44:07, vandebo wrote:
> On 2013/02/07 02:16:10, Lei Zhang wrote:
> > On 2013/02/07 02:08:04, vandebo wrote:
> > > I just don't know how the dialog actually works so I didn't know if it was
> ok
> > to
> > > have two entries for the same gallery.
> > 
> > It would be really confusing to have two entries for the same gallery. Since
> the
> > copy in the |preferences_| is "more established" and the copy in
> > |new_galleries_| has yet to be saved, |preferences_| wins.
> 
> Not sure how to take your reply.  The current code has the existing "new"
> gallery entry.  Calls update to add the one from preferences and then removes
> the "new" one.  My concern is that will confuse the dialog because at step two
> in that flow there are two entries for the same gallery.  If that's ok, then I
> don't object.

In my testing, albeit on fast machines, I never saw the duplicate entries
visually.

[Lei Zhang, 2013-02-08 06:48:11]

estade, sail: want to take a look at the chrome/browser/ui/ changes?

[Lei Zhang, 2013-02-08 06:59:07]

vandebo: take a look at the new patch set. Patch set 5 and 6 shows how it got
there.

[sail, 2013-02-08 19:48:22]

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:225:
[checkboxes_ removeObjectAtIndex:index];
Hm.. is this ok to do while the iterator is still alive?
Maybe move the for loop into a helper function? Something like:
  NSButton* checkbox = [self checkboxForID:unique_id];
  [checkboxes_ removeObject:checkbox]

You could then use that method in UpdateGallery() as well.

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_unittest.mm
(right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_unittest.mm:160:
new_container_height = NSHeight([dialog->checkbox_container_ frame]);
this test is a bit long. How about removing the two "should be taller" test and
only do the "should be shorter" test. Something like:
dialog->UpdateGallery(&gallery1, true);
dialog->UpdateGallery(&gallery2, true);
CGFloat new_container_height = NSHeight([dialog->checkbox_container_ frame]);

dialog->ForgetGallery(&gallery2);

...

[vandebo (ex-Chrome), 2013-02-08 19:57:41]

Media Galleries code LGTM

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_preferences.h:81: virtual void
OnGalleryChanged(MediaGalleriesPreferences* pref,
nit: add comment about empty string when the change isn't extension specific.

[vandebo (ex-Chrome), 2013-02-08 19:58:01]

Rather, controller code LGTM

[Greg Billock, 2013-02-08 20:09:25]

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:232: if
(extension_id.empty() || extension_id == extension_->id())
Will this check end up being useful? That is, it seems likely that simultaneous
actions by the user will be on the same extension, and a rescan is pretty cheap
anyway if it isn't (we don't need the extension_id to do it).

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:284:
InitializePermissions();
Could we basically just re-initialize here, clearing everything? That'd be
simpler to follow and maintain. Or does that make the dialog flash in a way
that's annoying?

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_preferences.h:81: virtual void
OnGalleryChanged(MediaGalleriesPreferences* pref,
Should this pass any information about which gallery changed? Or it's expected
the client will do a full rescan? In that case, do we really need to pass the
extension id?

[Lei Zhang, 2013-02-08 20:48:02]

sail: Patch set 8 addresses your comments.

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:225:
[checkboxes_ removeObjectAtIndex:index];
On 2013/02/08 19:48:22, sail wrote:
> Hm.. is this ok to do while the iterator is still alive?
> Maybe move the for loop into a helper function? Something like:
>   NSButton* checkbox = [self checkboxForID:unique_id];
>   [checkboxes_ removeObject:checkbox]
> 
> You could then use that method in UpdateGallery() as well.

Done.

BTW, I changed the test to forget the gallery1 instead of gallery2. It seems I
need to do more layout here to make it pass. I'm WFH today, so I am relying on
the unit test and haven't visually confirmed the this issue + fix.

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_unittest.mm
(right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_unittest.mm:160:
new_container_height = NSHeight([dialog->checkbox_container_ frame]);
On 2013/02/08 19:48:22, sail wrote:
> this test is a bit long. How about removing the two "should be taller" test
and
> only do the "should be shorter" test. Something like:
> dialog->UpdateGallery(&gallery1, true);
> dialog->UpdateGallery(&gallery2, true);
> CGFloat new_container_height = NSHeight([dialog->checkbox_container_ frame]);
> 
> dialog->ForgetGallery(&gallery2);
> 
> ...

Done.

[sail, 2013-02-08 20:57:28]

LGTM!

https://codereview.chromium.org/12095074/diff/41018/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/12095074/diff/41018/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:228: while (i
< [checkboxes_ count]) {
for (; i < [checkboxes_ count]; ++i) {}

[Lei Zhang, 2013-02-08 21:02:12]

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:284:
InitializePermissions();
On 2013/02/08 20:09:25, Greg Billock wrote:
> Could we basically just re-initialize here, clearing everything? That'd be
> simpler to follow and maintain. Or does that make the dialog flash in a way
> that's annoying?

The dialog UI code has raw pointers to the GalleryPermissions in
|known_galleries_|. Thus if we just clear and repopulate it, the UI would be
pointing to invalid memory.

We could rewrite all the UI to avoid using GalleryPermission pointers, but that
seems like a lot of work.

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_preferences.h:81: virtual void
OnGalleryChanged(MediaGalleriesPreferences* pref,
On 2013/02/08 20:09:25, Greg Billock wrote:
> Should this pass any information about which gallery changed? Or it's expected
> the client will do a full rescan? In that case, do we really need to pass the
> extension id?

It's expecting the client to do a rescan. The situation where having a non-empty
|extension_id| is useful is when:

- extension A and extension B both have permission dialogs open.
- the permission for a gallery in extension A changes.
- checking the extension id allows the dialog controller for extension B to
avoid doing any work.

It could be argued that this is fairly rare, and we don't need the extension id
as an optimization.

[Lei Zhang, 2013-02-09 00:07:42]

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://codereview.chromium.org/12095074/diff/38023/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_preferences.h:81: virtual void
OnGalleryChanged(MediaGalleriesPreferences* pref,
On 2013/02/08 19:57:41, vandebo wrote:
> nit: add comment about empty string when the change isn't extension specific.

Done.

https://codereview.chromium.org/12095074/diff/41018/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/12095074/diff/41018/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:228: while (i
< [checkboxes_ count]) {
On 2013/02/08 20:57:28, sail wrote:
> for (; i < [checkboxes_ count]; ++i) {}

Done.

[Lei Zhang, 2013-02-12 22:10:26]

Evan: PTAL at the GTK and Views code when you get a chance.

[Evan Stade, 2013-02-13 00:18:51]

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc (right):

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc:130:
gtk_widget_set_sensitive(confirm_, FALSE);
why the behavior difference between views and gtk here?

I don't think this is really necessary anyway.

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk_unittest.cc
(right):

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk_unittest.cc:114:
TEST_F(MediaGalleriesDialogTest, ForgetDeletes) {
comment for this test. Why no equivalent test for views?

[Lei Zhang, 2013-02-13 01:51:07]

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc (right):

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc:130:
gtk_widget_set_sensitive(confirm_, FALSE);
On 2013/02/13 00:18:51, Evan Stade wrote:
> why the behavior difference between views and gtk here?
> 
> I don't think this is really necessary anyway.

Ok, I will remove it, probably tomorrow.

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk_unittest.cc
(right):

https://chromiumcodereview.appspot.com/12095074/diff/36020/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk_unittest.cc:114:
TEST_F(MediaGalleriesDialogTest, ForgetDeletes) {
On 2013/02/13 00:18:51, Evan Stade wrote:
> comment for this test. Why no equivalent test for views?

There's no tests for views what so ever.

[Lei Zhang, 2013-02-13 22:38:37]

https://codereview.chromium.org/12095074/diff/36020/chrome/browser/ui/gtk/ext...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc (right):

https://codereview.chromium.org/12095074/diff/36020/chrome/browser/ui/gtk/ext...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.cc:130:
gtk_widget_set_sensitive(confirm_, FALSE);
On 2013/02/13 00:18:51, Evan Stade wrote:
> why the behavior difference between views and gtk here?
> 
> I don't think this is really necessary anyway.

Done.

[Evan Stade, 2013-02-20 00:00:01]

lgtm

[commit-bot: I haz the power, 2013-02-20 00:12:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/12095074/45001

[commit-bot: I haz the power, 2013-02-20 13:26:17]

Change committed as 183502