Closed Bug 1259661 Opened 8 years ago Closed 8 years ago

Clean up WidgetMouseEvent

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(10 files)

MozReview Request: Bug 1259661 part.1 Rename WidgetMouseEvent::reasonType to WidgetMouseEvent::Reason r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.2 Rename WidgetMouseEvent::context to WidgetMouseEvent::ContextMenuTrigger r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.3 Rename WidgetMouseEvent::exitType to WidgetMouseEvent::ExitFrom r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.4 Rename WidgetMouseEvent::reason to WidgetMouseEvent::mReason r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.5 Rename WidgetMouseEvent::context to WidgetMouseEvent::mContextMenuTrigger r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.6 Rename WidgetMouseEvent::exit to WidgetMouseEvent::mExitFrom r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.7 Get rid of WidgetMouseEvent::acceptActivation because of unused r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.8 Rename WidgetMouseEvent::ignoreRootScrollFrame to WidgetMouseEvent::mIgnoreRootScrollFrame r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.9 Rename WidgetMouseEvent::clickCount to WidgetMouseEvent::mClickCount r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1259661 part.10 Clean up some nits of WidgetMouseEvent definition r?smaug
58 bytes, text/x-review-board-request
smaug
: review+
Details
Mentor: masayuki
I would like to work on this bug. Can you please assign this to me. Thank you
Thank you, agore1. I'd like to explain what I'd like you to do in this bug:

1. Rename buttonType to ButtonType and define its type as int16_t. I.e.,

typedef int16_t ButtonTypeType;
enum ButtonType : ButtonTypeType
{
  ...
};
ButtonType button;

2. Rename buttonsFlag to ButtonsFlag and define its type as int16_t but buttons shouldn't be the enum because the enum defines bit flags. Therefore, the code should be:

typedef int16_t ButtonFlagType;
enum ButtonFlag : ButtonFlagType
{
 ...
};
ButtonFlagType buttons;

3. Reorder the members for optimizing memory alignment, the order should be:
  1. nsCOMPtr<nsISupports> relatedTarget;
  2. nsString region;
  3. float pressure;
  4. ButtonType button;
  5. ButtonFlagType buttons;
  6. bool hitCluster;

Don't forget to reorder the initialing list of constructor too.

4. Rename relatedTarget to mRelatedTarget.
5. Rename region to mRegion.
6. Rename pressure to mPressure.
7. Rename button to mButton.
8. Rename buttons to mButtons.
9. Rename hitCluster to mHistCluster.

When you modify the code, please make sure that each line is same or less than 80 characters. Even if it's already overflown from this rule, please correct it (this is a change to clean up a lot of ugly lines).

And you should separate the changes to 9 patches as above. Each commit message should starts with "Bug 1259661 part n " and explain what you do in the changeset.

# This is pretty big change, so, if you'd like to work on smaller bug, let me know.
Assignee: nobody → agore1
And the commit message should have " r?masayuki" at the last of the message. Then, please use |hg push -r firstchangeset:lastchangeset review| to request review. Then, I'll post the patches to tryserver for testing your patches don't cause bustage on any platforms.
https://pastebin.mozilla.org/8868532

getting error during build.
Flags: needinfo?(masayuki)
(In reply to agore1 from comment #4)
> https://pastebin.mozilla.org/8868532
> 
> getting error during build.

after implementing 1st part
Of course, I need to know your change ;-)

Could you attach the output of |hg diff|?
Flags: needinfo?(masayuki)
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan] → [good first bug]
Ah, I guess you don't cast the members in nsGUIEventIPC.h like this:
https://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#68-69,79,86
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> Ah, I guess you don't cast the members in nsGUIEventIPC.h like this:
> https://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#68-69,
> 79,86

After trying changes as per your instruction I am getting following errors

https://pastebin.mozilla.org/8868602


the diff file is


https://pastebin.mozilla.org/8868603



