Issue 310383004: Make a controller interface for media galleries dialogs.

Issue 310383004: Make a controller interface for media galleries dialogs. (Closed)

Created:
6 years, 6 months ago by vandebo (ex-Chrome)

Modified:
6 years, 6 months ago

Reviewers:
Lei Zhang

CC:
chromium-reviews, extensions-reviews_chromium.org, vandebo (ex-Chrome), Lei Zhang, tfarina, tommycli, Greg Billock, chromium-apps-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Make a controller interface for media galleries dialogs. Abstract the media galleries controller interfaces (permissions and scan result dialogs) and use it for the permission dialog. Scan result dialog will use it in a follow up CL: https://codereview.chromium.org/318733003/ BUG=NONE Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275486

Patch Set 1 #

Total comments: 24

Patch Set 2 : Comments, spelling, test fixes #

Total comments: 2

Patch Set 3 : nit #

Patch Set 4 : Rebase and nit #

Patch Set 5 : Fix rebase #

Patch Set 6 : rebase #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+523 lines, -1556 lines)			Patch
M	chrome/browser/extensions/api/media_galleries/media_galleries_api.cc	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/media_galleries/media_galleries_dialog_controller.h	View	1 2	2 chunks	+42 lines, -201 lines	0 comments	Download
M	chrome/browser/media_galleries/media_galleries_dialog_controller.cc	View	1 2 3 4 5	1 chunk	+3 lines, -506 lines	0 comments	Download
M	chrome/browser/media_galleries/media_galleries_dialog_controller_mock.h	View	1	1 chunk	+12 lines, -8 lines	0 comments	Download
M	chrome/browser/media_galleries/media_galleries_dialog_controller_mock.cc	View	1	1 chunk	+13 lines, -19 lines	0 comments	Download
D	chrome/browser/media_galleries/media_galleries_dialog_controller_unittest.cc	View		1 chunk	+0 lines, -322 lines	0 comments	Download
A +	chrome/browser/media_galleries/media_galleries_permission_controller.h	View	1	5 chunks	+38 lines, -87 lines	0 comments	Download
A +	chrome/browser/media_galleries/media_galleries_permission_controller.cc	View	1 2 3 4 5	25 chunks	+117 lines, -81 lines	0 comments	Download
A +	chrome/browser/media_galleries/media_galleries_permission_controller_unittest.cc	View		9 chunks	+31 lines, -31 lines	0 comments	Download
M	chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h	View	1	5 chunks	+10 lines, -14 lines	0 comments	Download
M	chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm	View	1	11 chunks	+51 lines, -42 lines	0 comments	Download
M	chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_browsertest.mm	View	1	3 chunks	+14 lines, -14 lines	0 comments	Download
M	chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa_unittest.mm	View		4 chunks	+76 lines, -95 lines	0 comments	Download
M	chrome/browser/ui/views/extensions/media_galleries_dialog_views.h	View	1 2 3 4 5	2 chunks	+6 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc	View	1 2 3 4 5	10 chunks	+64 lines, -62 lines	0 comments	Download
M	chrome/browser/ui/views/extensions/media_galleries_dialog_views_unittest.cc	View	1	5 chunks	+41 lines, -65 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

Lei Zhang

https://codereview.chromium.org/310383004/diff/1/chrome/browser/media_galleries/media_galleries_dialog_controller.h File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/310383004/diff/1/chrome/browser/media_galleries/media_galleries_dialog_controller.h#newcode65 chrome/browser/media_galleries/media_galleries_dialog_controller.h:65: virtual bool ShowFolderViewer(const Entry& entry) const = 0; ShouldShowFolderViewer ...

6 years, 6 months ago (2014-06-05 07:56:24 UTC) #2

vandebo (ex-Chrome)

6 years, 6 months ago (2014-06-05 19:25:30 UTC) #3

Lei Zhang

