Bug 139020 – Page previews should have titles

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Conrad Shultz]

Page previews should have titles.

[Radar WebKit Bug Importer]

<rdar://problem/19070840>

[Conrad Shultz]

Created attachment 242142 [details]
Patch

[Tim Horton]

Comment on attachment 242142 [details]
Patch

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

Wonder if we should move these preview related things into their own file; they're growing fast!

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:164
> +    [titleTextField setStringValue:title ? title : @""];

Is it going to magically collapse in this case? It seems like it might, and that's probably good.

[Conrad Shultz]

Comment on attachment 242142 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:164
>> +    [titleTextField setStringValue:title ? title : @""];
> 
> Is it going to magically collapse in this case? It seems like it might, and that's probably good.

Yes and no: the text field itself will, but we make no effort to resize the preview view; the title region remains a fixed previewViewTitleHeight points tall irrespective of title content.

We could of course collapse the title region in such an event, but it would add unnecessary complexity at the moment. (The nil check on line 164 is really only there so we don't raise an exception by trying to set a nil stringValue).

Thanks for the review!

[Conrad Shultz]

(In reply to comment #3)
> Comment on attachment 242142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242142&action=review
> 
> Wonder if we should move these preview related things into their own file;
> they're growing fast!

It's something we should probably consider, I agree.

[Conrad Shultz]

Committed r176511: <http://trac.webkit.org/changeset/176511>

Comment on attachment 242142 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:79
>  @protocol WKPagePreviewViewControllerDelegate <NSObject>

Not new to this patch, but it’s unconventional to have a delegate protocol with required methods. Classes should be able to function with the delegate being nil as well as not implementing all methods.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:162
> +        title = [_url absoluteString];

The URLS may change because of redirects and script-initiated navigation. It seems wrong (and sometimes considered to have security implications) to not update the URL (or URL-derived) in the UI a when the URL being shows changes.

[Conrad Shultz]

(In reply to comment #7)
> Comment on attachment 242142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242142&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:79
> >  @protocol WKPagePreviewViewControllerDelegate <NSObject>
> 
> Not new to this patch, but it’s unconventional to have a delegate protocol
> with required methods. Classes should be able to function with the delegate
> being nil as well as not implementing all methods.

For the moment, since this class is purely internal to this file, I don't think this is a major issue. If we move it out as discussed with Tim above then we should definitely do this (at least for this method, which is pretty optional on its face).

> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:162
> > +        title = [_url absoluteString];
> 
> The URLS may change because of redirects and script-initiated navigation. It
> seems wrong (and sometimes considered to have security implications) to not
> update the URL (or URL-derived) in the UI a when the URL being shows changes.

Good point, I'll address this in a subsequent patch.