content: remove NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED

content: remove NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED

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

[jam, 2013-01-18 22:45:33]

the notification changes lg

I'm not sure i see much value in WindowedObserver at this point. i'm hesitant to
add new classes/methods in the public test directories because it makes finding
the existing ones harder. really this is just wrapping message loop run and
quit.

[jam, 2013-01-18 22:49:56]

https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
content/browser/download/download_manager_impl.h:50: void
OnSavePackageSuccessfullyFinished(DownloadItem* download_item);
nit: add comment

[Paweł Hajdan Jr., 2013-01-18 23:10:16]

Eventually WindowedNotificationObserver would disappear, or at least move out of
content.

As test OWNER I find this valuable. There are some pieces there that are easy to
get wrong in subtle ways, and can also help with possible further simplification
of WindowedObserver.

PTAL.

https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
content/browser/download/download_manager_impl.h:50: void
OnSavePackageSuccessfullyFinished(DownloadItem* download_item);
On 2013/01/18 22:49:56, John Abd-El-Malek wrote:
> nit: add comment

Done.

[jam, 2013-01-22 01:21:57]

I think for notifications WindowedNotificationObserver was helpful because there
was one way of registering for all of them. In the generic class, not all
classes have an AddObserver/RemoveObserver, or sometimes they exist and they're
private (i.e. for RenderViewObserver, RenderViewHostObserver,
WebContentsObserver). other classes use methods that take a callback. so this
why i don't think this is going to be a helpful generic class, even though this
specific instance makes it look like it'll be common.

On 2013/01/18 23:10:16, Paweł Hajdan Jr. wrote:
> Eventually WindowedNotificationObserver would disappear, or at least move out
of
> content.
> 
> As test OWNER I find this valuable. There are some pieces there that are easy
to
> get wrong in subtle ways, and can also help with possible further
simplification
> of WindowedObserver.
> 
> PTAL.
> 
>
https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
> File content/browser/download/download_manager_impl.h (right):
> 
>
https://codereview.chromium.org/11867023/diff/1/content/browser/download/down...
> content/browser/download/download_manager_impl.h:50: void
> OnSavePackageSuccessfullyFinished(DownloadItem* download_item);
> On 2013/01/18 22:49:56, John Abd-El-Malek wrote:
> > nit: add comment
> 
> Done.

[Paweł Hajdan Jr., 2013-01-22 22:49:09]

PTAL

[jam, 2013-01-23 01:30:28]

lgtm

https://codereview.chromium.org/11867023/diff/10001/content/public/test/test_...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/11867023/diff/10001/content/public/test/test_...
content/public/test/test_utils.h:52: void Run();
nit: please add a small comment here saying that the message loop won't run if
the closure

[Paweł Hajdan Jr., 2013-01-23 01:48:03]

https://codereview.chromium.org/11867023/diff/10001/content/public/test/test_...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/11867023/diff/10001/content/public/test/test_...
content/public/test/test_utils.h:52: void Run();
On 2013/01/23 01:30:28, John Abd-El-Malek wrote:
> nit: please add a small comment here saying that the message loop won't run if
> the closure 

Done.

[jam, 2013-01-23 05:25:31]

https://codereview.chromium.org/11867023/diff/15002/content/public/test/test_...
File content/public/test/test_utils.cc (right):

https://codereview.chromium.org/11867023/diff/15002/content/public/test/test_...
content/public/test/test_utils.cc:118: quit_closure_called_ = true;
actually, I think you need to check here as well to make sure that the next line
doesn't execute if Run() hasn't been called.

[Jói, 2013-01-23 10:21:24]

Hi guys,

I took a quick look at the NotificationObserver idea and I agree it might not
end up as general as that, so makes sense to skip it for now and see.

Did we completely outlaw gMock?  I ask because for pure-virtual delegate
interfaces, it is a pretty ideal tool for implementing them in tests.

Cheers,
Jói

[Paweł Hajdan Jr., 2013-01-23 17:41:40]

https://codereview.chromium.org/11867023/diff/15002/content/public/test/test_...
File content/public/test/test_utils.cc (right):

https://codereview.chromium.org/11867023/diff/15002/content/public/test/test_...
content/public/test/test_utils.cc:118: quit_closure_called_ = true;
On 2013/01/23 05:25:31, John Abd-El-Malek wrote:
> actually, I think you need to check here as well to make sure that the next
line
> doesn't execute if Run() hasn't been called.

Done.

[Paweł Hajdan Jr., 2013-01-28 09:19:49]

On 2013/01/23 10:21:24, Jói wrote:
> Did we completely outlaw gMock?  I ask because for pure-virtual delegate
> interfaces, it is a pretty ideal tool for implementing them in tests.

I think we didn't. Maybe I should give it a try then for following changes.

[jam, 2013-01-29 03:17:32]

On Mon, Jan 28, 2013 at 1:19 AM, <phajdan.jr@chromium.org> wrote:

> On 2013/01/23 10:21:24, Jói wrote:
>
>> Did we completely outlaw gMock?  I ask because for pure-virtual delegate
>> interfaces, it is a pretty ideal tool for implementing them in tests.
>>
>
> I think we didn't. Maybe I should give it a try then for following changes.
>

i would strongly prefer not introducing more usage of gmock in src\content.


>
>
https://codereview.chromium.**org/11867023/<https://codereview.chromium.org/1...
>