Replace ENSURE_TRUE in nsFrame.h with NS_ENSURE_TRUE_VOID

RESOLVED FIXED in Firefox 63

Status

()

P4
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: TYLin, Assigned: sahilbhosale63, Mentored)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [good first bug] )

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 months ago
ENSURE_TRUE in nsFrame.h [1] is equivalent to NS_ENSURE_TRUE_VOID in nsDebug.h [2]. We can replace ENSURE_TRUE by NS_ENSURE_TRUE_VOID, and remove ENSURE_TRUE to avoid defining the macro twice.

[1] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsFrame.h#893-901

[2] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/xpcom/base/nsDebug.h#232
(Reporter)

Updated

8 months ago
Whiteboard: [good first bug]

Comment 1

8 months ago
Can I work on this?
(Reporter)

Comment 2

8 months ago
Sure. Feel free to work on this. If you have any questions about building and testing Firefox, or submit a patch for review. There is some good documentation on MDN such as https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Comment 3

8 months ago
Sorry if this isn't the right place to ask for build help, but when I try to run the ./mach bootstrap (I'm on Windows 10, python 3.5.2 (Although the mozilla shell shows 2.7). I get the error

Error running mach:

    ['bootstrap']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

TypeError: argument of type 'NoneType' is not iterable

  File "c:\mozilla-source\mozilla-central\python/mozboot/mozboot/mach_commands.py", line 32, in bootstrap
    bootstrapper.bootstrap()
  File "c:\mozilla-source\mozilla-central\python/mozboot\mozboot\bootstrap.py", line 283, in bootstrap
    hg_installed, hg_modern = self.instance.ensure_mercurial_modern()
  File "c:\mozilla-source\mozilla-central\python/mozboot\mozboot\mozillabuild.py", line 29, in ensure_mercurial_modern
    self.run([self.which('pip'), 'install', '--upgrade', 'Mercurial'])
  File "c:\mozilla-source\mozilla-central\python/mozboot\mozboot\mozillabuild.py", line 55, in run
    subprocess.check_call(command, stdin=sys.stdin)
  File "c:\mozilla-build\python\lib\subprocess.py", line 185, in check_call
    retcode = call(*popenargs, **kwargs)
  File "c:\mozilla-build\python\lib\subprocess.py", line 172, in call
    return Popen(*popenargs, **kwargs).wait()
  File "c:\mozilla-build\python\lib\subprocess.py", line 394, in __init__
    errread, errwrite)
  File "c:\mozilla-build\python\lib\subprocess.py", line 599, in _execute_child
    args = list2cmdline(args)
  File "c:\mozilla-build\python\lib\subprocess.py", line 266, in list2cmdline
    needquote = (" " in arg) or ("\t" in arg) or not arg


The weird thing is, it used to fail on something else (The directory where my "Home" environment variable/where echo ~ would print had already been deleted to changed it to ;

What should I do? I'm thinking about reformatting my computer.
(Reporter)

Comment 4

8 months ago
Sorry, I'm not familiar with the Windows build environment. Perhaps you could try asking in #introduction on irc.mozilla.org. There should be some developers using Windows. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Need_help
(Assignee)

Comment 5

7 months ago
Can I work on this issue?
Flags: needinfo?(aethanyc)
(Reporter)

Comment 6

7 months ago
This bug has no progress so far, so anyone is welcome to contribute a patch :)
Flags: needinfo?(aethanyc)
(Assignee)

Comment 7

