Closed Bug 923351 Opened 6 years ago Closed 6 years ago

Make layers borders more visible

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: nrc, Assigned: nrc)

Details

Attachments

(1 file, 1 obsolete file)

Some image layers get an odd border due to BIGIMAGE borders and the lines are too small on mobile.
We should probably augment the layer border thickness on hiDPI screens.
Attached patch patch (obsolete) — Splinter Review
Attachment #813417 - Flags: review?(nical.bugzilla)
Comment on attachment 813417 [details] [diff] [review]
patch

Review of attachment 813417 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Compositor.cpp
@@ +47,5 @@
>      return;
>    }
>  
> +#ifdef ANDROID
> +  int lWidth = 10;

wow that's a lot bigger indeed! :)

@@ +69,5 @@
>    }
>  
>    // make tile borders a bit more transparent to keep layer borders readable.
> +  if (((aFlags & DIAGNOSTIC_TILE) && (mDiagnosticTypes & DIAGNOSTIC_TILE_BORDERS)) ||
> +      ((aFlags & DIAGNOSTIC_BIGIMAGE) && (mDiagnosticTypes & DIAGNOSTIC_BIGIMAGE_BORDERS)))  {

why do we need this change?
(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 813417 [details] [diff] [review]
> patch
> 
> Review of attachment 813417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Compositor.cpp
> @@ +47,5 @@
> >      return;
> >    }
> >  
> > +#ifdef ANDROID
> > +  int lWidth = 10;
> 
> wow that's a lot bigger indeed! :)
> 

Heh, I'd been using 20 for debugging my bug today. That's on a pretty hires device, I worry a bit about how this looks on b2g, but it needs to be pretty fat to distinguish colours. I'm willing to compromise if you think it gets in the way but I figure it is partially transparent anyway, so we are not blocking much so can ere on the side of visibility.

> @@ +69,5 @@
> >    }
> >  
> >    // make tile borders a bit more transparent to keep layer borders readable.
> > +  if (((aFlags & DIAGNOSTIC_TILE) && (mDiagnosticTypes & DIAGNOSTIC_TILE_BORDERS)) ||
> > +      ((aFlags & DIAGNOSTIC_BIGIMAGE) && (mDiagnosticTypes & DIAGNOSTIC_BIGIMAGE_BORDERS)))  {
> 
> why do we need this change?

Because image layers are highlighted using IMAGE | BIGIMAGE, so without the check they are made smaller and darker even if we are not highlighting tiles/bigimages. (I guess the extra check for tiles is not strictly necessary, but I think it is correct, so why not? :-) ).
Here is how it looks on Firefox OS: https://dl.dropboxusercontent.com/u/7742672/IMG_20131003_095458.jpg

