Bug 155413 - <attachment> should be a runtime-enabled feature
Summary: <attachment> should be a runtime-enabled feature
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: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-13 16:20 PDT by Dean Jackson
Modified: 2016-03-17 06:01 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.35 KB, patch)
2016-03-13 16:43 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2016-03-13 17:21 PDT, Dean Jackson
sam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (26.82 KB, patch)
2016-03-13 17:49 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (28.29 KB, patch)
2016-03-13 17:55 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (28.32 KB, patch)
2016-03-13 18:01 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (763.64 KB, application/zip)
2016-03-13 18:07 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2016-03-13 16:20:29 PDT
<attachment> should be a runtime-enabled feature
Comment 1 Dean Jackson 2016-03-13 16:38:23 PDT
<rdar://problem/25120753>
Comment 2 Dean Jackson 2016-03-13 16:43:59 PDT
Created attachment 273906 [details]
Patch
Comment 3 Tim Horton 2016-03-13 16:50:14 PDT
Comment on attachment 273906 [details]
Patch

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

Should we have a test for the default behavior of each of these flags? (and to make sure that it actually disables it? since surely we have tests that enable it).

Also, are you planning on writing the code to make the existing tests tests turn it on when you switch the default to NO?

> Source/WebKit/mac/WebView/WebView.mm:2518
> +    settings.setAttachmentElementEnabled([preferences attachmentElementEnabled]);

So there was already a WebCore setting? I wonder how well it works!
Comment 4 Dean Jackson 2016-03-13 17:18:36 PDT
Comment on attachment 273906 [details]
Patch

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

>> Source/WebKit/mac/WebView/WebView.mm:2518
>> +    settings.setAttachmentElementEnabled([preferences attachmentElementEnabled]);
> 
> So there was already a WebCore setting? I wonder how well it works!

It works in my testing :)
Comment 5 Dean Jackson 2016-03-13 17:19:36 PDT
(In reply to comment #3)
> Comment on attachment 273906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273906&action=review
> 
> Should we have a test for the default behavior of each of these flags? (and
> to make sure that it actually disables it? since surely we have tests that
> enable it).
> 
> Also, are you planning on writing the code to make the existing tests tests
> turn it on when you switch the default to NO?

Actually a new patch is coming that does both of these. You already wrote a test to exercise disabling. That's just now the default.

Watch this space.
Comment 6 Dean Jackson 2016-03-13 17:21:22 PDT
Created attachment 273908 [details]
Patch
Comment 7 Dean Jackson 2016-03-13 17:49:41 PDT
Created attachment 273911 [details]
Patch
Comment 8 Dean Jackson 2016-03-13 17:55:17 PDT
Created attachment 273912 [details]
Patch
Comment 9 Dean Jackson 2016-03-13 18:01:01 PDT
Created attachment 273913 [details]
Patch
Comment 10 Build Bot 2016-03-13 18:06:58 PDT
Comment on attachment 273908 [details]
Patch

Attachment 273908 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/973748

New failing tests:
editing/pasteboard/copy-paste-attachment.html
editing/pasteboard/drag-and-drop-attachment-contenteditable.html
Comment 11 Build Bot 2016-03-13 18:07:01 PDT
Created attachment 273914 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Dean Jackson 2016-03-13 18:09:31 PDT
Committed r198088: <http://trac.webkit.org/changeset/198088>
Comment 13 Csaba Osztrogonác 2016-03-17 06:01:22 PDT
(In reply to comment #12)
> Committed r198088: <http://trac.webkit.org/changeset/198088>

It broke the Apple Mac cmake build:
/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit/mac/WebView/WebView.mm:2515:14: error: no member named 'setAttachmentElementEnabled' in 'WebCore::Settings'
    settings.setAttachmentElementEnabled([preferences attachmentElementEnabled]);
    ~~~~~~~~ ^
Comment 14 Csaba Osztrogonác 2016-03-17 06:01:51 PDT
fix landed in http://trac.webkit.org/changeset/198328