Bug 144610 - Add a layout mode for computing fixed layout size from a minimum size
Summary: Add a layout mode for computing fixed layout size from a minimum size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-04 17:14 PDT by Tim Horton
Modified: 2015-05-05 19:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (112.97 KB, patch)
2015-05-04 17:14 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (125.21 KB, patch)
2015-05-05 12:06 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (141.37 KB, patch)
2015-05-05 16:25 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-05-04 17:14:01 PDT
Add a layout mode for computing fixed layout size from a minimum size
Comment 1 Tim Horton 2015-05-04 17:14:39 PDT
Created attachment 252357 [details]
Patch
Comment 2 Tim Horton 2015-05-05 12:06:27 PDT
Created attachment 252395 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-05-05 12:16:41 PDT
Comment on attachment 252395 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKLayoutMode.h:40
> +    // Equivalent to kWKLayoutModeFixedSize where the fixed size is equal to the view's size scaled by the inverse of the view scale.
> +    // This is useful for scaling down a view but having it lay out as if it were not scaled down.

My brain hurts. Is there an easier way to say this?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:99
> +// For use with _layoutMode = _WKLayoutModeFixedSize:
>  @property (nonatomic, setter=_setFixedLayoutSize:) CGSize _fixedLayoutSize;
> +// For use with _layoutMode = _WKLayoutModeDynamicSizeWithMinimumViewSize:
> +@property (nonatomic, setter=_setMinimumViewSize:) CGSize _minimumViewSize;

I don't really like properties that are inter-dependent like this. Is there a way to design the SPI to avoid this?

> Source/WebKit2/UIProcess/API/Cocoa/_WKLayoutMode.h:39
> +    // Equivalent to _WKLayoutModeFixedSize where the fixed size is equal to the view's size scaled by the inverse of the view scale.
> +    // This is useful for scaling down a view but having it lay out as if it were not scaled down.
> +    _WKLayoutModeDynamicSizeComputedFromViewScale,
> +
> +    // Equivalent to _WKLayoutModeFixedSize where the fixed size is heuristically determined based on the layout of the page and the given minimum view size.
> +    // This is useful for having a view that tries to keep the page usable at varying sizes, potentially even scaling the page down to fit.

Don't think you need the long comments in two places.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:625
> +        MachSendRight fencePort = MachSendRight::adopt([[self.layer context] createFencePort]);

It's weird to see something as low-level as MachSendRight in code like this. Can't we bundle up fences in a nice class that's exported from WebCore?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:4446
> +    _data->_page->setUseFixedLayout(layoutMode == kWKLayoutModeFixedSize || layoutMode == kWKLayoutModeDynamicSizeComputedFromViewScale || layoutMode == kWKLayoutModeDynamicSizeWithMinimumViewSize);
> +
> +    if (_data->_layoutMode == kWKLayoutModeDynamicSizeComputedFromViewScale)
> +        [self _updateFixedLayoutSizeFromViewScale];
> +
> +    if (_data->_layoutMode == kWKLayoutModeDynamicSizeWithMinimumViewSize)
> +        [self _updateFixedLayoutSizeFromMinimumViewSize];

