1417549 - IntoLuminanceSource happens on the mainthread even when OMTP is enabled

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Bas Schouten (:bas.schouten)]

Currently IntoLuminanceSource is happening on the main thread even when OMTP is enabled.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1417549: Execute IntoLuminanceSource during replay rather than synchronously.

Review commit: https://reviewboard.mozilla.org/r/199852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199852/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8928991 [details]
Bug 1417549: Execute IntoLuminanceSource during replay rather than synchronously.

https://reviewboard.mozilla.org/r/199852/#review205394


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/SourceSurfaceCapture.cpp:116
(Diff revision 1)
>      DrawingCommand* cmd = iter.Get();
>      cmd->ExecuteOnDT(dt, nullptr);
>    }
> +  if (!mShouldResolveToLuminance) {
> -  return dt->Snapshot();
> +    return dt->Snapshot();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Static Analysis Bot [:clangbot] from comment #2)
> Comment on attachment 8928991 [details]
> Bug 1417549: Execute IntoLuminanceSource during replay rather than
> synchronously.
> 
> https://reviewboard.mozilla.org/r/199852/#review205394
> 
> 
> C/C++ static analysis found 1 defect in this patch.
> 
> You can run this analysis locally with: `./mach static-analysis check
> path/to/file.cpp`
> 
> 
> ::: gfx/2d/SourceSurfaceCapture.cpp:116
> (Diff revision 1)
> >      DrawingCommand* cmd = iter.Get();
> >      cmd->ExecuteOnDT(dt, nullptr);
> >    }
> > +  if (!mShouldResolveToLuminance) {
> > -  return dt->Snapshot();
> > +    return dt->Snapshot();
> > +  } else {
> 
> Warning: Do not use 'else' after 'return' [clang-tidy:
> readability-else-after-return]

I disagree with you Clangbot.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8928991 [details]
Bug 1417549: Execute IntoLuminanceSource during replay rather than synchronously.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199852/diff/1-2/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8928991 [details]
Bug 1417549: Execute IntoLuminanceSource during replay rather than synchronously.

https://reviewboard.mozilla.org/r/199852/#review205562


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/SourceSurfaceCapture.cpp:121
(Diff revision 2)
>      DrawingCommand* cmd = iter.Get();
>      cmd->ExecuteOnDT(dt, nullptr);
>    }
> +  if (!mShouldResolveToLuminance) {
> -  return dt->Snapshot();
> +    return dt->Snapshot();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8928991 [details]
Bug 1417549: Execute IntoLuminanceSource during replay rather than synchronously.

https://reviewboard.mozilla.org/r/199852/#review205536

::: gfx/2d/SourceSurfaceCapture.cpp:16
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace gfx {
>  
> -SourceSurfaceCapture::SourceSurfaceCapture(DrawTargetCaptureImpl* aOwner)
> +SourceSurfaceCapture::SourceSurfaceCapture(DrawTargetCaptureImpl* aOwner,
> +                                           bool aShouldResolveToLuminance /* = false */,

I could potentially see more cases like this coming up, so I'd prefer two separate constructors. One that just takes the DrawTargetCaptureImpl, and one that takes that + LuminanceType & opacity. It should be able to call into the base constructor.

::: gfx/2d/SourceSurfaceCapture.cpp:116
(Diff revision 1)
>      DrawingCommand* cmd = iter.Get();
>      cmd->ExecuteOnDT(dt, nullptr);
>    }
> +  if (!mShouldResolveToLuminance) {
> -  return dt->Snapshot();
> +    return dt->Snapshot();
> +  } else {

It's not really clear to me that this kind of case is a readability problem, but if that's a style rule, so be it...

::: gfx/2d/SourceSurfaceCapture.cpp:18
(Diff revision 2)
>  namespace gfx {
>  
> -SourceSurfaceCapture::SourceSurfaceCapture(DrawTargetCaptureImpl* aOwner)
> +SourceSurfaceCapture::SourceSurfaceCapture(DrawTargetCaptureImpl* aOwner,
> +                                           bool aShouldResolveToLuminance /* = false */,
> +                                           LuminanceType aLuminanceType /* = LuminanceType::LINEARRGB */,
> +                                           Float aOpacity /* = 1.0f */)

I'd prefer to see a separate constructor here, one that takes a DrawTargetCaptureImpl and one that takes a DrawTargetCaptureImpl, LuminanceType, and opacity. The second ctor can call into the first.

Then we don't need the bool parameter or the mShouldResolveToLuminance check below.

::: gfx/2d/SourceSurfaceCapture.cpp:121
(Diff revision 2)
>      DrawingCommand* cmd = iter.Get();
>      cmd->ExecuteOnDT(dt, nullptr);
>    }
> +  if (!mShouldResolveToLuminance) {
> -  return dt->Snapshot();
> +    return dt->Snapshot();
> +  } else {

I disagree with this too. No logic is being shortcut or anything. It's not really clear what readability is gained by eliminating the "else" (maybe it's even less clear).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c992f927177
Execute IntoLuminanceSource during replay rather than synchronously. r=dvander

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6c992f927177