Bug 144610 – Add a layout mode for computing fixed layout size from a minimum size

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Add a layout mode for computing fixed layout size from a minimum size

[Tim Horton]

Created attachment 252357 [details]
Patch

[Tim Horton]

Created attachment 252395 [details]
Patch

[Simon Fraser (smfr)]

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?

[Tim Horton]

(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!

[Tim Horton]

Created attachment 252420 [details]
Patch

[Simon Fraser (smfr)]

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?

[Tim Horton]

(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]

[Tim Horton]

http://trac.webkit.org/changeset/183841

[Tim Horton]

Follow up build fix in http://trac.webkit.org/changeset/183844

[Alexey Proskuryakov]

The build is still broken: <https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/builds/16759/steps/compile-webkit/logs/errors>.

[Tim Horton]

(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.

[Tim Horton]

(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