Bug 133410 – Address some review comments on bug 133408

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Geoffrey Garen]

Comment #5 From Darin Adler 2014-05-30 11:54:49 PST (-) [reply] 
(From update of attachment 232297 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=232297&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKBackForwardListItem.h:32
> -/*! A @link WKBackForwardListItem @/link represents a visited webpage in a WKWebView's back-forward list.
> +/*! A WKBackForwardListItem object represents a previously visited webpage in the back-forward list of a web view.

Not sure this is perfectly accurate. Is the page you are currently on a previously visited webpage?

> Source/WebKit2/UIProcess/API/Cocoa/WKFrameInfo.h:41
> +/*! @abstract A Boolean value indicating whether the frame is the main frame
> + or a subframe.

Are we capitalizing the word “boolean” here because of its origin as a person’s name? I suggest “boolean” with a lowercase b.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:76
> + @discussion Clients should visually indicate that this panel comes from
> + JavaScript initiated by the specified frame.

I think this is confusing wording. The point here is that for good security it's important to indicate that the panel comes from JavaScript and which website controls the content of the panel. But “initiated by the specified frame” is an oblique way to say this.

[Radar WebKit Bug Importer]

<rdar://problem/17081195>

[Geoffrey Garen]

> > Source/WebKit2/UIProcess/API/Cocoa/WKBackForwardListItem.h:32
> > -/*! A @link WKBackForwardListItem @/link represents a visited webpage in a WKWebView's back-forward list.
> > +/*! A WKBackForwardListItem object represents a previously visited webpage in the back-forward list of a web view.
> 
> Not sure this is perfectly accurate. Is the page you are currently on a previously visited webpage?

Not necessarily. Also, I suppose the details of why something ended up in the list involve subtleties of loading, redirection, and DOM APIs. So, I think it's best not to comment on how something got in the list. I removed "previously visited" and just left it at "webpage".

> > Source/WebKit2/UIProcess/API/Cocoa/WKFrameInfo.h:41
> > +/*! @abstract A Boolean value indicating whether the frame is the main frame
> > + or a subframe.
> 
> Are we capitalizing the word “boolean” here because of its origin as a person’s name? I suggest “boolean” with a lowercase b.

I wondered the same thing. It seems to a de facto standard in Apple headerdoc, so I think we should just click agree:

<https://developer.apple.com/library/ios/documentation/uikit/reference/uiview_class/uiview/uiview.html>
<https://developer.apple.com/library/ios/documentation/uikit/reference/UIWebView_Class/Reference/Reference.html>

> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:76
> > + @discussion Clients should visually indicate that this panel comes from
> > + JavaScript initiated by the specified frame.
> 
> I think this is confusing wording. The point here is that for good security it's important to indicate that the panel comes from JavaScript and which website controls the content of the panel. But “initiated by the specified frame” is an oblique way to say this.

How about:

 @discussion For user security, your app should visually indicate that
 a specific websites controls the content in this panel.

[Radar WebKit Bug Importer]

<rdar://problem/17081315>

[Geoffrey Garen]

Created attachment 232300 [details]
Patch

[Geoffrey Garen]

s/websites/website/

(In reply to comment #2)

> How about:
> 
>  @discussion For user security, your app should visually indicate that
>  a specific websites controls the content in this panel.

Can you drop “visually”? The app should indicate this in a way that works for visually-impaired users as well.

[Darin Adler]

Comment on attachment 232300 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:76
> + @discussion For user security, your app should visually indicate that
> + a specific website controls the content in this panel.

Besides Dan’s suggestion of omitting the word “visually” it also might be useful to point at the simple recipe for figuring out which specific website to indicate. I presume that it’s one of the properties of WKFrameInfo, maybe something that returns a URL that the client should extract a hostname from using an NSURL method. Of course, they would also want to have use a “user visible host name” with IDN decoded, but I’m not sure the API makes a helper method that can decode IDN public.

[Geoffrey Garen]

Created attachment 232306 [details]
Patch

[Geoffrey Garen]

Changed "visually indicate" to "call attention to".

Added the example of "[[[frame request] URL] host]".

[Anders Carlsson]

Comment on attachment 232306 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:64
> + for identifying the controlling website is [[[frame request] URL] host].

Please use property syntax, i.e. frame.request.URL.host.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:78
> + for identifying the controlling website is [[[frame request] URL] host].

Please use property syntax, i.e. frame.request.URL.host.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:93
> + for identifying the controlling website is [[[frame request] URL] host].

Please use property syntax, i.e. frame.request.URL.host.

[Geoffrey Garen]

Committed r169542: <http://trac.webkit.org/changeset/169542>