Bug 209210 - Default attachment icon is not necessarily the same as the file attachment icon whose extension is ".dat"
Summary: Default attachment icon is not necessarily the same as the file attachment ic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-17 19:27 PDT by Said Abou-Hallawa
Modified: 2020-03-18 09:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2020-03-17 19:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (1.69 KB, patch)
2020-03-18 07:53 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-03-17 19:27:15 PDT
The goal is to make fast/attachment/attachment-default-icon.html more reliable. The test page has a default attachment while the expected page has a file attachment. The extension of this file is ".dat". The icon for this file attachment is not necessarily the same as the icon for the default attachment.
Comment 1 Said Abou-Hallawa 2020-03-17 19:31:41 PDT
Created attachment 393813 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-03-17 19:32:47 PDT
<rdar://problem/59606671>
Comment 3 Daniel Bates 2020-03-17 19:52:56 PDT
Comment on attachment 393813 [details]
Patch

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

This patch looks good.

> LayoutTests/ChangeLog:9
> +        returns for âpublic.dataâ icon.

Ok as-is. No change needed. There are some fancy quotes here.
Comment 4 Daniel Bates 2020-03-17 20:11:28 PDT
Comment on attachment 393813 [details]
Patch

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

> LayoutTests/fast/attachment/attachment-default-icon-expected.html:4
> +<attachment id="attachment" title="  " type="public.data"></attachment>

After looking at other attachment tests, I think I understand what this test was going for....it's trying to test the code path for the file attribute matches the code path for the missing type attribute (which yields the default icon). The implicit assumption this test makes is that a .dat file yields the default icon. For this test (and only this test as I haven't studied the others enough,yet) this patch seems reasonable because:

1. There are other tests that actually check that the file attribute  affects the icon
2. Because of (1) this test just needs to focus on testing that the icon is for the missing type attribute is equivalent to the icon for a .dat file.
Comment 5 Daniel Bates 2020-03-17 20:14:05 PDT
Comment on attachment 393813 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +

Ok as-is. No change needed. The optimal ChangeLog includes the radar URL here because:

1. Convenient for Apple engineers
Comment 6 Said Abou-Hallawa 2020-03-18 07:53:37 PDT
Created attachment 393854 [details]
Patch
Comment 7 WebKit Commit Bot 2020-03-18 09:06:56 PDT
The commit-queue encountered the following flaky tests while processing attachment 393854 [details]:

imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-large.html bug 201849
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2020-03-18 09:07:30 PDT
Comment on attachment 393854 [details]
Patch

Clearing flags on attachment: 393854

Committed r258640: <https://trac.webkit.org/changeset/258640>
Comment 9 WebKit Commit Bot 2020-03-18 09:07:32 PDT
All reviewed patches have been landed.  Closing bug.