Bug 139020 - Page previews should have titles
Summary: Page previews should have titles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Conrad Shultz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-11-23 20:51 PST by Conrad Shultz
Modified: 2014-11-23 22:55 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.72 KB, patch)
2014-11-23 21:00 PST, Conrad Shultz
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Shultz 2014-11-23 20:51:02 PST
Page previews should have titles.
Comment 1 Radar WebKit Bug Importer 2014-11-23 20:51:44 PST
<rdar://problem/19070840>
Comment 2 Conrad Shultz 2014-11-23 21:00:33 PST
Created attachment 242142 [details]
Patch
Comment 3 Tim Horton 2014-11-23 21:08:16 PST
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.
Comment 4 Conrad Shultz 2014-11-23 21:12:57 PST
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!
Comment 5 Conrad Shultz 2014-11-23 21:16:01 PST
(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.
Comment 6 Conrad Shultz 2014-11-23 21:20:59 PST
Committed r176511: <http://trac.webkit.org/changeset/176511>
Comment 7 mitz 2014-11-23 22:49:51 PST
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.
Comment 8 Conrad Shultz 2014-11-23 22:55:24 PST
(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.