Bug 155413 – <attachment> should be a runtime-enabled feature

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

<attachment> should be a runtime-enabled feature

[Dean Jackson]

<rdar://problem/25120753>

[Dean Jackson]

Created attachment 273906 [details]
Patch

[Tim Horton]

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!

[Dean Jackson]

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 :)

[Dean Jackson]

(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.

[Dean Jackson]

Created attachment 273908 [details]
Patch

[Dean Jackson]

Created attachment 273911 [details]
Patch

[Dean Jackson]

Created attachment 273912 [details]
Patch

[Dean Jackson]

Created attachment 273913 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

Committed r198088: <http://trac.webkit.org/changeset/198088>

[Csaba Osztrogonác]

(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]);
    ~~~~~~~~ ^

[Csaba Osztrogonác]

fix landed in http://trac.webkit.org/changeset/198328