Bug 194294 – Add a new DownloadMap type that manages taking an assertion automatically

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Add a new DownloadMap type that manages taking an assertion automatically

This is to prevent mistakes in the future similar to https://bugs.webkit.org/show_bug.cgi?id=194239#c8

[Brady Eidson]

Created attachment 361200 [details]
Patch

[Brady Eidson]

Created attachment 361203 [details]
Patch

[Alex Christensen]

Comment on attachment 361203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361203&action=review

> Source/WebKit/NetworkProcess/Downloads/DownloadMap.h:40
> +#if !ENABLE(TAKE_DOWNLOAD_ASSERTION)
> +typedef HashMap<DownloadID, std::unique_ptr<Download>> DownloadMap;
> +#else
> +
> +class DownloadMap {

I think it would be nicer to always use the DownloadMap class, and only protect the parts about assertion taking with ENABLE(TAKE_DOWNLOAD_ASSERTION)

[Brady Eidson]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 361203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361203&action=review
> 
> > Source/WebKit/NetworkProcess/Downloads/DownloadMap.h:40
> > +#if !ENABLE(TAKE_DOWNLOAD_ASSERTION)
> > +typedef HashMap<DownloadID, std::unique_ptr<Download>> DownloadMap;
> > +#else
> > +
> > +class DownloadMap {
> 
> I think it would be nicer to always use the DownloadMap class, and only
> protect the parts about assertion taking with ENABLE(TAKE_DOWNLOAD_ASSERTION)

Why? Always using it adds to size and runtime unnecessarily.

[Alex Christensen]

Comment on attachment 361203 [details]
Patch

That way we will be able to add functions to DownloadMap that are not in HashMap in the future.  We can also cross that bridge if/when we come to it.  r=me.

[WebKit Commit Bot]

Comment on attachment 361203 [details]
Patch

Clearing flags on attachment: 361203

Committed r240990: <https://trac.webkit.org/changeset/240990>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47829082>