please help
Flags: needinfo?(masayuki)
(In reply to agore1 from comment #8)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> > Ah, I guess you don't cast the members in nsGUIEventIPC.h like this:
> > https://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#68-69,
> > 79,86
> 
> After trying changes as per your instruction I am getting following errors
> 
> https://pastebin.mozilla.org/8868602
> 
> 
> the diff file is
> 
> 
> https://pastebin.mozilla.org/8868603

Hmm, I cannot see any text in those URLs. Why don't you attach those text files with using above form?
Flags: needinfo?(masayuki)
Hi I can solve from 4 to 9 but I am getting hard time to solve 1 to 3 and do "./mack build" can you please suggest some other easy bugs..?

thank you
No longer blocks: 1259660
Flags: needinfo?(masayuki)
(In reply to agore1 from comment #10)
> Hi I can solve from 4 to 9 but I am getting hard time to solve 1 to 3

Hmm, that's odd, is this a first time to use C++ for you?

> can you please suggest some other easy bugs..?

Um, there are no bugs which I can suggest to you...
Flags: needinfo?(masayuki)
If you'd like to continue this bug, I'll write part.1-3 for you. Then, you can write 4-last. How about this?
> 1. Rename buttonType to ButtonType and define its type as int16_t. I.e.,
> 
> typedef int16_t ButtonTypeType;
> enum ButtonType : ButtonTypeType
> {
>   ...
> };
> ButtonType button;
> 
> 2. Rename buttonsFlag to ButtonsFlag and define its type as int16_t but buttons shouldn't be the enum because the enum defines bit flags. Therefore, the code should be:
> 
> typedef int16_t ButtonFlagType;
> enum ButtonFlag : ButtonFlagType
> {
>  ...
> };
> ButtonFlagType buttons;

Oh, I'm sorry. I was introduced wrong enums. They are in WidgetMosueEventBase. So, in this bug, needs to rename reasonType, contextType and exitType.
> 3. Reorder the members for optimizing memory alignment, the order should be:
>   1. nsCOMPtr<nsISupports> relatedTarget;
>   2. nsString region;
>   3. float pressure;
>   4. ButtonType button;
>   5. ButtonFlagType buttons;
>   6. bool hitCluster;
> 
> Don't forget to reorder the initialing list of constructor too.
> 
> 4. Rename relatedTarget to mRelatedTarget.
> 5. Rename region to mRegion.
> 6. Rename pressure to mPressure.
> 7. Rename button to mButton.
> 8. Rename buttons to mButtons.
> 9. Rename hitCluster to mHistCluster.

And also, these are WidgetMouseEventBase, sorry for the mistake.
Okay, this bug has been already confused. I take this.
Assignee: agore1 → masayuki
Mentor: masayuki
Status: NEW → ASSIGNED
Whiteboard: [good first bug]
Review commit: https://reviewboard.mozilla.org/r/51849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51849/
Attachment #8751121 - Flags: review?(bugs)
Attachment #8751122 - Flags: review?(bugs)
Attachment #8751123 - Flags: review?(bugs)
Attachment #8751124 - Flags: review?(bugs)
Attachment #8751125 - Flags: review?(bugs)
Attachment #8751126 - Flags: review?(bugs)
Attachment #8751127 - Flags: review?(bugs)
Attachment #8751128 - Flags: review?(bugs)
Attachment #8751129 - Flags: review?(bugs)
Attachment #8751130 - Flags: review?(bugs)
Comment on attachment 8751121 [details]
MozReview Request: Bug 1259661 part.1 Rename WidgetMouseEvent::reasonType to WidgetMouseEvent::Reason r?smaug

https://reviewboard.mozilla.org/r/51849/#review48799

::: widget/MouseEvents.h:184
(Diff revision 1)
>  private:
>    friend class mozilla::dom::PBrowserParent;
>    friend class mozilla::dom::PBrowserChild;
>  
>  public:
> -  enum reasonType
> +  typedef bool ReasonType;

tiny bit surprising to see this kind of typedef but fine.

::: widget/MouseEvents.h:205
(Diff revision 1)
>      eTopLevel
>    };
>  
>  protected:
>    WidgetMouseEvent()
> -    : acceptActivation(false)
> +    : reason(eSynthesized)

I would probably default to eReal here.
Attachment #8751121 - Flags: review?(bugs) → review+
Attachment #8751122 - Flags: review?(bugs) → review+
Comment on attachment 8751122 [details]
MozReview Request: Bug 1259661 part.2 Rename WidgetMouseEvent::context to WidgetMouseEvent::ContextMenuTrigger r?smaug

https://reviewboard.mozilla.org/r/51851/#review48805
Comment on attachment 8751123 [details]
MozReview Request: Bug 1259661 part.3 Rename WidgetMouseEvent::exitType to WidgetMouseEvent::ExitFrom r?smaug

https://reviewboard.mozilla.org/r/51853/#review48807

::: widget/cocoa/nsChildView.mm:4591
(Diff revision 1)
>    NS_OBJC_END_TRY_ABORT_BLOCK;
>  }
>  
>  - (void)sendMouseEnterOrExitEvent:(NSEvent*)aEvent
>                              enter:(BOOL)aEnter
> -                             type:(WidgetMouseEvent::exitType)aType
> +                         exitFrom:(WidgetMouseEvent::ExitFrom)aExitFrom

is there some odd indentation here or is MozReview just showing something random
Attachment #8751123 - Flags: review?(bugs) → review+
Comment on attachment 8751124 [details]
MozReview Request: Bug 1259661 part.4 Rename WidgetMouseEvent::reason to WidgetMouseEvent::mReason r?smaug

