[Pointer Event] Refine pointer events related codes with modern our style

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stone, Assigned: stone)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Blocks: 1303704
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih
(Assignee)

Comment 1

2 years ago
Created attachment 8800159 [details] [diff] [review]
Bug 1307707 - [Pointer Event] Refine pointer events related codes with modern our style
(Assignee)

Comment 2

2 years ago
Comment on attachment 8800159 [details] [diff] [review]
Bug 1307707 - [Pointer Event] Refine pointer events related codes with modern our style

Follow bug 1303704 comments#2 to refine pointer events related implementation.
Attachment #8800159 - Flags: review?(masayuki)
Comment on attachment 8800159 [details] [diff] [review]
Bug 1307707 - [Pointer Event] Refine pointer events related codes with modern our style

I'd be happy if you use MozReview because it's better tool to review like this change (it shows where is changed in each line).

>@@ -744,25 +744,25 @@ public:
>   void ReleasePointerCapture(int32_t aPointerId, ErrorResult& aError)
>   {
>     bool activeState = false;
>     if (!nsIPresShell::GetPointerInfo(aPointerId, activeState)) {
>       aError.Throw(NS_ERROR_DOM_INVALID_POINTER_ERR);
>       return;
>     }
>     nsIPresShell::PointerCaptureInfo* pointerCaptureInfo = nullptr;
>-    if (nsIPresShell::gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&
>+    if (nsIPresShell::sPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&

No, you should separate this line for keeping 80 columns per line rule.

For example,

>    if (nsIPresShell::sPointerCaptureList->Get(aPointerId,
>                                               &pointerCaptureInfo) &&

or

>    if (nsIPresShell::sPointerCaptureList->
>                        Get(aPointerId, &pointerCaptureInfo) &&

or if the last method is too long name,

>    if (nsIPresShell::sPointerCaptureList->
>          Get(aPointerId, &pointerCaptureInfo) &&

>   bool HasPointerCapture(long aPointerId)
>   {
>     nsIPresShell::PointerCaptureInfo* pointerCaptureInfo = nullptr;
>-    if (nsIPresShell::gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&
>+    if (nsIPresShell::sPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&

Same.

Although, I don't like that interface has static member variables and which are accessed directly...

>-  struct PointerCaptureInfo
>+  class PointerCaptureInfo

I think that this is not inherited by anyone. So, should be |class PointerCaptureInfor final|

>   {
>+  public:
>     nsCOMPtr<nsIContent> mPendingContent;
>     nsCOMPtr<nsIContent> mOverrideContent;
>-    bool                 mPrimaryState;
>+    bool mPrimaryState;
> 
>     explicit PointerCaptureInfo(nsIContent* aPendingContent, bool aPrimaryState) :
>       mPendingContent(aPendingContent), mPrimaryState(aPrimaryState)
>     {
>       MOZ_COUNT_CTOR(PointerCaptureInfo);
>     }

In modern style, this should be:

> explicit PointerCaptureInfo(nsIContent* aPendingContent, bool aPrimaryState)
>   : mPendingContent(aPendingContent)
>   , mPrimaryState(aPrimaryState)
> {

because next change of members doesn't cause change different line to add/remove ",".

>-  struct PointerInfo
>+  class PointerInfo

Please use |final|.

>   {
>-    bool      mActiveState;
>-    uint16_t  mPointerType;
>-    bool      mPrimaryState;
>+  public:
>+    bool mActiveState;
>+    uint16_t mPointerType;
>+    bool mPrimaryState;

For the alignment in memory if somebody would add some members here, could you sort this as:

uint16_t
bool
bool

?

>     PointerInfo(bool aActiveState, uint16_t aPointerType, bool aPrimaryState) :
>       mActiveState(aActiveState), mPointerType(aPointerType), mPrimaryState(aPrimaryState) {}

Same, could you change this as:

>     PointerInfo(bool aActiveState, uint16_t aPointerType, bool aPrimaryState)
>       : mActiveState(aActiveState)
>       , mPointerType(aPointerType)
>       , mPrimaryState(aPrimaryState)
>     {
>     }

Although, I don't like the member of constructor. For example, PointerInfo(true, 0, false) isn't clear what are specified. Taking WidgetPointerEvent or WidgetMouseEvent can make this easier to read? But I guess that it's not scope of this bug.

>   };
>   // Keeps information about pointers such as pointerId, activeState, pointerType, primaryState

Could you wrap this too long line?

>-  static nsClassHashtable<nsUint32HashKey, PointerInfo>* gActivePointersIds;
>+  static nsClassHashtable<nsUint32HashKey, PointerInfo>* sActivePointersIds;
> 
>   static void DispatchGotOrLostPointerCaptureEvent(bool aIsGotCapture,
>                                                    uint32_t aPointerId,
>                                                    uint16_t aPointerType,
>                                                    bool aIsPrimary,
>                                                    nsIContent* aCaptureTarget);
>   static void SetPointerCapturingContent(uint32_t aPointerId, nsIContent* aContent);

Same.

>-nsClassHashtable<nsUint32HashKey, nsIPresShell::PointerCaptureInfo>* nsIPresShell::gPointerCaptureList;
>-nsClassHashtable<nsUint32HashKey, nsIPresShell::PointerInfo>* nsIPresShell::gActivePointersIds;
>+nsClassHashtable<nsUint32HashKey, nsIPresShell::PointerCaptureInfo>* nsIPresShell::sPointerCaptureList;
>+nsClassHashtable<nsUint32HashKey, nsIPresShell::PointerInfo>* nsIPresShell::sActivePointersIds;

Same.

>-  if (gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&
>+  if (sPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) &&
>       pointerCaptureInfo) {
>     pointerCaptureInfo->mPendingContent = aContent;
>   } else {
>-    gPointerCaptureList->Put(aPointerId,
>+    sPointerCaptureList->Put(aPointerId,
>                              new PointerCaptureInfo(aContent, GetPointerPrimaryState(aPointerId)));

Same. I guess that you should create a temporary variable to store new PointerCaptureInfo.

> /* static */ nsIContent*
> nsIPresShell::GetPointerCapturingContent(uint32_t aPointerId)
> {
>   PointerCaptureInfo* pointerCaptureInfo = nullptr;
>-  if (gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) && pointerCaptureInfo) {
>+  if (sPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) && pointerCaptureInfo) {

Same, too long.


> void nsIPresShell::InitializeStatics()
> {
>-  NS_ASSERTION(!gPointerCaptureList, "InitializeStatics called multiple times!");
>-  gPointerCaptureList = new nsClassHashtable<nsUint32HashKey, PointerCaptureInfo>;
>-  gActivePointersIds = new nsClassHashtable<nsUint32HashKey, PointerInfo>;
>+  NS_ASSERTION(!sPointerCaptureList, "InitializeStatics called multiple times!");

If this isn't hit in any automated tests, I'd like you to use MOZ_ASSERT().

>+  sPointerCaptureList = new nsClassHashtable<nsUint32HashKey, PointerCaptureInfo>;

Too long, please wrap this line after |=|.

> void nsIPresShell::ReleaseStatics()
> {
>-  NS_ASSERTION(gPointerCaptureList, "ReleaseStatics called without Initialize!");
>-  delete gPointerCaptureList;
>-  gPointerCaptureList = nullptr;
>-  delete gActivePointersIds;
>-  gActivePointersIds = nullptr;
>+  NS_ASSERTION(sPointerCaptureList, "ReleaseStatics called without Initialize!");

Same, I like MOZ_ASSERT().


I'd like to check new patch. Thank you for cleaning up the code. The code easier to read should include less bugs.
Attachment #8800159 - Flags: review?(masayuki) → review-
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8800535 [details]
Bug 1307707 - [Pointer Event] Refine pointer events related codes with modern our style.

https://reviewboard.mozilla.org/r/85456/#review84358

Thank you for cleaning up!

::: layout/base/nsIPresShell.h
(Diff revision 1)
> -  struct PointerCaptureInfo
> +  class PointerCaptureInfo final
>    {
> +  public:
>      nsCOMPtr<nsIContent> mPendingContent;
>      nsCOMPtr<nsIContent> mOverrideContent;
> -    bool                 mPrimaryState;

Looks great to remove this.  But if I were you, I'd separate this patch to 2 or more patches for each patch fixing only one issue.

FYI: You can use autoland from the MozReview's [Automation] -> [Land Commits] from "Review Summary" to land this patch.
Attachment #8800535 - Flags: review?(masayuki) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8801026 [details] [diff] [review]
Bug 1307707 Part1: [Pointer Event] Refine pointer events related codes with modern our style. r=masayuki

Separated the original patch to 2 and updated the patch summary
Attachment #8800159 - Attachment is obsolete: true
Attachment #8800535 - Attachment is obsolete: true
Attachment #8801026 - Flags: review+
(Assignee)

Comment 7

2 years ago
Created attachment 8801027 [details] [diff] [review]
Bug 1307707 Part2: [Pointer Event] Remove redundant member variable PointerCaptureInfo::mPrimaryState. r=masayuki

Separated the original patch to 2 and updated the patch summary
Attachment #8801027 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
> Keywords: checkin-needed

Don't you have enough permission to land them? If you have, you can land them from MozReview only with 3 clicks.

Comment 10

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b4e8981076
Part 1: [Pointer Event] Refine pointer events related codes with modern our style. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/00fd40882351
Part 2: [Pointer Event] Remove redundant member variable PointerCaptureInfo::mPrimaryState. r=masayuki
Keywords: checkin-needed
(Assignee)

Comment 11

2 years ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #9)
> Don't you have enough permission to land them? If you have, you can land
> them from MozReview only with 3 clicks.

Thanks for this information.

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2b4e8981076
https://hg.mozilla.org/mozilla-central/rev/00fd40882351
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.