Bug 58066 – REGRESSION (WebKit2): Reverse conversion doesn't work in Kotoeri

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

We should implement attributedSubstringFromRange:.

[Alexey Proskuryakov]

<rdar://problem/8965302>

[Alexey Proskuryakov]

Created attachment 88671 [details]
proposed fix

[WebKit Review Bot]

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.

[Enrica Casucci]

Comment on attachment 88671 [details]
proposed fix

Wow! you did it. Looks good to me.

[Anders Carlsson]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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

[WebKit Review Bot]

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