lgtm https://codereview.chromium.org/310383004/diff/20001/chrome/browser/media_galleries/media_galleries_dialog_controller.h File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/310383004/diff/20001/chrome/browser/media_galleries/media_galleries_dialog_controller.h#newcode70 chrome/browser/media_galleries/media_galleries_dialog_controller.h:70: // Get the set of permissions for the ...

6 years, 6 months ago (2014-06-05 19:51:10 UTC) #4

vandebo (ex-Chrome)

https://codereview.chromium.org/310383004/diff/20001/chrome/browser/media_galleries/media_galleries_dialog_controller.h File chrome/browser/media_galleries/media_galleries_dialog_controller.h (right): https://codereview.chromium.org/310383004/diff/20001/chrome/browser/media_galleries/media_galleries_dialog_controller.h#newcode70 chrome/browser/media_galleries/media_galleries_dialog_controller.h:70: // Get the set of permissions for the |index|th ...

6 years, 6 months ago (2014-06-05 21:22:51 UTC) #5

vandebo (ex-Chrome)

The CQ bit was checked by vandebo@chromium.org

6 years, 6 months ago (2014-06-05 21:22:57 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/310383004/60001

6 years, 6 months ago (2014-06-05 21:25:52 UTC) #7

vandebo (ex-Chrome)

The CQ bit was checked by vandebo@chromium.org

6 years, 6 months ago (2014-06-05 23:10:53 UTC) #8

commit-bot: I haz the power

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

6 years, 6 months ago (2014-06-05 23:11:27 UTC) #9

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-06 03:31:49 UTC) #10

commit-bot: I haz the power

Failed to apply patch for chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 6 months ago (2014-06-06 03:31:50 UTC) #11

Failed to apply patch for
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
  Hunk #1 succeeded at 14 (offset -2 lines).
  Hunk #2 FAILED at 63.
  Hunk #3 FAILED at 81.
  Hunk #4 succeeded at 81 (offset -20 lines).
  Hunk #5 succeeded at 119 (offset -20 lines).
  Hunk #6 succeeded at 172 (offset -20 lines).
  Hunk #7 succeeded at 225 (offset -20 lines).
  Hunk #8 succeeded at 240 (offset -24 lines).
  Hunk #9 succeeded at 265 (offset -25 lines).
  Hunk #10 succeeded at 299 (offset -25 lines).
  Hunk #11 succeeded at 316 (offset -25 lines).
  2 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc.rej

Patch:       chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
Index: chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
diff --git a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
index
0eb28901cba2c586dd7f52e4293c833957d44a95..90a0e3582373aac009daf2e1893f47fbc60c7818
100644
--- a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
+++ b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
@@ -16,6 +16,7 @@
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/checkbox.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/menu/menu_runner.h"
@@ -62,15 +63,12 @@ void ScrollableView::Layout() {
 
 }  // namespace
 
-typedef MediaGalleriesDialogController::GalleryPermissionsVector
-    GalleryPermissionsVector;
-
 MediaGalleriesDialogViews::MediaGalleriesDialogViews(
     MediaGalleriesDialogController* controller)
     : controller_(controller),
       window_(NULL),
       contents_(new views::View()),
