[webkit-reviews] review granted: [Bug 197523] NSAttributedString conversion in a loop returns nil and WKUnknownError every other time : [Attachment 368812] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Timothy Hatcher <timothy at apple.com>'s
request for review:
Bug 197523: NSAttributedString conversion in a loop returns nil and
WKUnknownError every other time
https://bugs.webkit.org/show_bug.cgi?id=197523

Attachment 368812: Patch

https://bugs.webkit.org/attachment.cgi?id=368812&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 368812
  --> https://bugs.webkit.org/attachment.cgi?id=368812
Patch

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

> Source/WebKit/ChangeLog:12
> +	   would be confused with the real content loading. Ultimatly this
wasn't needed
> +	   since a process swap will likely happen, and cached views are closed
quickly.
> +	   It also avoids extra work and speeds up conversions a bit.

Typo: Ultimatly

The word "likely" here does not inspire confidence in me that this code is
correct. How can something that will "likely" happen make something not needed.
It seems that it will make it "likely not needed", which is not good enough for
correctness.

There must be some other reason it’s not needed.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:211
> +    result.string =
attributedStringFromRange(rangeOfContents(*m_page->mainFrame().document()->docu
mentElement()), &documentAttributes);

I think the old code would fail cleanly if documentElement() was null, so maybe
we want to preserve that property. Or is there a guarantee that
documentElement() is non-null?