7 months ago
(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #0)
> ENSURE_TRUE in nsFrame.h [1] is equivalent to NS_ENSURE_TRUE_VOID in
> nsDebug.h [2]. We can replace ENSURE_TRUE by NS_ENSURE_TRUE_VOID, and remove
> ENSURE_TRUE to avoid defining the macro twice.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsFrame.h#893-901
> 
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 085cdfb90903d4985f0de1dc7786522d9fb45596/xpcom/base/nsDebug.h#232

When I made this changes and started to compile it gives me compilation error:

 3:23.36 In file included from /home/sahil/src/mozilla-central/obj-x86_64-pc-linux-gnu/layout/generic/Unified_cpp_layout_generic2.cpp:11:
 3:23.36 /home/sahil/src/mozilla-central/layout/generic/nsFrameSetFrame.cpp:1293:3: error: use of undeclared identifier 'ENSURE_TRUE'
 3:23.36   ENSURE_TRUE(weakFrame.IsAlive());
 3:23.37   ^
 3:24.69 1 warning generated.
 3:26.65 1 warning and 1 error generated.
 3:26.67 /home/sahil/src/mozilla-central/config/rules.mk:1088: recipe for target 'Unified_cpp_layout_generic2.o' failed
 3:26.67 make[4]: *** [Unified_cpp_layout_generic2.o] Error 1
 3:26.67 make[4]: *** Waiting for unfinished jobs....
 3:32.45 1 warning generated.
 3:38.06 1 warning generated.
 3:41.82 1 warning generated.
 3:41.86 /home/sahil/src/mozilla-central/config/recurse.mk:74: recipe for target 'layout/generic/target' failed
 3:41.86 make[3]: *** [layout/generic/target] Error 2
 3:47.18 1 warning generated.
 3:47.22 /home/sahil/src/mozilla-central/config/recurse.mk:32: recipe for target 'compile' failed
 3:47.22 make[2]: *** [compile] Error 2
 3:47.22 /home/sahil/src/mozilla-central/config/rules.mk:423: recipe for target 'default' failed
 3:47.22 make[1]: *** [default] Error 2
 3:47.22 client.mk:150: recipe for target 'build' failed
 3:47.22 make: *** [build] Error 2
 3:47.25 254 compiler warnings present.
(Reporter)

Comment 8

7 months ago
(In reply to Sahil Bhosale from comment #7)
 
>  3:23.36 In file included from
> /home/sahil/src/mozilla-central/obj-x86_64-pc-linux-gnu/layout/generic/
> Unified_cpp_layout_generic2.cpp:11:
>  3:23.36
> /home/sahil/src/mozilla-central/layout/generic/nsFrameSetFrame.cpp:1293:3:
> error: use of undeclared identifier 'ENSURE_TRUE'
>  3:23.36   ENSURE_TRUE(weakFrame.IsAlive());

Looks like you forget to change ENSURE_TRUE to NS_ENSURE_TRUE_VOID?
(Assignee)

Comment 9

7 months ago
Posted patch tip2.patch (obsolete) — Splinter Review
I have changed the ENSURE_TRUE to NS_ENSURE_TRUE_VOID.
Attachment #8999447 - Flags: review?(aethanyc)
(Reporter)

Comment 10

7 months ago
Comment on attachment 8999447 [details] [diff] [review]
tip2.patch

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

This looks good to me. I'll accept this patch if you address my other inline comment.

::: layout/generic/nsFrame.h
@@ +897,3 @@
>  // End Display Reflow Debugging
>  
>  // similar to NS_ENSURE_TRUE but with no return value

No other code uses ENSURE_TRUE() macro after your patch, so please delete the comment as well as this macro definition.
Attachment #8999447 - Flags: review?(aethanyc)
(Reporter)

Updated

7 months ago
Assignee: nobody → sahilbhosale63
Status: NEW → ASSIGNED
(Reporter)

Comment 11

7 months ago
Also, your patch lacks a proper commit message to be able to check in. You can use something like

   Bug 1479605 - Replace ENSURE_TRUE in nsFrame.h by NS_ENSURE_TRUE_VOID.

If you're using hg, commit with your changes and the message, and use "hg export . > my-change.patch" to make sure the patch is generated with the commit message. If you're using git, it's "git format-patch @~".
(Assignee)

Comment 12

7 months ago
Posted patch tip.patch (obsolete) — Splinter Review
Replace ENSURE_TRUE in nsFrame.h by NS_ENSURE_TRUE_VOID.
Attachment #8999480 - Flags: review?(aethanyc)
(Reporter)

Comment 13

7 months ago
Comment on attachment 8999480 [details] [diff] [review]
tip.patch

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

I still don't see the commit message in this patch. If you have any problem with the step in comment 11, please let me know. Or I can help adding a commit message for you before landing the patch if you'd like. Thanks.

::: layout/generic/nsFrame.h
@@ -898,2 @@
>  
> -// similar to NS_ENSURE_TRUE but with no return value

Err, sorry to be unclear in comment 10. I mean delete the original line 899 - 906, i.e. the following.

  899 // similar to NS_ENSURE_TRUE ...
      ...
  906 PR_END_MACRO

NS_ENSURE_TRUE_VOID is already defined in xpcom/base/nsDebug.h, so no need to redefine it here.
Attachment #8999480 - Flags: review?(aethanyc)
(Assignee)

Comment 14

7 months ago
Yes I am getting a problem while commit as when create my patch using hg diff >tip patch and hg export -o FILE then everything is fine but after that if I run hg commit then vi editor gets open and when I type the commit message and save then after that the contents in my patch file disappears and my tip.patch file becomes blank. 

From yesterday only I am facing this problem can you please help? 

Even I tried to delete the source code and re-download and compiled it and made changes and created the patch and after running hg commit the same thing happen i.e my tip.patch file became blank and all my contents in the patch file lost.
(Reporter)

Comment 15

7 months ago
Hmm ... I cannot see the problem from your description. In theory, after you commit with "hg commit", and you are able to see your modification by "hg log -p -r .", then "hg export -r . -o my.patch" should generate the same patch content.

Anyway, don't worry about the commit message too much, I can fix the message for your in your new patch after you address my request in comment 13.
(Assignee)

Comment 16

7 months ago
Posted patch final.patch (obsolete) — Splinter Review
The changes which you have suggested I have made those changes. Can you please add the commit message for this bug because I tried it but was unable to achieve it.
Attachment #8999515 - Flags: review?(aethanyc)
(Reporter)

Comment 17

7 months ago
Comment on attachment 8999515 [details] [diff] [review]
final.patch

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

This looks good to me. Thank you for working on this. I'll upload a landable patch with commit message later.
Attachment #8999515 - Flags: review?(aethanyc) → review+
(Reporter)

Updated

7 months ago
Attachment #8999447 - Attachment is obsolete: true
Attachment #8999480 - Attachment is obsolete: true
Attachment #8999515 - Attachment is obsolete: true
Attachment #8999721 - Attachment is obsolete: true
(Reporter)

Updated

7 months ago
Summary: Replace ENSURE_TRUE in nsFrame.h by NS_ENSURE_TRUE_VOID → Replace ENSURE_TRUE in nsFrame.h with NS_ENSURE_TRUE_VOID
(Reporter)

Updated

7 months ago
Keywords: checkin-needed
(Assignee)

Comment 21

7 months ago
(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #19)
> Created attachment 8999723 [details] [diff] [review]
> Remove ENSURE_TRUE in nsFrame.h, and replace it with NS_ENSURE_TRUE_VOID.

Thanks Ting-Yu Lin for adding the commit message for me. 

I have a question, I want to submit the patches through command line but when I run this command "hg push review" it is giving me an error which says: abort access denied (public key) . 
I have also added the API key then why I am getting this error can you please help me?

Comment 22

7 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a627240fa20
Remove ENSURE_TRUE in nsFrame.h, and replace it with NS_ENSURE_TRUE_VOID. r=TYLin
Keywords: checkin-needed

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a627240fa20
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Comment 24

7 months ago
(In reply to Sahil Bhosale from comment #21)
> (In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #19)
> > Created attachment 8999723 [details] [diff] [review]
> > Remove ENSURE_TRUE in nsFrame.h, and replace it with NS_ENSURE_TRUE_VOID.
> 
> Thanks Ting-Yu Lin for adding the commit message for me. 

You're welcome!

> I have a question, I want to submit the patches through command line but
> when I run this command "hg push review" it is giving me an error which
> says: abort access denied (public key) . 
> I have also added the API key then why I am getting this error can you
> please help me?

"hg push review" will push to mozreview, which is decommissioned in early August. See [1] for the announcement if you're interested.

If you'd like to submit patches for other bugs, you can still use attachments to bugzilla as you did. You can also setup Phabricator [2], which is the replacement for mozreview.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/Bxomd5VAgKU/8Z3o5F5kCAAJ
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed
(Assignee)

Comment 25

7 months ago
 > "hg push review" will push to mozreview, which is decommissioned in early
> August. See [1] for the announcement if you're interested.
> 
> If you'd like to submit patches for other bugs, you can still use
> attachments to bugzilla as you did. You can also setup Phabricator [2],
> which is the replacement for mozreview.
> 
> [1]
> https://groups.google.com/d/msg/mozilla.dev.platform/Bxomd5VAgKU/8Z3o5F5kCAAJ
> [2]
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction#Step_4_Get_your_code_reviewed


Thanks for sharing this post.
You need to log in before you can comment on or make changes to this bug.