search.xml should use AppConstants from the global scope

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: ryan.b.riley, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

+++ 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
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.
(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?
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)
(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
Posted file updated search.xml (obsolete) —
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+
Attachment #8857305 - Attachment is patch: false
(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 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!
Ryan, we'll need all changes in one patch rather than two.
Attachment #8857933 - Attachment is obsolete: true
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+
Attachment #8857305 - Attachment is obsolete: true
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
Assignee: nobody → ryan.b.riley
https://hg.mozilla.org/mozilla-central/rev/6c7077f794e7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.