Closed Bug 1259669 Opened 4 years ago Closed 4 years ago

Clean up WidgetCommandEvent

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: masayuki, Assigned: takahirox, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug])

Attachments

(2 files)

Hi, can I take this?
I'm new and would like to try.
(In reply to Takahiro Aoyagi from comment #1)
> Hi, can I take this?
> I'm new and would like to try.

Of course. Do I need to explain how to fix this?
Flags: needinfo?(hogehoge)
Just in case, could you explain tho I think I basically understand from bug 1259654?
Flags: needinfo?(hogehoge)
Okay, so, this is one of the simplest fix of blocking bug 1259654.

WidgetCommandEvent is defined in widget/MiscEvents.h and its member |command| doesn't conform to our coding rules. In our coding rules, all members should have "m" prefix. So, |command| should be renamed to |mCommand|.

https://dxr.mozilla.org/mozilla-central/source/widget/MiscEvents.h#104,113,124,130

I guess that if you build Firefox after writing a patch, it must be correct fix. But I'll post your patch to tryserver to test to build in whole platforms we support.

Do you know how to write patch and request review? If not, please read following documents:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html
Especially here:
http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/firefoxworkflow.html

If you have some questions, feel free to ask me. (I should write simpler tutorial in my blog, though...)
Attached patch bug1259669.patchSplinter Review
Hi, I've made a patch and succeeded in build with this change.

I'm not confident about how to make a patch.
The following commands are what I did.
Is this a correct flow? Or is there any better ways?

$ hg clone
$ change code
$ hg commit -m "comment"
$ hg export -r changeset -o bug1259669.patch
Attachment #8742156 - Flags: review?(masayuki)
First, thank you for your work!

(In reply to Takahiro Aoyagi (github@takahirox) from comment #5)
> Created attachment 8742156 [details] [diff] [review]
> bug1259669.patch
> 
> Hi, I've made a patch and succeeded in build with this change.
> 
> I'm not confident about how to make a patch.
> The following commands are what I did.
> Is this a correct flow? Or is there any better ways?
> 
> $ hg clone

Did you do ./mach mercurial-setup? Then, you can set up whole special utilities for developing Gecko/Firefox.

> $ change code
> $ hg commit -m "comment"

Exactly.

> $ hg export -r changeset -o bug1259669.patch

If you did the setup and set up SSH connection which is documented here:
http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/auth.html#ssh-configuration

Then, you must be able to do this:

$ hg push -c changeset review

or

$ hg push -r firstchangeset:lastchangeset review

Then, your patch will be sent to ReviewBoard automatically. After that I can build your patch on all platforms on tryserver from ReviewBoard. Could you do that?  If you couldn't set up ReviewBoard and SSH related settings, I'd try to post your attached patch to tryserver by my hand.
Yeah, I ran ./mach mercurial-setup with all Y to wizard.

But I've failed to run "hg push -c changeset review" due to permission denied(publickey).
I login to bugzilla with GitHub account but should I get a bugzilla account to push to review?

$ cat ~/.ssh/config
Host hg.mozilla.org
        User hogehoge@gachapin.jp

Host reviewboard-hg.mozilla.org
        User hogehoge@gachapin.jp

$ hg push -c 293163:fec01c615b5b review
pushing to ssh://reviewboard-hg.mozilla.org/gecko
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
Ikezoe-san, do you know how to fix the issue mentioned in comment 7?
Flags: needinfo?(hiikezoe)
(In reply to Takahiro Aoyagi (github@takahirox) from comment #7)
> Yeah, I ran ./mach mercurial-setup with all Y to wizard.
> 
> But I've failed to run "hg push -c changeset review" due to permission
> denied(publickey).
> I login to bugzilla with GitHub account but should I get a bugzilla account
> to push to review?
> 
> $ cat ~/.ssh/config
> Host hg.mozilla.org
>         User hogehoge@gachapin.jp
> 
> Host reviewboard-hg.mozilla.org
>         User hogehoge@gachapin.jp
> 
> $ hg push -c 293163:fec01c615b5b review
> pushing to ssh://reviewboard-hg.mozilla.org/gecko

The problem is here. This should be https://reviewboard-hg.mozilla.org/autoreview .
You need below line in your ~/.hgrc or .hg/hgrc in source tree.

review = https://reviewboard-hg.mozilla.org/autoreview
Flags: needinfo?(hiikezoe)
Thanks guys, I should've read this page

http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-mercurial.html

Anyways, I seem like succeeded in push to review.

https://reviewboard.mozilla.org/r/47413/
Good to hear. Not published the review yet. ;-)
Comment on attachment 8742683 [details]
MozReview Request: Bug 1259669 Rename WidgetCommandEvent::command to WidgetCommandEvent::mCommand r=masayuki

https://reviewboard.mozilla.org/r/47415/#review44125

Okay, all green. Could you change the commit message to:

"Bug 1259669 Rename WidgetCommandEvent::command to WidgetCommandEvent::mCommand r=masayuki"

for consistency with other commits (you should write the detail of what you did in the changeset) and push it to review again? Then, I'll land the patch to mozilla-inbound.

Note that you can change the commit message with |hg histedit| and modify "pick" to "m" of the text file opened by histedit and save and quit the editor. Then, you can modify the commit message.

Thanks in advance!
Attachment #8742683 - Flags: review+
Comment on attachment 8742683 [details]
MozReview Request: Bug 1259669 Rename WidgetCommandEvent::command to WidgetCommandEvent::mCommand r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47415/diff/1-2/
Attachment #8742683 - Attachment description: MozReview Request: Bug 1259669 - Clean up WidgetCommandEvent → MozReview Request: Bug 1259669 Rename WidgetCommandEvent::command to WidgetCommandEvent::mCommand r=masayuki
My commit log update went on correctly?
Oh, sorry, I missed to catch your patch failed to autoland into mozilla-inbound. I'll try to land it manually.
Thanks

> author	Takahiro Aoyagi <github@takahirox>

If possible, could you replace my mail address with hogehoge@gachapin.jp?
https://hg.mozilla.org/mozilla-central/rev/7aeac9da4265
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to Takahiro Aoyagi (takahirox@github) from comment #19)
> Thanks
> 
> > author	Takahiro Aoyagi <github@takahirox>
> 
> If possible, could you replace my mail address with hogehoge@gachapin.jp?

Oh, I'm very sorry. I copied it from your bugzilla account name at committing your patch into my local repository manually (due to conflicting with other commits). The patch was landed to Mozilla 49, fortunately. So, I can reland it with the email address again. However, even if I'll do that, the old commit log isn't removed (modified), unfortunately. But if you want me to do that, I'll do it.
Flags: needinfo?(hogehoge)
Assignee: nobody → hogehoge
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan] → [good first bug]
Attachment #8742156 - Flags: review?(masayuki)
So, could you do that for me if it wouldn't be too much trouble for you.

> However, even if I'll do that, the old commit log isn't removed (modified), unfortunately.

Yeah, that't ok.
(In reply to Takahiro Aoyagi (takahirox@github) from comment #22)
> So, could you do that for me if it wouldn't be too much trouble for you.
> 
> > However, even if I'll do that, the old commit log isn't removed (modified), unfortunately.
> 
> Yeah, that't ok.

Sorry for the delay, I relanded your patch with right email address right now.
Flags: needinfo?(hogehoge)
Thanks! I had a great experience. I'll try to keep contributing to Mozilla!
You need to log in before you can comment on or make changes to this bug.