Bug 58066

<rdar://problem/8965302>

Created attachment 88671 [details] proposed fix

Attachment 88671 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/mac/AttributedString.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/platform/mac/HTMLConverter.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 88671 [details] proposed fix Wow! you did it. Looks good to me.

Comment on attachment 88671 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=88671&action=review > Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:193 > + if (!decoder->decode(start)) If something fails to decode, it will mark the argument decoder as invalid, so you can't use that as a check for whether there are enough attributes. A better solution is to encode the number of attributes. > Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:196 > + return false; You should return false if start > end.

Comment on attachment 88671 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=88671&action=review > Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:47 > +enum NSType { > + NSAttributedStringType, > + NSColorType, > + NSDictionaryType, > + NSFontType, > + NSNumberType, > + NSStringType, > + Unknown, > +}; Even inside a namespace, I’m not sure it’s safe to use the AppKit/Foundation prefix, NS, for our own identifiers.

Comment on attachment 88671 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=88671&action=review > Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:155 > + case Unknown: > + ASSERT_NOT_REACHED(); > + return false; > + } > + > + return false; I think you want ASSERT_NOT_REACHED after the if statement too, because a type that is not equal to any constant is an even worse problem than the type Unknown. But also we typically don’t assert things we can’t guarantee, and we can’t guarantee anything about the data we are decoding. So I suggest removing the assertion. You could do something else, but I think an assertion is not quite right. > Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:160 > + // Even though NSAttributedString is toll free bridged with CFAttributedStringRef, attributes' values may be not, so we should stay within this file's code. I don’t understand what “within this file’s code” means exactly or why that is needed.

Landed before seeing Darin's comments in <http://trac.webkit.org/changeset/83204>. >But also we typically don’t assert things we can’t guarantee, and we can’t guarantee anything about the data we are decoding. So I suggest removing the assertion. You could do something else, but I think an assertion is not quite right. I considered that, but the only case when we can't really guarantee it is when the Web process is compromised, and is sending bogus data. In debug builds, it's much more likely to hit a mistake in our own code, and getting an assertion is better than silently ignoring the problem.

At this abstraction level decoder knows only that it’s decoding a stream, so I’m not sure it’s good for it to enforce an “assert if there is bad data in the stream” policy. Lets worry about it later, though. Seems fine for now.

> I don’t understand what “within this file’s code” means exactly or why that is needed. The real issue is that we can not mix CF and NS types when encoding or decoding now. If an NSDictionary contains an CFString, it won't be encoded successfully, and vice versa. We could lift this limitation in the future.

http://trac.webkit.org/changeset/83204 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug The following tests are not passing: media/context-menu-actions.html media/media-fullscreen-inline.html media/media-fullscreen-not-in-document.html