Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1047243002:
Add initial browsertest of download notification (Closed)
DescriptionAdd initial browsertest of download notification
This patch introduces the basic browser tests of download notification.
BUG=480489
TEST=run newly added browsertest
Committed: https://crrev.com/c6f2f80dfb217d42ffe7e28ea26fb4e0d2a5459f
Cr-Commit-Position: refs/heads/master@{#326760}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments #Patch Set 3 : Make the test runs only in chromeos build #
Total comments: 1
Messages
Total messages: 23 (10 generated)
yoshiki@chromium.org changed reviewers: + benjhayden@chromium.org
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
benjhayden, could you take a look? Thanks.
On 2015/04/17 at 23:29:15, yoshiki wrote: > benjhayden, could you take a look? Thanks. Asanka will be back on Monday. I haven't been keeping up with download notifications.
yoshiki@chromium.org changed reviewers: + asanka@chromium.org
Asanka, could you take a look? Thanks. Ben, thank you for quick reply!
Additional tests to consider: * Incognito profile. * Download removed externally (I.e. someone calls DownloadItem::Remove()). * Hidden downloads (those that shouldn't be shown to the user via this UI. * Dangerous downloads. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:109: base::MessageLoopForUI::current()->Quit(); You should either run closure from base::RunLoop::QuitClosure() or keep run base::RunLoop::Quit() instead of invoking Quit() on the current message loop. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:116: bool waiting_; You can now initialize member variables right here instead of initializing them in each constructor. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:145: base::MessageLoopForUI::current()->Quit(); Ditto. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:224: // Call parent method. No need to comment this. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:236: GURL file(net::URLRequestSlowDownloadJob::kUnknownSizeUrl); file? https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:314: GetDownloadManager(browser())->GetAllDownloads(&downloads); Note that we don't give any guarantees that the ordering is preserved across calls to GetAllDownloads(). Rather than capturing download1 from the previous call to GetAllDownloads(), I'd suggest capturing both download1 and download2 here. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:457: content::DownloadItem* download3 = downloads[1]; Ditto.
Asanka, PTAL at the patch? I agree to the needs of more tests. Let me add the additional browser tests later. I filed the task: crbug.com/480489. I'd like to add this framework and the basic tests in advance, to make it easier to add new tests for coming changes of download notification. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:109: base::MessageLoopForUI::current()->Quit(); On 2015/04/22 21:45:43, asanka wrote: > You should either run closure from base::RunLoop::QuitClosure() or keep run > base::RunLoop::Quit() instead of invoking Quit() on the current message loop. Done. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:116: bool waiting_; On 2015/04/22 21:45:43, asanka wrote: > You can now initialize member variables right here instead of initializing them > in each constructor. I didn't know that we can use C++11 feature. Done. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:145: base::MessageLoopForUI::current()->Quit(); On 2015/04/22 21:45:43, asanka wrote: > Ditto. Done. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:224: // Call parent method. On 2015/04/22 21:45:43, asanka wrote: > No need to comment this. Done. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:236: GURL file(net::URLRequestSlowDownloadJob::kUnknownSizeUrl); On 2015/04/22 21:45:43, asanka wrote: > file? s/file/url/g https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:314: GetDownloadManager(browser())->GetAllDownloads(&downloads); On 2015/04/22 21:45:43, asanka wrote: > Note that we don't give any guarantees that the ordering is preserved across > calls to GetAllDownloads(). Rather than capturing download1 from the previous > call to GetAllDownloads(), I'd suggest capturing both download1 and download2 > here. Done. https://codereview.chromium.org/1047243002/diff/60001/chrome/browser/download... chrome/browser/download/notification/download_notification_browsertest.cc:457: content::DownloadItem* download3 = downloads[1]; On 2015/04/22 21:45:43, asanka wrote: > Ditto. Done.
lgtm
thank you for reviewing. I'm going to commit
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047243002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1047243002/#ps100001 (title: "Make the test runs only in chromeos build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1047243002/100001
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c6f2f80dfb217d42ffe7e28ea26fb4e0d2a5459f Cr-Commit-Position: refs/heads/master@{#326760}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1047243002/diff/100001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_browsertest.cc (right): https://codereview.chromium.org/1047243002/diff/100001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_browsertest.cc:132: MessageCenterChangeObserver(); This is not how you call a superclass constructor. This creates an unnamed temporary of type MessageCenterChangeObserver. The superclass constructor is called automatically; if you want to call it explicitly put it in the initializer list (see http://stackoverflow.com/questions/6923722/how-do-i-call-the-base-class-const... – also in 3 more places in this file) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
