Bug 142023 - Implement <attachment> element appearance on Mac
Summary: Implement <attachment> element appearance on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-25 13:38 PST by Tim Horton
Modified: 2015-02-26 18:08 PST (History)
10 users (show)

See Also:


Attachments
Patch (27.09 KB, patch)
2015-02-25 13:39 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (590.81 KB, application/zip)
2015-02-25 13:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (709.54 KB, application/zip)
2015-02-25 14:35 PST, Build Bot
no flags Details
Patch (49.03 KB, patch)
2015-02-25 15:44 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (26.77 KB, patch)
2015-02-25 17:58 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (673.69 KB, application/zip)
2015-02-25 18:24 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (524.80 KB, application/zip)
2015-02-25 18:46 PST, Build Bot
no flags Details
Patch (55.50 KB, patch)
2015-02-25 20:01 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2015-02-26 14:44 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-02-25 13:38:30 PST
Implement <attachment> element appearance on Mac
Comment 1 Tim Horton 2015-02-25 13:39:06 PST
Created attachment 247338 [details]
Patch
Comment 2 WebKit Commit Bot 2015-02-25 13:42:24 PST
Attachment 247338 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "URL" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  _LSBindingCreateWithURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2015-02-25 13:57:34 PST
Comment on attachment 247338 [details]
Patch

Attachment 247338 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5561068952748032

New failing tests:
fast/attachment/attachment-rendering.html
Comment 4 Build Bot 2015-02-25 13:57:38 PST
Created attachment 247345 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-02-25 14:35:42 PST
Comment on attachment 247338 [details]
Patch

Attachment 247338 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6332820888748032

New failing tests:
fast/attachment/attachment-rendering.html
Comment 6 Build Bot 2015-02-25 14:35:45 PST
Created attachment 247348 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Tim Horton 2015-02-25 15:44:24 PST
Created attachment 247354 [details]
Patch
Comment 8 WebKit Commit Bot 2015-02-25 15:47:17 PST
Attachment 247354 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "URL" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  _LSBindingCreateWithURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Tim Horton 2015-02-25 17:58:58 PST
Created attachment 247376 [details]
Patch
Comment 10 WebKit Commit Bot 2015-02-25 18:01:29 PST
Attachment 247376 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "URL" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  _LSBindingCreateWithURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Build Bot 2015-02-25 18:24:53 PST
Comment on attachment 247376 [details]
Patch

Attachment 247376 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6394601845817344

New failing tests:
fast/attachment/attachment-rendering.html
Comment 12 Build Bot 2015-02-25 18:24:56 PST
Created attachment 247383 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 13 Build Bot 2015-02-25 18:46:11 PST
Comment on attachment 247376 [details]
Patch

Attachment 247376 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5695077837963264

New failing tests:
fast/attachment/attachment-rendering.html
Comment 14 Build Bot 2015-02-25 18:46:14 PST
Created attachment 247388 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 15 Tim Horton 2015-02-25 20:01:23 PST
Created attachment 247397 [details]
Patch
Comment 16 WebKit Commit Bot 2015-02-25 20:03:41 PST
Attachment 247397 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  The parameter name "URL" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  _LSBindingCreateWithURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Enrica Casucci 2015-02-26 09:41:09 PST
Comment on attachment 247397 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:811
>              return true;

I think you need #if ENABLE(ATTACHMENT_ELEMENT)

> Source/WebCore/rendering/RenderThemeMac.h:174
>  private:

You probably need #if ENABLE(ATTACHMENT_ELEMENT)
Comment 18 Enrica Casucci 2015-02-26 10:06:10 PST
Comment on attachment 247397 [details]
Patch

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

> Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34
> +LSBindingRef _LSBindingCreateWithURL(CFAllocatorRef allocator, CFURLRef URL);

No need for parameter names.
Comment 19 Tim Horton 2015-02-26 13:15:47 PST
Comment on attachment 247397 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:811
>>              return true;
> 
> I think you need #if ENABLE(ATTACHMENT_ELEMENT)

Annoying. I'll re-order the values so that this works.
Comment 20 Tim Horton 2015-02-26 14:44:26 PST
Created attachment 247446 [details]
Patch
Comment 21 WebKit Commit Bot 2015-02-26 14:46:24 PST
Attachment 247446 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/mac/LaunchServicesSPI.h:34:  _LSBindingCreateWithURL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Enrica Casucci 2015-02-26 15:19:45 PST
Comment on attachment 247446 [details]
Patch

I don't feel qualified to review the RenderThemeMac.mm changes, The rest looks good to me.
Comment 23 Dean Jackson 2015-02-26 16:50:14 PST
Comment on attachment 247446 [details]
Patch

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

> Source/WebCore/rendering/RenderAttachment.cpp:68
> +bool RenderAttachment::isFocused() const
> +{
> +    return frame().selection().isFocusedAndActive() && document().focusedElement() == &attachmentElement();
> +}
> +
> +bool RenderAttachment::isSelectedOrFocused() const
> +{
> +    return isSelected() || isFocused();
> +}

Could these go in the header?

> Source/WebCore/rendering/RenderTheme.cpp:989
> +#if ENABLE(ATTACHMENT_ELEMENT)
> +void RenderTheme::adjustAttachmentStyle(StyleResolver&, RenderStyle&, Element*) const
> +{
> +}

What's the point of this?
Comment 24 Sam Weinig 2015-02-26 17:06:03 PST
Comment on attachment 247446 [details]
Patch

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

> Source/WebCore/html/HTMLAttachmentElement.cpp:83
> +const String HTMLAttachmentElement::filePath() const
> +{
> +    return m_file ? m_file->path() : String();
> +}
> +
> +const String HTMLAttachmentElement::fileName() const
> +{
> +    return m_file ? m_file->name() : String();
>  }

I don't think these helpers are too helpful.  Also, if you do keep it, remove the const from the "const String".
Comment 25 Tim Horton 2015-02-26 18:08:15 PST
http://trac.webkit.org/changeset/180720