Last Comment Bug 1285940 - Remove unnecessary SetTargetAPZC disambiguation code
: Remove unnecessary SetTargetAPZC disambiguation code
Status: RESOLVED FIXED
[gfx-noted] [lang=c++]
:
Product: Core
Classification: Components
Component: Panning and Zooming (show other bugs)
: Trunk
: All All
P5 normal (vote)
: mozilla51
Assigned To: Tanuja
:
: Kartikaya Gupta (email:kats@mozilla.com)
Mentors: Botond Ballo [:botond]
Depends on: 1281575
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-11 05:41 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2016-09-19 10:36 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
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. (109.03 KB, patch)
2016-09-09 02:18 PDT, Tanuja
no flags Details | Diff | Splinter Review
bug1285940.patch (2.99 KB, patch)
2016-09-12 00:35 PDT, Tanuja
botond: feedback+
Details | Diff | Splinter Review
bug1285940.patch (2.61 KB, patch)
2016-09-16 02:51 PDT, Tanuja
no flags Details | Diff | Splinter Review
bug1285940.patch (2.83 KB, patch)
2016-09-17 01:10 PDT, Tanuja
botond: review+
Details | Diff | Splinter Review

Description User image Kartikaya Gupta (email:kats@mozilla.com) 2016-07-11 05:41:06 PDT
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 User image Botond Ballo [:botond] 2016-07-11 08:38:41 PDT
This probably makes a good mentored bug.
Comment 2 User image PRAFFUL MEHROTRA 2016-07-14 09:08:14 PDT
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 User image Kartikaya Gupta (email:kats@mozilla.com) 2016-07-15 11:56:24 PDT
Unfortunately this can't really be worked on until bug 1281575 is landed.
Comment 4 User image Botond Ballo [:botond] 2016-09-07 09:19:30 PDT
Assigning to Tanuja who has expressed interest in working on this.

Note that the prerequisite bug 1281575 has now landed.
Comment 5 User image Tanuja 2016-09-09 02:18:02 PDT
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));
}
Comment 6 User image Botond Ballo [:botond] 2016-09-09 11:41:15 PDT
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 User image Tanuja 2016-09-12 00:35:16 PDT
Created attachment 8790190 [details] [diff] [review]
bug1285940.patch

Sorry for the previous error. I've attached the patch.
Comment 8 User image Botond Ballo [:botond] 2016-09-12 10:33:55 PDT
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 User image Tanuja 2016-09-16 02:51:02 PDT
Created attachment 8791927 [details] [diff] [review]
bug1285940.patch

Please check this patch.
Comment 10 User image Botond Ballo [:botond] 2016-09-16 10:52:19 PDT
Comment on attachment 8791927 [details] [diff] [review]
bug1285940.patch

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

Looks great - thank you!
Comment 11 User image Botond Ballo [:botond] 2016-09-16 11:04:22 PDT
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 User image Tanuja 2016-09-17 01:10:28 PDT
Created attachment 8792180 [details] [diff] [review]
bug1285940.patch

Can you try now?
Comment 13 User image Botond Ballo [:botond] 2016-09-17 11:48:05 PDT
Comment on attachment 8792180 [details] [diff] [review]
bug1285940.patch

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

This applies successfully now, thanks.
Comment 14 User image Pulsebot 2016-09-17 11:49:36 PDT
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
Comment 15 User image Botond Ballo [:botond] 2016-09-17 11:50:29 PDT
I committed the patch to mozilla-inbound. It should be merged to mozilla-central within a day or two.
Comment 16 User image Carsten Book [:Tomcat] 2016-09-19 03:16:51 PDT
https://hg.mozilla.org/mozilla-central/rev/ba5f5d39976d
Comment 17 User image Botond Ballo [:botond] 2016-09-19 10:36:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.