1353711 - search.xml should use AppConstants from the global scope

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement)

Description

[Dão Gottwald [:dao]]

+++ 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

Comment 1

[Ryan Riley]

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.

Comment 2

[Dão Gottwald [:dao]]

(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

[Utsav Saha]

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.

Comment 4

[Ryan Riley]

(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

Comment 5

[Ryan Riley]

Attachment: 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.

Comment 6

[Dão Gottwald [:dao]]

(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

Comment 7

[Ryan Riley]

Attachment: Bug 1353711 - Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field

Review commit: https://reviewboard.mozilla.org/r/129584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129584/

Comment 8

[Dão Gottwald [:dao]]

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 9

[Ryan Riley]

Attachment: Bug 1353711 - removing empty line 934

Review commit: https://reviewboard.mozilla.org/r/129964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129964/

Comment 10

[Dão Gottwald [:dao]]

Ryan, we'll need all changes in one patch rather than two.

Comment 11

[Ryan Riley]

Comment on attachment 8857623 [details]
Bug 1353711 - Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129584/diff/1-2/

Comment 12

[Dão Gottwald [:dao]]

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!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c7077f794e7
Changing reference to this.AppConstants to parent AppConstants, removing local AppConstants field r=dao

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6c7077f794e7