Remove unnecessary SetTargetAPZC disambiguation code

RESOLVED FIXED in Firefox 51

Status

()

Core
Panning and Zooming
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kats, Assigned: Tanuja, Mentored)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

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
OS: Unspecified → All
Priority: -- → P5
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → Trunk
This probably makes a good mentored bug.
Mentor: botond@mozilla.com
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++]

Comment 2

a year ago
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.
Unfortunately this can't really be worked on until bug 1281575 is landed.
Assigning to Tanuja who has expressed interest in working on this.

Note that the prerequisite bug 1281575 has now landed.
Assignee: nobody → f2013658
(Assignee)

Comment 5

a year ago
Created 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.

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));
}
Attachment #8789710 - Flags: review?(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.
Attachment #8789710 - Flags: review?(botond)
(Assignee)

Comment 7

a year ago
Created attachment 8790190 [details] [diff] [review]
bug1285940.patch

Sorry for the previous error. I've attached the patch.
Attachment #8790190 - Flags: review?(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.
Attachment #8790190 - Flags: review?(botond) → feedback+
Attachment #8789710 - Attachment is obsolete: true
(Assignee)

Comment 9

a year ago
Created attachment 8791927 [details] [diff] [review]
bug1285940.patch

Please check this patch.
Attachment #8791927 - Flags: review?(botond)
Comment on attachment 8791927 [details] [diff] [review]
bug1285940.patch

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

Looks great - thank you!
Attachment #8791927 - Flags: review?(botond) → review+
Attachment #8790190 - Attachment is obsolete: true
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!
Flags: needinfo?(f2013658)
Attachment #8791927 - Flags: review+
(Assignee)

Comment 12

a year ago
Created attachment 8792180 [details] [diff] [review]
bug1285940.patch

Can you try now?
Attachment #8792180 - Flags: review?(botond)
Comment on attachment 8792180 [details] [diff] [review]
bug1285940.patch

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

This applies successfully now, thanks.
Attachment #8792180 - Flags: review?(botond) → review+
Attachment #8791927 - Attachment is obsolete: true

Comment 14

a year ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5f5d39976d
Remove code to disambiguate overloads that is no longer necessary. r=botond
I committed the patch to mozilla-inbound. It should be merged to mozilla-central within a day or two.
Flags: needinfo?(f2013658)

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba5f5d39976d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
You need to log in before you can comment on or make changes to this bug.