I kinda wonder whether we should make 'view layout strategy' class for each mode type, and push logic into those?
Comment 4 Tim Horton 2015-05-05 12:29:20 PDT
(In reply to comment #3)
> Comment on attachment 252395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252395&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKLayoutMode.h:40
> > +    // Equivalent to kWKLayoutModeFixedSize where the fixed size is equal to the view's size scaled by the inverse of the view scale.
> > +    // This is useful for scaling down a view but having it lay out as if it were not scaled down.
> 
> My brain hurts. Is there an easier way to say this?

Probably! Could also just use some variant of the second line and ignore how it works.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:99
> > +// For use with _layoutMode = _WKLayoutModeFixedSize:
> >  @property (nonatomic, setter=_setFixedLayoutSize:) CGSize _fixedLayoutSize;
> > +// For use with _layoutMode = _WKLayoutModeDynamicSizeWithMinimumViewSize:
> > +@property (nonatomic, setter=_setMinimumViewSize:) CGSize _minimumViewSize;
> 
> I don't really like properties that are inter-dependent like this. Is there
> a way to design the SPI to avoid this?

You mean that you don't like having the enum of modes plus separate setters for the parameters to the modes? I imagine the best way to avoid that would be to provide the parameters when turning on the modes, but you suggested the mode enum instead :D

I'm not sure what the alternative, with the mode enum, would be.

> > Source/WebKit2/UIProcess/API/Cocoa/_WKLayoutMode.h:39
> > +    // Equivalent to _WKLayoutModeFixedSize where the fixed size is equal to the view's size scaled by the inverse of the view scale.
> > +    // This is useful for scaling down a view but having it lay out as if it were not scaled down.
> > +    _WKLayoutModeDynamicSizeComputedFromViewScale,
> > +
> > +    // Equivalent to _WKLayoutModeFixedSize where the fixed size is heuristically determined based on the layout of the page and the given minimum view size.
> > +    // This is useful for having a view that tries to keep the page usable at varying sizes, potentially even scaling the page down to fit.
> 
> Don't think you need the long comments in two places.

Agreed. But, do I put them in the modern SPI header, where they're more likely to survive (with editing), or in the antique SPI header, where current users will be looking?

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:625
> > +        MachSendRight fencePort = MachSendRight::adopt([[self.layer context] createFencePort]);
> 
> It's weird to see something as low-level as MachSendRight in code like this.
> Can't we bundle up fences in a nice class that's exported from WebCore?

MachSendRight is already a bundled-up nice class that's exported from WebCore :D

I think the API for a Fence class would be identical to the MachSendRight API (adopt the fence upon creation, retrieve the port from it to apply to the CAContext upon application), so another class wouldn't be much better than a #define.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:4446
> > +    _data->_page->setUseFixedLayout(layoutMode == kWKLayoutModeFixedSize || layoutMode == kWKLayoutModeDynamicSizeComputedFromViewScale || layoutMode == kWKLayoutModeDynamicSizeWithMinimumViewSize);
> > +
> > +    if (_data->_layoutMode == kWKLayoutModeDynamicSizeComputedFromViewScale)
> > +        [self _updateFixedLayoutSizeFromViewScale];
> > +
> > +    if (_data->_layoutMode == kWKLayoutModeDynamicSizeWithMinimumViewSize)
> > +        [self _updateFixedLayoutSizeFromMinimumViewSize];
> 
> I kinda wonder whether we should make 'view layout strategy' class for each
> mode type, and push logic into those?

That doesn't sound horrible!
Comment 5 Tim Horton 2015-05-05 16:25:31 PDT
Created attachment 252420 [details]
Patch
Comment 6 Simon Fraser (smfr) 2015-05-05 16:34:41 PDT
Comment on attachment 252420 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKViewLayoutStrategy.mm:86
> +    _page = &page;
> +    _wkView = wkView;

Can we not always get a WebPageProxy from the WKView?

> Tools/MiniBrowser/mac/WK1BrowserWindowController.m:235
> +- (IBAction)toggleUseMinimumViewSize:(id)sender
> +{
> +
> +}

Wouldn't it be better to just not implement this, and have the menu item automatically disable?
Comment 7 Tim Horton 2015-05-05 16:38:16 PDT
(In reply to comment #6)
> Comment on attachment 252420 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252420&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKViewLayoutStrategy.mm:86
> > +    _page = &page;
> > +    _wkView = wkView;
> 
> Can we not always get a WebPageProxy from the WKView?

I don't think so. The only think you can do is get a WKPageRef and toImpl() it.

> > Tools/MiniBrowser/mac/WK1BrowserWindowController.m:235
> > +- (IBAction)toggleUseMinimumViewSize:(id)sender
> > +{
> > +
> > +}
> 
> Wouldn't it be better to just not implement this, and have the menu item
> automatically disable?

I don't think so:

error: method 'toggleUseMinimumViewSize:' in protocol 'BrowserController' not implemented [-Werror,-Wprotocol]
Comment 8 Tim Horton 2015-05-05 16:41:06 PDT
http://trac.webkit.org/changeset/183841
Comment 9 Tim Horton 2015-05-05 17:23:30 PDT
Follow up build fix in http://trac.webkit.org/changeset/183844
Comment 11 Tim Horton 2015-05-05 19:07:14 PDT
(In reply to comment #10)
> The build is still broken:
> <https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/
> builds/16759/steps/compile-webkit/logs/errors>.

:( I hate that one. Will fix.
Comment 12 Tim Horton 2015-05-05 19:12:19 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > The build is still broken:
> > <https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/
> > builds/16759/steps/compile-webkit/logs/errors>.
> 
> :( I hate that one. Will fix.

Fix hopefully in http://trac.webkit.org/changeset/183846