Move some TabChild functionality into base class to be reusable in Embedlite implementation

RESOLVED FIXED in mozilla31

Status

()

Core
Embedding: APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tatiana, Assigned: tatiana)

Tracking

Trunk
mozilla31
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Embedlite currently duplicate lot's of TabChild functionality https://github.com/tmeshkova/gecko-dev/blob/embedlite_26/embedding/embedlite/utils/TabChildHelper.cpp

We want to share as much functionality as possible and reuse some parts especially "viewport stuff" between b2g/WinMetro and Embedlite.
(Assignee)

Comment 1

3 years ago
Created attachment 8393935 [details] [diff] [review]
Split TabChild functionality into reusable class
Attachment #8393935 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8393935 [details] [diff] [review]
Split TabChild functionality into reusable class

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

This seems reasonable to me, but it needs more documentation about what belongs in TabChild vs TabChildBase. For instance, if I were to add some more stuff how do I know whether it belongs in TabChild or TabChildBase? I don't want to have to go look at the Embedlite code every time.
Attachment #8393935 - Flags: feedback?(bugmail.mozilla) → feedback+
(Assignee)

Comment 3

3 years ago
yep this is very reasonable question, which is hard to answer as is. straight answer is TabChildBase should keep functionality which would be shareable between IPC/ITC(OMTC) implementations. For know we have found hot space is Viewport related functionality and relevant APZC changes. so I want to move it in order to prevent hard time of merges Embedlite branch with upstream.

It would be nice to move UpdateTapState relevant functionality to TabChildBase too, but it is in my TODO list still.
I will try to formulate description of TabChildBase class purpose and move all TabChildBase in single place of TabChild.cpp file. if you want I can event try to move TabChildBase class into separate files
(Assignee)

Comment 4

3 years ago
Created attachment 8395208 [details] [diff] [review]
Split TabChild functionality into reusable class. v2

This is the same version with functionality moved up. plus comment added.
let me know if you want TabChildBase to be moved into separate file
Attachment #8393935 - Attachment is obsolete: true
Attachment #8395208 - Flags: review?(bugmail.mozilla)
Comment on attachment 8395208 [details] [diff] [review]
Split TabChild functionality into reusable class. v2

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

This looks ok to me, but I didn't go through each function to ensure the code wasn't modified. Did you make any changes to the code when shuffling it around? I noticed you added a UpdateFrameHandler helper function which should be fine. Also please push this to try with "-p all -u all -t none" to ensure it doesn't break anything before landing. r=me with the comment updated and question answered.

::: dom/ipc/TabChild.h
@@ +145,5 @@
>  
> +// This is base clase which helps to share Viewport and touch related functionality
> +// between b2g/android FF/embedlite clients implementation.
> +// puth if this class all helper functions, and functionality which could be shared between
> +// Cross process/Cross thread implmentations.

This comment needs to be expanded a bit. Also "puth if this class" doesn't make much sense.
Attachment #8395208 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 6

3 years ago
greentry
See previous version of the patch, it does not have any functional changes, except return values for nsEventStatus DispatchWidgetEvent and HandlePossibleViewportChange.
Filled account reactivation bug 987488, in the meantime
https://tbpl.mozilla.org/?tree=Try&rev=c1fc368b5371
It looks like you landed this on inbound also:

http://hg.mozilla.org/integration/mozilla-inbound/rev/daada068c02f
https://hg.mozilla.org/mozilla-central/rev/daada068c02f
Assignee: nobody → tanya.meshkova
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.