Closed
Bug 1353711
Opened 7 years ago
Closed 7 years ago
search.xml should use AppConstants from the global scope
Categories
(Firefox :: Address Bar, enhancement)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: dao, Assigned: ryan.b.riley, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 2 obsolete files)
+++ This bug was initially created as a clone of Bug #1350063 +++ Instead of 'this.AppConstants', search.xml should use the global 'AppConstants', and we should remove this field: http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/browser/components/search/content/search.xml#934-937
Assignee | ||
Comment 1•7 years ago
|
||
I am new and would like to work on this. The actual bug fix should only take a minute for me but I could use guidance on the process here -- I come from a GitHub background.
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Ryan Riley from comment #1) > I am new and would like to work on this. The actual bug fix should only take > a minute for me but I could use guidance on the process here -- I come from > a GitHub background. Have you read a guide like <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction>? Have you built Firefox yet?
Comment 3•7 years ago
|
||
Hi, I would like to work on this. Could you assign this to me? i have a local build ready with me to work on.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #2) > (In reply to Ryan Riley from comment #1) > > I am new and would like to work on this. The actual bug fix should only take > > a minute for me but I could use guidance on the process here -- I come from > > a GitHub background. > > Have you read a guide like > <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Introduction>? Have you built Firefox yet? Yes I have. I have built from source and made the relevant changes to the search.xml file. I am assuming the next step is to commit the changes in mercurial and somehow submit them here. How would I go about attaching the changes to this bug report? By the way, thank you for your time and guidance. I am excited to become a contributor and appreciate your help. -Ryan
Assignee | ||
Comment 5•7 years ago
|
||
this.AppConstants reference changed to parent AppConstants and overriden AppConstant field removed. Please correct me if this is not the preferred workflow, I am trying to follow the guideline as stated here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch. I can remove this attached file and follow the accepted procedure if I am in error.
Attachment #8857305 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8857305 -
Attachment is patch: false
Reporter | ||
Comment 6•7 years ago
|
||
(In reply to Ryan Riley from comment #5) > Created attachment 8857305 [details] > updated search.xml > > this.AppConstants reference changed to parent AppConstants and overriden > AppConstant field removed. > > Please correct me if this is not the preferred workflow, I am trying to > follow the guideline as stated here: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > How_to_Submit_a_Patch. I can remove this attached file and follow the > accepted procedure if I am in error. Hi Ryan, please use either hg diff, or hg commit and hg export to produce a patch, or the newer MozReview system: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8857623 [details] Bug 1353711 - Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field https://reviewboard.mozilla.org/r/129584/#review132366 ::: browser/components/search/content/search.xml:934 (Diff revision 1) > <xul:treechildren class="autocomplete-treebody"/> > </xul:tree> > <xul:vbox anonid="search-one-off-buttons" class="search-one-offs"/> > </content> > <implementation> > - <field name="AppConstants" readonly="true"> > + Please remove this empty line. Looks good otherwise!
Comment hidden (mozreview-request) |
Reporter | ||
Comment 10•7 years ago
|
||
Ryan, we'll need all changes in one patch rather than two.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8857933 -
Attachment is obsolete: true
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8857623 [details] Bug 1353711 - Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field https://reviewboard.mozilla.org/r/129584/#review132870 Looks good, thanks!
Attachment #8857623 -
Flags: review+
Reporter | ||
Updated•7 years ago
|
Attachment #8857305 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c7077f794e7 Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field r=dao
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → ryan.b.riley
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6c7077f794e7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in
before you can comment on or make changes to this bug.
Description
•