Closed Bug 1417549 Opened 2 years ago Closed 2 years ago

IntoLuminanceSource happens on the mainthread even when OMTP is enabled

Categories

(Core :: Graphics, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(1 file)

Currently IntoLuminanceSource is happening on the main thread even when OMTP is enabled.
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]
(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 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 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).
Attachment #8928991 - Flags: review?(dvander) → review+
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c992f927177
Execute IntoLuminanceSource during replay rather than synchronously. r=dvander
https://hg.mozilla.org/mozilla-central/rev/6c992f927177
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.