Bug 214896 – ProcessThrottler should register itself as client to its new assertion if already registered to the old assertion

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Page Loading

[youenn fablet]

ProcessThrottler should register itself as client to its new assertion if already registered to the old assertion

[youenn fablet]

Created attachment 405416 [details]
Patch

[Chris Dumez]

Comment on attachment 405416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review

> Source/WebKit/UIProcess/ProcessThrottler.cpp:138
> +        m_assertion->setClient(*this);

Good catch but I think this should be done unconditionally.

[Chris Dumez]

Comment on attachment 405416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review

>> Source/WebKit/UIProcess/ProcessThrottler.cpp:138
>> +        m_assertion->setClient(*this);
> 
> Good catch but I think this should be done unconditionally.

Also we probably want to drop the m_assertion->setClient(*this) call in ProcessThrottler::didConnectToProcess()

[Chris Dumez]

Comment on attachment 405416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405416&action=review

> Source/WebKit/UIProcess/ProcessThrottler.cpp:129
> +    bool hasClient = !!m_assertion->client();

m_assertion can be null so this will crash.

[youenn fablet]

Created attachment 405445 [details]
Patch

[youenn fablet]

<rdar://problem/65138628>

[Chris Dumez]

Comment on attachment 405445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405445&action=review

> Source/WebKit/UIProcess/ProcessAssertion.h:69
> +    ProcessAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr);

It is not really less error-prone if the parameter is optional.. Also, why can it be null?

> Source/WebKit/UIProcess/ProcessAssertion.h:100
> +    ProcessAndUIAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr);

Ditto.

> Source/WebKit/UIProcess/ProcessThrottler.h:54
> +class ProcessThrottler : public CanMakeWeakPtr<ProcessThrottler>, public ProcessAssertion::Client {

Why is this needed?

[Chris Dumez]

Comment on attachment 405445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405445&action=review

>> Source/WebKit/UIProcess/ProcessAssertion.h:69
>> +    ProcessAssertion(ProcessID, const String& reason, ProcessAssertionType, Client* = nullptr);
> 
> It is not really less error-prone if the parameter is optional.. Also, why can it be null?

Ok, I see that ProcessAssertion is used by other things than ProcessThrottler (like DownloadMap) and those don't set themselves as client. So using a raw pointer seems OK. That said, I don't think the parameter should be optional. If it is optional, then it is not any less error-prone than setting the client later using a setter.

[youenn fablet]

> Ok, I see that ProcessAssertion is used by other things than
> ProcessThrottler (like DownloadMap) and those don't set themselves as
> client. So using a raw pointer seems OK. That said, I don't think the
> parameter should be optional. If it is optional, then it is not any less
> error-prone than setting the client later using a setter.

Plan is to make it mandatory and pass a ref instead of a pointer.
But this should be a follow-up because of other assertions (download and media ones at least).
Probably we should be also using a WeakPtr instead of a pointer.

[youenn fablet]

Keeping it optional just reduces the size of the patch basically.

[Chris Dumez]

(In reply to youenn fablet from comment #9)
> > Ok, I see that ProcessAssertion is used by other things than
> > ProcessThrottler (like DownloadMap) and those don't set themselves as
> > client. So using a raw pointer seems OK. That said, I don't think the
> > parameter should be optional. If it is optional, then it is not any less
> > error-prone than setting the client later using a setter.
> 
> Plan is to make it mandatory and pass a ref instead of a pointer.
> But this should be a follow-up because of other assertions (download and
> media ones at least).
> Probably we should be also using a WeakPtr instead of a pointer.

I think parameter should be mandatory. Clients can explicitly pass null for now.

[Chris Dumez]

(In reply to youenn fablet from comment #10)
> Keeping it optional just reduces the size of the patch basically.

But makes it error prone so I am against it.

[Chris Dumez]

(In reply to youenn fablet from comment #10)
> Keeping it optional just reduces the size of the patch basically.

There are not that many call sites and it is just an extra parameter. If you want to keep the patch small, then we don't have to do the refactoring at all and we can keep setClient().

[Chris Dumez]

I uploaded a refactoring proposal at Bug 214976 because I don't like the Client interface. If you land this fix first, I will rebase my patch.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> I uploaded a refactoring proposal at Bug 214976 because I don't like the
> Client interface. If you land this fix first, I will rebase my patch.

My personal preference would be you landing the 2 line change to fix the bug here (moving setClient from didConnectToProcess to setAssertionType) and then doing the refactoring in Bug 214976.

[youenn fablet]

Fixed by https://trac.webkit.org/r265162