Bug 194294 - Add a new DownloadMap type that manages taking an assertion automatically
Summary: Add a new DownloadMap type that manages taking an assertion automatically
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-05 10:51 PST by Brady Eidson
Modified: 2019-02-05 13:28 PST (History)
4 users (show)

See Also:

Attachments
Patch (16.07 KB, text/plain)
2019-02-05 10:53 PST, Brady Eidson 		no flags Details
Patch (14.93 KB, patch)
2019-02-05 11:04 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2019-02-05 10:51:20 PST 
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
Comment 1 Brady Eidson 2019-02-05 10:53:58 PST 
Created attachment 361200 [details]
Patch
Comment 2 Brady Eidson 2019-02-05 11:04:45 PST 
Created attachment 361203 [details]
Patch
Comment 3 Alex Christensen 2019-02-05 11:59:16 PST 
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)
Comment 4 Brady Eidson 2019-02-05 12:27:24 PST 
(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.
Comment 5 Alex Christensen 2019-02-05 12:28:30 PST 
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.
Comment 6 WebKit Commit Bot 2019-02-05 13:27:27 PST 
Comment on attachment 361203 [details]
Patch

Clearing flags on attachment: 361203

Committed r240990: <https://trac.webkit.org/changeset/240990>
Comment 7 WebKit Commit Bot 2019-02-05 13:27:28 PST 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-02-05 13:28:30 PST 
<rdar://problem/47829082>