-      add_gallery_button_(NULL),
+      auxiliary_button_(NULL),
       confirm_available_(false),
       accepted_(false) {
   InitChildViews();
@@ -80,7 +78,7 @@ MediaGalleriesDialogViews::MediaGalleriesDialogViews(
     // care of deleting itself after calling DeleteDelegate().
     WebContentsModalDialogManager* web_contents_modal_dialog_manager =
         WebContentsModalDialogManager::FromWebContents(
-            controller->web_contents());
+            controller->WebContents());
     DCHECK(web_contents_modal_dialog_manager);
     WebContentsModalDialogManagerDelegate* modal_delegate =
         web_contents_modal_dialog_manager->delegate();
@@ -100,6 +98,8 @@ MediaGalleriesDialogViews::~MediaGalleriesDialogViews() {
 void MediaGalleriesDialogViews::InitChildViews() {
   // Outer dialog layout.
   contents_->RemoveAllChildViews(true);
+  checkbox_map_.clear();
+
   int dialog_content_width = views::Widget::GetLocalizedContentsWidth(
       IDS_MEDIA_GALLERIES_DIALOG_CONTENT_WIDTH_CHARS);
   views::GridLayout* layout = views::GridLayout::CreatePanel(contents_);
@@ -136,43 +136,36 @@ void MediaGalleriesDialogViews::InitChildViews() {
                                        views::kRelatedControlVerticalSpacing,
                                        0));
 
-  // Add attached galleries checkboxes.
-  checkbox_map_.clear();
-  GalleryPermissionsVector permissions = controller_->AttachedPermissions();
-  for (GalleryPermissionsVector::const_iterator iter = permissions.begin();
-       iter != permissions.end(); ++iter) {
-    int spacing = 0;
-    if (iter + 1 == permissions.end())
-      spacing = views::kRelatedControlSmallVerticalSpacing;
-    AddOrUpdateGallery(*iter, scroll_container, spacing);
-  }
+  std::vector<base::string16> section_headers =
+      controller_->GetSectionHeaders();
+  for (size_t i = 0; i < section_headers.size(); i++) {
+    MediaGalleriesDialogController::Entries entries =
+        controller_->GetSectionEntries(i);
+
+    // Header and separator line.
+    if (!section_headers[i].empty() && !entries.empty()) {
+      views::Separator* separator = new views::Separator(
+          views::Separator::HORIZONTAL);
+      scroll_container->AddChildView(separator);
+
+      views::Label* header = new views::Label(section_headers[i]);
+      header->SetMultiLine(true);
+      header->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      header->SetBorder(views::Border::CreateEmptyBorder(
+          views::kRelatedControlVerticalSpacing,
+          views::kPanelHorizMargin,
+          views::kRelatedControlVerticalSpacing,
+          0));
+      scroll_container->AddChildView(header);
+    }
 
-  GalleryPermissionsVector unattached_permissions =
-      controller_->UnattachedPermissions();
-
-  if (!unattached_permissions.empty()) {
-    // Separator line.
-    views::Separator* separator = new views::Separator(
-        views::Separator::HORIZONTAL);
-    scroll_container->AddChildView(separator);
-
-    // Unattached locations section.
-    views::Label* unattached_text = new views::Label(
-        controller_->GetUnattachedLocationsHeader());
-    unattached_text->SetMultiLine(true);
-    unattached_text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-    unattached_text->SetBorder(
-        views::Border::CreateEmptyBorder(views::kRelatedControlVerticalSpacing,
-                                         views::kPanelHorizMargin,
-                                         views::kRelatedControlVerticalSpacing,
-                                         0));
-    scroll_container->AddChildView(unattached_text);
-
-    // Add unattached galleries checkboxes.
-    for (GalleryPermissionsVector::const_iterator iter =
-             unattached_permissions.begin();
-         iter != unattached_permissions.end(); ++iter) {
-      AddOrUpdateGallery(*iter, scroll_container, 0);
+    // Checkboxes.
+    MediaGalleriesDialogController::Entries::const_iterator iter;
+    for (iter = entries.begin(); iter != entries.end(); ++iter) {
+      int spacing = 0;
+      if (iter + 1 == entries.end())
+        spacing = views::kRelatedControlSmallVerticalSpacing;
+      AddOrUpdateGallery(*iter, scroll_container, spacing);
     }
   }
 
@@ -196,30 +189,33 @@ void MediaGalleriesDialogViews::UpdateGalleries() {
 }
 
 bool MediaGalleriesDialogViews::AddOrUpdateGallery(
-    const MediaGalleriesDialogController::GalleryPermission& gallery,
+    const MediaGalleriesDialogController::Entry& gallery,
     views::View* container,
     int trailing_vertical_space) {
   base::string16 label = gallery.pref_info.GetGalleryDisplayName();
   base::string16 tooltip_text = gallery.pref_info.GetGalleryTooltip();
   base::string16 details = gallery.pref_info.GetGalleryAdditionalDetails();
+  bool show_folder_viewer = controller_->ShouldShowFolderViewer(gallery);
 
-  CheckboxMap::iterator iter = checkbox_map_.find(gallery.gallery_id);
+  CheckboxMap::iterator iter = checkbox_map_.find(gallery.pref_info.pref_id);
   if (iter != checkbox_map_.end()) {
     views::Checkbox* checkbox = iter->second->checkbox();
-    checkbox->SetChecked(gallery.allowed);
+    checkbox->SetChecked(gallery.selected);
     checkbox->SetText(label);
     checkbox->SetTooltipText(tooltip_text);
     iter->second->secondary_text()->SetText(details);
     iter->second->secondary_text()->SetVisible(details.length() > 0);
+    iter->second->folder_viewer_button()->SetVisible(show_folder_viewer);
     return false;
   }
 
   MediaGalleryCheckboxView* gallery_view =
-      new MediaGalleryCheckboxView(label, tooltip_text, details, false,
-                                   trailing_vertical_space, this, this);
-  gallery_view->checkbox()->SetChecked(gallery.allowed);
+      new MediaGalleryCheckboxView(label, tooltip_text, details,
+                                   show_folder_viewer, trailing_vertical_space,
+                                   this, this);
+  gallery_view->checkbox()->SetChecked(gallery.selected);
   container->AddChildView(gallery_view);
-  checkbox_map_[gallery.gallery_id] = gallery_view;
+  checkbox_map_[gallery.pref_info.pref_id] = gallery_view;
 
   return true;
 }
@@ -246,9 +242,9 @@ views::View* MediaGalleriesDialogViews::GetContentsView() {
 
 base::string16 MediaGalleriesDialogViews::GetDialogButtonLabel(
     ui::DialogButton button) const {
-  return l10n_util::GetStringUTF16(button == ui::DIALOG_BUTTON_OK ?
-      IDS_MEDIA_GALLERIES_DIALOG_CONFIRM :
-      IDS_MEDIA_GALLERIES_DIALOG_CANCEL);
+  if (button == ui::DIALOG_BUTTON_OK)
+    return controller_->GetAcceptButtonText();
+  return l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_CANCEL);
 }
 
 bool MediaGalleriesDialogViews::IsDialogButtonEnabled(
@@ -265,11 +261,13 @@ ui::ModalType MediaGalleriesDialogViews::GetModalType()
const {
 }
 
 views::View* MediaGalleriesDialogViews::CreateExtraView() {
-  DCHECK(!add_gallery_button_);
-  add_gallery_button_ = new views::LabelButton(this,
-      l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_ADD_GALLERY));
-  add_gallery_button_->SetStyle(views::Button::STYLE_BUTTON);
-  return add_gallery_button_;
+  DCHECK(!auxiliary_button_);
+  base::string16 button_label = controller_->GetAuxiliaryButtonText();
+  if (!button_label.empty()) {
+    auxiliary_button_ = new views::LabelButton(this, button_label);
+    auxiliary_button_->SetStyle(views::Button::STYLE_BUTTON);
+  }
+  return auxiliary_button_;
 }
 
 bool MediaGalleriesDialogViews::Cancel() {
@@ -289,16 +287,20 @@ void
MediaGalleriesDialogViews::ButtonPressed(views::Button* sender,
   if (ControllerHasWebContents())
     GetWidget()->client_view()->AsDialogClientView()->UpdateDialogButtons();
 
-  if (sender == add_gallery_button_) {
-    controller_->OnAddFolderClicked();
+  if (sender == auxiliary_button_) {
+    controller_->DidClickAuxiliaryButton();
     return;
   }
 
   for (CheckboxMap::const_iterator…
(message too large)

vandebo (ex-Chrome)

The CQ bit was checked by vandebo@chromium.org

6 years, 6 months ago (2014-06-06 15:59:05 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/310383004/100001

6 years, 6 months ago (2014-06-06 16:00:38 UTC) #13

Message was sent while issue was closed.

Change committed as 275486

Expand Messages | Collapse Messages