Bug 58066 - REGRESSION (WebKit2): Reverse conversion doesn't work in Kotoeri
Summary: REGRESSION (WebKit2): Reverse conversion doesn't work in Kotoeri
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-04-07 11:55 PDT by Alexey Proskuryakov
Modified: 2011-04-07 15:38 PDT (History)
6 users (show)

See Also:


Attachments
proposed fix (39.62 KB, patch)
2011-04-07 12:10 PDT, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-04-07 11:55:27 PDT
We should implement attributedSubstringFromRange:.
Comment 1 Alexey Proskuryakov 2011-04-07 11:55:40 PDT
<rdar://problem/8965302>
Comment 2 Alexey Proskuryakov 2011-04-07 12:10:58 PDT
Created attachment 88671 [details]
proposed fix
Comment 3 WebKit Review Bot 2011-04-07 12:15:18 PDT
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 4 Enrica Casucci 2011-04-07 12:18:45 PDT
Comment on attachment 88671 [details]
proposed fix

Wow! you did it. Looks good to me.
Comment 5 Anders Carlsson 2011-04-07 12:58:18 PDT
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 6 Darin Adler 2011-04-07 13:37:41 PDT
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 7 Darin Adler 2011-04-07 13:40:55 PDT
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.
Comment 8 Alexey Proskuryakov 2011-04-07 13:54:07 PDT
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.
Comment 9 Darin Adler 2011-04-07 13:56:58 PDT
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.
Comment 10 Alexey Proskuryakov 2011-04-07 14:01:34 PDT
> 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.
Comment 11 WebKit Review Bot 2011-04-07 15:38:35 PDT
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