Bug 157062 – Purge PassRefPtr in Modules/battery

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 277452 [details]
Patch

[Alex Christensen]

Comment on attachment 277452 [details]
Patch

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

This isn't compiled on mac, right?

> Source/WebCore/Modules/battery/BatteryController.cpp:84
> -void BatteryController::didChangeBatteryStatus(const AtomicString& eventType, PassRefPtr<BatteryStatus> batteryStatus)
> +void BatteryController::didChangeBatteryStatus(const AtomicString& eventType, RefPtr<BatteryStatus>&& batteryStatus)

That's a lot of copyRef.  If you made this a RefPtr<BatteryStatus>&, it could probably eliminate a lot of them.

> Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:78
> -    RefPtr<BatteryStatus> status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);
> +    Ref<BatteryStatus> status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);

auto

> Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:88
> -    RefPtr<BatteryStatus> status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);
> +    Ref<BatteryStatus> status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);

auto

[Darin Adler]

Comment on attachment 277452 [details]
Patch

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

> Source/WebCore/Modules/battery/BatteryController.cpp:51
> -        batteryManager->updateBatteryStatus(m_batteryStatus);
> +        batteryManager->updateBatteryStatus(WTFMove(m_batteryStatus));

This looks wrong. The old code would not null out m_batteryStatus, but this new code will.

[Gyuyoung Kim]

Created attachment 279624 [details]
Patch

[Gyuyoung Kim]

Sorry for too late update. I'm busy at work lately.

[Darin Adler]

Comment on attachment 279624 [details]
Patch

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

> Source/WebCore/Modules/battery/BatteryClient.h:38
> +void provideBatteryTo(Page*, BatteryClient&);

Should be Page& also.

> Source/WebCore/Modules/battery/BatteryController.cpp:65
>      RefPtr<BatteryStatus> status = batteryStatus;

No need for a local variable now that we are no longer using PassRefPtr.

> Source/WebCore/Modules/battery/BatteryController.cpp:81
>      m_batteryStatus = status.release();

This should be WTFMove instead of release(). The release() function creates a PassRefPtr.

> Source/WebCore/Modules/battery/BatteryController.cpp:87
>      RefPtr<BatteryStatus> battery = batteryStatus;

No need for this local variable once we are no longer using PassRefPtr.

> Source/WebCore/Modules/battery/BatteryController.h:40
> +    void updateBatteryStatus(RefPtr<BatteryStatus>&);

This is wrong; should be RefPtr<BatteryStatus>&&.

> Source/WebCore/Modules/battery/BatteryController.h:41
> +    void didChangeBatteryStatus(const AtomicString& eventType, RefPtr<BatteryStatus>&);

This is wrong; should be BatteryStatus*, not RefPtr.

> Source/WebCore/Modules/battery/BatteryManager.cpp:88
>      m_batteryStatus = batteryStatus;

Needs WTFMove.

> Source/WebCore/Modules/battery/BatteryManager.h:49
> +    void didChangeBatteryStatus(Event&, RefPtr<BatteryStatus>&);

This is wrong; should be RefPtr<BatteryStatus>&&.

> Source/WebCore/Modules/battery/BatteryManager.h:50
> +    void updateBatteryStatus(RefPtr<BatteryStatus>&);

This is wrong; should be BatteryStatus*, not RefPtr.

> Source/WebCore/testing/Internals.cpp:1564
> -    BatteryController::from(document->page())->didChangeBatteryStatus(eventType, BatteryStatus::create(charging, chargingTime, dischargingTime, level));
> +    RefPtr<BatteryStatus> status = BatteryStatus::create(charging, chargingTime, dischargingTime, level);
> +    BatteryController::from(document->page())->didChangeBatteryStatus(eventType, status);

Why the change? The code reads better on a single line, and the use of RefPtr is incorrect here; should be Ref, I think. Normally we’d use auto. But it should just work all on one line.

[Gyuyoung Kim]

Created attachment 279735 [details]
Patch

[Alex Christensen]

Comment on attachment 279735 [details]
Patch

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

> Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:92
>      for (auto* page : m_pageSet) {
>          if (page->corePage())
> -            BatteryController::from(page->corePage())->updateBatteryStatus(status.get());
> +            BatteryController::from(page->corePage())->updateBatteryStatus(WTFMove(status));

Because this could happen more than once, this needs to use copyRef.  It will not have the intended behavior after the first page.

[Gyuyoung Kim]

Created attachment 279751 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #8)
> Comment on attachment 279735 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279735&action=review
> 
> > Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:92
> >      for (auto* page : m_pageSet) {
> >          if (page->corePage())
> > -            BatteryController::from(page->corePage())->updateBatteryStatus(status.get());
> > +            BatteryController::from(page->corePage())->updateBatteryStatus(WTFMove(status));
> 
> Because this could happen more than once, this needs to use copyRef.  It
> will not have the intended behavior after the first page.

yes, it can cause a crash. Fixed.

[Darin Adler]

Comment on attachment 279751 [details]
Patch

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

> Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:78
> +    auto status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);

The storage model of this code seems quite peculiar. It’s so strange that we create a temporary BatteryStatus and call didChangeBatteryStatus and then throw it away. It’s not stored anywhere?

[Gyuyoung Kim]

Comment on attachment 279751 [details]
Patch

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

>> Source/WebKit2/WebProcess/Battery/WebBatteryManager.cpp:78
>> +    auto status = BatteryStatus::create(data.isCharging, data.chargingTime, data.dischargingTime, data.level);
> 
> The storage model of this code seems quite peculiar. It’s so strange that we create a temporary BatteryStatus and call didChangeBatteryStatus and then throw it away. It’s not stored anywhere?

Previous implementation had been passed a pointer from here to BatteryManager class. Then BatteryManager manages it using a RefPtr. I thought that this is fine because BatteryManager will maintain the *BatteryStatus*. But now, as your question, it would be better to pass an ownership of the *BatteryStatus* to BatteryManager.

[Gyuyoung Kim]

Created attachment 279848 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279848 [details]
Patch

Clearing flags on attachment: 279848

Committed r201440: <http://trac.webkit.org/changeset/201440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.