[webkit-reviews] review granted: [Bug 105774] Add context to the console message generated by Document::printNavigationErrorMessage. : [Attachment 180787] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 105774: Add context to the console message generated by
Document::printNavigationErrorMessage.
https://bugs.webkit.org/show_bug.cgi?id=105774

Attachment 180787: Patch
https://bugs.webkit.org/attachment.cgi?id=180787&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=180787&action=review


> Source/WebCore/dom/Document.cpp:395
> -static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL)

> +static void printNavigationErrorMessage(Frame* frame, const KURL& activeURL,
const String& reason)

The reason string should be a const char* rather than a const String&. It is
not helpful to create a String object and then destroy it just to carry the
reason string from the call site into this function.

> Source/WebCore/dom/Document.cpp:2721
> -	   printNavigationErrorMessage(targetFrame, url());
> +	   if (isSandboxed(SandboxTopNavigation) && targetFrame ==
m_frame->tree()->top())
> +	       printNavigationErrorMessage(targetFrame, url(), "The frame
attempting navigation of the top-level window is sandboxed, but the
'allow-top-navigation' flag is not set.");
> +	   else
> +	       printNavigationErrorMessage(targetFrame, url(), "The frame
attempting navigation is sandboxed, and is therefore disallowed from navigating
its ancestors.");

It seems inelegant to repeat the printNavigationErrorMessage call. Instead I
suggest either a local variable of type const char* or a helper function that
returns the reason string.