I personally find it too thick but I could be convinced otherwise. I need feedback from more people.
Vivien, you use this feature a lot, is the layer border thickness okay for you on this image?
Flags: needinfo?(21)
This looks way too big for me. I think the thickness should adapt with screen density.
(In reply to Nicolas Silva [:nical] from comment #5)
> Here is how it looks on Firefox OS:
> https://dl.dropboxusercontent.com/u/7742672/IMG_20131003_095458.jpg
> 
> I personally find it too thick but I could be convinced otherwise. I need
> feedback from more people.
> Vivien, you use this feature a lot, is the layer border thickness okay for
> you on this image?

Yeah, that looks too thick. How about half that?
> > >    // make tile borders a bit more transparent to keep layer borders readable.
> > > +  if (((aFlags & DIAGNOSTIC_TILE) && (mDiagnosticTypes & DIAGNOSTIC_TILE_BORDERS)) ||
> > > +      ((aFlags & DIAGNOSTIC_BIGIMAGE) && (mDiagnosticTypes & DIAGNOSTIC_BIGIMAGE_BORDERS)))  {
> > 
> > why do we need this change?
> 
> Because image layers are highlighted using IMAGE | BIGIMAGE, so without the
> check they are made smaller and darker even if we are not highlighting
> tiles/bigimages. (I guess the extra check for tiles is not strictly
> necessary, but I think it is correct, so why not? :-) ).

Hm that would be a bug, ImageLayer should have the layer border drawn without BIGIMAGE
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp?from=ImageHost.cpp#l124

and, when iterating over a bigimage part, do the thinner borders additionally.
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp?from=ImageHost.cpp#l119

It looks like I forgot to test for the whether or not to allow BIGIMAGE borders though, the following test
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/Compositor.cpp?from=Compositor.cpp#l42
should have also been done for BIGIMAGE
(In reply to Anthony Ricaud (:rik) from comment #6)
> This looks way too big for me. I think the thickness should adapt with
> screen density.

I think it is hard to get this info in gfx. Also I'm sure we can find a value we agree on without adding the extra complexity.
In reply to Nick Cameron [:nrc] from comment #9)
> (In reply to Anthony Ricaud (:rik) from comment #6)
> > This looks way too big for me. I think the thickness should adapt with
> > screen density.
> 
> I think it is hard to get this info in gfx. Also I'm sure we can find a
> value we agree on without adding the extra complexity.

I see APZCTreeManager::GetDPI(), not 100% sure we can use it though.
(In reply to Nicolas Silva [:nical] from comment #10)
> In reply to Nick Cameron [:nrc] from comment #9)
> > (In reply to Anthony Ricaud (:rik) from comment #6)
> > > This looks way too big for me. I think the thickness should adapt with
> > > screen density.
> > 
> > I think it is hard to get this info in gfx. Also I'm sure we can find a
> > value we agree on without adding the extra complexity.
> 
> I see APZCTreeManager::GetDPI(), not 100% sure we can use it though.

That seems to be set only in Metro, so I guess not.
What about changing the draw-borders preference to an integer representing the size of the border? Fits everyone's preference and all densities.
with 5px border (half of the initial patch) https://dl.dropboxusercontent.com/u/7742672/IMG_20131003_112955.jpg

I also tested on android and I have to admit that on hiDPI screens it is tempting to make it much thicker.
(In reply to Anthony Ricaud (:rik) from comment #12)
> What about changing the draw-borders preference to an integer representing
> the size of the border? Fits everyone's preference and all densities.

prefs can only be checked from the main thread so at this point it's also a lot of code just to get the info through to the compositor. I wouldn't refuse a patch doing that but if there is a compromise that works for every body in the short term we can take it.

fwiw I am ok with the 5px border
(In reply to Anthony Ricaud (:rik) from comment #12)
> What about changing the draw-borders preference to an integer representing
> the size of the border? Fits everyone's preference and all densities.

We would still have the same problem, we would need to pick sensible defaults for the pref rather than the implementation, although of course the impact would be smaller because they are only defaults. Not sure it is really worth the effort, but if we can't agree on a value, then we could do this.

We could have a separate ifdef for b2g/Android/desktop - I think low dpi Android is not worth worrying about.

Say 2/4/10?
er, 4/10/2
4/10/2 works for me
FYI, some people are already working with hidpi b2g devices. And the borders are really thin on a hidpi desktop too. My plan is to get the devtools to add an option to turn this pref on. So long term, I really think adapting to dpis is the way to go.
Flags: needinfo?(21)
Attachment #813417 - Flags: review?(nical.bugzilla)
(In reply to Anthony Ricaud (:rik) from comment #18)
> FYI, some people are already working with hidpi b2g devices. And the borders
> are really thin on a hidpi desktop too. My plan is to get the devtools to
> add an option to turn this pref on. So long term, I really think adapting to
> dpis is the way to go.

Filed bug 925333. It's going to be a bit more work so I'm not going to do it right now.
Attached patch patchSplinter Review
Attachment #813417 - Attachment is obsolete: true
Attachment #815454 - Flags: review?(nical.bugzilla)
Attachment #815454 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/f31625242374
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.