https://reviewboard.mozilla.org/r/51855/#review48813
Attachment #8751124 - Flags: review?(bugs) → review+
Comment on attachment 8751125 [details]
MozReview Request: Bug 1259661 part.5 Rename WidgetMouseEvent::context to WidgetMouseEvent::mContextMenuTrigger r?smaug

https://reviewboard.mozilla.org/r/51857/#review48817
Attachment #8751125 - Flags: review?(bugs) → review+
Attachment #8751126 - Flags: review?(bugs) → review+
Comment on attachment 8751126 [details]
MozReview Request: Bug 1259661 part.6 Rename WidgetMouseEvent::exit to WidgetMouseEvent::mExitFrom r?smaug

https://reviewboard.mozilla.org/r/51859/#review48819
Comment on attachment 8751127 [details]
MozReview Request: Bug 1259661 part.7 Get rid of WidgetMouseEvent::acceptActivation because of unused r?smaug

https://reviewboard.mozilla.org/r/51861/#review48825
Attachment #8751127 - Flags: review?(bugs) → review+
Comment on attachment 8751128 [details]
MozReview Request: Bug 1259661 part.8 Rename WidgetMouseEvent::ignoreRootScrollFrame to WidgetMouseEvent::mIgnoreRootScrollFrame r?smaug

https://reviewboard.mozilla.org/r/51863/#review48827
Attachment #8751128 - Flags: review?(bugs) → review+
Comment on attachment 8751129 [details]
MozReview Request: Bug 1259661 part.9 Rename WidgetMouseEvent::clickCount to WidgetMouseEvent::mClickCount r?smaug

https://reviewboard.mozilla.org/r/51865/#review48833
Attachment #8751129 - Flags: review?(bugs) → review+
Comment on attachment 8751130 [details]
MozReview Request: Bug 1259661 part.10 Clean up some nits of WidgetMouseEvent definition r?smaug

https://reviewboard.mozilla.org/r/51867/#review48835
Attachment #8751130 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8751123 [details]
> MozReview Request: Bug 1259661 part.3 Rename WidgetMouseEvent::exitType to
> WidgetMouseEvent::ExitFrom r?smaug
> 
> https://reviewboard.mozilla.org/r/51853/#review48807
> 
> ::: widget/cocoa/nsChildView.mm:4591
> (Diff revision 1)
> >    NS_OBJC_END_TRY_ABORT_BLOCK;
> >  }
> >  
> >  - (void)sendMouseEnterOrExitEvent:(NSEvent*)aEvent
> >                              enter:(BOOL)aEnter
> > -                             type:(WidgetMouseEvent::exitType)aType
> > +                         exitFrom:(WidgetMouseEvent::ExitFrom)aExitFrom
> 
> is there some odd indentation here or is MozReview just showing something
> random

In Objective-C code, we align second or later arguments to first line's ":".
Comment on attachment 8751121 [details]
MozReview Request: Bug 1259661 part.1 Rename WidgetMouseEvent::reasonType to WidgetMouseEvent::Reason r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51849/diff/1-2/
Comment on attachment 8751122 [details]
MozReview Request: Bug 1259661 part.2 Rename WidgetMouseEvent::context to WidgetMouseEvent::ContextMenuTrigger r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51851/diff/1-2/
Comment on attachment 8751123 [details]
MozReview Request: Bug 1259661 part.3 Rename WidgetMouseEvent::exitType to WidgetMouseEvent::ExitFrom r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51853/diff/1-2/
Comment on attachment 8751124 [details]
MozReview Request: Bug 1259661 part.4 Rename WidgetMouseEvent::reason to WidgetMouseEvent::mReason r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51855/diff/1-2/
Comment on attachment 8751125 [details]
MozReview Request: Bug 1259661 part.5 Rename WidgetMouseEvent::context to WidgetMouseEvent::mContextMenuTrigger r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51857/diff/1-2/
Comment on attachment 8751126 [details]
MozReview Request: Bug 1259661 part.6 Rename WidgetMouseEvent::exit to WidgetMouseEvent::mExitFrom r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51859/diff/1-2/
Comment on attachment 8751127 [details]
MozReview Request: Bug 1259661 part.7 Get rid of WidgetMouseEvent::acceptActivation because of unused r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51861/diff/1-2/
Comment on attachment 8751128 [details]
MozReview Request: Bug 1259661 part.8 Rename WidgetMouseEvent::ignoreRootScrollFrame to WidgetMouseEvent::mIgnoreRootScrollFrame r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51863/diff/1-2/
Comment on attachment 8751129 [details]
MozReview Request: Bug 1259661 part.9 Rename WidgetMouseEvent::clickCount to WidgetMouseEvent::mClickCount r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51865/diff/1-2/
Comment on attachment 8751130 [details]
MozReview Request: Bug 1259661 part.10 Clean up some nits of WidgetMouseEvent definition r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51867/diff/1-2/
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: