Wires through scale factor and bounds in html_viewer

Wires through scale factor and bounds in html_viewer

R=jamesr@chromium.org

Committed: https://crrev.com/fa83647c380d7a8f3ac2a8de372281035f3312cd
Cr-Commit-Position: refs/heads/master@{#320998}

[jamesr, 2015-03-16 23:26:45]

https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should really
take pixels (or float) so it has the real size.
you're picking the real size here. Blink operates in an integer number of DIPs
and it's up to the embedder (HTMLDocument in this case) to figure out what that
should be.  That won't always map to an integer number of physical pixels, which
is why the embedder has to decide what it wants. It doesn't make any sense for
Blink to take a float or physical pixel number.

ceiled is a bit iffy since it means that the part of the content that doesn't
fit inside the physical bounds will be cut off. i.e. if the DSF is 1.5 and the
physical size is 10x10 this will compute a dip size of ceil(10 * 1/1.5) =
ceil(6.6667) = 7, but the last third of the dip will be cut off assuming that
this view starts on an exact physical pixel boundary.  i think you want to do
something like compute the largest fully enclosed DIP rectangle within this
view's bounds

[sky, 2015-03-16 23:33:00]

https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should really
take pixels (or float) so it has the real size.
On 2015/03/16 23:26:45, jamesr wrote:
> you're picking the real size here. Blink operates in an integer number of DIPs
> and it's up to the embedder (HTMLDocument in this case) to figure out what
that
> should be.  That won't always map to an integer number of physical pixels,
which
> is why the embedder has to decide what it wants. It doesn't make any sense for
> Blink to take a float or physical pixel number.
> 
> ceiled is a bit iffy since it means that the part of the content that doesn't
> fit inside the physical bounds will be cut off. i.e. if the DSF is 1.5 and the
> physical size is 10x10 this will compute a dip size of ceil(10 * 1/1.5) =
> ceil(6.6667) = 7, but the last third of the dip will be cut off assuming that
> this view starts on an exact physical pixel boundary.  i think you want to do
> something like compute the largest fully enclosed DIP rectangle within this
> view's bounds
> 

But if I do 6, then we're not rendering the last pixel, right?

Seems like there are only two options: don't render some amount of pixels on the
right side, or render past the size. I'm favoring rendering past the size so we
don't get some amount of pixels that don't match the web content on the right
side.

[jamesr, 2015-03-16 23:40:51]

On 2015/03/16 23:33:00, sky wrote:
>
https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
> File mojo/services/html_viewer/html_document.cc (right):
> 
>
https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
> mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should really
> take pixels (or float) so it has the real size.
> On 2015/03/16 23:26:45, jamesr wrote:
> > you're picking the real size here. Blink operates in an integer number of
DIPs
> > and it's up to the embedder (HTMLDocument in this case) to figure out what
> that
> > should be.  That won't always map to an integer number of physical pixels,
> which
> > is why the embedder has to decide what it wants. It doesn't make any sense
for
> > Blink to take a float or physical pixel number.
> > 
> > ceiled is a bit iffy since it means that the part of the content that
doesn't
> > fit inside the physical bounds will be cut off. i.e. if the DSF is 1.5 and
the
> > physical size is 10x10 this will compute a dip size of ceil(10 * 1/1.5) =
> > ceil(6.6667) = 7, but the last third of the dip will be cut off assuming
that
> > this view starts on an exact physical pixel boundary.  i think you want to
do
> > something like compute the largest fully enclosed DIP rectangle within this
> > view's bounds
> > 
> 
> But if I do 6, then we're not rendering the last pixel, right?
> 
> Seems like there are only two options: don't render some amount of pixels on
the
> right side, or render past the size. I'm favoring rendering past the size so
we
> don't get some amount of pixels that don't match the web content on the right
> side.

Depends on what the top/left dimension is.  If this view starts at a half pixel
(in DIPS) then the thing will fit snugly at 7x7, if it doesn't then it'll only
fit into 6x6 (or if the X and Y are mismatched then it might fit into 6x7 or
7x6).

Giving Blink a larger size and then cutting it off could result in it
constructing content that's inaccessible, which seems worse to me than having
some wasted space on the edges.  Wasting space will never make controls
completely unreachable.

[sky, 2015-03-17 16:53:20]

On 2015/03/16 23:40:51, jamesr wrote:
> On 2015/03/16 23:33:00, sky wrote:
> >
>
https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
> > File mojo/services/html_viewer/html_document.cc (right):
> > 
> >
>
https://codereview.chromium.org/1018543002/diff/1/mojo/services/html_viewer/h...
> > mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should
really
> > take pixels (or float) so it has the real size.
> > On 2015/03/16 23:26:45, jamesr wrote:
> > > you're picking the real size here. Blink operates in an integer number of
> DIPs
> > > and it's up to the embedder (HTMLDocument in this case) to figure out what
> > that
> > > should be.  That won't always map to an integer number of physical pixels,
> > which
> > > is why the embedder has to decide what it wants. It doesn't make any sense
> for
> > > Blink to take a float or physical pixel number.
> > > 
> > > ceiled is a bit iffy since it means that the part of the content that
> doesn't
> > > fit inside the physical bounds will be cut off. i.e. if the DSF is 1.5 and
> the
> > > physical size is 10x10 this will compute a dip size of ceil(10 * 1/1.5) =
> > > ceil(6.6667) = 7, but the last third of the dip will be cut off assuming
> that
> > > this view starts on an exact physical pixel boundary.  i think you want to
> do
> > > something like compute the largest fully enclosed DIP rectangle within
this
> > > view's bounds
> > > 
> > 
> > But if I do 6, then we're not rendering the last pixel, right?
> > 
> > Seems like there are only two options: don't render some amount of pixels on
> the
> > right side, or render past the size. I'm favoring rendering past the size so
> we
> > don't get some amount of pixels that don't match the web content on the
right
> > side.
> 
> Depends on what the top/left dimension is.  If this view starts at a half
pixel
> (in DIPS) then the thing will fit snugly at 7x7, if it doesn't then it'll only
> fit into 6x6 (or if the X and Y are mismatched then it might fit into 6x7 or
> 7x6).

Not sure what you mean by origin. Each view has it's own coordinate spaces that
starts at 0,0.

> 
> Giving Blink a larger size and then cutting it off could result in it
> constructing content that's inaccessible, which seems worse to me than having
> some wasted space on the edges.  Wasting space will never make controls
> completely unreachable.

I can't imagine a few pixels missing making something inaccessible, but it's
certainly possible. What I'm more worried about is a page with an all black
background and seeing a white line along the right/bottom edge. But, I'm not
sure that's possible as I don't know this code. There seem to be two things that
are told the size: WebView and WebLayerTreeViewImpl. How do the two relate?

[sky, 2015-03-17 17:01:29]

I changed to ConvertSizeToDIP, which does a floor. Not too excited about that,
but it seems as though that is what we use in other similar places (eg
WindowTreeHost::UpdateRootWindowSize).

[jamesr, 2015-03-17 17:28:45]

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should really
take pixels (or float) so it has the real size.
i still think this TODO is incorrect and should be removed or fixed

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/html_document.cc:179: const gfx::Size size_in_dips =
gfx::ConvertSizeToDIP(
do we always snap the x/y offset of a view to a physical pixel boundary? if not,
to get a good result here you should include the initial offset (i.e. use
gfx::ConvertRectToDIP) so it can handle that case

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/html_document.cc:183:
web_layer_tree_view_impl_->setViewportSize(size_in_pixels);
this is the number of physical pixels to use for compositing, which should be
big enough to fit everything that blink will want to render. this looks right

[sky, 2015-03-17 22:05:38]

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/html_document.cc:178: // TODO: WebView should really
take pixels (or float) so it has the real size.
On 2015/03/17 17:28:45, jamesr wrote:
> i still think this TODO is incorrect and should be removed or fixed

Done.

https://codereview.chromium.org/1018543002/diff/20001/mojo/services/html_view...
mojo/services/html_viewer/html_document.cc:179: const gfx::Size size_in_dips =
gfx::ConvertSizeToDIP(
On 2015/03/17 17:28:45, jamesr wrote:
> do we always snap the x/y offset of a view to a physical pixel boundary? if
not,
> to get a good result here you should include the initial offset (i.e. use
> gfx::ConvertRectToDIP) so it can handle that case

Yes. mojo::Views is pixel based.

[jamesr, 2015-03-17 22:07:12]

lgtm

[sky, 2015-03-17 22:14:57]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2015-03-17 22:16:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018543002/40001

[commit-bot: I haz the power, 2015-03-17 22:32:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 22:32:55]

Patchset 3 (id:??) landed as
https://crrev.com/fa83647c380d7a8f3ac2a8de372281035f3312cd
Cr-Commit-Position: refs/heads/master@{#320998}