1285940 - Remove unnecessary SetTargetAPZC disambiguation code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P5)

Description

[Kartikaya Gupta (email:[e-mail])]

At [1] and [2], we have some code to get a function pointer to a specific overload of APZCTreeManager::SetTargetAPZC. Bug 1281575 changes these callsites to refer to IAPZCTreeManager, and that has only one version of SetTargetAPZC. So we should be able to clean up those callsites a little bit to remove the goop.

[1] http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/gfx/layers/ipc/RemoteContentController.cpp#226
[2] http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/widget/nsBaseWidget.cpp#1043

Comment 1

[Botond Ballo [:botond]]

This probably makes a good mentored bug.

Comment 2

[PRAFFUL MEHROTRA]

I would like to work on this bug. I am a newbie and would require your help to solve it. I have prior coding experience in C/C++ and JAVA.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Unfortunately this can't really be worked on until bug 1281575 is landed.

Comment 4

[Botond Ballo [:botond]]

Assigning to Tanuja who has expressed interest in working on this.

Note that the prerequisite bug 1281575 has now landed.

Comment 5

[Tanuja]

Attachment: Fixed bug 1285940 by passing the pointer to the member function directly as an argument to NewCallableMethod, without declaring a variable as an intermediate step.

Replaced

	void
	nsBaseWidget::SetConfirmedTargetAPZC(uint64_t aInputBlockId,
	                                     const nsTArray<ScrollableLayerGuid>& aTargets) const
	{
	  // Need to specifically bind this since it's overloaded.
	  void (IAPZCTreeManager::*setTargetApzcFunc)(uint64_t, const nsTArray<ScrollableLayerGuid>&)
	          = &IAPZCTreeManager::SetTargetAPZC;
	  APZThreadUtils::RunOnControllerThread(NewRunnableMethod
	    <uint64_t, StoreCopyPassByRRef<nsTArray<ScrollableLayerGuid>>>(mAPZC,
	                                                                   setTargetApzcFunc,
	                                                                   aInputBlockId, aTargets));
	}
in nsBaseWidget.cpp (line:1024)

with

void
nsBaseWidget::SetConfirmedTargetAPZC(uint64_t aInputBlockId,
                                     const nsTArray<ScrollableLayerGuid>& aTargets) const
{
  // Need to specifically bind this since it's overloaded.

  APZThreadUtils::RunOnControllerThread(NewRunnableMethod
    <uint64_t, StoreCopyPassByRRef<nsTArray<ScrollableLayerGuid>>>(mAPZC,
                                                                   &IAPZCTreeManager::SetTargetAPZC,
                                                                   aInputBlockId, aTargets));
}

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8789710 [details] [diff] [review]
Fixed bug 1285940 by passing the pointer to the member function directly as an argument to NewCallableMethod, without declaring a variable as an intermediate step.

Thanks for working on this!

This attachment isn't a patch, it's the modified version of the entire nsBaseWidget.cpp file.

Please generate a patch file as follows:

  - Commit the changes you made locally with "hg commit"
  - Generate a patch file with "hg export . > bug1285940.patch"

and then upload the generated patch file (bug1285940.patch) as an attachment.

Comment 7

[Tanuja]

Attachment: bug1285940.patch

Sorry for the previous error. I've attached the patch.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8790190 [details] [diff] [review]
bug1285940.patch

Review of attachment 8790190 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This patch generally looks good, I just have a few comments:

Could you please upload a new version of the patch with the comments addressed?

::: browser/themes/linux/browser.css
@@ +1535,4 @@
>  #tabbrowser-tabs {
>    /* override the global style to allow the selected tab to be above the nav-bar */
>    z-index: auto;
> +  background-color: #FC0FC0;

This is an unrelated change, which should not be included in this patch.

::: gfx/layers/ipc/APZCTreeManagerParent.cpp
@@ +186,3 @@
>  
> +
> +APZThreadUtils::RunOnControllerThread(NewRunnableMethod

Please keep this line intended one level (two spaces) from the beginning of the line. Also, remove the added new line before it.

::: widget/nsBaseWidget.cpp
@@ +1021,5 @@
>  void
>  nsBaseWidget::SetConfirmedTargetAPZC(uint64_t aInputBlockId,
>                                       const nsTArray<ScrollableLayerGuid>& aTargets) const
>  {
>    // Need to specifically bind this since it's overloaded.

This comment isn't necessary any more.

Comment 9

[Tanuja]

Attachment: bug1285940.patch

Please check this patch.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8791927 [details] [diff] [review]
bug1285940.patch

Review of attachment 8791927 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - thank you!

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8791927 [details] [diff] [review]
bug1285940.patch

Unfortunately, I'm not able to apply this patch. When I try, I get:

patch: **** malformed patch at line 36:      const ScrollableLayerGuid& aGuid,

Did you perhaps try to modify the patch file manually to address my comments? If so, that would explain why the patch is malformed; patch files are not meant to be modified manually. Could you please make the requested changes to the source files instead, and then create a new patch? Thanks!

Comment 12

[Tanuja]

Attachment: bug1285940.patch

Can you try now?

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8792180 [details] [diff] [review]
bug1285940.patch

Review of attachment 8792180 [details] [diff] [review]:
-----------------------------------------------------------------

This applies successfully now, thanks.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5f5d39976d
Remove code to disambiguate overloads that is no longer necessary. r=botond

Comment 15

[Botond Ballo [:botond]]

I committed the patch to mozilla-inbound. It should be merged to mozilla-central within a day or two.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba5f5d39976d

Comment 17

[Botond Ballo [:botond]]

The patch has now merged into mozilla-central. Thanks, Tanuja!

If you're interested in working on some other bugs, and would like some suggestions, let me know.