Bug 133410 - Address some review comments on bug 133408
Summary: Address some review comments on bug 133408
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-30 12:23 PDT by Geoffrey Garen
Modified: 2014-06-02 13:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2014-05-30 12:35 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2014-05-30 14:47 PDT, Geoffrey Garen
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2014-05-30 12:23:37 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-05-30 12:24:15 PDT
<rdar://problem/17081195>
Comment 2 Geoffrey Garen 2014-05-30 12:34:33 PDT
> > 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.
Comment 3 Radar WebKit Bug Importer 2014-05-30 12:34:47 PDT
<rdar://problem/17081315>
Comment 4 Geoffrey Garen 2014-05-30 12:35:39 PDT
Created attachment 232300 [details]
Patch
Comment 5 Geoffrey Garen 2014-05-30 12:36:27 PDT
s/websites/website/
Comment 6 mitz 2014-05-30 12:41:55 PDT
(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.
Comment 7 Darin Adler 2014-05-30 13:37:22 PDT
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.
Comment 8 Geoffrey Garen 2014-05-30 14:47:44 PDT
Created attachment 232306 [details]
Patch
Comment 9 Geoffrey Garen 2014-05-30 14:48:25 PDT
Changed "visually indicate" to "call attention to".

Added the example of "[[[frame request] URL] host]".
Comment 10 Anders Carlsson 2014-05-30 15:32:35 PDT
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.
Comment 11 Geoffrey Garen 2014-06-02 13:22:51 PDT
Committed r169542: <http://trac.webkit.org/changeset/169542>