Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(112)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 134163014: Added tests for views flavor of MediaGalleriesDialogTest. (Closed)

Created:
6 years, 10 months ago by Sheridan Rawlins
Modified:
6 years, 10 months ago
CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org
Visibility:
Public.

Description

Added tests for views flavor of MediaGalleriesDialogTest. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251297

Patch Set 1 #

Patch Set 2 : Added missing unittest file. #

Total comments: 4

Patch Set 3 : Ensure contents_ is freed when its not attached. #

Total comments: 4

Patch Set 4 : Update comment for ownership of contents_ when testing. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -34 lines) Patch
M chrome/browser/ui/views/extensions/media_galleries_dialog_views.h View 1 2 3 2 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc View 1 2 2 chunks +25 lines, -14 lines 0 comments Download
A + chrome/browser/ui/views/extensions/media_galleries_dialog_views_unittest.cc View 1 6 chunks +17 lines, -19 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Sheridan Rawlins
When testing the previous CL, I noticed there were no tests for views. Added this ...
6 years, 10 months ago (2014-02-11 02:59:51 UTC) #1
vandebo (ex-Chrome)
LGTM after comments. You'll also want a views owner. https://codereview.chromium.org/134163014/diff/30001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/134163014/diff/30001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc#newcode86 chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:86: ...
6 years, 10 months ago (2014-02-11 18:26:24 UTC) #2
Sheridan Rawlins
Sky, can you review for views?
6 years, 10 months ago (2014-02-13 04:55:52 UTC) #3
Sheridan Rawlins
https://codereview.chromium.org/134163014/diff/30001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/134163014/diff/30001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc#newcode86 chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:86: // TODO(scr): Ensure contents_ is really deleted. On 2014/02/11 ...
6 years, 10 months ago (2014-02-13 04:57:19 UTC) #4
sky
LGTM https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right): https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h#newcode91 chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:91: // The contents of the dialog. Owned by ...
6 years, 10 months ago (2014-02-13 17:41:46 UTC) #5
vandebo (ex-Chrome)
https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc#newcode91 chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:91: web_contents_modal_dialog_manager->delegate(); nit: indent
6 years, 10 months ago (2014-02-13 20:33:54 UTC) #6
vandebo (ex-Chrome)
https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right): https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc#newcode91 chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:91: web_contents_modal_dialog_manager->delegate(); On 2014/02/13 20:33:55, vandebo wrote: > nit: indent ...
6 years, 10 months ago (2014-02-13 20:49:38 UTC) #7
Sheridan Rawlins
https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right): https://codereview.chromium.org/134163014/diff/100001/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h#newcode91 chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:91: // The contents of the dialog. Owned by |window_|'s ...
6 years, 10 months ago (2014-02-13 20:53:57 UTC) #8
Sheridan Rawlins
The CQ bit was checked by scr@chromium.org
6 years, 10 months ago (2014-02-13 20:54:10 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scr@chromium.org/134163014/200001
6 years, 10 months ago (2014-02-13 20:56:55 UTC) #10
commit-bot: I haz the power
6 years, 10 months ago (2014-02-14 13:53:21 UTC) #11
Message was sent while issue was closed. 
Change committed as 251297

Powered by Google App Engine
This is Rietveld 408576698