Re: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

[Jan-Marek Glogowski <glogow@xxxxxxxxxx>]

Chiming in a little late.

Am 15.04.19 um 09:50 schrieb Kaganski Mike:
> On 15.04.2019 10:44, Adrien Ollier wrote:
>> OutputDevice::AcquireGraphics() is a pure virtual function, so we cannot 
>> know in the general case what it does.
>> And what is written is not really equivalent to what you wrote because 
>> if AcquireGraphics() is false, there is not a second check for 
>> mpGraphics. That's why it is an issue.
> 
> AcquireGraphics() is not just "any function". Despite someone could of 
> course implement it to play poker, its purpose is to acquire mpGraphics 
> and return if it succeeded. Failing that is programmer's error breaking 
> contract. There's no use to introduce that kind of checks here. Please 
> see existing implementations, like VirtualDevice::AcquireGraphics() in 
> vcl/source/gdi/virdev.cxx.
> 

This is not the nicest API - probably the bool function should be renamed, or
also return the SalGraphics* for consistency; but then naming is hard...

$ git grep AcquireGraphics include/ vcl/inc/
include/vcl/outdev.hxx:    virtual bool AcquireGraphics() const = 0;
vcl/inc/salframe.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salprn.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salvd.hxx:    virtual SalGraphics* AcquireGraphics() = 0;

I was interested in the fact that the bool variant is const, which is a lie.
Looking at the code it's not true and in fact ~ half of OutputDevice's member
variables are mutable. That's probably done to do some lazy init for other const
functions, quite probably all the const font based ones. Don't know if it buys a
lot of time for some usage, or should be factored out somehow... Anyway, I did:

$ git log -G AcquireGraphics include/vcl/outdev.hxx

commit 665bc42d7504c3896a485c8bab17b8eff7d119df
Author: Chris Sherlock <chris.sherlock79@xxxxxxxxx>
Date:   Wed Apr 23 00:57:50 2014 +1000

    Rename VCL's ImplInitGraphics to AcquireGraphics

    Turns out, we don't try to initialize a graphics context, much less
    *acquire* one. e.g. Window instances can have many frames (subwindows),
    of which one some are really being used at any time so we try to
    "steal" one of the graphics contexts from the frame to use ourself,
    later on that frame will steal it from someone else, etc.

    Change-Id: I66d5dbb7015301bc2d2be51627061c91e1f2ee5d

This has nothing to do with the const, but explains the clash - kind of.

Normally the bool variants will - at some point - call
  mpGraphics = ...->AcquireGraphics();

so much about the background stuff I was looking into.
Back to the original mail.

$ git grep "mpGraphics.*AcquireGraphics" vcl/
if ( mpGraphics || AcquireGraphics() )
if ( !mpGraphics && !AcquireGraphics() )
... 69 times

shows this idom is used some times in vcl. And IMHO it should be everywhere
reduced to:

if ( !AcquireGraphics() )
or
if ( AcquireGraphics() )

All the three bool variants start with:

$ git grep "bool.*::AcquireGraphics" vcl
vcl/source/gdi/print.cxx:bool Printer::AcquireGraphics() const
vcl/source/gdi/virdev.cxx:bool VirtualDevice::AcquireGraphics() const
vcl/source/window/window.cxx:bool Window::AcquireGraphics() const

bool *::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

The additional mutex check in debug builds is nice too.

For security I would rename the variants to Do / Impl and add an inline to the
OutputDevice header like so the "header" can't be stripped accidently:

inline bool OutputDevice::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

    return DoAcqureGraphics();
}

That also prevents the function call, if that really matters here.

Anyone want to make this an easy hack?
Revert the function name to ImplInitGraphics?

Comments?

Jan-Marek
_______________________________________________
LibreOffice mailing list
LibreOffice@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/libreoffice